Skip to content

gnrc_tcp: check if option has valid length set #12368

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
Merged
merged 1 commit into from
Oct 5, 2019
Merged

gnrc_tcp: check if option has valid length set #12368

merged 1 commit into from
Oct 5, 2019

Conversation

miri64
Copy link
Member

@miri64 miri64 commented Oct 3, 2019

Contribution description

While adding some regression tests for the issues @nmeum found to the new test application added in #11930, I sadly found that #12298 reintroduced parts of #12086. If the test provided with that issue is used, the infinite loop pointed out there is triggered in current master, since the test provides an option with its length field set to zero.

Testing procedure

Redo the test in #12086. On current master they show the exact same behavior, but not with this fix.
tests/gnrc_tcp should still pass on both master and a different board (please test, I only have access to master right now!).

Issues/PRs references

Fixes a regression introduced in #12298

@miri64 miri64 added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Area: network Area: Networking CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Oct 3, 2019
@miri64 miri64 added this to the Release 2019.10 milestone Oct 3, 2019
@miri64 miri64 requested a review from benpicco October 3, 2019 16:38
@miri64
Copy link
Member Author

miri64 commented Oct 3, 2019

@brummer-simon please also have a look!

@miri64 miri64 mentioned this pull request Oct 3, 2019
Merged
brummer-simon
brummer-simon approved these changes Oct 5, 2019
View reviewed changes
Copy link
Member

@brummer-simon brummer-simon left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good by me. This fixes the failing test "test_option_parsing_term" in #12369 right?
Its really hard to get Input validation correctly. Thanks for finding such issues.

@miri64
Copy link
Member Author

miri64 commented Oct 5, 2019

@RIOT-OS/maintainers can someone proxy-ACK?

@kb2ma
Copy link
Member

kb2ma commented Oct 5, 2019

I was just taking a look at this. Any justification for the build failure? I tried the failing test several times on my Ubuntu laptop, and the timeout was within the threshold value.

@miri64
Copy link
Member Author

miri64 commented Oct 5, 2019

I was just taking a look at this. Any justification for the build failure? I tried the failing test several times on my Ubuntu laptop, and the timeout was within the threshold value.

It's not a build error. It's native being flaky in the tests...

@miri64 miri64 added 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 Oct 5, 2019
@miri64
Copy link
Member Author

miri64 commented Oct 5, 2019

@kb2ma This time Murdock passed. The test that failed is unrelated to gnrc_tcp.

@kb2ma
Copy link
Member

kb2ma commented Oct 5, 2019

Perfect, thanks @miri64.

kb2ma
kb2ma approved these changes Oct 5, 2019
View reviewed changes
Copy link
Member

@kb2ma kb2ma left a comment

Choose a reason for hiding this comment

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

Simple fix. Proxy-ACK for @brummer-simon.

@miri64 miri64 merged commit b99ec92 into RIOT-OS:master Oct 5, 2019
@miri64 miri64 deleted the gnrc_tcp/fix/check-option-length branch October 5, 2019 13:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kb2ma kb2ma kb2ma approved these changes

@benpicco benpicco Awaiting requested review from benpicco

+1 more reviewer

@brummer-simon brummer-simon brummer-simon approved these changes

Reviewers whose approvals may not affect merge requirements
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 Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Milestone
Release 2019.10
Development

Successfully merging this pull request may close these issues.
3 participants
@miri64 @kb2ma @brummer-simon