Auto merge of #31056 - kamalmarhubi:std-process-nul-chars, r=alexcrichton

This reports an error at the point of calling `Command::spawn()` or one of
its equivalents.

Fixes #30858
Fixes #30862

Author: bors

src/libstd/process.rs

@@ -353,11 +353,7 @@ impl fmt::Debug for Command {
     /// non-utf8 data is lossily converted using the utf8 replacement
     /// character.
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-        try!(write!(f, "{:?}", self.inner.program));
-        for arg in &self.inner.args {
-            try!(write!(f, " {:?}", arg));
-        }
-        Ok(())
+        self.inner.fmt(f)
     }
 }
 
@@ -887,4 +883,54 @@ mod tests {
         assert!(output.contains("RUN_TEST_NEW_ENV=123"),
                 "didn't find RUN_TEST_NEW_ENV inside of:\n\n{}", output);
     }
+
+    // Regression tests for #30858.
+    #[test]
+    fn test_interior_nul_in_progname_is_error() {
+        match Command::new("has-some-\0\0s-inside").spawn() {
+            Err(e) => assert_eq!(e.kind(), ErrorKind::InvalidInput),
+            Ok(_) => panic!(),
+        }
+    }
+
+    #[test]
+    fn test_interior_nul_in_arg_is_error() {
+        match Command::new("echo").arg("has-some-\0\0s-inside").spawn() {
+            Err(e) => assert_eq!(e.kind(), ErrorKind::InvalidInput),
+            Ok(_) => panic!(),
+        }
+    }
+
+    #[test]
+    fn test_interior_nul_in_args_is_error() {
+        match Command::new("echo").args(&["has-some-\0\0s-inside"]).spawn() {
+            Err(e) => assert_eq!(e.kind(), ErrorKind::InvalidInput),
+            Ok(_) => panic!(),
+        }
+    }
+
+    #[test]
+    fn test_interior_nul_in_current_dir_is_error() {
+        match Command::new("echo").current_dir("has-some-\0\0s-inside").spawn() {
+            Err(e) => assert_eq!(e.kind(), ErrorKind::InvalidInput),
+            Ok(_) => panic!(),
+        }
+    }
+
+    // Regression tests for #30862.
+    #[test]
+    fn test_interior_nul_in_env_key_is_error() {
+        match env_cmd().env("has-some-\0\0s-inside", "value").spawn() {
+            Err(e) => assert_eq!(e.kind(), ErrorKind::InvalidInput),
+            Ok(_) => panic!(),
+        }
+    }
+
+    #[test]
+    fn test_interior_nul_in_env_value_is_error() {
+        match env_cmd().env("key", "has-some-\0\0s-inside").spawn() {
+            Err(e) => assert_eq!(e.kind(), ErrorKind::InvalidInput),
+            Ok(_) => panic!(),
+        }
+    }
 }

src/libstd/sys/unix/ext/process.rs

