From c7c7837e1d534f209c3c7fc0fafa92893a936b1a Mon Sep 17 00:00:00 2001 From: Robert Lubos Date: Fri, 17 Jan 2025 12:26:46 +0100 Subject: [PATCH 1/2] tests: net: coap: Add test case for matching pending replies Add test case for matching pending replies with received responses. Cover corner cases that are failing with the current implementation. Signed-off-by: Robert Lubos --- tests/net/lib/coap/src/main.c | 83 +++++++++++++++++++++++++++++++++++ 1 file changed, 83 insertions(+) diff --git a/tests/net/lib/coap/src/main.c b/tests/net/lib/coap/src/main.c index 10c7cef1d2c025..e385b75b5b2a8a 100644 --- a/tests/net/lib/coap/src/main.c +++ b/tests/net/lib/coap/src/main.c @@ -1855,4 +1855,87 @@ ZTEST(coap, test_age_is_newer) "Max age should be marked as newer"); } +struct test_coap_request { + uint16_t id; + uint8_t token[COAP_TOKEN_MAX_LEN]; + uint8_t tkl; + enum coap_msgtype type; + struct coap_reply *match; +}; + +static int reply_cb(const struct coap_packet *response, + struct coap_reply *reply, + const struct sockaddr *from) +{ + return 0; +} + +ZTEST(coap, test_response_matching) +{ + struct coap_reply matches[] = { + { .id = 100, .reply = reply_cb }, + { .id = 101, .token = { 1, 2, 3, 4 }, .tkl = 4, .reply = reply_cb }, + }; + struct test_coap_request test_responses[] = { + /* Piggybacked ACK, no token */ + { .id = 100, .type = COAP_TYPE_ACK, .match = &matches[0] }, + /* Piggybacked ACK, matching token */ + { .id = 101, .type = COAP_TYPE_ACK, .match = &matches[1], + .token = { 1, 2, 3, 4 }, .tkl = 4 }, + /* Piggybacked ACK, token mismatch */ + { .id = 101, .type = COAP_TYPE_ACK, .match = NULL, + .token = { 1, 2, 3, 3 }, .tkl = 4 }, + /* Piggybacked ACK, token mismatch 2 */ + { .id = 100, .type = COAP_TYPE_ACK, .match = NULL, + .token = { 1, 2, 3, 4 }, .tkl = 4 }, + /* Piggybacked ACK, token mismatch 3 */ + { .id = 101, .type = COAP_TYPE_ACK, .match = NULL }, + /* Piggybacked ACK, id mismatch */ + { .id = 102, .type = COAP_TYPE_ACK, .match = NULL, + .token = { 1, 2, 3, 4 }, .tkl = 4 }, + /* Separate reply, no token */ + { .id = 100, .type = COAP_TYPE_CON, .match = NULL }, + /* Separate reply, no token 2 */ + { .id = 101, .type = COAP_TYPE_CON, .match = NULL }, + /* Separate reply, matching token 1 */ + { .id = 101, .type = COAP_TYPE_CON, .match = &matches[1], + .token = { 1, 2, 3, 4 }, .tkl = 4 }, + /* Separate reply, matching token 2 */ + { .id = 102, .type = COAP_TYPE_CON, .match = &matches[1], + .token = { 1, 2, 3, 4 }, .tkl = 4 }, + /* Separate reply, token mismatch */ + { .id = 101, .type = COAP_TYPE_CON, .match = NULL, + .token = { 1, 2, 3, 3 }, .tkl = 4 }, + /* Separate reply, token mismatch 2 */ + { .id = 100, .type = COAP_TYPE_CON, .match = NULL, + .token = { 1, 2, 3, 3 }, .tkl = 4 }, + }; + + ARRAY_FOR_EACH_PTR(test_responses, response) { + struct coap_packet response_pkt = { 0 }; + struct sockaddr from = { 0 }; + struct coap_reply *match; + uint8_t data[64]; + int ret; + + ret = coap_packet_init(&response_pkt, data, sizeof(data), COAP_VERSION_1, + response->type, response->tkl, response->token, + COAP_RESPONSE_CODE_CONTENT, response->id); + zassert_ok(ret, "Failed to initialize test packet: %d", ret); + + match = coap_response_received(&response_pkt, &from, matches, + ARRAY_SIZE(matches)); + if (response->match != NULL) { + zassert_not_null(match, "Did not found a response match when expected"); + zassert_equal_ptr(response->match, match, + "Wrong response match, test %d match %d", + response - test_responses, match - matches); + } else { + zassert_is_null(match, + "Found unexpected response match, test %d match %d", + response - test_responses, match - matches); + } + } +} + ZTEST_SUITE(coap, NULL, NULL, NULL, NULL, NULL); From e64b48183c29db48da5ce55a32f5b7524981b3a4 Mon Sep 17 00:00:00 2001 From: Robert Lubos Date: Fri, 17 Jan 2025 15:54:30 +0100 Subject: [PATCH 2/2] net: coap: Fix response matching algorithm The algorithm for matching request with response was incorrect, which could lead to false matches (for example if request had a token, and piggybacked reply had no token but matching message ID only, that would still be counted as a match). This commit fixes it. The request/reply matching is implemented based on RFC now, with separate conditions for piggybacked/separate responses. Signed-off-by: Robert Lubos --- subsys/net/lib/coap/coap.c | 35 +++++++++++++++++++++++++++++------ 1 file changed, 29 insertions(+), 6 deletions(-) diff --git a/subsys/net/lib/coap/coap.c b/subsys/net/lib/coap/coap.c index 70f2610adf0227..93852fd29298ef 100644 --- a/subsys/net/lib/coap/coap.c +++ b/subsys/net/lib/coap/coap.c @@ -1808,6 +1808,8 @@ struct coap_reply *coap_response_received( { struct coap_reply *r; uint8_t token[COAP_TOKEN_MAX_LEN]; + bool piggybacked = false; + uint8_t type; uint16_t id; uint8_t tkl; size_t i; @@ -1817,9 +1819,14 @@ struct coap_reply *coap_response_received( return NULL; } + type = coap_header_get_type(response); id = coap_header_get_id(response); tkl = coap_header_get_token(response, token); + if (type == COAP_TYPE_ACK || type == COAP_TYPE_RESET) { + piggybacked = true; + } + for (i = 0, r = replies; i < len; i++, r++) { int age; @@ -1827,13 +1834,29 @@ struct coap_reply *coap_response_received( continue; } - /* Piggybacked must match id when token is empty */ - if ((r->id != id) && (tkl == 0U)) { - continue; - } + if (piggybacked) { + /* In a piggybacked response, the Message ID of the + * Confirmable request and the Acknowledgment MUST + * match, and the tokens of the response and original + * request MUST match. + */ + if ((r->id != id || (r->tkl != tkl))) { + continue; + } - if (tkl > 0 && memcmp(r->token, token, tkl)) { - continue; + if (r->tkl > 0) { + if (memcmp(r->token, token, r->tkl) != 0) { + continue; + } + } + } else { + /* In a separate response, just the tokens of the + * response and original request MUST match. + */ + if ((r->tkl == 0) || (r->tkl != tkl) || + (memcmp(r->token, token, r->tkl) != 0)) { + continue; + } } age = coap_get_option_int(response, COAP_OPTION_OBSERVE);