diff options
author | Joel Galenson <jgalenson@google.com> | 2021-06-21 23:18:40 +0000 |
---|---|---|
committer | Automerger Merge Worker <android-build-automerger-merge-worker@system.gserviceaccount.com> | 2021-06-21 23:18:40 +0000 |
commit | aa492a37b4f9e8fde2a251a12ee7cd88cccadd3f (patch) | |
tree | 62eec897fe804822895e61aeb64428836827b674 | |
parent | 713e6b824c42d702e9eeadc04aad1c9400d2f6f4 (diff) | |
parent | bd13c06228d65bfca078eacb24359c5af4b1c315 (diff) | |
download | command-fds-aa492a37b4f9e8fde2a251a12ee7cd88cccadd3f.tar.gz |
Upgrade rust/crates/command-fds to 0.2.0 am: bd13c06228
Original change: https://android-review.googlesource.com/c/platform/external/rust/crates/command-fds/+/1740263
Change-Id: Ifab56f2eb90b847c26f64ad73dd8d2e368f4b617
-rw-r--r-- | .cargo_vcs_info.json | 2 | ||||
-rw-r--r-- | .gitignore | 1 | ||||
-rw-r--r-- | Android.bp | 6 | ||||
-rw-r--r-- | Cargo.toml | 2 | ||||
-rw-r--r-- | Cargo.toml.orig | 2 | ||||
-rw-r--r-- | METADATA | 8 | ||||
-rw-r--r-- | src/lib.rs | 189 |
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" } } @@ -1,2 +1,3 @@ /target Cargo.lock +.vscode/ @@ -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" @@ -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" @@ -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 } } @@ -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()); |