sys/nanocoap_sock: fix handling bockwise requests - #22518
Conversation
|
Let's first see if a quick Murdock run and the static test already find issues |
There was a problem hiding this comment.
Pull request overview
This PR updates RIOT’s nanocoap_sock blockwise client logic to correctly handle servers that respond with a different Block2 SZX than requested (eg, responding with 64B blocks after requesting 512B), and simplifies buffer-assembly helpers by introducing a coap_blksize_to_bytes() utility plus unit coverage.
Changes:
- Track blockwise progress using byte offsets (and adapt to server-selected SZX) instead of assuming fixed block numbers.
- Add
coap_blksize_to_bytes()helper incoap.hand a unit test validating its mapping. - Refactor slice/buffer assembly helpers in
nanocoapsocket client code and adjust header includes/docs.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
tests/unittests/tests-nanocoap/tests-nanocoap.c |
Adds unit test coverage for the new blocksize-to-bytes conversion helper. |
sys/net/application_layer/nanocoap/sock.c |
Reworks blockwise fetch logic to be offset-based and adapt to server SZX; refactors slice/buffer handling. |
sys/include/net/nanocoap_sock.h |
Updates includes and adjusts nanocoap_sock_get_slice() error documentation. |
sys/include/net/coap.h |
Introduces coap_blksize_to_bytes() utility and minor comment/include updates. |
Comments suppressed due to low confidence (2)
sys/net/application_layer/nanocoap/sock.c:818
- _fetch_block() currently does
assert(0)on invalid ctx->blocksize. That will abort in debug builds if a malformed/hostile peer causes ctx->blocksize to become out-of-range. Use an assertion on the actual condition and return a normal error without forcibly aborting.
if ((unsigned)ctx->blocksize > COAP_BLOCKSIZE_1024) {
assert(0);
return -EINVAL;
}
sys/net/application_layer/nanocoap/sock.c:999
- nanocoap_sock_get_slice(): the destination pointer is advanced by
offsetand the block ctx is initialized with an unalignedoffset. Because the blockwise callback receives absolute resource offsets, this makes the first memcpy use a huge offset (or immediately hit -ENOBUFS), and the offset mismatch check in _block_cb will keep returning -EAGAIN for non-block-aligned offsets. Use a slice-aware buffer context (tracking the wanted resource offset and remaining length) and align the initial request offset to the negotiated block size.
_block_ctx_t ctx = {
.callback = _2buf,
.arg = &dst_ctx,
.offset = offset,
.more = true,
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The conversion is a bit magic to read and the exact details are not relevant, so add a small helper with a self-explaining name.
- the code was written under the assumption that server never replies with an SZX different to what was requested. But e.g. the nanocoap server does just that for e.g. a blocksize of 512 B. - the _2buf_slice helper was not actually needed. Just adding the offset upfront and the _2buf helper can be used, greatly simplifying the logic With the following patch applied: ```diff diff --git a/tests/net/nanocoap_cli/nanocli_client.c b/tests/net/nanocoap_cli/nanocli_client.c index e9012e6..193e85701c 100644 --- a/tests/net/nanocoap_cli/nanocli_client.c +++ b/tests/net/nanocoap_cli/nanocli_client.c @@ -234,7 +234,7 @@ static int _cmd_url(int argc, char **argv) switch (code_pos) { case COAP_METHOD_GET - 1: - res = nanocoap_get_blockwise_url(argv[2], COAP_BLOCKSIZE_32, + res = nanocoap_get_blockwise_url(argv[2], COAP_BLOCKSIZE_512, _blockwise_cb, NULL); break; case COAP_METHOD_POST - 1: ``` and flashing the `tests/net/nanocoap_cli`, we now get get the following interactipon: ```console > url get coap://[fe80::9826:30ff:feb8:31f4]/.well-known/core 2026-07-28 19:11:15,839 # url get coap://[fe80::9826:30ff:feb8:31f4]/.well-known/core 2026-07-28 19:11:15,840 # offset 000: </vfs>,</time>,</separate>,</sha256>,</riot/ver>,</riot/value>,< 2026-07-28 19:11:15,841 # offset 064: /riot/board>,</echo/>,</.well-known/core> ``` Or graphically (with some data taking from wireshark): ``` CLIENT SERVER | | | CON [MID=19460], GET, /.well-known/core, 2:0/0/512 ----> | | | | <---- ACK [MID=19460], 2.05 Content, 2:0/1/64 | | | | CON [MID=19461], GET, /.well-known/core, 2:1/0/64 ----> | | | | <---- ACK [MID=19461], 2.05 Content, 2:1/0/64 | | | ``` The same would interaction would also somewhat work before: ``` CLIENT SERVER | | | CON [MID=32239], GET, /.well-known/core, 2:0/0/512 ----> | | | | <---- ACK [MID=32239], 2.05 Content, 2:0/1/64 | | | | CON [MID=32240], GET, /.well-known/core, 2:1/0/512 ----> | | | | <---- ACK [MID=32240], 2.05 Content, 2:1/0/64 | | | ``` Note that here the client got 64 B (not 512 B) but is asking for the next 512 B chunk (despite there are still 448 B of the first 512 B chunk being missing). And apparently the server is just taking the block number 1 from the request, changes the block size, and returns the bock number 1 with the new size. So the server is in need of fixing as well here.
a91d9f0 to
f0ee9d5
Compare
|
Oh... I didn't see this was still in draft stage. Sorry :/ |
Co-authored-by: crasbe <crasbe@gmail.com>
Co-authored-by: crasbe <crasbe@gmail.com>
Co-authored-by: crasbe <crasbe@gmail.com>
Co-authored-by: crasbe <crasbe@gmail.com>
Co-authored-by: crasbe <crasbe@gmail.com>
Co-authored-by: crasbe <crasbe@gmail.com>
🐐 |
| if ((unsigned)blocksize > (unsigned)COAP_BLOCKSIZE_1024) { | ||
| return 0; | ||
| } | ||
|
|
||
| /* > The block size is represented as a three-bit | ||
| * > unsigned integer indicating the size of a block to the power of | ||
| * > two. Thus, block size = 2**(SZX + 4). | ||
| * | ||
| * https://www.rfc-editor.org/info/rfc7959/#section-2.2 */ | ||
| return (uint32_t)1 << (4U + (unsigned)blocksize); |
There was a problem hiding this comment.
| if ((unsigned)blocksize > (unsigned)COAP_BLOCKSIZE_1024) { | |
| return 0; | |
| } | |
| /* > The block size is represented as a three-bit | |
| * > unsigned integer indicating the size of a block to the power of | |
| * > two. Thus, block size = 2**(SZX + 4). | |
| * | |
| * https://www.rfc-editor.org/info/rfc7959/#section-2.2 */ | |
| return (uint32_t)1 << (4U + (unsigned)blocksize); | |
| switch (blocksize) { | |
| case COAP_BLOCKSIZE_16: | |
| return 16; | |
| case COAP_BLOCKSIZE_32: | |
| return 32; | |
| case COAP_BLOCKSIZE_64: | |
| return 64; | |
| case COAP_BLOCKSIZE_128: | |
| return 128; | |
| case COAP_BLOCKSIZE_256: | |
| return 256; | |
| case COAP_BLOCKSIZE_512: | |
| return 512; | |
| case COAP_BLOCKSIZE_1024: | |
| return 1024; | |
| default: | |
| return 0; | |
| } |
Why not like this?
Contribution description
Testing procedure
With the following patch applied:
and flashing the
tests/net/nanocoap_cli, we now get get the followinginteractipon:
Or graphically (with some data taking from wireshark):
The same would interaction would also somewhat work before:
Note that here the client got 64 B (not 512 B) but is asking for the
next 512 B chunk (despite there are still 448 B of the first 512 B chunk
being missing). And apparently the server is just taking the block number
1 from the request, changes the block size, and returns the bock number 1
with the new size. So the server is in need of fixing as well here.
Issues/PRs references
In addition to the issue in behavior, this also fixes the issue reported in https://github.com/RIOT-OS/RIOT/security/advisories/GHSA-x924-p5fq-26pc
Declaration of AI-Tools / LLMs usage:
AI-Tools / LLMs that were used are: