Skip to content
New issue

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

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

Already on GitHub? # to your account

nanocoap_sock: defuse nanocoap_sock_get() API footgun #19535

Merged
merged 1 commit into from
May 2, 2023

Conversation

benpicco
Copy link
Contributor

@benpicco benpicco commented May 2, 2023

Contribution description

nanocoap_sock_get() would use the supplied response buffer to assemble the request header.
While this can be an efficient use of space, it's also really surprising for users who only expect to receive a small payload and have the request fail (actually corrupt the stack because nanoCoAP only checks if a buffer overflowed after it has written to it).

All other nanocoap_sock_*() functions use a small stack based buffer for the header - let's do the same for nanocoap_sock_get() to avoid nasty surprises.

Testing procedure

Issues/PRs references

@github-actions github-actions bot added Area: CoAP Area: Constrained Application Protocol implementations Area: network Area: Networking Area: sys Area: System labels May 2, 2023
@benpicco benpicco requested review from chrysn and maribu May 2, 2023 13:08
@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label May 2, 2023
Copy link
Contributor

@kaspar030 kaspar030 left a comment

Choose a reason for hiding this comment

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

ACK.

@chrysn
Copy link
Member

chrysn commented May 2, 2023

I was about to complain about this getting into gcoap's area of resource usage (gcoap's stack is one frequent point of complaints) ... but given I see @kaspar030 as the defendant of nanocoap's slimness and he's fine with it, go :-)

@kaspar030
Copy link
Contributor

Heh, now that you said that, I actually confirmed that CONFIG_NANOCOAP_BLOCK_HEADER_MAX can't be that much. :)

❯ git grep "define CONFIG_NANOCOAP_BLOCK_HEADER_MAX"
sys/include/net/nanocoap.h:#define CONFIG_NANOCOAP_BLOCK_HEADER_MAX   (80)

@riot-ci
Copy link

riot-ci commented May 2, 2023

Murdock results

✔️ PASSED

f6f5b13 nanocoap_sock: don't use return buffer for request

Success Failures Total Runtime
6930 0 6931 09m:35s

Artifacts

@benpicco
Copy link
Contributor Author

benpicco commented May 2, 2023

bors merge

@bors
Copy link
Contributor

bors bot commented May 2, 2023

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot merged commit c4b27d8 into RIOT-OS:master May 2, 2023
@benpicco benpicco deleted the nanocoap_sock_get-defuse branch May 2, 2023 15:38
@benpicco benpicco added this to the Release 2023.07 milestone Aug 2, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Area: CoAP Area: Constrained Application Protocol implementations Area: network Area: Networking Area: sys Area: System CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants