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

net/nanocoap: validate option length #10823

Merged
merged 3 commits into from
Jan 24, 2019

Conversation

kb2ma
Copy link
Member

@kb2ma kb2ma commented Jan 18, 2019

Contribution description

The Buffer Append API added in #9085 uses the coap_pkt_t struct to track to amount of buffer space remaining as the CoAP PDU is built. _add_opt_pkt() is the low level function in that API to add an option to the buffer. This function uses coap_put_option() to actually write to the buffer. However, that function is part of the minimal Buffer Put API, and does not check the length of the buffer before writing. This PR updates _add_opt_pkt() to test the available buffer space before writing the option.

The PR also adds function documentation for _put_delta_optlen(), which writes the option header, and a unit test for building a message that completely fills the buffer.

The use of an assert in _add_opt_pkt() when the buffer is too small is not ideal. We plan to create a follow-on PR that returns a negative value from the function so the caller can recover.

Testing procedure

The 'tests-nanocoap' unit tests include a new test, test_nanocoap__option_add_buffer_max(), which completely fills the buffer used to write a request. Run 'tests-nanocoap' both without and with assertions enabled (compile with FORCE_ASSERTS=1). Both runs should pass.

Next, manually reduce the size of the buffer in the new test from 70 to 69 and rerun the tests. Without asserts, on native the test fails abruptly when the buffer overflows ("stack smashing detected"). With asserts enabled, the relevant assert in _add_opt_pkt() is tripped.

Issues/PRs references

-none-

@kb2ma kb2ma added Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation Area: CoAP Area: Constrained Application Protocol implementations labels Jan 18, 2019
@kb2ma kb2ma requested review from kaspar030 and smlng January 18, 2019 17:52
Copy link
Member

@smlng smlng left a comment

Choose a reason for hiding this comment

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

checks out okay, looks good and tests succeeded even on 8 bit atmega I tested. ACK

@smlng smlng added Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jan 23, 2019
@smlng smlng merged commit 4423478 into RIOT-OS:master Jan 24, 2019
@aabadie aabadie added this to the Release 2019.01 milestone Jan 24, 2019
@kb2ma kb2ma deleted the nanocoap/verify_before_write_option branch February 5, 2019 02:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: CoAP Area: Constrained Application Protocol implementations CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants