Merge pull request #20917 from maribu/sys/net/nanocoap/fix-ub

sys/net/nanocoap: fix UB when building hdr
RIOT-OS · Oct 17, 2024 · 52dd2c7 · 52dd2c7
2 parents 4f5c0ed + 835571c
commit 52dd2c7
Show file tree

Hide file tree

Showing 2 changed files with 15 additions and 2 deletions.
diff --git a/sys/include/net/nanocoap.h b/sys/include/net/nanocoap.h
@@ -1946,6 +1946,11 @@ ssize_t coap_block2_build_reply(coap_pkt_t *pkt, unsigned code,
  * @param[in]    code       CoAP code (e.g., COAP_CODE_204, ...)
  * @param[in]    id         CoAP request id
  *
+ * @pre     @p token is either not overlapping with the memory buffer
+ *          @p hdr points to, or is already at the right offset (e.g.
+ *          when building the response inside the buffer the contained
+ *          the request).
+ *
  * @returns      length of resulting header
  */
 ssize_t coap_build_hdr(coap_hdr_t *hdr, unsigned type, const void *token,

diff --git a/sys/net/application_layer/nanocoap/nanocoap.c b/sys/net/application_layer/nanocoap/nanocoap.c
@@ -724,8 +724,16 @@ ssize_t coap_build_hdr(coap_hdr_t *hdr, unsigned type, const void *token,
         memcpy(hdr + 1, &tkl_ext, tkl_ext_len);
     }
 
-    if (token_len) {
-        memcpy(coap_hdr_data_ptr(hdr), token, token_len);
+    /* Some users build a response packet in the same buffer that contained
+     * the request. In this case, the argument token already points inside
+     * the target, or more specifically, it is already at the correct place.
+     * Having `src` and `dest` in `memcpy(dest, src, len)` overlap is
+     * undefined behavior, so have to treat this explicitly. We could use
+     * memmove(), but we know that either `src` and `dest` do not overlap
+     * at all, or fully. So we can be a bit more efficient here. */
+    void *token_dest = coap_hdr_data_ptr(hdr);
+    if (token_dest != token) {
+        memcpy(token_dest, token, token_len);
     }
 
     return sizeof(coap_hdr_t) + token_len + tkl_ext_len;