gcoap: Forego IP address comparison in memo finding of multicasts

[chrysn]

Contribution description

gcoap's _find_req_memo compares tokens and IP addresses. That's generally a good thing, but when requests are sent to a multicast address, it is not right (RFC7252 has an explicit exception there).

There may be other areas of multicast support that are lacking, but this is a small enhancement step.

Testing procedure

Have a local CoAP server that responds to multicasts (aiocoap-fileserver will do, even though that might be a bug there, gotta check out whether it's supposed to do that by default).

$ make -C examples/gcoap
> coap get ff01::2 5683 /.well-known/core
</>;ct=40

Previously, the request timed out.

Note that good multicast behavior would be to accept several servers' responses, and (in the command line client) to show the address of the server where the response came from, both is left for further work.

Issues/PRs references

Brought up by ad6sh in https://forum.riot-os.org/t/coap-resource-discovery/3599; the testing instructions are his.

[chrysn]

I should also point out that this only detects IPv6 multicast addresses; IPv4 multicast (broadcast) is harder to detect, and out of scope here. (I prefer not to sink time into a protocol that should long have been sunset).

[benpicco]

Want to move that to a helper function? sock_udp_ep_is_multicast()
(Or just a local one - _ep_is_multicast())

[chrysn]

Yes, that'd be a good idea; will update when I get back to it.

[benpicco]

This just needs a tiny helper function to avoid cluttering the code like that

[benpicco]

How about this patch

diff --git a/sys/net/application_layer/gcoap/gcoap.c b/sys/net/application_layer/gcoap/gcoap.c
index e21f22211f..6c57929ebc 100644
--- a/sys/net/application_layer/gcoap/gcoap.c
+++ b/sys/net/application_layer/gcoap/gcoap.c
@@ -764,6 +764,12 @@ static int _find_resource(gcoap_socket_type_t tl_type,
     return ret;
 }
 
+static bool _memo_ep_is_multicast(const gcoap_request_memo_t *memo)
+{
+    return memo->remote_ep.family == AF_INET6 &&
+           ipv6_addr_is_multicast((const ipv6_addr_t *)&memo->remote_ep.addr.ipv6);
+}
+
 /*
  * Finds the memo for an outstanding request within the _coap_state.open_reqs
  * array. Matches on remote endpoint and token.
@@ -803,15 +809,10 @@ static void _find_req_memo(gcoap_request_memo_t **memo_ptr, coap_pkt_t *src_pdu,
             }
         } else if (coap_get_token_len(memo_pdu) == cmplen) {
             memo_pdu->token = coap_hdr_data_ptr(memo_pdu->hdr);
-            if ((memcmp(src_pdu->token, memo_pdu->token, cmplen) == 0)
-                    && (
-                     sock_udp_ep_equal(&memo->remote_ep, remote) ||
-                     /* Multicast addresses are not considered in matching responses */
-                     (
-                      memo->remote_ep.family == AF_INET6 &&
-                      ipv6_addr_is_multicast((const ipv6_addr_t *)&memo->remote_ep.addr.ipv6)
-                     )
-                     )) {
+            if ((memcmp(src_pdu->token, memo_pdu->token, cmplen) == 0) &&
+                (sock_udp_ep_equal(&memo->remote_ep, remote) ||
+                 /* Multicast addresses are not considered in matching responses */
+                 _memo_ep_is_multicast(memo))) {
                 *memo_ptr = memo;
                 break;
             }

[chrysn]

Thanks, I lost track of this one. Added your patch, and squashed onto current master to get rid of a merge conflict due to get_token now being an accessor.