From 20d7bd315c11632d2e524a383dfc82aab4d9adc8 Mon Sep 17 00:00:00 2001 From: Marian Buschsieweke Date: Thu, 9 Jan 2025 21:45:10 +0100 Subject: [PATCH] sys/net/gcoap: get rid of API abuse Calling `coap_get_token()` and `coap_get_token_length()` on an (mostly) uninitialized `coap_pkt_t` did work so far due to implementation details matching the expectations, but this is not backed up by any API contract. This fixes the API abuse by introducing and using a new API that does read a token and token length from a CoAP over UDP packet out of a buffer. This now provides the behavior expected by the caller and commits to it via API contract. Co-authored-by: mguetschow Co-authored-by: benpicco --- sys/include/net/nanocoap.h | 23 +++++++++++++++++++++++ sys/net/application_layer/gcoap/gcoap.c | 13 +++++-------- 2 files changed, 28 insertions(+), 8 deletions(-) diff --git a/sys/include/net/nanocoap.h b/sys/include/net/nanocoap.h index b77b557b5bad..f72694d7de17 100644 --- a/sys/include/net/nanocoap.h +++ b/sys/include/net/nanocoap.h @@ -355,6 +355,7 @@ struct _coap_request_ctx { /* 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); +static inline size_t coap_hdr_get_token(const coap_hdr_t *hdr); /** * @brief Get resource path associated with a CoAP request @@ -757,6 +758,28 @@ static inline size_t coap_hdr_get_token_len(const coap_hdr_t *hdr) return 0; } +/** + * @brief Get the Token of a CoAP over UDP (DTLS) packet + * @param[in] hdr CoAP over UDP header + * @return The CoAP Token inside the packet that @p hdr belongs to + * + * @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 void * coap_hdr_get_token(coap_hdr_t *hdr) +{ + uint8_t *token = (void *)hdr; + token += sizeof(*hdr) + coap_hdr_tkl_ext_len(hdr); + return token; +} + /** * @brief Get the header length of a CoAP packet. * diff --git a/sys/net/application_layer/gcoap/gcoap.c b/sys/net/application_layer/gcoap/gcoap.c index 6f36a13594e2..aa2841f98caf 100644 --- a/sys/net/application_layer/gcoap/gcoap.c +++ b/sys/net/application_layer/gcoap/gcoap.c @@ -945,17 +945,13 @@ static int _find_resource(gcoap_socket_type_t tl_type, static gcoap_request_memo_t* _find_req_memo_by_token(const sock_udp_ep_t *remote, const uint8_t *token, size_t tkl) { - /* no need to initialize struct; we only care about buffer contents below */ - coap_pkt_t memo_pdu_data; - coap_pkt_t *memo_pdu = &memo_pdu_data; - for (int i = 0; i < CONFIG_GCOAP_REQ_WAITING_MAX; i++) { if (_coap_state.open_reqs[i].state == GCOAP_MEMO_UNUSED) { continue; } gcoap_request_memo_t *memo = &_coap_state.open_reqs[i]; - memo_pdu->hdr = gcoap_request_memo_get_hdr(memo); + coap_hdr_t *hdr = gcoap_request_memo_get_hdr(memo); /* verbose debug to catch bugs with request/response matching */ #if SOCK_HAS_IPV4 @@ -972,11 +968,12 @@ static gcoap_request_memo_t* _find_req_memo_by_token(const sock_udp_ep_t *remote tkl); #endif - if (coap_get_token_len(memo_pdu) != tkl) { - DEBUG("Token length mismatch %u\n", coap_get_token_len(memo_pdu)); + size_t memo_tkl = coap_hdr_get_token_len(hdr); + if (memo_tkl != tkl) { + DEBUG("Token length mismatch %" PRIuSIZE "\n", memo_tkl); continue; } - const uint8_t *memo_token = coap_get_token(memo_pdu); + const uint8_t *memo_token = coap_hdr_get_token(hdr); if (memcmp(token, memo_token, tkl)) { DEBUG("Token mismatch 0x%02x%02x%02x%02x%02x%02x%02x%02x\n", memo_token[0], memo_token[1], memo_token[2], memo_token[3],