From 36fe3b605a7a7143a14565272140ba1b43c1b041 Mon Sep 17 00:00:00 2001 From: Alex Gaynor Date: Thu, 25 Oct 2018 19:44:32 +0000 Subject: [PATCH] Fixes #46775 -- don't mutate the process's environment in Command::exec Instead, pass the environment to execvpe, so the kernel can apply it directly to the new process. This avoids a use-after-free in the case where exec'ing the new process fails for any reason, as well as a race condition if there are other threads alive during the exec. --- src/libstd/sys/unix/process/process_common.rs | 8 ++ src/libstd/sys/unix/process/process_unix.rs | 99 +++++++++++++++++-- src/test/run-pass/command-exec.rs | 12 +++ 3 files changed, 111 insertions(+), 8 deletions(-) diff --git a/src/libstd/sys/unix/process/process_common.rs b/src/libstd/sys/unix/process/process_common.rs index 77f125f3c5b..3d5920dfb69 100644 --- a/src/libstd/sys/unix/process/process_common.rs +++ b/src/libstd/sys/unix/process/process_common.rs @@ -141,6 +141,10 @@ impl Command { pub fn get_argv(&self) -> &Vec<*const c_char> { &self.argv.0 } + #[cfg(not(target_os = "fuchsia"))] + pub fn get_program(&self) -> &CString { + return &self.program; + } #[allow(dead_code)] pub fn get_cwd(&self) -> &Option { @@ -244,6 +248,10 @@ impl CStringArray { pub fn as_ptr(&self) -> *const *const c_char { self.ptrs.as_ptr() } + #[cfg(not(target_os = "fuchsia"))] + pub fn get_items(&self) -> &[CString] { + return &self.items; + } } fn construct_envp(env: BTreeMap, saw_nul: &mut bool) -> CStringArray { diff --git a/src/libstd/sys/unix/process/process_unix.rs b/src/libstd/sys/unix/process/process_unix.rs index 7f1f9353c6d..f41bd2c2072 100644 --- a/src/libstd/sys/unix/process/process_unix.rs +++ b/src/libstd/sys/unix/process/process_unix.rs @@ -8,6 +8,8 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. +use env; +use ffi::CString; use io::{self, Error, ErrorKind}; use libc::{self, c_int, gid_t, pid_t, uid_t}; use ptr; @@ -39,13 +41,15 @@ impl Command { return Ok((ret, ours)) } + let possible_paths = self.compute_possible_paths(envp.as_ref()); + let (input, output) = sys::pipe::anon_pipe()?; let pid = unsafe { match cvt(libc::fork())? { 0 => { drop(input); - let err = self.do_exec(theirs, envp.as_ref()); + let err = self.do_exec(theirs, envp.as_ref(), possible_paths); let errno = err.raw_os_error().unwrap_or(libc::EINVAL) as u32; let bytes = [ (errno >> 24) as u8, @@ -113,12 +117,48 @@ impl Command { "nul byte found in provided data") } + let possible_paths = self.compute_possible_paths(envp.as_ref()); match self.setup_io(default, true) { - Ok((_, theirs)) => unsafe { self.do_exec(theirs, envp.as_ref()) }, + Ok((_, theirs)) => unsafe { self.do_exec(theirs, envp.as_ref(), possible_paths) }, Err(e) => e, } } + fn compute_possible_paths(&self, maybe_envp: Option<&CStringArray>) -> Option> { + let program = self.get_program().as_bytes(); + if program.contains(&b'/') { + return None; + } + // Outside the match so we can borrow it for the lifetime of the function. + let parent_path = env::var("PATH").ok(); + let paths = match maybe_envp { + Some(envp) => { + match envp.get_items().iter().find(|var| var.as_bytes().starts_with(b"PATH=")) { + Some(p) => &p.as_bytes()[5..], + None => return None, + } + }, + // maybe_envp is None if the process isn't changing the parent's env at all. + None => { + match parent_path.as_ref() { + Some(p) => p.as_bytes(), + None => return None, + } + }, + }; + + let mut possible_paths = vec![]; + for path in paths.split(|p| *p == b':') { + let mut binary_path = Vec::with_capacity(program.len() + path.len() + 1); + binary_path.extend_from_slice(path); + binary_path.push(b'/'); + binary_path.extend_from_slice(program); + let c_binary_path = CString::new(binary_path).unwrap(); + possible_paths.push(c_binary_path); + } + return Some(possible_paths); + } + // And at this point we've reached a special time in the life of the // child. The child must now be considered hamstrung and unable to // do anything other than syscalls really. Consider the following @@ -152,7 +192,8 @@ impl Command { unsafe fn do_exec( &mut self, stdio: ChildPipes, - maybe_envp: Option<&CStringArray> + maybe_envp: Option<&CStringArray>, + maybe_possible_paths: Option>, ) -> io::Error { use sys::{self, cvt_r}; @@ -193,9 +234,6 @@ impl Command { if let Some(ref cwd) = *self.get_cwd() { t!(cvt(libc::chdir(cwd.as_ptr()))); } - if let Some(envp) = maybe_envp { - *sys::os::environ() = envp.as_ptr(); - } // emscripten has no signal support. #[cfg(not(any(target_os = "emscripten")))] @@ -231,8 +269,53 @@ impl Command { t!(callback()); } - libc::execvp(self.get_argv()[0], self.get_argv().as_ptr()); - io::Error::last_os_error() + // If the program isn't an absolute path, and our environment contains a PATH var, then we + // implement the PATH traversal ourselves so that it honors the child's PATH instead of the + // parent's. This mirrors the logic that exists in glibc's execvpe, except using the + // child's env to fetch PATH. + match maybe_possible_paths { + Some(possible_paths) => { + let mut pending_error = None; + for path in possible_paths { + libc::execve( + path.as_ptr(), + self.get_argv().as_ptr(), + maybe_envp.map(|envp| envp.as_ptr()).unwrap_or_else(|| *sys::os::environ()) + ); + let err = io::Error::last_os_error(); + match err.kind() { + io::ErrorKind::PermissionDenied => { + // If we saw a PermissionDenied, and none of the other entries in + // $PATH are successful, then we'll return the first EACCESS we see. + if pending_error.is_none() { + pending_error = Some(err); + } + }, + // Errors which indicate we failed to find a file are ignored and we try + // the next entry in the path. + io::ErrorKind::NotFound | io::ErrorKind::TimedOut => { + continue + }, + // Any other error means we found a file and couldn't execute it. + _ => { + return err; + } + } + } + if let Some(err) = pending_error { + return err; + } + return io::Error::from_raw_os_error(libc::ENOENT); + }, + _ => { + libc::execve( + self.get_argv()[0], + self.get_argv().as_ptr(), + maybe_envp.map(|envp| envp.as_ptr()).unwrap_or_else(|| *sys::os::environ()) + ); + return io::Error::last_os_error() + } + } } #[cfg(not(any(target_os = "macos", target_os = "freebsd", diff --git a/src/test/run-pass/command-exec.rs b/src/test/run-pass/command-exec.rs index 46b409fb13a..96f9da67790 100644 --- a/src/test/run-pass/command-exec.rs +++ b/src/test/run-pass/command-exec.rs @@ -48,6 +48,13 @@ fn main() { println!("passed"); } + "exec-test5" => { + env::set_var("VARIABLE", "ABC"); + Command::new("definitely-not-a-real-binary").env("VARIABLE", "XYZ").exec(); + assert_eq!(env::var("VARIABLE").unwrap(), "ABC"); + println!("passed"); + } + _ => panic!("unknown argument: {}", arg), } return @@ -72,4 +79,9 @@ fn main() { assert!(output.status.success()); assert!(output.stderr.is_empty()); assert_eq!(output.stdout, b"passed\n"); + + let output = Command::new(&me).arg("exec-test5").output().unwrap(); + assert!(output.status.success()); + assert!(output.stderr.is_empty()); + assert_eq!(output.stdout, b"passed\n"); }