Upgrade rust/crates/command-fds to 0.2.0 am: bd13c06228 am: aa492a37b4 am: bf9eff4d9b am: 12b66c5267

Original change: https://android-review.googlesource.com/c/platform/external/rust/crates/command-fds/+/1740263

Change-Id: I0acf66d83c2afb0e0965f3be87e22025f28a7d6a

7 files changed, 144 insertions, 66 deletions

diff --git a/.cargo_vcs_info.json b/.cargo_vcs_info.json
index 0ecc3ba..111ab26 100644
--- a/.cargo_vcs_info.json
+++ b/.cargo_vcs_info.json
@@ -1,5 +1,5 @@
 {
   "git": {
-    "sha1": "21ffd14511b405853cd2468d6e8f7f9d17135154"
+    "sha1": "5fe7a67415bd0559fee13392880fd6e4aceeb922"
   }
 }
diff --git a/.gitignore b/.gitignore
index 96ef6c0..97e6d83 100644
--- a/.gitignore
+++ b/.gitignore
@@ -1,2 +1,3 @@
 /target
 Cargo.lock
+.vscode/
diff --git a/Android.bp b/Android.bp
index aaaf601..42b61df 100644
--- a/Android.bp
+++ b/Android.bp
@@ -1,8 +1,6 @@
 // This file is generated by cargo2android.py --config cargo2android.json.
 // Do not modify this file as changes will be overridden on upgrade.
 
-
-
 package {
     default_applicable_licenses: ["external_rust_crates_command-fds_license"],
 }
@@ -39,11 +37,11 @@ rust_library {
 // dependent_library ["feature_list"]
 //   bitflags-1.2.1 "default"
 //   cfg-if-1.0.0
-//   libc-0.2.95 "default,extra_traits,std"
+//   libc-0.2.97 "default,extra_traits,std"
 //   nix-0.20.0
 //   proc-macro2-1.0.27 "default,proc-macro"
 //   quote-1.0.9 "default,proc-macro"
-//   syn-1.0.72 "clone-impls,default,derive,parsing,printing,proc-macro,quote"
+//   syn-1.0.73 "clone-impls,default,derive,parsing,printing,proc-macro,quote"
 //   thiserror-1.0.25
 //   thiserror-impl-1.0.25
 //   unicode-xid-0.2.2 "default"
diff --git a/Cargo.toml b/Cargo.toml
index bcc6791..f6f0c8c 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -13,7 +13,7 @@
 [package]
 edition = "2018"
 name = "command-fds"
-version = "0.1.0"
+version = "0.2.0"
 authors = ["Andrew Walbran <qwandor@google.com>"]
 description = "A library for passing arbitrary file descriptors when spawning child processes."
 keywords = ["command", "process", "child", "subprocess", "fd"]
diff --git a/Cargo.toml.orig b/Cargo.toml.orig
index ea39b35..9de1d83 100644
--- a/Cargo.toml.orig
+++ b/Cargo.toml.orig
@@ -1,6 +1,6 @@
 [package]
 name = "command-fds"
-version = "0.1.0"
+version = "0.2.0"
 edition = "2018"
 authors = ["Andrew Walbran <qwandor@google.com>"]
 license = "Apache-2.0"
diff --git a/METADATA b/METADATA
index dde0db4..35b27e9 100644
--- a/METADATA
+++ b/METADATA
@@ -7,13 +7,13 @@ third_party {
   }
   url {
     type: ARCHIVE
-    value: "https://static.crates.io/crates/command-fds/command-fds-0.1.0.crate"
+    value: "https://static.crates.io/crates/command-fds/command-fds-0.2.0.crate"
   }
-  version: "0.1.0"
+  version: "0.2.0"
   license_type: NOTICE
   last_upgrade_date {
     year: 2021
-    month: 5
-    day: 4
+    month: 6
+    day: 21
   }
 }
diff --git a/src/lib.rs b/src/lib.rs
index 8ce1071..2435983 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -48,7 +48,7 @@
 //! child.wait().unwrap();
 //! ```
 
-use nix::fcntl::{fcntl, FcntlArg};
+use nix::fcntl::{fcntl, FcntlArg, FdFlag};
 use nix::unistd::dup2;
 use std::cmp::max;
 use std::io::{self, ErrorKind};
