clean up, fix, and document some unsafety

3 files changed, 46 insertions, 7 deletions

diff --git a/src/command.rs b/src/command.rs
index 926e097..f5357d2 100644
--- a/src/command.rs
+++ b/src/command.rs
@@ -15,17 +15,42 @@ impl Command for std::process::Command {
         let pt_fd = pty.pt().as_raw_fd();
         let pts_fd = pts.as_raw_fd();
 
-        self.stdin(unsafe { std::process::Stdio::from_raw_fd(pts_fd) })
-            .stdout(unsafe { std::process::Stdio::from_raw_fd(pts_fd) })
-            .stderr(unsafe { std::process::Stdio::from_raw_fd(pts_fd) });
+        let stdin = nix::unistd::dup(pts_fd).map_err(Error::SpawnNix)?;
+        let stdout = nix::unistd::dup(pts_fd).map_err(Error::SpawnNix)?;
+        let stderr = nix::unistd::dup(pts_fd).map_err(Error::SpawnNix)?;
 
+        // safe because the fds are valid (otherwise pty.pts() or dup() would
+        // have returned an Err and we would have exited early) and are not
+        // owned by any other structure (since dup() returns a fresh copy of
+        // the file descriptor), allowing from_raw_fd to take ownership of it.
+        self.stdin(unsafe { std::process::Stdio::from_raw_fd(stdin) })
+            .stdout(unsafe { std::process::Stdio::from_raw_fd(stdout) })
+            .stderr(unsafe { std::process::Stdio::from_raw_fd(stderr) });
+
+        // safe because the only thing this function does is call close(),
+        // which is an async-signal-safe function
         unsafe {
             self.pre_exec(move || {
+                // in the parent, destructors will handle closing these file
+                // descriptors (other than pt, used by the parent to
+                // communicate with the child) when the function ends, but in
+                // the child, we end by calling exec(), which doesn't call
+                // destructors.
+
                 // XXX unwrap
                 nix::unistd::close(pt_fd)
                     .map_err(|e| e.as_errno().unwrap())?;
                 nix::unistd::close(pts_fd)
                     .map_err(|e| e.as_errno().unwrap())?;
+                // at this point, stdin/stdout/stderr have already been
+                // reopened as fds 0/1/2 in the child, so we can (and should)
+                // close the originals
+                nix::unistd::close(stdin)
+                    .map_err(|e| e.as_errno().unwrap())?;
+                nix::unistd::close(stdout)
+                    .map_err(|e| e.as_errno().unwrap())?;
+                nix::unistd::close(stderr)
+                    .map_err(|e| e.as_errno().unwrap())?;
                 Ok(())
             });
         }
diff --git a/src/error.rs b/src/error.rs
index 97fd8a0..9b65d6a 100644
--- a/src/error.rs
+++ b/src/error.rs
@@ -11,6 +11,9 @@ pub enum Error {
 
     #[error("error spawning subprocess")]
     Spawn(#[source] std::io::Error),
+
+    #[error("error spawning subprocess")]
+    SpawnNix(#[source] nix::Error),
 }
 
 pub type Result<T> = std::result::Result<T, Error>;
diff --git a/src/pty.rs b/src/pty.rs
index a557f56..39ffd70 100644
--- a/src/pty.rs
+++ b/src/pty.rs
@@ -63,6 +63,12 @@ impl Pty {
             nix::pty::ptsname_r(&pt).map_err(Error::CreatePty)?.into();
 
         let pt_fd = pt.into_raw_fd();
+
+        // safe because posix_openpt (or the previous functions operating on
+        // the result) would have returned an Err (causing us to return early)
+        // if the file descriptor was invalid. additionally, into_raw_fd gives
+        // up ownership over the file descriptor, allowing the newly created
+        // File object to take full ownership.
         let pt = unsafe { std::fs::File::from_raw_fd(pt_fd) };
 
         Ok(Self { pt, ptsname })
@@ -81,10 +87,15 @@ impl Pty {
         let fd = fh.as_raw_fd();
         if let Some(size) = size {
             let size = size.into();
-            unsafe {
-                set_term_size(fd, &size as *const nix::pty::Winsize)
-                    .map_err(Error::SetTermSize)?;
-            }
+
+            // safe because fd is guaranteed to be valid here (or else the
+            // previous open call would have returned an error and exited the
+            // function early), and size is guaranteed to be initialized
+            // because it's a normal rust value, and nix::pty::Winsize is a
+            // repr(C) struct with the same layout as `struct winsize` from
+            // sys/ioctl.h.
+            unsafe { set_term_size(fd, &size as *const nix::pty::Winsize) }
+                .map_err(Error::SetTermSize)?;
         }
         Ok(fh)
     }