Skip to content

Commit

Permalink
Use std::os::unix::net::UnixStream::pair instead of rustix::net::sock…
Browse files Browse the repository at this point in the history
…etpair

Assuming UnixStream in std is more widely used / battle tested, and has
more eyes on it.

Also, UnixStream is a higher-level wrapper, even though we are just
directly converting it into OwnedFd for now.

Signed-off-by: Teoh Han Hui <[email protected]>
  • Loading branch information
teohhanhui committed May 18, 2024
1 parent e35c7da commit 098baa2
Show file tree
Hide file tree
Showing 3 changed files with 18 additions and 21 deletions.
2 changes: 1 addition & 1 deletion crates/krun/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ env_logger = { workspace = true, features = ["auto-color", "humantime", "unstabl
krun-sys = { workspace = true, features = [] }
log = { workspace = true, features = ["kv"] }
nix = { workspace = true, features = ["user"] }
rustix = { workspace = true, features = ["net", "process", "std", "use-libc-auxv"] }
rustix = { workspace = true, features = ["process", "std", "use-libc-auxv"] }
utils = { workspace = true, features = [] }

[features]
Expand Down
2 changes: 1 addition & 1 deletion crates/krun/src/bin/krun.rs
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ fn main() -> Result<()> {
.context("Failed to connect to `passt`")?
.into()
} else {
start_passt().context("Failed to start `passt`")?
start_passt().context("Failed to start `passt`")?.into()
};
// SAFETY: `passt_fd` is an `OwnedFd` and consumed to prevent closing on drop.
// See https://doc.rust-lang.org/std/io/index.html#io-safety
Expand Down
35 changes: 16 additions & 19 deletions crates/krun/src/net.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use std::{
fmt,
os::{
fd::{AsRawFd, IntoRawFd, OwnedFd},
fd::{AsRawFd, IntoRawFd},
unix::net::UnixStream,
},
path::Path,
Expand All @@ -10,10 +10,7 @@ use std::{

use anyhow::{Context, Result};
use log::debug;
use rustix::{
io::dup,
net::{socketpair, AddressFamily, SocketFlags, SocketType},
};
use rustix::io::dup;

#[derive(Copy, Clone, Eq, PartialEq, Debug)]
pub enum NetMode {
Expand All @@ -28,23 +25,23 @@ where
Ok(UnixStream::connect(passt_socket_path)?)
}

pub fn start_passt() -> Result<OwnedFd> {
let (parent_fd, child_fd) = socketpair(
AddressFamily::UNIX,
SocketType::STREAM,
// SAFETY: The child process should not inherit `parent_fd`.
SocketFlags::CLOEXEC,
None,
)?;

// SAFETY: The parent process should not keep `child_fd` open. It is an `OwnedFd` so it will be
// closed on drop.
pub fn start_passt() -> Result<UnixStream> {
// SAFETY: The child process should not inherit the file descriptor of `parent_socket`.
// There is no documented guarantee of this, but the implementation as of writing atomically
// sets `SOCK_CLOEXEC`.
// See https://github.com/rust-lang/rust/blob/1.77.0/library/std/src/sys/pal/unix/net.rs#L124-L125
// See https://github.com/rust-lang/rust/issues/47946#issuecomment-364776373
let (parent_socket, child_socket) =
UnixStream::pair().context("Failed to create socket pair for `passt` child process")?;

// SAFETY: The parent process should not keep the file descriptor of `child_socket` open.
// It is a `UnixStream` so the file descriptor will be closed on drop.
// See https://doc.rust-lang.org/std/io/index.html#io-safety
//
// The `dup` call clears the `FD_CLOEXEC` flag on the new `child_fd`, which should be inherited
// by the child process.
let child_fd =
dup(child_fd).context("Failed to duplicate file descriptor for `passt` child process")?;
let child_fd = dup(child_socket)
.context("Failed to duplicate file descriptor for `passt` child process")?;

debug!(fd = child_fd.as_raw_fd(); "passing fd to passt");

Expand All @@ -59,5 +56,5 @@ pub fn start_passt() -> Result<OwnedFd> {
return Err(err).context("Failed to execute `passt` as child process");
}

Ok(parent_fd)
Ok(parent_socket)
}

0 comments on commit 098baa2

Please sign in to comment.