wasi-sockets: Unify the p2 & p3 implementations#13934
Conversation
The shared socket code (crates/wasi/src/sockets/*) was modeled after the p2 interface, with p3 bindings papering over the differences, and some functionality duplicated between p2/* and p3/*. This has already caused bugs in the past (bytecodealliance#13631). Since the p3 interface is strictly a superset of p2, invert that relationship: the shared code becomes the canonical (p3-shaped) implementation, and p3's bindings are now a thin wrapper around it. All p2-only behavior & state moves into the p2 bindings layer instead (artificial restrictions like disallowing implicit binds, and the start/finish async bookkeeping). Other notable changes: - Each socket now stores its own copy of the WasiCtx's socket_addr_check at creation time, so all permission checks live in the shared socket code instead of being split across p2/p3. The p2 Network resource is now just an unforgeable capability token. - Removed the Bound state; whether a socket is bound is now determined lazily via getsockname/local_addr instead of tracked explicitly. This lets UdpSocket::send return a future that runs to completion independently, without needing to rendezvous back with the socket to update its state. - Updated SocketAddrUse: added TcpListen, TcpAccept, UdpReceive; replaced UdpConnect with UdpSend/UdpReceive checks, since connecting a UDP socket is just a local operation that sets the default remote address and does no I/O of its own. connect() now succeeds as long as either send or receive is permitted for the address. - Tcp/UdpSocket::connect now also check the implicit bind the OS performs when connecting an unbound socket, matching the check already done for explicit binds. Added test coverage for previously-untested behavior: - test_udp_disconnect_local_address (p2 & p3): disconnecting a UDP socket keeps it bound. - test_udp_send_receive_multiple (p2): bulk send/receive of multiple datagrams in a single call over a connected stream(). - test_udp_receive_after_connect (p2): datagrams from other peers queued before connecting are filtered out once connected.
alexcrichton
left a comment
alexcrichton
left a comment
There was a problem hiding this comment.
Thanks so much for this! This all sounds great to me and I think the refactoring's a good idea. Do you think long-term it would be possible to fold the utils.rs module into the tcp/udp modules directly? That seems a vestige of a time where it was a home of common functionality between p2/p3, but nowadays that should just be {Tcp,Udp}Socket. (Fine to happen in a follow-up PR of course)
| if finish { | ||
| return Poll::Ready(Ok(StreamResult::Cancelled)); | ||
| } |
There was a problem hiding this comment.
For this the handling of finish is a bit subtle, but the basic idea is that this continues to do everything below and then instead of returning Pending at the last second it returns Cancelled instead. That gives sort of a "final chance" to get everything completed
There was a problem hiding this comment.
My reasoning was that; if the guest cancels the read/write, there's no point in us to querying the OS again for something the guest isn't interested in anymore. Which is why I had the early return.
But I'm fine with giving it one last poll. See latest commit.
There was a problem hiding this comment.
Yeah the intent of the guest is a bit ambiguous because it could mean "I still want to try to finish, but clear out otherwise" vs "I'm totally uninterested in the result just clean up" which is conflated right now
For the things that are explicitly marked as There are also a bunch of functions that are shared between tcp & udp, like:
For those , I think the best we can do is move them into mod.rs. |
|
Makes sense yeah, not something worth proactively doing necessarily, just curious! |
2 participants
Currently, the shared socket code (
crates/wasi/src/sockets/*) is modeled after thep2 interface, with p3 bindings papering over the differences. Some
functionality is duplicated between the
p2/*andp3/*implementations. This hasalready caused bugs in the past.
Since the p3 interface is strictly a superset of p2, I think it makes more sense to invert that relationship. This PR updates the shared code to become the canonical (p3-shaped)
implementation with the p3's bindings now a thin wrapper around it. All
p2-only behavior & state is moved into the p2 bindings layer. Think of: artificial restrictions like disallowing implicit
binds, and the start/finish async bookkeeping.
Other notable changes:
WasiCtx'ssocket_addr_checkat creation time, allowing all permission checks to live inthe shared socket code instead of being split across p2/p3. The p2
Networkresource is now just an unforgeable capability token (itspermission-checking responsibilities are gone).
Boundstate; whether a socket is bound is now determinedlazily via
getsockname/local_addrinstead of tracked explicitly.This simplifies the implementation of
UdpSocket::send, allowing it to return a future that can run to completion independently, without needing to rendezvous back with the socket to update its state.SocketAddrUse:TcpListen,TcpAccept,UdpReceivechecks.UdpConnectwith calls toUdpSendandUdpReceive. On UDP sockets, "connecting" is just a local operation that sets the default remote address for future sends and receives. It does not actually do any I/O on its own.Tcp&UdpSocket::connectnow also issue a permissioncheck for the implicit bind that the OS performs when connecting an
unbound socket, matching the check already done for explicit binds. Following through the changes of Consult the host's
socket_addr_checkfor implicit binds #13677StdViewtype introduced in Reduce dependencies oncap-std-family crates #13872 infavour of direct usage ofBorrowedFdAdded test coverage for previously-untested behavior:
test_udp_disconnect_local_address(p2 & p3): disconnecting a UDPsocket should keep it bound. Exercises the removal of the explicit
Boundstate in favor of lazily-derived bound status.
test_udp_send_receive_multiple(p2): a singlesend/receivecallcan carry multiple datagrams at once (bulk send/receive over a
connected
stream()).test_udp_receive_after_connect(p2): datagrams from different peersqueued before a socket is connected are correctly filtered so that
stream(Some(addr))only returns datagrams from the provided address.All-in-all, the goal of this PR is to consolidate the socket code (even further) into a single place and make it less error-prone to maintain. I realize there's quite a bit of unannounced code movement here, so let me know what you think.