Skip to content

Conversation

brummer-simon
Copy link
Member

The latest hardening of the gnrc_tcp option parser was a bit too restrictive.

Packets with unknown options were dropped by gnrc_tcp. This leads to problems then communicating with feature complete tcp implementations, since valid options like SACK are often announced in first SYN Packet. Without this PR such a SYN would be dropped by gnrc_tcp.

This PR allows packets with unknown options to be valid as long as they respect the boundaries of the option field and the option minimal length.

@brummer-simon
Copy link
Member Author

@miri64 @nmeum : As non-maintainer I can't request a review from a specific person. Would you both do me a favour an review this PR?

@miri64 miri64 self-requested a review September 24, 2019 17:35
@miri64 miri64 added 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) labels Sep 24, 2019
@brummer-simon brummer-simon force-pushed the gnrc_tcp-allow_unknown_options branch from 632a0c5 to dee092e Compare September 24, 2019 17:47
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

@brummer-simon brummer-simon force-pushed the gnrc_tcp-allow_unknown_options branch from dee092e to fdda22c Compare September 24, 2019 18:03
@miri64 miri64 merged commit 58e3384 into RIOT-OS:master Sep 24, 2019
@brummer-simon brummer-simon deleted the gnrc_tcp-allow_unknown_options branch September 24, 2019 18:55
@kb2ma kb2ma added this to the Release 2019.10 milestone Sep 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

3 participants