From 83de263441105e669acf79498788e1b66c6e3945 Mon Sep 17 00:00:00 2001 From: Jesse Luehrs Date: Sat, 8 Jan 2022 08:21:16 -0500 Subject: safety comments --- src/pipeline/builtins/command.rs | 24 +++++++++++++++++++++--- src/pipeline/mod.rs | 12 +++++++++--- 2 files changed, 30 insertions(+), 6 deletions(-) diff --git a/src/pipeline/builtins/command.rs b/src/pipeline/builtins/command.rs index 2e6b4af..3a1dd91 100644 --- a/src/pipeline/builtins/command.rs +++ b/src/pipeline/builtins/command.rs @@ -143,6 +143,8 @@ impl Io { } self.fds.insert( 0, + // Safety: we just acquired stdin via into_raw_fd, which acquires + // ownership of the fd, so we are now the sole owner crate::mutex::new(unsafe { File::input(stdin.into_raw_fd()) }), ); } @@ -157,6 +159,8 @@ impl Io { } self.fds.insert( 1, + // Safety: we just acquired stdout via into_raw_fd, which acquires + // ownership of the fd, so we are now the sole owner crate::mutex::new(unsafe { File::output(stdout.into_raw_fd()) }), ); } @@ -171,6 +175,8 @@ impl Io { } self.fds.insert( 2, + // Safety: we just acquired stderr via into_raw_fd, which acquires + // ownership of the fd, so we are now the sole owner crate::mutex::new(unsafe { File::output(stderr.into_raw_fd()) }), ); } @@ -185,10 +191,14 @@ impl Io { let fd = redirect.dir.open(path).unwrap(); match redirect.dir { crate::parse::Direction::In => { + // Safety: we just opened fd, and nothing else has + // or can use it crate::mutex::new(unsafe { File::input(fd) }) } crate::parse::Direction::Out | crate::parse::Direction::Append => { + // Safety: we just opened fd, and nothing else has + // or can use it crate::mutex::new(unsafe { File::output(fd) }) } } @@ -240,7 +250,9 @@ impl Io { if let Some(stdin) = crate::mutex::unwrap(stdin) { let stdin = stdin.into_raw_fd(); if stdin != 0 { - // Safety: TODO this is likely unsafe + // Safety: we just acquired stdin via into_raw_fd, which + // acquires ownership of the fd, so we are now the sole + // owner cmd.stdin(unsafe { std::fs::File::from_raw_fd(stdin) }); self.fds.remove(&0); } @@ -250,7 +262,9 @@ impl Io { if let Some(stdout) = crate::mutex::unwrap(stdout) { let stdout = stdout.into_raw_fd(); if stdout != 1 { - // Safety: TODO this is likely unsafe + // Safety: we just acquired stdout via into_raw_fd, which + // acquires ownership of the fd, so we are now the sole + // owner cmd.stdout(unsafe { std::fs::File::from_raw_fd(stdout) }); self.fds.remove(&1); } @@ -260,7 +274,9 @@ impl Io { if let Some(stderr) = crate::mutex::unwrap(stderr) { let stderr = stderr.into_raw_fd(); if stderr != 2 { - // Safety: TODO this is likely unsafe + // Safety: we just acquired stderr via into_raw_fd, which + // acquires ownership of the fd, so we are now the sole + // owner cmd.stderr(unsafe { std::fs::File::from_raw_fd(stderr) }); self.fds.remove(&2); } @@ -284,12 +300,14 @@ pub enum File { } impl File { + // Safety: fd must not be owned by any other File object pub unsafe fn input(fd: std::os::unix::io::RawFd) -> Self { Self::In(async_std::io::BufReader::new( async_std::fs::File::from_raw_fd(fd), )) } + // Safety: fd must not be owned by any other File object pub unsafe fn output(fd: std::os::unix::io::RawFd) -> Self { Self::Out(async_std::fs::File::from_raw_fd(fd)) } diff --git a/src/pipeline/mod.rs b/src/pipeline/mod.rs index 5c6567e..fab6793 100644 --- a/src/pipeline/mod.rs +++ b/src/pipeline/mod.rs @@ -14,10 +14,11 @@ pub use command::{Child, Command}; mod prelude; pub async fn run() -> anyhow::Result { + // Safety: we don't create File instances for or read/write data on fds + // 0-4 anywhere else let stdin = unsafe { async_std::fs::File::from_raw_fd(0) }; let stdout = unsafe { async_std::fs::File::from_raw_fd(1) }; let stderr = unsafe { async_std::fs::File::from_raw_fd(2) }; - // Safety: we don't create File instances for fd 3 or 4 anywhere else let shell_read = unsafe { async_std::fs::File::from_raw_fd(3) }; let shell_write = unsafe { async_std::fs::File::from_raw_fd(4) }; cloexec(3)?; @@ -49,7 +50,7 @@ async fn run_with_env( let pipeline = crate::parse::ast::Pipeline::parse(env.pipeline().unwrap())?; let (children, pg) = spawn_children(pipeline, env, io)?; - let status = wait_children(children, pg, env, shell_write).await; + let status = wait_children(children, pg, env, io, shell_write).await; env.set_status(status); Ok(()) } @@ -114,6 +115,7 @@ async fn wait_children( children: Vec>, pg: Option, env: &Env, + io: &builtins::Io, shell_write: &async_std::fs::File, ) -> std::process::ExitStatus { enum Res { @@ -123,7 +125,11 @@ async fn wait_children( macro_rules! bail { ($e:expr) => { - eprintln!("nbsh: {}", $e); + // if writing to stderr is not possible, we still want to exit + // normally with a failure exit code + #[allow(clippy::let_underscore_drop)] + let _ = + io.write_stderr(format!("nbsh: {}\n", $e).as_bytes()).await; return std::process::ExitStatus::from_raw(1 << 8); }; } -- cgit v1.2.3-54-g00ecf