@@ -49,17 +49,17 @@ pub trait CommandExt {
 #[stable(feature = "rust1", since = "1.0.0")]
 impl CommandExt for process::Command {
     fn uid(&mut self, id: uid_t) -> &mut process::Command {
-        self.as_inner_mut().uid = Some(id);
+        self.as_inner_mut().uid(id);
         self
     }
 
     fn gid(&mut self, id: gid_t) -> &mut process::Command {
-        self.as_inner_mut().gid = Some(id);
+        self.as_inner_mut().gid(id);
         self
     }
 
     fn session_leader(&mut self, on: bool) -> &mut process::Command {
-        self.as_inner_mut().session_leader = on;
+        self.as_inner_mut().session_leader(on);
         self
     }
 }

src/libstd/sys/unix/process.rs

@@ -31,57 +31,95 @@ use sys::{self, cvt, cvt_r};
 
 #[derive(Clone)]
 pub struct Command {
-    pub program: CString,
-    pub args: Vec<CString>,
-    pub env: Option<HashMap<OsString, OsString>>,
-    pub cwd: Option<CString>,
-    pub uid: Option<uid_t>,
-    pub gid: Option<gid_t>,
-    pub session_leader: bool,
+    program: CString,
+    args: Vec<CString>,
+    env: Option<HashMap<OsString, OsString>>, // Guaranteed to have no NULs.
+    cwd: Option<CString>,
+    uid: Option<uid_t>,
+    gid: Option<gid_t>,
+    session_leader: bool,
+    saw_nul: bool,
 }
 
 impl Command {
     pub fn new(program: &OsStr) -> Command {
+        let mut saw_nul = false;
         Command {
-            program: os2c(program),
+            program: os2c(program, &mut saw_nul),
             args: Vec::new(),
             env: None,
             cwd: None,
             uid: None,
             gid: None,
             session_leader: false,
+            saw_nul: saw_nul,
         }
     }
 
     pub fn arg(&mut self, arg: &OsStr) {
-        self.args.push(os2c(arg));
+        self.args.push(os2c(arg, &mut self.saw_nul));
     }
     pub fn args<'a, I: Iterator<Item = &'a OsStr>>(&mut self, args: I) {
-        self.args.extend(args.map(os2c));
+        let mut saw_nul = self.saw_nul;
+        self.args.extend(args.map(|arg| os2c(arg, &mut saw_nul)));
+        self.saw_nul = saw_nul;
     }
     fn init_env_map(&mut self) {
         if self.env.is_none() {
+            // Will not add NULs to env: preexisting environment will not contain any.
             self.env = Some(env::vars_os().collect());
         }
     }
     pub fn env(&mut self, key: &OsStr, val: &OsStr) {
+        let k = OsString::from_vec(os2c(key, &mut self.saw_nul).into_bytes());
+        let v = OsString::from_vec(os2c(val, &mut self.saw_nul).into_bytes());
+
+        // Will not add NULs to env: return without inserting if any were seen.
+        if self.saw_nul {
+            return;
+        }
+
         self.init_env_map();
-        self.env.as_mut().unwrap().insert(key.to_os_string(), val.to_os_string());
+        self.env.as_mut()
+            .unwrap()
+            .insert(k, v);
     }
     pub fn env_remove(&mut self, key: &OsStr) {
         self.init_env_map();
-        self.env.as_mut().unwrap().remove(&key.to_os_string());
+        self.env.as_mut().unwrap().remove(key);
     }
     pub fn env_clear(&mut self) {
         self.env = Some(HashMap::new())
     }
     pub fn cwd(&mut self, dir: &OsStr) {
-        self.cwd = Some(os2c(dir));
+        self.cwd = Some(os2c(dir, &mut self.saw_nul));
+    }
+    pub fn uid(&mut self, id: uid_t) {
+        self.uid = Some(id);
     }
+    pub fn gid(&mut self, id: gid_t) {
+        self.gid = Some(id);
+    }
+    pub fn session_leader(&mut self, session_leader: bool) {
+        self.session_leader = session_leader;
+    }
+}
+
+fn os2c(s: &OsStr, saw_nul: &mut bool) -> CString {
+    CString::new(s.as_bytes()).unwrap_or_else(|_e| {
+        *saw_nul = true;
+        CString::new("<string-with-nul>").unwrap()
+    })
 }
 
-fn os2c(s: &OsStr) -> CString {
-    CString::new(s.as_bytes()).unwrap()
+impl fmt::Debug for Command {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        try!(write!(f, "{:?}", self.program));
+        for arg in &self.args {
+            try!(write!(f, " {:?}", arg));
+        }
+        Ok(())
+    }
 }
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -175,6 +213,10 @@ impl Process {
                  in_fd: Stdio,
                  out_fd: Stdio,
                  err_fd: Stdio) -> io::Result<Process> {
+        if cfg.saw_nul {
+            return Err(io::Error::new(ErrorKind::InvalidInput, "nul byte found in provided data"));
+        }
+
         let dirp = cfg.cwd.as_ref().map(|c| c.as_ptr()).unwrap_or(ptr::null());
 
         let (envp, _a, _b) = make_envp(cfg.env.as_ref());

src/libstd/sys/windows/process.rs

@@ -18,7 +18,7 @@ use env;
 use ffi::{OsString, OsStr};
 use fmt;
 use fs;
-use io::{self, Error};
+use io::{self, Error, ErrorKind};
 use libc::c_void;
 use mem;
 use os::windows::ffi::OsStrExt;
@@ -43,13 +43,21 @@ fn mk_key(s: &OsStr) -> OsString {
     })
 }
 
+fn ensure_no_nuls<T: AsRef<OsStr>>(str: T) -> io::Result<T> {
+    if str.as_ref().encode_wide().any(|b| b == 0) {
+        Err(io::Error::new(ErrorKind::InvalidInput, "nul byte found in provided data"))
+    } else {
+        Ok(str)
+    }
+}
+
 #[derive(Clone)]
 pub struct Command {
-    pub program: OsString,
-    pub args: Vec<OsString>,
-    pub env: Option<HashMap<OsString, OsString>>,
-    pub cwd: Option<OsString>,
-    pub detach: bool, // not currently exposed in std::process
+    program: OsString,
+    args: Vec<OsString>,
+    env: Option<HashMap<OsString, OsString>>,
+    cwd: Option<OsString>,
+    detach: bool, // not currently exposed in std::process
 }
 
 impl Command {
@@ -92,6 +100,16 @@ impl Command {
     }
 }
 
+impl fmt::Debug for Command {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        try!(write!(f, "{:?}", self.program));
+        for arg in &self.args {
+            try!(write!(f, " {:?}", arg));
+        }
+        Ok(())
+    }
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 // Processes
 ////////////////////////////////////////////////////////////////////////////////
