net: coap: Fix response matching algorithm #84160
Conversation
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 <[email protected]>
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 <[email protected]>
| id = coap_header_get_id(response); | ||
| tkl = coap_header_get_token(response, token); | ||
|
|
||
| if (type == COAP_TYPE_ACK || type == COAP_TYPE_RESET) { | ||
| piggybacked = true; |
There was a problem hiding this comment.
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
| if (memcmp(r->token, token, r->tkl) != 0) { | ||
| continue; | ||
| } | ||
| } |
There was a problem hiding this comment.
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.
| @@ -1817,23 +1819,44 @@ struct coap_reply *coap_response_received( | |||
| return NULL; | |||
| } | |||
There was a problem hiding this comment.
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.
| if (type == COAP_TYPE_ACK || type == COAP_TYPE_RESET) { | ||
| piggybacked = true; | ||
| } | ||
|
|
||
| for (i = 0, r = replies; i < len; i++, r++) { | ||
| int age; | ||
|
|
||
| if ((r->id == 0U) && (r->tkl == 0U)) { |
There was a problem hiding this comment.
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
| { .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 }, |
There was a problem hiding this comment.
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 */ | ||
| { .id = 100, .type = COAP_TYPE_CON, .match = NULL }, | ||
| /* Separate reply, no token 2 */ | ||
| { .id = 101, .type = COAP_TYPE_CON, .match = NULL }, |
There was a problem hiding this comment.
separate response, MID must be ignored, empty token matches first reply.
| /* In a separate response, just the tokens of the | ||
| * response and original request MUST match. | ||
| */ | ||
| if ((r->tkl == 0) || (r->tkl != tkl) || |
There was a problem hiding this comment.
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))) {
Add test case for matching pending replies with received responses.
Cover corner cases that are failing with the current implementation.
Fix response matching algorithm
Fixes #84100