From 765dc3a299e6de524f4ec6c6afeac32439d33a83 Mon Sep 17 00:00:00 2001 From: Marian Buschsieweke Date: Fri, 1 Nov 2024 13:02:53 +0100 Subject: [PATCH] sys/net/gcoap: reduce insanity of hack MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit gcoap contains a hack where a `coap_pkt_t` is pulled out of thin air, parts of the members are left uninitialized and a function is called on that mostly uninitialized data while crossing fingers hard that the result will be correct. (With the current implementation of the used function this hack does actually work.) Estimated level of insanity: 😱😱😱😱😱 This adds to insane functions to get the length of a token and the length of a header of a CoAP packet while crossing fingers hard that the packet is valid and that the functions do not overread. Estimated level of insanity: 😱😱😱 The newly introduced insane functions are used to replace the old insane hack, resulting in an estimated reduction of insanity of 😱😱. Side note: This actually does fix a bug, as the old code did not take into account the length of the extended TKL field in case of RFC 8974 being used. But that is a bug in the abused API, and not in the caller abusing the API. --- sys/include/net/nanocoap.h | 84 +++++++++++++++++++------ sys/net/application_layer/gcoap/gcoap.c | 7 +-- 2 files changed, 67 insertions(+), 24 deletions(-) diff --git a/sys/include/net/nanocoap.h b/sys/include/net/nanocoap.h index 257e5ba491..0459de9690 100644 --- a/sys/include/net/nanocoap.h +++ b/sys/include/net/nanocoap.h @@ -352,6 +352,10 @@ struct _coap_request_ctx { #endif }; +/* forward declarations */ +static inline uint8_t *coap_hdr_data_ptr(const coap_hdr_t *hdr); +static inline size_t coap_hdr_get_token_len(const coap_hdr_t *hdr); + /** * @brief Get resource path associated with a CoAP request * @@ -548,28 +552,9 @@ static inline unsigned coap_get_id(const coap_pkt_t *pkt) */ static inline unsigned coap_get_token_len(const coap_pkt_t *pkt) { - uint8_t tkl = pkt->hdr->ver_t_tkl & 0xf; - - if (!IS_USED(MODULE_NANOCOAP_TOKEN_EXT)) { - return tkl; - } - - void *ext = pkt->hdr + 1; - switch (tkl) { - case 13: - return tkl + *(uint8_t *)ext; - case 14: - return tkl + 255 + byteorder_bebuftohs(ext); - case 15: - assert(0); - /* fall-through */ - default: - return tkl; - } + return coap_hdr_get_token_len(pkt->hdr); } -static inline uint8_t *coap_hdr_data_ptr(const coap_hdr_t *hdr); - /** * @brief Get pointer to a message's token * @@ -714,6 +699,65 @@ static inline void coap_hdr_set_type(coap_hdr_t *hdr, unsigned type) hdr->ver_t_tkl &= ~0x30; hdr->ver_t_tkl |= type << 4; } + +/** + * @brief Get the token length of a CoAP over UDP (DTLS) packet + * @param[in] hdr CoAP over UDP header + * @return The size of the token in bytes + * + * @warning This API is super goofy. It assumes that the packet is valid + * and will read more than `sizeof(*hdr)` into the data `hdr` + * points to while crossing fingers hard. + * + * @deprecated This function was introduced to keep legacy code alive. + * Introducing new callers should be avoided. In the RX path an + * @ref coap_pkt_t will be available, so that you can call + * @ref coap_get_token instead. In the TX path the token was + * added by us, so we really should know. + */ +static inline size_t coap_hdr_get_token_len(const coap_hdr_t *hdr) +{ + const uint8_t *buf = (const void *)hdr; + /* Regarding use unnamed magic numbers 13 and 269: + * - If token length is < 13 it fits into TKL field (4 bit) + * - If token length is < 269 it fits into 8-bit extended TKL field + * - Otherwise token length goes into 16-bit extended TKL field. + * + * (Not using named constants here, as RFC 8974 also has no names for those + * magic numbers.) + * + * See: https://www.rfc-editor.org/rfc/rfc8974#name-extended-token-length-tkl-f + */ + switch (coap_hdr_tkl_ext_len(hdr)) { + case 0: + return hdr->ver_t_tkl & 0xf; + case 1: + return buf[sizeof(coap_hdr_t)] + 13; + case 2: + return byteorder_bebuftohs(buf + sizeof(coap_hdr_t)) + 269; + } + + return 0; +} + +/** + * @brief Get the header length of a CoAP packet. + * + * @warning This API is super goofy. It assumes that the packet is valid + * and will read more than `sizeof(*hdr)` into the data `hdr` + * points to while crossing fingers hard. + * + * @deprecated This function was introduced to keep legacy code alive. + * Introducing new callers should be avoided. In the RX path an + * @ref coap_pkt_t will be available, so that you can call + * @ref coap_get_total_hdr_len instead. In the TX path the header + * was created by us (e.g. using @ref coap_build_hdr which returns + * the header size), so we really should know already. + */ +static inline size_t coap_hdr_len(const coap_hdr_t *hdr) +{ + return sizeof(*hdr) + coap_hdr_tkl_ext_len(hdr) + coap_hdr_get_token_len(hdr); +} /**@}*/ /** diff --git a/sys/net/application_layer/gcoap/gcoap.c b/sys/net/application_layer/gcoap/gcoap.c index 10efb6a215..6f36a13594 100644 --- a/sys/net/application_layer/gcoap/gcoap.c +++ b/sys/net/application_layer/gcoap/gcoap.c @@ -1454,10 +1454,9 @@ static ssize_t _cache_build_response(nanocoap_cache_entry_t *ce, coap_pkt_t *pdu static void _copy_hdr_from_req_memo(coap_pkt_t *pdu, gcoap_request_memo_t *memo) { - coap_pkt_t req_pdu; - - req_pdu.hdr = gcoap_request_memo_get_hdr(memo); - memcpy(pdu->hdr, req_pdu.hdr, coap_get_total_hdr_len(&req_pdu)); + const coap_hdr_t *hdr = gcoap_request_memo_get_hdr(memo); + size_t hdr_len = coap_hdr_len(hdr); + memcpy(pdu->hdr, hdr, hdr_len); } static void _receive_from_cache_cb(void *ctx)