From 6bfb280189ef6960525f18364c1b4644a913f4ce Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Fri, 1 Feb 2019 19:53:32 +0100 Subject: [PATCH 1/8] deprecate before_exec in favor of unsafe pre_exec --- src/libstd/sys/unix/ext/process.rs | 26 ++++++++++++++++--- src/libstd/sys/unix/process/process_common.rs | 2 +- 2 files changed, 24 insertions(+), 4 deletions(-) diff --git a/src/libstd/sys/unix/ext/process.rs b/src/libstd/sys/unix/ext/process.rs index 0282aaae90923..f5fc26dc9cca6 100644 --- a/src/libstd/sys/unix/ext/process.rs +++ b/src/libstd/sys/unix/ext/process.rs @@ -45,12 +45,32 @@ pub trait CommandExt { /// like `malloc` or acquiring a mutex are not guaranteed to work (due to /// other threads perhaps still running when the `fork` was run). /// + /// This also means that all resources such as file descriptors and + /// memory-mapped regions got duplicated. It is your responsibility to make + /// sure that the closure does not violate library invariants by making + /// invalid use of these duplicates. + /// /// When this closure is run, aspects such as the stdio file descriptors and /// working directory have successfully been changed, so output to these /// locations may not appear where intended. + #[stable(feature = "process_pre_exec", since = "1.34.0")] + unsafe fn pre_exec(&mut self, f: F) -> &mut process::Command + where F: FnMut() -> io::Result<()> + Send + Sync + 'static; + + /// Schedules a closure to be run just before the `exec` function is + /// invoked. + /// + /// This method should be unsafe, so it got deprecated in favor of the + /// unsafe [`pre_exec`]. + /// + /// [`pre_exec`]: #tymethod.pre_exec #[stable(feature = "process_exec", since = "1.15.0")] + #[rustc_deprecated(since = "1.34.0", reason = "should be unsafe, use `pre_exec` instead")] fn before_exec(&mut self, f: F) -> &mut process::Command - where F: FnMut() -> io::Result<()> + Send + Sync + 'static; + where F: FnMut() -> io::Result<()> + Send + Sync + 'static + { + unsafe { self.pre_exec(f) } + } /// Performs all the required setup by this `Command`, followed by calling /// the `execvp` syscall. @@ -97,10 +117,10 @@ impl CommandExt for process::Command { self } - fn before_exec(&mut self, f: F) -> &mut process::Command + unsafe fn pre_exec(&mut self, f: F) -> &mut process::Command where F: FnMut() -> io::Result<()> + Send + Sync + 'static { - self.as_inner_mut().before_exec(Box::new(f)); + self.as_inner_mut().pre_exec(Box::new(f)); self } diff --git a/src/libstd/sys/unix/process/process_common.rs b/src/libstd/sys/unix/process/process_common.rs index 2c55813c5cd39..9975064ca655f 100644 --- a/src/libstd/sys/unix/process/process_common.rs +++ b/src/libstd/sys/unix/process/process_common.rs @@ -149,7 +149,7 @@ impl Command { &mut self.closures } - pub fn before_exec(&mut self, + pub unsafe fn pre_exec(&mut self, f: Box io::Result<()> + Send + Sync>) { self.closures.push(f); } From d48433d920ad27ab57a27f087bcdec79ab36bfdc Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Fri, 1 Feb 2019 19:57:06 +0100 Subject: [PATCH 2/8] also replace before_exec by pre_exec on redox --- src/libstd/sys/redox/ext/process.rs | 28 ++++++++++++++++++++++++---- src/libstd/sys/redox/process.rs | 2 +- src/libstd/sys/unix/ext/process.rs | 2 +- 3 files changed, 26 insertions(+), 6 deletions(-) diff --git a/src/libstd/sys/redox/ext/process.rs b/src/libstd/sys/redox/ext/process.rs index 941fba8755b4a..78917ea91886b 100644 --- a/src/libstd/sys/redox/ext/process.rs +++ b/src/libstd/sys/redox/ext/process.rs @@ -36,7 +36,7 @@ pub trait CommandExt { /// will be called and the spawn operation will immediately return with a /// failure. /// - /// # Notes + /// # Notes and Safety /// /// This closure will be run in the context of the child process after a /// `fork`. This primarily means that any modifications made to memory on @@ -45,12 +45,32 @@ pub trait CommandExt { /// like `malloc` or acquiring a mutex are not guaranteed to work (due to /// other threads perhaps still running when the `fork` was run). /// + /// This also means that all resources such as file descriptors and + /// memory-mapped regions got duplicated. It is your responsibility to make + /// sure that the closure does not violate library invariants by making + /// invalid use of these duplicates. + /// /// When this closure is run, aspects such as the stdio file descriptors and /// working directory have successfully been changed, so output to these /// locations may not appear where intended. + #[stable(feature = "process_pre_exec", since = "1.34.0")] + unsafe fn pre_exec(&mut self, f: F) -> &mut process::Command + where F: FnMut() -> io::Result<()> + Send + Sync + 'static; + + /// Schedules a closure to be run just before the `exec` function is + /// invoked. + /// + /// This method should be unsafe, so it got deprecated in favor of the + /// unsafe [`pre_exec`]. + /// + /// [`pre_exec`]: #tymethod.pre_exec #[stable(feature = "process_exec", since = "1.15.0")] + #[rustc_deprecated(since = "1.34.0", reason = "should be unsafe, use `pre_exec` instead")] fn before_exec(&mut self, f: F) -> &mut process::Command - where F: FnMut() -> io::Result<()> + Send + Sync + 'static; + where F: FnMut() -> io::Result<()> + Send + Sync + 'static + { + unsafe { self.pre_exec(f) } + } /// Performs all the required setup by this `Command`, followed by calling /// the `execvp` syscall. @@ -87,10 +107,10 @@ impl CommandExt for process::Command { self } - fn before_exec(&mut self, f: F) -> &mut process::Command + unsafe fn pre_exec(&mut self, f: F) -> &mut process::Command where F: FnMut() -> io::Result<()> + Send + Sync + 'static { - self.as_inner_mut().before_exec(Box::new(f)); + self.as_inner_mut().pre_exec(Box::new(f)); self } diff --git a/src/libstd/sys/redox/process.rs b/src/libstd/sys/redox/process.rs index 4199ab98cf17e..9b85fa41a0a4f 100644 --- a/src/libstd/sys/redox/process.rs +++ b/src/libstd/sys/redox/process.rs @@ -116,7 +116,7 @@ impl Command { self.gid = Some(id); } - pub fn before_exec(&mut self, + pub unsafe fn pre_exec(&mut self, f: Box io::Result<()> + Send + Sync>) { self.closures.push(f); } diff --git a/src/libstd/sys/unix/ext/process.rs b/src/libstd/sys/unix/ext/process.rs index f5fc26dc9cca6..7cc5e9945938d 100644 --- a/src/libstd/sys/unix/ext/process.rs +++ b/src/libstd/sys/unix/ext/process.rs @@ -36,7 +36,7 @@ pub trait CommandExt { /// will be called and the spawn operation will immediately return with a /// failure. /// - /// # Notes + /// # Notes and Safety /// /// This closure will be run in the context of the child process after a /// `fork`. This primarily means that any modifications made to memory on From b1709d25e12fbffca53c30d05c16854256185900 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Fri, 1 Feb 2019 20:00:08 +0100 Subject: [PATCH 3/8] update test --- ...and-before-exec.rs => command-pre-exec.rs} | 26 +++++++++---------- 1 file changed, 13 insertions(+), 13 deletions(-) rename src/test/run-pass/{command-before-exec.rs => command-pre-exec.rs} (75%) diff --git a/src/test/run-pass/command-before-exec.rs b/src/test/run-pass/command-pre-exec.rs similarity index 75% rename from src/test/run-pass/command-before-exec.rs rename to src/test/run-pass/command-pre-exec.rs index 91d2636b2ae63..bca2b8410fa84 100644 --- a/src/test/run-pass/command-before-exec.rs +++ b/src/test/run-pass/command-pre-exec.rs @@ -29,53 +29,53 @@ fn main() { let me = env::current_exe().unwrap(); - let output = Command::new(&me).arg("test1").before_exec(|| { + let output = unsafe { Command::new(&me).arg("test1").pre_exec(|| { println!("hello"); Ok(()) - }).output().unwrap(); + }).output().unwrap() }; assert!(output.status.success()); assert!(output.stderr.is_empty()); assert_eq!(output.stdout, b"hello\nhello2\n"); - let output = Command::new(&me).arg("test2").before_exec(|| { + let output = unsafe { Command::new(&me).arg("test2").pre_exec(|| { env::set_var("FOO", "BAR"); Ok(()) - }).output().unwrap(); + }).output().unwrap() }; assert!(output.status.success()); assert!(output.stderr.is_empty()); assert!(output.stdout.is_empty()); - let output = Command::new(&me).arg("test3").before_exec(|| { + let output = unsafe { Command::new(&me).arg("test3").pre_exec(|| { env::set_current_dir("/").unwrap(); Ok(()) - }).output().unwrap(); + }).output().unwrap() }; assert!(output.status.success()); assert!(output.stderr.is_empty()); assert!(output.stdout.is_empty()); - let output = Command::new(&me).arg("bad").before_exec(|| { + let output = unsafe { Command::new(&me).arg("bad").pre_exec(|| { Err(Error::from_raw_os_error(102)) - }).output().unwrap_err(); + }).output().unwrap_err() }; assert_eq!(output.raw_os_error(), Some(102)); let pid = unsafe { libc::getpid() }; assert!(pid >= 0); - let output = Command::new(&me).arg("empty").before_exec(move || { - let child = unsafe { libc::getpid() }; + let output = unsafe { Command::new(&me).arg("empty").pre_exec(move || { + let child = libc::getpid(); assert!(child >= 0); assert!(pid != child); Ok(()) - }).output().unwrap(); + }).output().unwrap() }; assert!(output.status.success()); assert!(output.stderr.is_empty()); assert!(output.stdout.is_empty()); let mem = Arc::new(AtomicUsize::new(0)); let mem2 = mem.clone(); - let output = Command::new(&me).arg("empty").before_exec(move || { + let output = unsafe { Command::new(&me).arg("empty").pre_exec(move || { assert_eq!(mem2.fetch_add(1, Ordering::SeqCst), 0); Ok(()) - }).output().unwrap(); + }).output().unwrap() }; assert!(output.status.success()); assert!(output.stderr.is_empty()); assert!(output.stdout.is_empty()); From cbbf8a7ff932b599227b27d34e9b015374f5b37a Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sat, 2 Feb 2019 11:00:55 +0100 Subject: [PATCH 4/8] deprecate things a bit slower --- src/libstd/sys/redox/ext/process.rs | 2 +- src/libstd/sys/unix/ext/process.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libstd/sys/redox/ext/process.rs b/src/libstd/sys/redox/ext/process.rs index 78917ea91886b..4e669bbb2d7dc 100644 --- a/src/libstd/sys/redox/ext/process.rs +++ b/src/libstd/sys/redox/ext/process.rs @@ -65,7 +65,7 @@ pub trait CommandExt { /// /// [`pre_exec`]: #tymethod.pre_exec #[stable(feature = "process_exec", since = "1.15.0")] - #[rustc_deprecated(since = "1.34.0", reason = "should be unsafe, use `pre_exec` instead")] + #[rustc_deprecated(since = "1.37.0", reason = "should be unsafe, use `pre_exec` instead")] fn before_exec(&mut self, f: F) -> &mut process::Command where F: FnMut() -> io::Result<()> + Send + Sync + 'static { diff --git a/src/libstd/sys/unix/ext/process.rs b/src/libstd/sys/unix/ext/process.rs index 7cc5e9945938d..da0507c65423d 100644 --- a/src/libstd/sys/unix/ext/process.rs +++ b/src/libstd/sys/unix/ext/process.rs @@ -65,7 +65,7 @@ pub trait CommandExt { /// /// [`pre_exec`]: #tymethod.pre_exec #[stable(feature = "process_exec", since = "1.15.0")] - #[rustc_deprecated(since = "1.34.0", reason = "should be unsafe, use `pre_exec` instead")] + #[rustc_deprecated(since = "1.37.0", reason = "should be unsafe, use `pre_exec` instead")] fn before_exec(&mut self, f: F) -> &mut process::Command where F: FnMut() -> io::Result<()> + Send + Sync + 'static { From 6c67a7625fbd38b4b986981c553dc7eb5a7a4765 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sat, 2 Feb 2019 11:05:43 +0100 Subject: [PATCH 5/8] pre_exec: expand docs --- src/libstd/sys/redox/ext/process.rs | 7 ++++--- src/libstd/sys/unix/ext/process.rs | 7 ++++--- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/src/libstd/sys/redox/ext/process.rs b/src/libstd/sys/redox/ext/process.rs index 4e669bbb2d7dc..55824dc4c059f 100644 --- a/src/libstd/sys/redox/ext/process.rs +++ b/src/libstd/sys/redox/ext/process.rs @@ -48,7 +48,8 @@ pub trait CommandExt { /// This also means that all resources such as file descriptors and /// memory-mapped regions got duplicated. It is your responsibility to make /// sure that the closure does not violate library invariants by making - /// invalid use of these duplicates. + /// invalid use of these duplicates. Moreover, POSIX demands that you only + /// perform operations that are explicitly documented as async-signal-safe. /// /// When this closure is run, aspects such as the stdio file descriptors and /// working directory have successfully been changed, so output to these @@ -60,8 +61,8 @@ pub trait CommandExt { /// Schedules a closure to be run just before the `exec` function is /// invoked. /// - /// This method should be unsafe, so it got deprecated in favor of the - /// unsafe [`pre_exec`]. + /// This method is stable and usable, but it should be unsafe. To fix + /// that, it got deprecated in favor of the unsafe [`pre_exec`]. /// /// [`pre_exec`]: #tymethod.pre_exec #[stable(feature = "process_exec", since = "1.15.0")] diff --git a/src/libstd/sys/unix/ext/process.rs b/src/libstd/sys/unix/ext/process.rs index da0507c65423d..ac0abc761ffb5 100644 --- a/src/libstd/sys/unix/ext/process.rs +++ b/src/libstd/sys/unix/ext/process.rs @@ -48,7 +48,8 @@ pub trait CommandExt { /// This also means that all resources such as file descriptors and /// memory-mapped regions got duplicated. It is your responsibility to make /// sure that the closure does not violate library invariants by making - /// invalid use of these duplicates. + /// invalid use of these duplicates. Moreover, POSIX demands that you only + /// perform operations that are explicitly documented as async-signal-safe. /// /// When this closure is run, aspects such as the stdio file descriptors and /// working directory have successfully been changed, so output to these @@ -60,8 +61,8 @@ pub trait CommandExt { /// Schedules a closure to be run just before the `exec` function is /// invoked. /// - /// This method should be unsafe, so it got deprecated in favor of the - /// unsafe [`pre_exec`]. + /// This method is stable and usable, but it should be unsafe. To fix + /// that, it got deprecated in favor of the unsafe [`pre_exec`]. /// /// [`pre_exec`]: #tymethod.pre_exec #[stable(feature = "process_exec", since = "1.15.0")] From 59da97d2b2d0e3d4baf70cd6fdf49c31c7def380 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sat, 2 Feb 2019 11:08:21 +0100 Subject: [PATCH 6/8] rustfmt the test --- src/test/run-pass/command-pre-exec.rs | 92 ++++++++++++++++++--------- 1 file changed, 62 insertions(+), 30 deletions(-) diff --git a/src/test/run-pass/command-pre-exec.rs b/src/test/run-pass/command-pre-exec.rs index bca2b8410fa84..21783fedd39c9 100644 --- a/src/test/run-pass/command-pre-exec.rs +++ b/src/test/run-pass/command-pre-exec.rs @@ -2,7 +2,6 @@ // ignore-windows - this is a unix-specific test // ignore-cloudabi no processes // ignore-emscripten no processes - #![feature(process_exec, rustc_private)] extern crate libc; @@ -11,71 +10,104 @@ use std::env; use std::io::Error; use std::os::unix::process::CommandExt; use std::process::Command; -use std::sync::Arc; use std::sync::atomic::{AtomicUsize, Ordering}; +use std::sync::Arc; fn main() { if let Some(arg) = env::args().nth(1) { match &arg[..] { "test1" => println!("hello2"), "test2" => assert_eq!(env::var("FOO").unwrap(), "BAR"), - "test3" => assert_eq!(env::current_dir().unwrap() - .to_str().unwrap(), "/"), + "test3" => assert_eq!(env::current_dir().unwrap().to_str().unwrap(), "/"), "empty" => {} _ => panic!("unknown argument: {}", arg), } - return + return; } let me = env::current_exe().unwrap(); - let output = unsafe { Command::new(&me).arg("test1").pre_exec(|| { - println!("hello"); - Ok(()) - }).output().unwrap() }; + let output = unsafe { + Command::new(&me) + .arg("test1") + .pre_exec(|| { + println!("hello"); + Ok(()) + }) + .output() + .unwrap() + }; assert!(output.status.success()); assert!(output.stderr.is_empty()); assert_eq!(output.stdout, b"hello\nhello2\n"); - let output = unsafe { Command::new(&me).arg("test2").pre_exec(|| { - env::set_var("FOO", "BAR"); - Ok(()) - }).output().unwrap() }; + let output = unsafe { + Command::new(&me) + .arg("test2") + .pre_exec(|| { + env::set_var("FOO", "BAR"); + Ok(()) + }) + .output() + .unwrap() + }; assert!(output.status.success()); assert!(output.stderr.is_empty()); assert!(output.stdout.is_empty()); - let output = unsafe { Command::new(&me).arg("test3").pre_exec(|| { - env::set_current_dir("/").unwrap(); - Ok(()) - }).output().unwrap() }; + let output = unsafe { + Command::new(&me) + .arg("test3") + .pre_exec(|| { + env::set_current_dir("/").unwrap(); + Ok(()) + }) + .output() + .unwrap() + }; assert!(output.status.success()); assert!(output.stderr.is_empty()); assert!(output.stdout.is_empty()); - let output = unsafe { Command::new(&me).arg("bad").pre_exec(|| { - Err(Error::from_raw_os_error(102)) - }).output().unwrap_err() }; + let output = unsafe { + Command::new(&me) + .arg("bad") + .pre_exec(|| Err(Error::from_raw_os_error(102))) + .output() + .unwrap_err() + }; assert_eq!(output.raw_os_error(), Some(102)); let pid = unsafe { libc::getpid() }; assert!(pid >= 0); - let output = unsafe { Command::new(&me).arg("empty").pre_exec(move || { - let child = libc::getpid(); - assert!(child >= 0); - assert!(pid != child); - Ok(()) - }).output().unwrap() }; + let output = unsafe { + Command::new(&me) + .arg("empty") + .pre_exec(move || { + let child = libc::getpid(); + assert!(child >= 0); + assert!(pid != child); + Ok(()) + }) + .output() + .unwrap() + }; assert!(output.status.success()); assert!(output.stderr.is_empty()); assert!(output.stdout.is_empty()); let mem = Arc::new(AtomicUsize::new(0)); let mem2 = mem.clone(); - let output = unsafe { Command::new(&me).arg("empty").pre_exec(move || { - assert_eq!(mem2.fetch_add(1, Ordering::SeqCst), 0); - Ok(()) - }).output().unwrap() }; + let output = unsafe { + Command::new(&me) + .arg("empty") + .pre_exec(move || { + assert_eq!(mem2.fetch_add(1, Ordering::SeqCst), 0); + Ok(()) + }) + .output() + .unwrap() + }; assert!(output.status.success()); assert!(output.stderr.is_empty()); assert!(output.stdout.is_empty()); From 33ee99b26a499027546f88a7f4eeddd8d4985c39 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 3 Feb 2019 11:16:37 +0100 Subject: [PATCH 7/8] more formatting --- src/libstd/sys/redox/process.rs | 6 ++++-- src/libstd/sys/unix/process/process_common.rs | 6 ++++-- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/src/libstd/sys/redox/process.rs b/src/libstd/sys/redox/process.rs index 9b85fa41a0a4f..62eb8cb23ab56 100644 --- a/src/libstd/sys/redox/process.rs +++ b/src/libstd/sys/redox/process.rs @@ -116,8 +116,10 @@ impl Command { self.gid = Some(id); } - pub unsafe fn pre_exec(&mut self, - f: Box io::Result<()> + Send + Sync>) { + pub unsafe fn pre_exec( + &mut self, + f: Box io::Result<()> + Send + Sync>, + ) { self.closures.push(f); } diff --git a/src/libstd/sys/unix/process/process_common.rs b/src/libstd/sys/unix/process/process_common.rs index 9975064ca655f..7fa256e59b2db 100644 --- a/src/libstd/sys/unix/process/process_common.rs +++ b/src/libstd/sys/unix/process/process_common.rs @@ -149,8 +149,10 @@ impl Command { &mut self.closures } - pub unsafe fn pre_exec(&mut self, - f: Box io::Result<()> + Send + Sync>) { + pub unsafe fn pre_exec( + &mut self, + f: Box io::Result<()> + Send + Sync>, + ) { self.closures.push(f); } From e023403da2da17ba7320c53c415b960c93348247 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 3 Feb 2019 11:17:59 +0100 Subject: [PATCH 8/8] POSIX requires async signal safety for fork in signal handlers, not in general --- src/libstd/sys/redox/ext/process.rs | 3 +-- src/libstd/sys/unix/ext/process.rs | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/src/libstd/sys/redox/ext/process.rs b/src/libstd/sys/redox/ext/process.rs index 55824dc4c059f..18aef768b5db1 100644 --- a/src/libstd/sys/redox/ext/process.rs +++ b/src/libstd/sys/redox/ext/process.rs @@ -48,8 +48,7 @@ pub trait CommandExt { /// This also means that all resources such as file descriptors and /// memory-mapped regions got duplicated. It is your responsibility to make /// sure that the closure does not violate library invariants by making - /// invalid use of these duplicates. Moreover, POSIX demands that you only - /// perform operations that are explicitly documented as async-signal-safe. + /// invalid use of these duplicates. /// /// When this closure is run, aspects such as the stdio file descriptors and /// working directory have successfully been changed, so output to these diff --git a/src/libstd/sys/unix/ext/process.rs b/src/libstd/sys/unix/ext/process.rs index ac0abc761ffb5..d869a2013dc27 100644 --- a/src/libstd/sys/unix/ext/process.rs +++ b/src/libstd/sys/unix/ext/process.rs @@ -48,8 +48,7 @@ pub trait CommandExt { /// This also means that all resources such as file descriptors and /// memory-mapped regions got duplicated. It is your responsibility to make /// sure that the closure does not violate library invariants by making - /// invalid use of these duplicates. Moreover, POSIX demands that you only - /// perform operations that are explicitly documented as async-signal-safe. + /// invalid use of these duplicates. /// /// When this closure is run, aspects such as the stdio file descriptors and /// working directory have successfully been changed, so output to these