@@ -153,7 +171,7 @@ impl Process {
         si.hStdError = stderr.raw();
 
         let program = program.as_ref().unwrap_or(&cfg.program);
-        let mut cmd_str = make_command_line(program, &cfg.args);
+        let mut cmd_str = try!(make_command_line(program, &cfg.args));
         cmd_str.push(0); // add null terminator
 
         // stolen from the libuv code.
@@ -162,8 +180,8 @@ impl Process {
             flags |= c::DETACHED_PROCESS | c::CREATE_NEW_PROCESS_GROUP;
         }
 
-        let (envp, _data) = make_envp(cfg.env.as_ref());
-        let (dirp, _data) = make_dirp(cfg.cwd.as_ref());
+        let (envp, _data) = try!(make_envp(cfg.env.as_ref()));
+        let (dirp, _data) = try!(make_dirp(cfg.cwd.as_ref()));
         let mut pi = zeroed_process_information();
         try!(unsafe {
             // `CreateProcess` is racy!
@@ -265,22 +283,24 @@ fn zeroed_process_information() -> c::PROCESS_INFORMATION {
     }
 }
 
-// Produces a wide string *without terminating null*
-fn make_command_line(prog: &OsStr, args: &[OsString]) -> Vec<u16> {
+// Produces a wide string *without terminating null*; returns an error if
+// `prog` or any of the `args` contain a nul.
+fn make_command_line(prog: &OsStr, args: &[OsString]) -> io::Result<Vec<u16>> {
     // Encode the command and arguments in a command line string such
     // that the spawned process may recover them using CommandLineToArgvW.
     let mut cmd: Vec<u16> = Vec::new();
-    append_arg(&mut cmd, prog);
+    try!(append_arg(&mut cmd, prog));
     for arg in args {
         cmd.push(' ' as u16);
-        append_arg(&mut cmd, arg);
+        try!(append_arg(&mut cmd, arg));
     }
-    return cmd;
+    return Ok(cmd);
 
-    fn append_arg(cmd: &mut Vec<u16>, arg: &OsStr) {
+    fn append_arg(cmd: &mut Vec<u16>, arg: &OsStr) -> io::Result<()> {
         // If an argument has 0 characters then we need to quote it to ensure
         // that it actually gets passed through on the command line or otherwise
         // it will be dropped entirely when parsed on the other end.
+        try!(ensure_no_nuls(arg));
         let arg_bytes = &arg.as_inner().inner.as_inner();
         let quote = arg_bytes.iter().any(|c| *c == b' ' || *c == b'\t')
             || arg_bytes.is_empty();
@@ -312,11 +332,12 @@ fn make_command_line(prog: &OsStr, args: &[OsString]) -> Vec<u16> {
             }
             cmd.push('"' as u16);
         }
+        Ok(())
     }
 }
 
 fn make_envp(env: Option<&collections::HashMap<OsString, OsString>>)
-             -> (*mut c_void, Vec<u16>) {
+             -> io::Result<(*mut c_void, Vec<u16>)> {
     // On Windows we pass an "environment block" which is not a char**, but
     // rather a concatenation of null-terminated k=v\0 sequences, with a final
     // \0 to terminate.
@@ -325,26 +346,27 @@ fn make_envp(env: Option<&collections::HashMap<OsString, OsString>>)
             let mut blk = Vec::new();
 
             for pair in env {
-                blk.extend(pair.0.encode_wide());
+                blk.extend(try!(ensure_no_nuls(pair.0)).encode_wide());
                 blk.push('=' as u16);
-                blk.extend(pair.1.encode_wide());
+                blk.extend(try!(ensure_no_nuls(pair.1)).encode_wide());
                 blk.push(0);
             }
             blk.push(0);
-            (blk.as_mut_ptr() as *mut c_void, blk)
+            Ok((blk.as_mut_ptr() as *mut c_void, blk))
         }
-        _ => (ptr::null_mut(), Vec::new())
+        _ => Ok((ptr::null_mut(), Vec::new()))
     }
 }
 
-fn make_dirp(d: Option<&OsString>) -> (*const u16, Vec<u16>) {
+fn make_dirp(d: Option<&OsString>) -> io::Result<(*const u16, Vec<u16>)> {
+
     match d {
         Some(dir) => {
-            let mut dir_str: Vec<u16> = dir.encode_wide().collect();
+            let mut dir_str: Vec<u16> = try!(ensure_no_nuls(dir)).encode_wide().collect();
             dir_str.push(0);
-            (dir_str.as_ptr(), dir_str)
+            Ok((dir_str.as_ptr(), dir_str))
         },
-        None => (ptr::null(), Vec::new())
+        None => Ok((ptr::null(), Vec::new()))
     }
 }
 
@@ -397,11 +419,12 @@ mod tests {
     #[test]
     fn test_make_command_line() {
         fn test_wrapper(prog: &str, args: &[&str]) -> String {
-            String::from_utf16(
-                &make_command_line(OsStr::new(prog),
-                                   &args.iter()
-                                        .map(|a| OsString::from(a))
-                                        .collect::<Vec<OsString>>())).unwrap()
+            let command_line = &make_command_line(OsStr::new(prog),
+                                                  &args.iter()
+                                                       .map(|a| OsString::from(a))
+                                                       .collect::<Vec<OsString>>())
+                                    .unwrap();
+            String::from_utf16(command_line).unwrap()
         }
 
         assert_eq!(