@@ -74,14 +74,22 @@ pub struct FdMappingCollision;
 
 /// Extension to add file descriptor mappings to a [`Command`].
 pub trait CommandFdExt {
-    /// Adds the given set of file descriptor to the command.
+    /// Adds the given set of file descriptors to the command.
     ///
-    /// Calling this more than once on the same command may result in unexpected behaviour.
-    fn fd_mappings(&mut self, mappings: Vec<FdMapping>) -> Result<(), FdMappingCollision>;
+    /// Warning: Calling this more than once on the same command, or attempting to run the same
+    /// command more than once after calling this, may result in unexpected behaviour.
+    fn fd_mappings(&mut self, mappings: Vec<FdMapping>) -> Result<&mut Self, FdMappingCollision>;
+
+    /// Adds the given set of file descriptors to be passed on to the child process when the command
+    /// is run.
+    fn preserved_fds(&mut self, fds: Vec<RawFd>) -> &mut Self;
 }
 
 impl CommandFdExt for Command {
-    fn fd_mappings(&mut self, mappings: Vec<FdMapping>) -> Result<(), FdMappingCollision> {
+    fn fd_mappings(
+        &mut self,
+        mut mappings: Vec<FdMapping>,
+    ) -> Result<&mut Self, FdMappingCollision> {
         // Validate that there are no conflicting mappings to the same child FD.
         let mut child_fds: Vec<RawFd> = mappings.iter().map(|mapping| mapping.child_fd).collect();
         child_fds.sort_unstable();
@@ -91,15 +99,29 @@ impl CommandFdExt for Command {
         }
 
         // Register the callback to apply the mappings after forking but before execing.
+        // Safety: `map_fds` will not allocate, so it is safe to call from this hook.
         unsafe {
-            self.pre_exec(move || map_fds(&mappings));
+            // If the command is run more than once, and hence this closure is called multiple
+            // times, then `mappings` may be in an incorrect state. It would be good if we could
+            // reset it to the initial state somehow, or use something else for saving the temporary
+            // mappings.
+            self.pre_exec(move || map_fds(&mut mappings, &child_fds));
         }
 
-        Ok(())
+        Ok(self)
+    }
+
+    fn preserved_fds(&mut self, fds: Vec<RawFd>) -> &mut Self {
+        unsafe {
+            self.pre_exec(move || preserve_fds(&fds));
+        }
+
+        self
     }
 }
 
-fn map_fds(mappings: &[FdMapping]) -> io::Result<()> {
+// This function must not do any allocation, as it is called from the pre_exec hook.
+fn map_fds(mappings: &mut [FdMapping], child_fds: &[RawFd]) -> io::Result<()> {
     if mappings.is_empty() {
         // No need to do anything, and finding first_unused_fd would fail.
         return Ok(());
@@ -116,30 +138,37 @@ fn map_fds(mappings: &[FdMapping]) -> io::Result<()> {
         + 1;
 
     // If any parent FDs conflict with child FDs, then first duplicate them to a temporary FD which
-    // is clear of either range.
-    let child_fds: Vec<RawFd> = mappings.iter().map(|mapping| mapping.child_fd).collect();
-    let mappings = mappings
-        .iter()
-        .map(|mapping| {
-            Ok(if child_fds.contains(&mapping.parent_fd) {
-                let temporary_fd =
-                    fcntl(mapping.parent_fd, FcntlArg::F_DUPFD_CLOEXEC(first_safe_fd))?;
-                FdMapping {
-                    parent_fd: temporary_fd,
-                    child_fd: mapping.child_fd,
-                }
-            } else {
-                mapping.to_owned()
-            })
-        })
-        .collect::<nix::Result<Vec<_>>>()
-        .map_err(nix_to_io_error)?;
+    // is clear of either range. Mappings to the same FD are fine though, we can handle them by just
+    // removing the FD_CLOEXEC flag from the existing (parent) FD.
+    for mapping in mappings.iter_mut() {
+        if child_fds.contains(&mapping.parent_fd) && mapping.parent_fd != mapping.child_fd {
+            mapping.parent_fd = fcntl(mapping.parent_fd, FcntlArg::F_DUPFD_CLOEXEC(first_safe_fd))
+                .map_err(nix_to_io_error)?;
+        }
+    }
 
     // Now we can actually duplicate FDs to the desired child FDs.
     for mapping in mappings {
-        // This closes child_fd if it is already open as something else, and clears the FD_CLOEXEC
-        // flag on child_fd.
-        dup2(mapping.parent_fd, mapping.child_fd).map_err(nix_to_io_error)?;
+        if mapping.child_fd == mapping.parent_fd {
+            // Remove the FD_CLOEXEC flag, so the FD will be kept open when exec is called for the
+            // child.
+            fcntl(mapping.parent_fd, FcntlArg::F_SETFD(FdFlag::empty()))
+                .map_err(nix_to_io_error)?;
+        } else {
+            // This closes child_fd if it is already open as something else, and clears the
+            // FD_CLOEXEC flag on child_fd.
+            dup2(mapping.parent_fd, mapping.child_fd).map_err(nix_to_io_error)?;
+        }
+    }
+
+    Ok(())
+}
+
+fn preserve_fds(fds: &[RawFd]) -> io::Result<()> {
+    for fd in fds {
+        // Remove the FD_CLOEXEC flag, so the FD will be kept open when exec is called for the
+        // child.
+        fcntl(*fd, FcntlArg::F_SETFD(FdFlag::empty())).map_err(nix_to_io_error)?;
     }
 
     Ok(())
@@ -174,8 +203,8 @@ mod tests {
         let mut command = Command::new("ls");
 
         // The same mapping can't be included twice.
-        assert_eq!(
-            command.fd_mappings(vec![
+        assert!(command
+            .fd_mappings(vec![
                 FdMapping {
                     child_fd: 4,
                     parent_fd: 5,
@@ -184,13 +213,12 @@ mod tests {
                     child_fd: 4,
                     parent_fd: 5,
                 },
-            ]),
-            Err(FdMappingCollision)
-        );
+            ])
+            .is_err());
 
         // Mapping two different FDs to the same FD isn't allowed either.
-        assert_eq!(
-            command.fd_mappings(vec![
+        assert!(command
+            .fd_mappings(vec![
                 FdMapping {
                     child_fd: 4,
                     parent_fd: 5,
@@ -199,9 +227,8 @@ mod tests {
                     child_fd: 4,
                     parent_fd: 6,
                 },
-            ]),
-            Err(FdMappingCollision)
-        );
+            ])
+            .is_err());
     }
 
     #[test]
@@ -211,7 +238,20 @@ mod tests {
         let mut command = Command::new("ls");
         command.arg("/proc/self/fd");
 
-        assert_eq!(command.fd_mappings(vec![]), Ok(()));
+        assert!(command.fd_mappings(vec![]).is_ok());
+
+        let output = command.output().unwrap();
+        expect_fds(&output, &[0, 1, 2, 3], 0);
+    }
+
+    #[test]
+    fn none_preserved() {
+        setup();
+
+        let mut command = Command::new("ls");
+        command.arg("/proc/self/fd");
+
+        command.preserved_fds(vec![]);
 
         let output = command.output().unwrap();
         expect_fds(&output, &[0, 1, 2, 3], 0);
@@ -226,19 +266,33 @@ mod tests {
 
         let file = File::open("testdata/file1.txt").unwrap();
         // Map the file an otherwise unused FD.
-        assert_eq!(
-            command.fd_mappings(vec![FdMapping {
+        assert!(command
+            .fd_mappings(vec![FdMapping {
                 parent_fd: file.as_raw_fd(),
                 child_fd: 5,
-            },]),
-            Ok(())
-        );
+            },])
+            .is_ok());
 
         let output = command.output().unwrap();
         expect_fds(&output, &[0, 1, 2, 3, 5], 0);
     }
 
     #[test]
+    fn one_preserved() {
+        setup();
+
+        let mut command = Command::new("ls");
+        command.arg("/proc/self/fd");
+
+        let file = File::open("testdata/file1.txt").unwrap();
+        let file_fd = file.as_raw_fd();
+        command.preserved_fds(vec![file_fd]);
+
+        let output = command.output().unwrap();
+        expect_fds(&output, &[0, 1, 2, 3, file_fd], 0);
+    }
+
+    #[test]
     fn swap_mappings() {
         setup();
 
@@ -250,8 +304,8 @@ mod tests {
         let fd1 = file1.as_raw_fd();
         let fd2 = file2.as_raw_fd();
         // Map files to each other's FDs, to ensure that the temporary FD logic works.
-        assert_eq!(
-            command.fd_mappings(vec![
+        assert!(command
+            .fd_mappings(vec![
                 FdMapping {
                     parent_fd: fd1,
                     child_fd: fd2,
@@ -260,9 +314,8 @@ mod tests {
                     parent_fd: fd2,
                     child_fd: fd1,
                 },
-            ]),
-            Ok(())
-        );
+            ])
+            .is_ok(),);
 
         let output = command.output().unwrap();
         // Expect one more Fd for the /proc/self/fd directory. We can't predict what number it will
@@ -271,6 +324,33 @@ mod tests {
     }
 
     #[test]
+    fn one_to_one_mapping() {
+        setup();
+
+        let mut command = Command::new("ls");
+        command.arg("/proc/self/fd");
+
+        let file1 = File::open("testdata/file1.txt").unwrap();
+        let file2 = File::open("testdata/file2.txt").unwrap();
+        let fd1 = file1.as_raw_fd();
+        // Map file1 to the same FD it currently has, to ensure the special case for that works.
+        assert!(command
+            .fd_mappings(vec![FdMapping {
+                parent_fd: fd1,
+                child_fd: fd1,
+            }])
+            .is_ok());
+
+        let output = command.output().unwrap();
+        // Expect one more Fd for the /proc/self/fd directory. We can't predict what number it will
+        // be assigned, because 3 might or might not be taken already by fd1 or fd2.
+        expect_fds(&output, &[0, 1, 2, fd1], 1);
+
+        // Keep file2 open until the end, to ensure that it's not passed to the child.
+        drop(file2);
+    }
+
+    #[test]
     fn map_stdin() {
         setup();
 
@@ -278,13 +358,12 @@ mod tests {
 
         let file = File::open("testdata/file1.txt").unwrap();
         // Map the file to stdin.
-        assert_eq!(
-            command.fd_mappings(vec![FdMapping {
+        assert!(command
+            .fd_mappings(vec![FdMapping {
                 parent_fd: file.as_raw_fd(),
                 child_fd: 0,
-            },]),
-            Ok(())
-        );
+            },])
+            .is_ok());
 
         let output = command.output().unwrap();
         assert!(output.status.success());