Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow receiving broadcast packets in UDP #66

Merged
merged 4 commits into from Nov 23, 2017
Merged

Allow receiving broadcast packets in UDP #66

merged 4 commits into from Nov 23, 2017

Conversation

jackpot51
Copy link
Contributor

@jackpot51 jackpot51 commented Oct 30, 2017

This allows the reception of UDP broadcast packets. They will fall through to the process_udp function. The tcp and icmp functions also receive potential broadcast packets, but they should already be discarded.

@dlrobertson
Copy link
Collaborator

The destination address is not checked to see if it is the broadcast address before sending ICMP error responses currently (RFC 1122 § 3.2.2). Currently this conditional is the only thing keeping us from causing a broadcast storm due to a weird broadcast packet. The specific panic is here, and is due to the packet created for the no_icmp_to_broadcast test having no payload.

@whitequark
Copy link
Contributor

@dlrobertson Wow, your tests already paid off. Thanks a lot!

@jackpot51
Copy link
Contributor Author

@whitequark, I believe @dlrobertson is working on a fix, to prevent the ICMP error message from being sent

@dlrobertson
Copy link
Collaborator

After a rebase, tests should pass here.

@whitequark
Copy link
Contributor

@jackpot51 Can you rebase?

@jackpot51
Copy link
Contributor Author

Yep, will do

@jackpot51
Copy link
Contributor Author

@whitequark it is done

@whitequark
Copy link
Contributor

Any chance you can write a test for this?

@jackpot51
Copy link
Contributor Author

Sure!

@jackpot51
Copy link
Contributor Author

@whitequark I do not see any current tests for UDP in the ethernet interface, am I mistaken?

@dlrobertson
Copy link
Collaborator

@jackpot51 https://github.com/m-labs/smoltcp/blob/master/src/iface/ethernet.rs#L828-L911 is an example of a test in the ethernet interface for UDP. Essentially you could do the same thing as the second half of that test, but add a socket to the socket set that is listening to the port of interest.

@jackpot51
Copy link
Contributor Author

Ok, cool

jackpot51 and others added 2 commits November 16, 2017 09:21
Add test for the processing of a UDP broadcast packets by `InterfaceInner`.
@whitequark
Copy link
Contributor

@jackpot51 ping?

Copy link
Contributor

@whitequark whitequark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs tests

@jackpot51
Copy link
Contributor Author

Sorry @whitequark, I have been busy porting cargo.

@dlrobertson
Copy link
Collaborator

I added tests with redox-os/smoltcp#1

Add test for UDP broadcast processing
@jackpot51
Copy link
Contributor Author

@whitequark the tests from @dlrobertson are now merged

@whitequark whitequark merged commit d8b25cd into smoltcp-rs:master Nov 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants