safety comments

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<i32> {
+    // 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<Child<'_>>,
     pg: Option<nix::unistd::Pid>,
     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);
         };
     }