Linux: UDP listen: Receive broadcasts (mimic Windows behavior) #149

kgraefe · 2023-03-22T07:48:49Z

On Windows, when listening on a specific IP address, you also receive corresponding subnet broadcasts and global broadcasts (Ipv4Addr::BROADCAST) received on the interface matching the IP.

In order to mimic that behavior on Linux, this change adds ingress filters to UDP/Listen so one can listen on Ipv4Addr::UNSPECIFIED but drop any packet that is not received on the specified interface or any of the specified IP addresses.

@lemunozm As I said in #142 I'm not sure if that's too much code for a too specific problem to be merged upstream. But I'll leave it here for you to decide.

lemunozm · 2023-03-22T08:12:24Z

Ok, I'll check later, but I think your PR goes with the message-io philosophy of abstracting different platform behaviors

lemunozm · 2023-03-22T08:15:18Z

I mean, with this change, a Linux user can achieve the same behavior that a Windows user, isn't it?

kgraefe · 2023-03-22T08:19:46Z

I mean, with this change, a Linux user can achieve the same behavior that a Windows user, isn't it?

yes

lemunozm · 2023-03-22T08:21:49Z

I'm ok with this PR if we extend the option docs to highlight just that.

kgraefe · 2023-03-22T08:27:27Z

but in the application I still have to use compile conditions:

// Listen on any IP but filter to only allow packets incoming on the specific interface and
// with one of the following destination addresses:
// - the interface's local IP,
// - the corresponding subnet broadcast IP, or
// - the global broadcast IP
#[cfg(target_os = "linux")]
let (socket_id, _) = node_handler.network().listen_with(
    TransportListen::Udp(UdpListenConfig {
        broadcast: true,
        ingress_addresses: vec![
            interface.local_ip.into(),
            interface.broadcast_ip.into(),
            Ipv4Addr::BROADCAST.into(),
        ],
        ingress_interface: Some(interface.index),
        ..Default::default()
    }),
    SocketAddrV4::new(Ipv4Addr::UNSPECIFIED, port),
)?;

// Windows does all of the above automatically for sockets listening on the interface's
// local IP.
#[cfg(target_os = "windows")]
let (socket_id, _) = node_handler.network().listen_with(
    TransportListen::Udp(UdpListenConfig {
        broadcast: true,
        ..Default::default()
    }),
    SocketAddrV4::new(interface.local_ip, port),
)?;

kgraefe · 2023-03-22T08:31:39Z

where interface is of a custom type with the information retrieved from nix::ifaddrs::getifaddrs() and nix::net::if_::if_nametoindex()

kgraefe · 2023-03-22T08:39:36Z

