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.

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

gcoap: Do not send responses from multicast addresses #19272

Merged

Conversation

chrysn
Copy link
Member

@chrysn chrysn commented Feb 13, 2023

Contribution description

Since #18026, CoAP requests to multicast addresses (eg. ff02::1) came back from that exact address, which Linux rightfully just drops.

The fix uses the existing multicast check from #17978 (thanks @benpicco for making me write this as dedicated function, I just had to generalize it removing one struct layer), and foregoes setting the source address when responding to multicasts.

Testing procedure

  • Run the gcoap example
  • Send a CoAP request to a multicast address RIOT listens to, eg. ./aiocoap-client coap://'[ff02::1%tapbr0]'/.well-known/core --non

Before, this got no response (while you see it arrive on wireshark). After, you get a correct response with two lines of note:

WARNING:coap:Sending request to multicast via unicast request method
Response arrived from different address; base URI is coap://[fe80::3c63:beff:fe85:ca96%tapbr0]/.well-known/core

(The former is aiocoap telling us that we're not using the nonexistent multicast API so it's really more of an anycast, the latter is useful factual information).

@chrysn chrysn added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Area: CoAP Area: Constrained Application Protocol implementations labels Feb 13, 2023
@github-actions github-actions bot added Area: network Area: Networking Area: sys Area: System labels Feb 13, 2023
@riot-ci
Copy link

riot-ci commented Feb 13, 2023

Murdock results

✔️ PASSED

ac3a9cd gcoap: Do not send responses from multicast addresses

Success Failures Total Runtime
6865 0 6865 13m:00s

Artifacts

@chrysn
Copy link
Member Author

chrysn commented Feb 13, 2023

Thanks

bors merge

bors bot added a commit that referenced this pull request Feb 13, 2023
18257: drivers/wdt: add periph_wdt_auto_start for early watchdog r=benpicco a=benpicco



19272: gcoap: Do not send responses from multicast addresses r=chrysn a=chrysn

### Contribution description

Since #18026, CoAP requests to multicast addresses (eg. `ff02::1`) came back from that exact address, which Linux rightfully just drops.

The fix uses the existing multicast check from #17978 (thanks `@benpicco` for making me write this as dedicated function, I just had to generalize it removing one struct layer), and foregoes setting the source address when responding to multicasts.

### Testing procedure

* Run the gcoap example
* Send a CoAP request to a multicast address RIOT listens to, eg. `./aiocoap-client coap://'[ff02::1%tapbr0]'/.well-known/core --non`

Before, this got no response (while you see it arrive on wireshark). After, you get a correct response with two lines of note:

```
WARNING:coap:Sending request to multicast via unicast request method
Response arrived from different address; base URI is coap://[fe80::3c63:beff:fe85:ca96%tapbr0]/.well-known/core
```

(The former is aiocoap telling us that we're not using the nonexistent multicast API so it's really more of an anycast, the latter is useful factual information).

Co-authored-by: Benjamin Valentin <benjamin.valentin@ml-pa.com>
Co-authored-by: chrysn <chrysn@fsfe.org>
sock_udp_aux_tx_t aux_out = {
.flags = SOCK_AUX_SET_LOCAL,
.local = aux_in.local,
};
if (_ep_is_multicast(&aux_in.local)) {
aux_out_ptr = NULL;
Copy link
Member

Choose a reason for hiding this comment

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

Probably I'm too late, but a comment about the semantics of setting this pointer to NULL here would have been nice.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, given it takes several indirections before this wins up in a function where one can read the documentation.

Comment added, let's see how mad bors will get at me for throwing logs before its merge train.

@chrysn chrysn force-pushed the coap-set-unicast-source-address branch from 96cabee to ac3a9cd Compare February 13, 2023 20:46
@bors
Copy link
Contributor

bors bot commented Feb 13, 2023

Canceled.

@chrysn
Copy link
Member Author

chrysn commented Feb 13, 2023

bors merge

@bors
Copy link
Contributor

bors bot commented Feb 13, 2023

🕐 Waiting for PR status (GitHub check) to be set, probably by CI. Bors will automatically try to run when all required PR statuses are set.

@bors
Copy link
Contributor

bors bot commented Feb 13, 2023

GitHub status checks took too long to complete, so bors is giving up. You can adjust bors configuration to have it wait longer if you like.

@benpicco
Copy link
Contributor

bors merge

@bors
Copy link
Contributor

bors bot commented Feb 14, 2023

Build succeeded:

@bors bors bot merged commit f78c375 into RIOT-OS:master Feb 14, 2023
@chrysn chrysn deleted the coap-set-unicast-source-address branch February 15, 2023 09:33
@MrKevinWeiss MrKevinWeiss added this to the Release 2023.04 milestone Apr 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: CoAP Area: Constrained Application Protocol implementations Area: network Area: Networking Area: sys Area: System CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants