Skip to content

sys/net/nanocoap: fix coap_build_reply_header()#20949

Merged
benpicco merged 2 commits intoRIOT-OS:masterfrom
maribu:sys/net/nanocoap/fix-coap_build_reply_header
Nov 5, 2024
Merged

sys/net/nanocoap: fix coap_build_reply_header()#20949
benpicco merged 2 commits intoRIOT-OS:masterfrom
maribu:sys/net/nanocoap/fix-coap_build_reply_header

Conversation

@maribu
Copy link
Copy Markdown
Member

@maribu maribu commented Nov 4, 2024

Contribution description

This fixes the return value of coap_build_reply_header() to also be correct when the request used an extended length token (RFC 8974). In master, the extend token length field is not taken into account.

The nanocoap unit tests were extended to catch the bug.

Testing procedure

Cherry-pick the second commit onto master and run:

 make BOARD=native -C tests/unittests all -j && make BOARD=native -C tests/unittests test

This will fail:

main(): This is RIOT! (Version: 2024.10-devel-373-g5ae4c)
Help: Press s to start test, r to print it is ready
READY
s
START
.......................==66399==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases!
.....................................................................................................................................................................................................................==66399==WARNING: ASan is ignoring requested __asan_handle_no_return: stack type: default top: 0xffb15000; bottom 0xea802000; size: 0x15313000 (355545088)
False positive error reports may follow
For details see https://github.com/google/sanitizers/issues/189
.................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................
nanocoap_tests.test_nanocoap__token_length_ext_16 (tests/unittests/tests-nanocoap/tests-nanocoap.c 1126) exp 21 was 20
.
nanocoap_tests.test_nanocoap__token_length_ext_269 (tests/unittests/tests-nanocoap/tests-nanocoap.c 1176) exp 275 was 273
...........................................................................................................................................................................................................................................................................................................................................................
run 1209 failures 2

Run the same test again in this PR, this passes:

main(): This is RIOT! (Version: 2024.10-devel-375-gad7e45-sys/net/nanocoap/fix-coap_build_reply_header)
Help: Press s to start test, r to print it is ready
READY
s
START
.......................==69252==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases!
.....................................................................................................................................................................................................................==69252==WARNING: ASan is ignoring requested __asan_handle_no_return: stack type: default top: 0xffa32000; bottom 0xee502000; size: 0x11530000 (290652160)
False positive error reports may follow
For details see https://github.com/google/sanitizers/issues/189
.............................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................
OK (1209 tests)

Issues/PRs references

None

@maribu maribu added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Nov 4, 2024
@github-actions github-actions bot added Area: network Area: Networking Area: tests Area: tests and testing framework Area: CoAP Area: Constrained Application Protocol implementations Area: sys Area: System labels Nov 4, 2024
@riot-ci
Copy link
Copy Markdown

riot-ci commented Nov 4, 2024 •
edited
Loading

Murdock results

✔️ PASSED

e52659c tests/unittests: Increase test coverage for coap_build_reply_header()

Success Failures Total Runtime
10215 0 10215 13m:36s

Artifacts

benpicco
benpicco reviewed Nov 4, 2024
View reviewed changes
@maribu maribu force-pushed the sys/net/nanocoap/fix-coap_build_reply_header branch from ad7e450 to bc2b30f Compare November 4, 2024 19:20
benpicco
benpicco reviewed Nov 4, 2024
View reviewed changes
benpicco
benpicco reviewed Nov 4, 2024
View reviewed changes
@maribu maribu force-pushed the sys/net/nanocoap/fix-coap_build_reply_header branch 2 times, most recently from 59c82ec to 517fdf9 Compare November 4, 2024 20:12
maribu and others added 2 commits November 4, 2024 21:18
@maribu @benpicco
sys/net/nanocoap: fix coap_build_reply_header()
2c93dc9 
In case no payload is added, `coap_build_reply_header()` would return
`sizeof(coap_hdr_t) + token_length` regardless of the actual header
length returned by `coap_build_hdr()`. These can be different if
RFC 8974 extended tokens are enabled (module `nanocoap_token_ext`
used): If an extended token length field is used, its size is not
considered.

Co-authored-by: benpicco <[email protected]>
@maribu
tests/unittests: Increase test coverage for coap_build_reply_header()
e52659c 
This increases test coverage for coap_build_reply_header() for the case
that extended tokens are used. The test validates the correctness of
the return value and the correctness of the assembled reply.
@maribu maribu force-pushed the sys/net/nanocoap/fix-coap_build_reply_header branch from 517fdf9 to e52659c Compare November 4, 2024 20:18
@maribu
Copy link
Copy Markdown
Member Author

maribu commented Nov 4, 2024

All green now :)

@benpicco benpicco enabled auto-merge November 5, 2024 08:51
@benpicco benpicco added this pull request to the merge queue Nov 5, 2024
Merged via the queue into RIOT-OS:master with commit 2734366 Nov 5, 2024
@maribu maribu deleted the sys/net/nanocoap/fix-coap_build_reply_header branch November 5, 2024 14:13
@maribu
Copy link
Copy Markdown
Member Author

maribu commented Nov 5, 2024

Thx :)

@MrKevinWeiss MrKevinWeiss added this to the Release 2025.01 milestone Jan 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@benpicco benpicco benpicco approved these changes

@miri64 miri64 Awaiting requested review from miri64 miri64 is a code owner

@PeterKietzmann PeterKietzmann Awaiting requested review from PeterKietzmann

@fabian18 fabian18 Awaiting requested review from fabian18

@mguetschow mguetschow Awaiting requested review from mguetschow

Assignees

No one assigned

Labels

Area: CoAP Area: Constrained Application Protocol implementations Area: network Area: Networking Area: sys Area: System Area: tests Area: tests and testing framework 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 2025.01

Development

Successfully merging this pull request may close these issues.

4 participants

@maribu @riot-ci @benpicco @MrKevinWeiss