maybe I can rework that to a flag mimic_windows or sth. (I don't have a good name yet) where message-io tries to figure out the ingress filter parameters internally

lemunozm · 2023-03-22T09:21:48Z

I really like the last idea! In that way, your app code will be really simple; that is what we looking for.

If the nix dependency is significantly big or annoying, it could exist under a feature. But for now, I think we can live with it.

lemunozm · 2023-03-22T09:22:37Z

By the way, feel free to add your project to the README open-source list if you want.

Signed-off-by: Konrad Gräfe <[email protected]>

kgraefe · 2023-03-23T08:14:26Z

I'm done :-)

lemunozm · 2023-03-23T09:39:38Z

src/adapters/udp.rs

+    #[cfg(target_os = "linux")]
+    fn accept(&self, mut accept_remote: impl FnMut(AcceptedType<'_, Self::Remote>)) {


I get a little lost here; why the Linux behavior changes so much now?

The reason is that I need to use nix::sys::socket::recvmsg() in order to pass the control_buffer which receives the ingress IP address. And as that's the core of the function it affects the rest of the function as well.

Ok, I see.

I'm thinking, that maybe we can use the non-linux accept() for all platforms (as before), and only if the ingress config is enabled and we are in linux run this accept() as a special case.

The main advantage of this is that the normal use case (99% of time) will use the usual accept() which are probably a little bit faster and simple to debug/understand if, in the future, any user finds an issue.

lemunozm · 2023-03-23T09:40:50Z

src/adapters/udp.rs

@@ -231,6 +324,66 @@ impl Local for LocalResource {
        }
    }

+    #[cfg(target_os = "linux")]
+    fn accept(&self, mut accept_remote: impl FnMut(AcceptedType<'_, Self::Remote>)) {
+        let mut input_buffer = vec![0; MAX_LOCAL_PAYLOAD_LEN];


Do we really need this to be a vector? Could it be an array to avoid allocations?

probably not, I will change that

lemunozm

Looks good! I left a last minor comment change, but not mandatory.

lemunozm · 2023-03-23T15:46:37Z

src/adapters/udp.rs

+        #[cfg(target_os = "linux")]
+        if !self.ingress_addresses.is_empty() {
+            self.accept_filtered(accept_remote);
+            return
+        }


lemunozm · 2023-03-23T15:50:23Z

src/adapters/udp.rs

+        #[cfg(target_os = "linux")]
+        match addr {
+            SocketAddr::V4 { .. } => {
+                socket::setsockopt(socket.as_raw_fd(), sockopt::Ipv4PacketInfo, &true)?
+            }
+            SocketAddr::V6 { .. } => {
+                socket::setsockopt(socket.as_raw_fd(), sockopt::Ipv6RecvPacketInfo, &true)?
+            }
+        }
+
+        #[cfg(target_os = "linux")]
+        let mut ingress_addresses = vec![];
+
+        #[cfg(target_os = "linux")]
+        if config.receive_broadcasts {


Minor last change, don't mandatory. Could the match addr { part and the let mut ingress_addresses = vec![]; be under the if config.receive_broadcasts { condition?

Yes for the match addr and it should be as the setsockopt()s are not necessary otherwise

No for the ingress_addresses as it has to land in the LocalResource later on. Do you think it would be better to use an Option<Vec<..>> here? Currently I'm using ingress_addresses.is_empty() as an indicator whether or not the option has been set. I'm not sure if it's better allocation-wise, but semantically it probably would be.

I have no strong opinion but it makes sense!

This option mimics the Windows behavior to also receive broadcasts on sockets bound to a specific IP. Signed-off-by: Konrad Gräfe <[email protected]>

lemunozm · 2023-03-23T16:47:05Z

Thanks for this @kgraefe !

kgraefe changed the title ~~Udp listen ingress filter~~ Linux: UDP listen: Add ingress filters Mar 22, 2023

kgraefe force-pushed the udp-listen-ingress-filter branch 2 times, most recently from 6df858a to 97eb83a Compare March 22, 2023 12:53

udp: listen: Rename broadcast config entry to send_broadcasts

480a7fb

Signed-off-by: Konrad Gräfe <[email protected]>

kgraefe force-pushed the udp-listen-ingress-filter branch from 97eb83a to c785981 Compare March 22, 2023 12:54

lemunozm reviewed Mar 23, 2023

View reviewed changes

kgraefe changed the title ~~Linux: UDP listen: Add ingress filters~~ Linux: UDP listen: Receive broadcasts (mimic Windows behavior) Mar 23, 2023

kgraefe force-pushed the udp-listen-ingress-filter branch from c785981 to 1e62022 Compare March 23, 2023 12:18

kgraefe requested a review from lemunozm March 23, 2023 12:19

lemunozm approved these changes Mar 23, 2023

View reviewed changes

udp: listen: linux: Implement receive_broadcasts option

b746f33

This option mimics the Windows behavior to also receive broadcasts on sockets bound to a specific IP. Signed-off-by: Konrad Gräfe <[email protected]>

kgraefe force-pushed the udp-listen-ingress-filter branch from 1e62022 to b746f33 Compare March 23, 2023 16:19

kgraefe requested a review from lemunozm March 23, 2023 16:20

lemunozm approved these changes Mar 23, 2023

View reviewed changes

lemunozm merged commit e6c01b8 into lemunozm:master Mar 23, 2023

kgraefe deleted the udp-listen-ingress-filter branch March 23, 2023 19:05

lemunozm mentioned this pull request Mar 24, 2023

Release v0.15.0 #142

Closed

3 tasks

lemunozm assigned kgraefe Mar 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Linux: UDP listen: Receive broadcasts (mimic Windows behavior) #149

Linux: UDP listen: Receive broadcasts (mimic Windows behavior) #149

kgraefe commented Mar 22, 2023 •

edited

Loading

lemunozm commented Mar 22, 2023

lemunozm commented Mar 22, 2023

kgraefe commented Mar 22, 2023

lemunozm commented Mar 22, 2023

kgraefe commented Mar 22, 2023

kgraefe commented Mar 22, 2023

kgraefe commented Mar 22, 2023

lemunozm commented Mar 22, 2023 •

edited

Loading

lemunozm commented Mar 22, 2023 •

edited

Loading

kgraefe commented Mar 23, 2023

lemunozm Mar 23, 2023

kgraefe Mar 23, 2023 •

edited

Loading

lemunozm Mar 23, 2023

lemunozm Mar 23, 2023

kgraefe Mar 23, 2023 •

edited

Loading

lemunozm left a comment

lemunozm Mar 23, 2023

lemunozm Mar 23, 2023

kgraefe Mar 23, 2023

lemunozm Mar 23, 2023

lemunozm commented Mar 23, 2023

		#[cfg(target_os = "linux")]
		fn accept(&self, mut accept_remote: impl FnMut(AcceptedType<'_, Self::Remote>)) {

Linux: UDP listen: Receive broadcasts (mimic Windows behavior) #149

Linux: UDP listen: Receive broadcasts (mimic Windows behavior) #149

Conversation

kgraefe commented Mar 22, 2023 • edited Loading

lemunozm commented Mar 22, 2023

lemunozm commented Mar 22, 2023

kgraefe commented Mar 22, 2023

lemunozm commented Mar 22, 2023

kgraefe commented Mar 22, 2023

kgraefe commented Mar 22, 2023

kgraefe commented Mar 22, 2023

lemunozm commented Mar 22, 2023 • edited Loading

lemunozm commented Mar 22, 2023 • edited Loading

kgraefe commented Mar 23, 2023

lemunozm Mar 23, 2023

Choose a reason for hiding this comment

kgraefe Mar 23, 2023 • edited Loading

Choose a reason for hiding this comment

lemunozm Mar 23, 2023

Choose a reason for hiding this comment

lemunozm Mar 23, 2023

Choose a reason for hiding this comment

kgraefe Mar 23, 2023 • edited Loading

Choose a reason for hiding this comment

lemunozm left a comment

Choose a reason for hiding this comment

lemunozm Mar 23, 2023

Choose a reason for hiding this comment

lemunozm Mar 23, 2023

Choose a reason for hiding this comment

kgraefe Mar 23, 2023

Choose a reason for hiding this comment

lemunozm Mar 23, 2023

Choose a reason for hiding this comment

lemunozm commented Mar 23, 2023

kgraefe commented Mar 22, 2023 •

edited

Loading

lemunozm commented Mar 22, 2023 •

edited

Loading

lemunozm commented Mar 22, 2023 •

edited

Loading

kgraefe Mar 23, 2023 •

edited

Loading

kgraefe Mar 23, 2023 •

edited

Loading