[wasi-sockets] reconnecting UDP socket unexpectedly changes local address

[dicej]

I've been adding WASIp2 support to mio and triaging failures in its test suite. This test is failing, which I tracked down to wasmtime-wasi spuriously changing the local address of a UDP socket when wasi:sockets/udp#udp-socket.stream is called a second time on the same socket (equivalent to calling connect(2) a second time in POSIX). The test fails because it uses the old local address, not realizing the socket suddenly has a new local address.

Here's a minimal test case:

// udp.rs
use std::{error::Error, net::UdpSocket};

fn main() -> Result<(), Box<dyn Error>> {
    let socket1 = UdpSocket::bind("127.0.0.1:0")?;
    let socket2 = UdpSocket::bind("127.0.0.1:0")?;

    let address1 = socket1.local_addr()?;
    let address2 = socket2.local_addr()?;

    println!(
        "before:\n  \
         socket1 local address: {address1:?}\n  \
         socket2 local address: {address2:?}"
    );

    socket1.connect(address1)?;
    socket1.connect(address2)?;
    socket2.connect(address1)?;

    println!(
        "after:\n  \
         socket1 local address: {address1:?} remote address: {remote1:?}\n  \
         socket2 local address: {address2:?} remote address: {remote2:?}",
        address1 = socket1.local_addr()?,
        address2 = socket2.local_addr()?,
        remote1 = socket1.peer_addr()?,
        remote2 = socket2.peer_addr()?
    );

    let data = b"foobar";
    socket1.send(data)?;

    let mut buffer = [0u8; 20];
    let count = socket2.recv(&mut buffer)?;

    assert_eq!(data, &buffer[..count]);

    println!("success!");

    Ok(())
}

If you build and run that natively on e.g. Linux using rustc udp.rs && ./udp, you'll see it succeed, with socket1's local address remaining unchanged before and after the UdpSocket::connect calls.

If you build and run for WASIp2 using rustc --target=wasm32-wasip2 udp.rs && wasmtime run -Sudp,inherit-network udp.wasm, you'll see it hang indefinitely due to socket1's local address changing due to the second UdpSocket::connect call.

While digging into this, I found that wasmtime-wasi proactively disconnects the socket by calling rustix::net::connect_unspec prior to reconnecting. When I comment out that code, the issue is fixed:

diff --git a/crates/wasi/src/p2/host/udp.rs b/crates/wasi/src/p2/host/udp.rs
index 1ef8350399..5dd426e736 100644
--- a/crates/wasi/src/p2/host/udp.rs
+++ b/crates/wasi/src/p2/host/udp.rs
@@ -71,9 +71,9 @@ impl udp::HostUdpSocket for WasiSocketsCtxView<'_> {
         //   if there isn't a disconnect in between.

         // Step #1: Disconnect
-        if socket.is_connected() {
-            socket.disconnect()?;
-        }
+        // if socket.is_connected() {
+        //     socket.disconnect()?;
+        // }

         // Step #2: (Re)connect
         if let Some(connect_addr) = remote_address {
diff --git a/crates/wasi/src/sockets/udp.rs b/crates/wasi/src/sockets/udp.rs
index 482c6aa090..ba1e93f18d 100644
--- a/crates/wasi/src/sockets/udp.rs
+++ b/crates/wasi/src/sockets/udp.rs
@@ -162,10 +162,10 @@ impl UdpSocket {
         //   if there isn't a disconnect in between.

         // Step #1: Disconnect
-        if let UdpState::Connected(..) = self.udp_state {
-            udp_disconnect(&self.socket)?;
-            self.udp_state = UdpState::Bound;
-        }
+        // if let UdpState::Connected(..) = self.udp_state {
+        //     udp_disconnect(&self.socket)?;
+        //     self.udp_state = UdpState::Bound;
+        // }
         // Step #2: (Re)connect
         connect(&self.socket, &addr).map_err(|error| match error {
             Errno::AFNOSUPPORT => ErrorCode::InvalidArgument, // See `udp_bind` implementation.

However, per the surrounding comments, that code is there for a reason, so I assume that removing it isn't the right solution. @badeend this seems to be related to #7411, so perhaps you have thoughts?

[badeend]

Every call to udp-socket::connect is allowed to change the local socket address. This is documented on the method itself: https://github.com/WebAssembly/WASI/blob/41d0de898ec7568e032129156a3bc567d7ebea5d/proposals/sockets/wit-0.3.0-draft/types.wit#L547-L550

AFAIK, this is standard POSIX behavior. Are you sure you're testing exactly the same code as the mio test? Because the mio test you linked reads: (no local address rebinding)

    socket1.connect(address2).unwrap();
    socket2.connect(address1).unwrap();
    socket3.connect(address1).unwrap();

whereas the code snippet you posted rebinds socket1:

    socket1.connect(address1)?;
    socket1.connect(address2)?;
    socket2.connect(address1)?;

[dicej]

Are you sure you're testing exactly the same code as the mio test?

No, it's not exactly the same; I reduced the mio test to a minimal reproduction. Note that the mio test does call connect twice on socket1: scroll down to https://github.com/tokio-rs/mio/blob/66ac9fab79bf191218488c4f35c99d13935b7e12/tests/udp_socket.rs#L442

[dicej]

BTW, if POSIX allows connect to change the local address, then the mio test is just wrong, and I can submit a PR to fix it (e.g. by calling socket3.connect(socket1.local_addr().unwrap()).unwrap() after the socket1.connect(address3).unwrap() line). It would help to have a document to reference to justify that, though, since apparently WASI is the only platform where the test currently fails.

[dicej]

(e.g. by calling socket3.connect(socket1.local_addr().unwrap()).unwrap() after the socket1.connect(address3).unwrap() line)

Oof, actually not sure that would work, because then socket3's local address could change. So I guess I'm not even sure how I would fix it, since every call to connect could invalidate the local address previously used.

[alexcrichton]

@badeend do you have further links for where the clause you linked in the WASI docs come from? Naively to me that seems pretty surprising that connect can arbitrarily change the locally bound address, so I'd be curious to read up more there. Is this something, for example, where platforms disagree on behavior and WASI is specifying a sort of least-common-denominator semantics?