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.

Sign up for GitHub

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

Jump to bottom

gnrc_udp: assert ports not zero #9721

Merged
merged 1 commit into from Aug 6, 2018
Merged

gnrc_udp: assert ports not zero #9721

merged 1 commit into from Aug 6, 2018

Conversation

smlng
Copy link
Member

@smlng smlng commented Aug 6, 2018 

UDP port 0 is reserved for system usage, e.g., to tell the OS to
set a random source port. Hence, neither source nor destination
port should be 0 when transmitting. This PR adds proper asserts.

Contribution description

Minor enhancement to ensure UDP ports are non-zero on transmission. I fiddled around
with the gnrc_networking example and found that there is no check to verify this and hence
I was able to send a packet with source port 0.

We could make this a proper runtime check, but I decided against it because of the overhead and
just a added an equivalent assert to verify this while testing.

Issues/PRs references

@smlng
gnrc_udp: assert ports not zero
fd718df 
    UDP port 0 is reserved for system usage, e.g., to tell the OS to
    set a random source port. Hence, neither source nor destination
    port should be 0 when transmitting. This PR adds proper asserts.
@smlng smlng added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer GNRC CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Aug 6, 2018
@smlng smlng requested a review from miri64 August 6, 2018 14:14
miri64
miri64 approved these changes Aug 6, 2018
View reviewed changes
Copy link
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

ACK

@miri64 miri64 added CI: run tests If set, CI server will run tests on hardware for the labeled PR CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Aug 6, 2018
@miri64
Copy link
Member

miri64 commented Aug 6, 2018

Though I tested some stuff locally, I rather feel safer if all the tests run on the CI

@miri64 miri64 merged commit ad17383 into RIOT-OS:master Aug 6, 2018
@smlng smlng deleted the pr/gnrc_udp/assert branch August 6, 2018 19:50
@cladmi cladmi added this to the Release 2018.10 milestone Aug 11, 2018
@miri64 miri64 added the Area: network Area: Networking label Sep 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@miri64 miri64 miri64 approved these changes

Assignees
No one assigned
Labels
Area: network Area: Networking CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: run tests If set, CI server will run tests on hardware for the labeled PR Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Milestone
Release 2018.10
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@smlng @miri64 @cladmi