Skip to content
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.

Sign up for GitHub

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

net: coap: Fix response matching algorithm #84160

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+112 −6
Open

net: coap: Fix response matching algorithm #84160

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c7c7837
tests: net: coap: Add test case for matching pending replies
rlubos Jan 17, 2025
e64b481
net: coap: Fix response matching algorithm
rlubos Jan 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 29 additions & 6 deletions subsys/net/lib/coap/coap.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -1817,23 +1819,44 @@ struct coap_reply *coap_response_received(
return NULL;
}
Copy link

@boaks boaks Jan 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

	if (!is_empty_message(response) && coap_packet_is_request(response)) {
		/* Request can't be response */
		return NULL;
	}

The ! before is_empty_message is wrong. Both requests and empty messages can't be responses, see table above.


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;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A RST is always a empty message and no piggybacked response.

See RFC 7252, page 23, table 1

+----------+-----+-----+-----+-----+
|          | CON | NON | ACK | RST |
+----------+-----+-----+-----+-----+
| Request  | X   | X   | -   | -   |
| Response | X   | X   | X   | -   |
| Empty    | *   | -   | X   | X   |
+----------+-----+-----+-----+-----+
  Table 1: Usage of Message Types

}

for (i = 0, r = replies; i < len; i++, r++) {
int age;

if ((r->id == 0U) && (r->tkl == 0U)) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the message ID 0 used to indicate something special in the zephyr implementation?
if not, what is the purpose of this check?

FMPOV, it's wrong. I don't see this is related to RFC 7252.
Considering a separate response with empty token, assumptions about the used MIDs will fail. The other implementation may use 0 without considering any special processing, as I don't see one in RFC 7252

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;
}
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

if (piggybacked) {
   if ((r->id != id) {
      continue;
   }   
}

if ((r->tkl != tkl) {
   continue;
}   

if (r->tkl > 0) {
   if (memcmp(r->token, token, r->tkl) != 0) {
      continue;
   }
}

will use less code-duplicates.

} else {
/* In a separate response, just the tokens of the
* response and original request MUST match.
*/
if ((r->tkl == 0) || (r->tkl != tkl) ||
Copy link

@boaks boaks Jan 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the code-duplicates are intended, then the (r->tkl == 0) is wrong and must be removed. That will then also change the results of the tests, as I commended there.

if ((r->tkl != tkl) || ((r->tkl > 0) && (memcmp(r->token, token, r->tkl) != 0))) {

(memcmp(r->token, token, r->tkl) != 0)) {
continue;
}
}

age = coap_get_option_int(response, COAP_OPTION_OBSERVE);
Expand Down
83 changes: 83 additions & 0 deletions tests/net/lib/coap/src/main.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 },
Copy link

@boaks boaks Jan 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For separate replies, there is no relation in the MIDs!
Using different MIDs for this test, e.g. 1000, would show that.

I'm not common with the used test framework, but for me this piggybacked response matches the first reply ".id = 100," (empty token).

"no token" never applies, that case is called "empty token" and all rules for tokens applies for empty tokens as well.

/* Separate reply, no token 2 */
{ .id = 101, .type = COAP_TYPE_CON, .match = NULL },
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

separate response, MID must be ignored, empty token matches first reply.

/* 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);
Loading