-
Notifications
You must be signed in to change notification settings - Fork 2k
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
nanocoap: fix coap_block2_finish() when option size is shrinking #20688
base: master
Are you sure you want to change the base?
Conversation
if (obytes != slicer->obytes) { | ||
/* if the option grew larger, we already corrupted content */ | ||
assert(obytes < slicer->obytes); | ||
memmove(slicer->opt + obytes, slicer->opt + slicer->obytes, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
arg, that's because the size generated by coap_opt_add_block2()
is used to calculate the header and therefore the packet length.
Even when updating the coap_pkt_t
struct in coap_block2_finish()
, the header length will already have been calculated by coap_opt_finish()
.
This now changes the API of |
I think when there is an option after the BLOCK2 option the diff --git a/sys/include/net/nanocoap.h b/sys/include/net/nanocoap.h
index 1c4cadbbe1..8c0f984bd2 100644
--- a/sys/include/net/nanocoap.h
+++ b/sys/include/net/nanocoap.h
@@ -988,8 +988,7 @@ void coap_block_object_init(coap_block1_t *block, size_t blknum, size_t blksize,
* This function finalizes the block response header
*
* Checks whether the `more` bit should be set in the block option and
- * sets/clears it if required. Doesn't return the number of bytes, as this
- * function overwrites bytes in the packet rather than adding new.
+ * sets/clears it if required.
*
* @param[in] slicer Preallocated slicer struct to use
* @param[in] option option number (block1 or block2)
diff --git a/sys/net/application_layer/nanocoap/fileserver.c b/sys/net/application_layer/nanocoap/fileserver.c
index 414a82dc63..09e83d7c50 100644
--- a/sys/net/application_layer/nanocoap/fileserver.c
+++ b/sys/net/application_layer/nanocoap/fileserver.c
@@ -206,12 +206,14 @@ static ssize_t _get_file(coap_pkt_t *pdu, uint8_t *buf, size_t len,
int err;
uint32_t etag;
coap_block1_t block2 = { .szx = CONFIG_NANOCOAP_BLOCK_SIZE_EXP_MAX };
+ size_t resp_len;
{
struct stat stat;
if ((err = vfs_stat(request->namebuf, &stat)) < 0) {
return _error_handler(pdu, buf, len, err);
}
stat_etag(&stat, &etag);
+ resp_len = stat.st_size;
}
if (request->options.exists.block2 && !coap_get_block2(pdu, &block2)) {
return _error_handler(pdu, buf, len, COAP_CODE_BAD_OPTION);
@@ -240,7 +242,8 @@ static ssize_t _get_file(coap_pkt_t *pdu, uint8_t *buf, size_t len,
&block2);
coap_block_slicer_init(&slicer, block2.blknum, coap_szx2size(block2.szx));
coap_opt_add_block2(pdu, &slicer, true);
- size_t resp_len = coap_opt_finish(pdu, COAP_OPT_FINISH_PAYLOAD);
+ coap_opt_add_uint(pdu, COAP_OPT_SIZE2, resp_len);
+ resp_len = coap_opt_finish(pdu, COAP_OPT_FINISH_PAYLOAD);
err = vfs_lseek(fd, slicer.start, SEEK_SET);
if (err < 0) {
@@ -268,8 +271,19 @@ static ssize_t _get_file(coap_pkt_t *pdu, uint8_t *buf, size_t len,
vfs_close(fd);
+ /* Solution for problem of re-encoded options with changing size:
+ Move everything behind a re-encoded option for which the size changed backwards */
+ int shrunk;
+
slicer.cur = slicer.end + more;
- resp_len -= coap_block2_finish(&slicer);
+ if ((shrunk = coap_block2_finish(&slicer))) {
+ resp_len -= shrunk;
+ memmove(slicer.opt + slicer.obytes,
+ slicer.opt + slicer.obytes + shrunk,
+ (pdu->payload + read) - (slicer.opt + slicer.obytes + shrunk));
+ pdu->payload -= shrunk;
+ }
if (read == 0) {
/* Rewind to clear payload marker */
diff --git a/sys/net/application_layer/nanocoap/nanocoap.c b/sys/net/application_layer/nanocoap/nanocoap.c
index 3d9f2121b9..ef845c2b3e 100644
--- a/sys/net/application_layer/nanocoap/nanocoap.c
+++ b/sys/net/application_layer/nanocoap/nanocoap.c
@@ -1342,13 +1342,12 @@ int coap_block_finish(coap_block_slicer_t *slicer, uint16_t option)
uint8_t obytes = coap_put_option(slicer->opt, option - delta, option,
(uint8_t *)&blkopt, olen);
- if (obytes != slicer->obytes) {
- /* if the option grew larger, we already corrupted content */
- assert(obytes < slicer->obytes);
- memmove(slicer->opt + obytes, slicer->opt + slicer->obytes,
- 1 + slicer->cur - slicer->obytes);
- }
- return slicer->obytes - obytes;
+
+ /* if the option grew larger, we already corrupted content */
+ assert(obytes <= slicer->obytes);
+ int odiff = slicer->obytes - obytes;
+ slicer->obytes = obytes;
+ return odiff;
}
ssize_t coap_block2_build_reply(coap_pkt_t *pkt, unsigned code,
This is the patch I tried with. Maybe you still find another issue. The other places of |
Contribution description
The CoAP block option gets written twice: First a 'dummy' value is written by
coap_opt_add_block2()
, later this gets overwritten by the real option value bycoap_block2_finish()
.The problem arises when the size of the option changes.
If the option ends up smaller than the dummy, we have garbage bytes after the real option value, corrupting the packet.
To fix this, move the packet contents in case the option size shrank.
Testing procedure
Issues/PRs references
fixes #20686