wasmtime serve: Allow inheriting sockets from system manager - #14294
wasmtime serve: Allow inheriting sockets from system manager#14294simolus3 wants to merge 5 commits into
wasmtime serve: Allow inheriting sockets from system manager#14294Conversation
github-actions
Bot
added
the
wasmtime:docs
alexcrichton
left a comment
alexcrichton
left a comment
There was a problem hiding this comment.
Thanks! I've got some suggestions to use rustix instead of libc since that helps us with more typesafe/safe wrappers, and I've additionally put a comment about moving the handshake-style protocol to just one function called in serve.rs. Otherwise looks reasonable to me.
Also, sorry I forgot this earlier, but can you add a test for this as well? It's difficult to keep functionality like this working if it doesn't have any tests.
| let fd = unsafe { | ||
| // Safety: This is called first in main and we checked the PID, so we have exclusive | ||
| // access to this fd. | ||
| libc::fcntl(fd, libc::F_SETFD, libc::FD_CLOEXEC); |
There was a problem hiding this comment.
Could this use rustix::io::fcntl_setfd with error handling?
|
|
||
| fn is_tcp_socket(fd: &OwnedFd) -> bool { | ||
| let mut stat: MaybeUninit<libc::stat> = MaybeUninit::uninit(); | ||
| if unsafe { libc::fstat(fd.as_raw_fd(), stat.as_mut_ptr()) } != 0 { |
| let sa_family = unsafe { | ||
| let mut sockaddr: MaybeUninit<libc::sockaddr> = MaybeUninit::uninit(); | ||
| let mut len = mem::size_of::<libc::sockaddr>() as libc::c_uint; | ||
|
|
||
| if libc::getsockname(fd.as_raw_fd(), sockaddr.as_mut_ptr(), &mut len) != 0 { | ||
| return false; | ||
| } | ||
| sockaddr.assume_init().sa_family | ||
| } as libc::c_int; |
There was a problem hiding this comment.
Could this use rustix::net::getsockname for a safe alternative?
| let mut socket_type: libc::c_int = 0; | ||
| unsafe { | ||
| let mut type_len = mem::size_of_val(&socket_type) as libc::c_uint; | ||
|
|
||
| if libc::getsockopt( | ||
| fd.as_raw_fd(), | ||
| libc::SOL_SOCKET, | ||
| libc::SO_TYPE, | ||
| std::ptr::from_mut(&mut socket_type).cast(), | ||
| &mut type_len, | ||
| ) != 0 | ||
| { | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| socket_type == libc::SOCK_STREAM |
There was a problem hiding this comment.
Could this use rustix::net::sockopt::socket_type for a safe alternative?
|
|
||
| #[allow(unreachable_code, reason = "empty enum with all features disabled")] | ||
| fn main() -> Result<()> { | ||
| setup(); |
There was a problem hiding this comment.
Personally I would prefer to keep all the serve.rs-related code in serve.rs and avoid extra abstractions here (which have more #[cfg] which is more to validate, etc). I think it'd be fine to have init_inherited_fds marked unsafe, that returns the inherited sockets, and the // SAFETY ... comment on the call in serve.rs is high enough in the function that it's clear that nothing happens inbetween. That should keep everything contained without the need for more #[cfg] without compromising on safety.
3 participants
This adds the
--listenfdoption towasmtime serve. When it's enabled andwasmtimeis launched from a systemd socket unit, we use the socket created by systemd instead of creating a new one. The main benefit of this is efficiency: Thewasmtimeprocess is only started when the first client connects. Additionally, this allows sandboxing the process in a private network namespace (since the only socket it will use is inherited).To test this, the systemfd utility may be convenient:
This also removes the old
-S listenfdoption: The only place using that option was a check that errors when it's set.Closes #14289. As suggested in that thread, I've also linked the used systemd protocol for this in a comment.