Skip to content

Conversation

fabian18
Copy link
Contributor

@fabian18 fabian18 commented Mar 5, 2025

Contribution description

How often have you been looking for a bug, and it felt like chasing a ghost because memory was messed up?
How often was it a buffer you have unsuspectingly increased in size, which was not static?
(The right answer is: Yes 😄)
That's why non-static uint8_t buffer[CONFIG_INCREASE_ON_DEMAND] is likely a footgun.

Most importantly, this PR makes that a response buffer for nanocoap_link_format_get() has to be user allocated.
I could have just made it static, but this would bring up a concurrency issue.

Secondly, a buffer [CONFIG_NANOCOAP_BLOCK_HEADER_MAX] is added to nanocoap_sock_t.
This was before allocated in various nanocoap functions. I moved it to the socket because this is likely allocated in a user thread. Therefore the stack size of that user's thread is no longer our problem 😅. And not so unlikely, the user allocates the socket with static.

Testing procedure

Start a CoAP fileserver: aiocoap-fileserver --bind [::] .
Flash a sam54-xpro and connect it via ethernet to the coap fileserver:
USEMODULE+=nanocoap_vfs CFLAGS+=-DCONFIG_NANOCOAP_QS_MAX=1024 BOARD=same54-xpro make -C examples/networking/gnrc/gnrc_networking flash term PORT=/dev/ttyACM0

On master: ncget coap://[fe80::be63:bb80:6473:faa9]/ crashes on my setup.
On this banch it succeeds and prints the directory content.

Issues/PRs references

@github-actions github-actions bot added Area: network Area: Networking Area: CoAP Area: Constrained Application Protocol implementations Area: sys Area: System labels Mar 5, 2025
@mguetschow mguetschow added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Mar 6, 2025
@riot-ci
Copy link

riot-ci commented Mar 6, 2025

Murdock results

✔️ PASSED

49243a5 nanocoap/sock: move request header buffer to socket

Success Failures Total Runtime
10287 0 10287 11m:32s

Artifacts

@github-actions github-actions bot added the Area: tests Area: tests and testing framework label Mar 9, 2025
@mguetschow mguetschow added the CI: no fast fail don't abort PR build after first error label Mar 10, 2025
Copy link
Contributor

@mguetschow mguetschow left a comment

Choose a reason for hiding this comment

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

LGTM! Let's see what else the CI comes up with, then feel free to squash!

@fabian18 fabian18 force-pushed the pr/nanocoap_static_buffers branch from c64d701 to 7f22009 Compare March 16, 2025 13:32
@fabian18 fabian18 force-pushed the pr/nanocoap_static_buffers branch from 7f22009 to 51ac476 Compare March 25, 2025 19:51
@benpicco benpicco added this pull request to the merge queue Mar 26, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 26, 2025
@fabian18 fabian18 force-pushed the pr/nanocoap_static_buffers branch from 51ac476 to b07e095 Compare March 31, 2025 09:56
@mguetschow mguetschow added this pull request to the merge queue Mar 31, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 31, 2025
@fabian18 fabian18 force-pushed the pr/nanocoap_static_buffers branch from b07e095 to 49243a5 Compare March 31, 2025 17:12
@fabian18 fabian18 enabled auto-merge March 31, 2025 17:14
@fabian18 fabian18 added this pull request to the merge queue Mar 31, 2025
Merged via the queue into RIOT-OS:master with commit 9e55e56 Apr 1, 2025
25 checks passed
@mguetschow mguetschow added this to the Release 2025.04 milestone Apr 8, 2025
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 Area: network Area: Networking Area: sys Area: System Area: tests Area: tests and testing framework CI: no fast fail don't abort PR build after first error 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