Skip to content

Commit 8fc73c7

Browse files
borsbors
committed
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
2 parents b14af1e + 7c64bf1 commit 8fc73c7

File tree

4 files changed

+163
-52
lines changed

4 files changed

+163
-52
lines changed

‎src/libstd/process.rs

+51-5
Original file line numberDiff line numberDiff line change
@@ -353,11 +353,7 @@ impl fmt::Debug for Command {
353353
/// non-utf8 data is lossily converted using the utf8 replacement
354354
/// character.
355355
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
356-
try!(write!(f, "{:?}", self.inner.program));
357-
for arg in &self.inner.args {
358-
try!(write!(f, " {:?}", arg));
359-
}
360-
Ok(())
356+
self.inner.fmt(f)
361357
}
362358
}
363359

@@ -887,4 +883,54 @@ mod tests {
887883
assert!(output.contains("RUN_TEST_NEW_ENV=123"),
888884
"didn't find RUN_TEST_NEW_ENV inside of:\n\n{}", output);
889885
}
886+
887+
// Regression tests for #30858.
888+
#[test]
889+
fn test_interior_nul_in_progname_is_error() {
890+
match Command::new("has-some-\0\0s-inside").spawn() {
891+
Err(e) => assert_eq!(e.kind(), ErrorKind::InvalidInput),
892+
Ok(_) => panic!(),
893+
}
894+
}
895+
896+
#[test]
897+
fn test_interior_nul_in_arg_is_error() {
898+
match Command::new("echo").arg("has-some-\0\0s-inside").spawn() {
899+
Err(e) => assert_eq!(e.kind(), ErrorKind::InvalidInput),
900+
Ok(_) => panic!(),
901+
}
902+
}
903+
904+
#[test]
905+
fn test_interior_nul_in_args_is_error() {
906+
match Command::new("echo").args(&["has-some-\0\0s-inside"]).spawn() {
907+
Err(e) => assert_eq!(e.kind(), ErrorKind::InvalidInput),
908+
Ok(_) => panic!(),
909+
}
910+
}
911+
912+
#[test]
913+
fn test_interior_nul_in_current_dir_is_error() {
914+
match Command::new("echo").current_dir("has-some-\0\0s-inside").spawn() {
915+
Err(e) => assert_eq!(e.kind(), ErrorKind::InvalidInput),
916+
Ok(_) => panic!(),
917+
}
918+
}
919+
920+
// Regression tests for #30862.
921+
#[test]
922+
fn test_interior_nul_in_env_key_is_error() {
923+
match env_cmd().env("has-some-\0\0s-inside", "value").spawn() {
924+
Err(e) => assert_eq!(e.kind(), ErrorKind::InvalidInput),
925+
Ok(_) => panic!(),
926+
}
927+
}
928+
929+
#[test]
930+
fn test_interior_nul_in_env_value_is_error() {
931+
match env_cmd().env("key", "has-some-\0\0s-inside").spawn() {
932+
Err(e) => assert_eq!(e.kind(), ErrorKind::InvalidInput),
933+
Ok(_) => panic!(),
934+
}
935+
}
890936
}

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

+3-3
Original file line numberDiff line numberDiff line change
@@ -49,17 +49,17 @@ pub trait CommandExt {
4949
#[stable(feature = "rust1", since = "1.0.0")]
5050
impl CommandExt for process::Command {
5151
fn uid(&mut self, id: uid_t) -> &mut process::Command {
52-
self.as_inner_mut().uid = Some(id);
52+
self.as_inner_mut().uid(id);
5353
self
5454
}
5555

5656
fn gid(&mut self, id: gid_t) -> &mut process::Command {
57-
self.as_inner_mut().gid = Some(id);
57+
self.as_inner_mut().gid(id);
5858
self
5959
}
6060

6161
fn session_leader(&mut self, on: bool) -> &mut process::Command {
62-
self.as_inner_mut().session_leader = on;
62+
self.as_inner_mut().session_leader(on);
6363
self
6464
}
6565
}

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

+57-15
Original file line numberDiff line numberDiff line change
@@ -31,57 +31,95 @@ use sys::{self, cvt, cvt_r};
3131

3232
#[derive(Clone)]
3333
pub struct Command {
34-
pub program: CString,
35-
pub args: Vec<CString>,
36-
pub env: Option<HashMap<OsString, OsString>>,
37-
pub cwd: Option<CString>,
38-
pub uid: Option<uid_t>,
39-
pub gid: Option<gid_t>,
40-
pub session_leader: bool,
34+
program: CString,
35+
args: Vec<CString>,
36+
env: Option<HashMap<OsString, OsString>>, // Guaranteed to have no NULs.
37+
cwd: Option<CString>,
38+
uid: Option<uid_t>,
39+
gid: Option<gid_t>,
40+
session_leader: bool,
41+
saw_nul: bool,
4142
}
4243

4344
impl Command {
4445
pub fn new(program: &OsStr) -> Command {
46+
let mut saw_nul = false;
4547
Command {
46-
program: os2c(program),
48+
program: os2c(program, &mut saw_nul),
4749
args: Vec::new(),
4850
env: None,
4951
cwd: None,
5052
uid: None,
5153
gid: None,
5254
session_leader: false,
55+
saw_nul: saw_nul,
5356
}
5457
}
5558

5659
pub fn arg(&mut self, arg: &OsStr) {
57-
self.args.push(os2c(arg));
60+
self.args.push(os2c(arg, &mut self.saw_nul));
5861
}
5962
pub fn args<'a, I: Iterator<Item = &'a OsStr>>(&mut self, args: I) {
60-
self.args.extend(args.map(os2c));
63+
let mut saw_nul = self.saw_nul;
64+
self.args.extend(args.map(|arg| os2c(arg, &mut saw_nul)));
65+
self.saw_nul = saw_nul;
6166
}
6267
fn init_env_map(&mut self) {
6368
if self.env.is_none() {
69+
// Will not add NULs to env: preexisting environment will not contain any.
6470
self.env = Some(env::vars_os().collect());
6571
}
6672
}
6773
pub fn env(&mut self, key: &OsStr, val: &OsStr) {
74+
let k = OsString::from_vec(os2c(key, &mut self.saw_nul).into_bytes());
75+
let v = OsString::from_vec(os2c(val, &mut self.saw_nul).into_bytes());
76+
77+
// Will not add NULs to env: return without inserting if any were seen.
78+
if self.saw_nul {
79+
return;
80+
}
81+
6882
self.init_env_map();
69-
self.env.as_mut().unwrap().insert(key.to_os_string(), val.to_os_string());
83+
self.env.as_mut()
84+
.unwrap()
85+
.insert(k, v);
7086
}
7187
pub fn env_remove(&mut self, key: &OsStr) {
7288
self.init_env_map();
73-
self.env.as_mut().unwrap().remove(&key.to_os_string());
89+
self.env.as_mut().unwrap().remove(key);
7490
}
7591
pub fn env_clear(&mut self) {
7692
self.env = Some(HashMap::new())
7793
}
7894
pub fn cwd(&mut self, dir: &OsStr) {
79-
self.cwd = Some(os2c(dir));
95+
self.cwd = Some(os2c(dir, &mut self.saw_nul));
96+
}
97+
pub fn uid(&mut self, id: uid_t) {
98+
self.uid = Some(id);
8099
}
100+
pub fn gid(&mut self, id: gid_t) {
101+
self.gid = Some(id);
102+
}
103+
pub fn session_leader(&mut self, session_leader: bool) {
104+
self.session_leader = session_leader;
105+
}
106+
}
107+
108+
fn os2c(s: &OsStr, saw_nul: &mut bool) -> CString {
109+
CString::new(s.as_bytes()).unwrap_or_else(|_e| {
110+
*saw_nul = true;
111+
CString::new("<string-with-nul>").unwrap()
112+
})
81113
}
82114

83-
fn os2c(s: &OsStr) -> CString {
84-
CString::new(s.as_bytes()).unwrap()
115+
impl fmt::Debug for Command {
116+
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
117+
try!(write!(f, "{:?}", self.program));
118+
for arg in &self.args {
119+
try!(write!(f, " {:?}", arg));
120+
}
121+
Ok(())
122+
}
85123
}
86124

87125
////////////////////////////////////////////////////////////////////////////////
@@ -175,6 +213,10 @@ impl Process {
175213
in_fd: Stdio,
176214
out_fd: Stdio,
177215
err_fd: Stdio) -> io::Result<Process> {
216+
if cfg.saw_nul {
217+
return Err(io::Error::new(ErrorKind::InvalidInput, "nul byte found in provided data"));
218+
}
219+
178220
let dirp = cfg.cwd.as_ref().map(|c| c.as_ptr()).unwrap_or(ptr::null());
179221

180222
let (envp, _a, _b) = make_envp(cfg.env.as_ref());

‎src/libstd/sys/windows/process.rs

+52-29
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ use env;
1818
use ffi::{OsString, OsStr};
1919
use fmt;
2020
use fs;
21-
use io::{self, Error};
21+
use io::{self, Error, ErrorKind};
2222
use libc::c_void;
2323
use mem;
2424
use os::windows::ffi::OsStrExt;
@@ -43,13 +43,21 @@ fn mk_key(s: &OsStr) -> OsString {
4343
})
4444
}
4545

46+
fn ensure_no_nuls<T: AsRef<OsStr>>(str: T) -> io::Result<T> {
47+
if str.as_ref().encode_wide().any(|b| b == 0) {
48+
Err(io::Error::new(ErrorKind::InvalidInput, "nul byte found in provided data"))
49+
} else {
50+
Ok(str)
51+
}
52+
}
53+
4654
#[derive(Clone)]
4755
pub struct Command {
48-
pub program: OsString,
49-
pub args: Vec<OsString>,
50-
pub env: Option<HashMap<OsString, OsString>>,
51-
pub cwd: Option<OsString>,
52-
pub detach: bool, // not currently exposed in std::process
56+
program: OsString,
57+
args: Vec<OsString>,
58+
env: Option<HashMap<OsString, OsString>>,
59+
cwd: Option<OsString>,
60+
detach: bool, // not currently exposed in std::process
5361
}
5462

5563
impl Command {
@@ -92,6 +100,16 @@ impl Command {
92100
}
93101
}
94102

103+
impl fmt::Debug for Command {
104+
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
105+
try!(write!(f, "{:?}", self.program));
106+
for arg in &self.args {
107+
try!(write!(f, " {:?}", arg));
108+
}
109+
Ok(())
110+
}
111+
}
112+
95113
////////////////////////////////////////////////////////////////////////////////
96114
// Processes
97115
////////////////////////////////////////////////////////////////////////////////
@@ -153,7 +171,7 @@ impl Process {
153171
si.hStdError = stderr.raw();
154172

155173
let program = program.as_ref().unwrap_or(&cfg.program);
156-
let mut cmd_str = make_command_line(program, &cfg.args);
174+
let mut cmd_str = try!(make_command_line(program, &cfg.args));
157175
cmd_str.push(0); // add null terminator
158176

159177
// stolen from the libuv code.
@@ -162,8 +180,8 @@ impl Process {
162180
flags |= c::DETACHED_PROCESS | c::CREATE_NEW_PROCESS_GROUP;
163181
}
164182

165-
let (envp, _data) = make_envp(cfg.env.as_ref());
166-
let (dirp, _data) = make_dirp(cfg.cwd.as_ref());
183+
let (envp, _data) = try!(make_envp(cfg.env.as_ref()));
184+
let (dirp, _data) = try!(make_dirp(cfg.cwd.as_ref()));
167185
let mut pi = zeroed_process_information();
168186
try!(unsafe {
169187
// `CreateProcess` is racy!
@@ -265,22 +283,24 @@ fn zeroed_process_information() -> c::PROCESS_INFORMATION {
265283
}
266284
}
267285

268-
// Produces a wide string *without terminating null*
269-
fn make_command_line(prog: &OsStr, args: &[OsString]) -> Vec<u16> {
286+
// Produces a wide string *without terminating null*; returns an error if
287+
// `prog` or any of the `args` contain a nul.
288+
fn make_command_line(prog: &OsStr, args: &[OsString]) -> io::Result<Vec<u16>> {
270289
// Encode the command and arguments in a command line string such
271290
// that the spawned process may recover them using CommandLineToArgvW.
272291
let mut cmd: Vec<u16> = Vec::new();
273-
append_arg(&mut cmd, prog);
292+
try!(append_arg(&mut cmd, prog));
274293
for arg in args {
275294
cmd.push(' ' as u16);
276-
append_arg(&mut cmd, arg);
295+
try!(append_arg(&mut cmd, arg));
277296
}
278-
return cmd;
297+
return Ok(cmd);
279298

280-
fn append_arg(cmd: &mut Vec<u16>, arg: &OsStr) {
299+
fn append_arg(cmd: &mut Vec<u16>, arg: &OsStr) -> io::Result<()> {
281300
// If an argument has 0 characters then we need to quote it to ensure
282301
// that it actually gets passed through on the command line or otherwise
283302
// it will be dropped entirely when parsed on the other end.
303+
try!(ensure_no_nuls(arg));
284304
let arg_bytes = &arg.as_inner().inner.as_inner();
285305
let quote = arg_bytes.iter().any(|c| *c == b' ' || *c == b'\t')
286306
|| arg_bytes.is_empty();
@@ -312,11 +332,12 @@ fn make_command_line(prog: &OsStr, args: &[OsString]) -> Vec<u16> {
312332
}
313333
cmd.push('"' as u16);
314334
}
335+
Ok(())
315336
}
316337
}
317338

318339
fn make_envp(env: Option<&collections::HashMap<OsString, OsString>>)
319-
-> (*mut c_void, Vec<u16>) {
340+
-> io::Result<(*mut c_void, Vec<u16>)> {
320341
// On Windows we pass an "environment block" which is not a char**, but
321342
// rather a concatenation of null-terminated k=v\0 sequences, with a final
322343
// \0 to terminate.
@@ -325,26 +346,27 @@ fn make_envp(env: Option<&collections::HashMap<OsString, OsString>>)
325346
let mut blk = Vec::new();
326347

327348
for pair in env {
328-
blk.extend(pair.0.encode_wide());
349+
blk.extend(try!(ensure_no_nuls(pair.0)).encode_wide());
329350
blk.push('=' as u16);
330-
blk.extend(pair.1.encode_wide());
351+
blk.extend(try!(ensure_no_nuls(pair.1)).encode_wide());
331352
blk.push(0);
332353
}
333354
blk.push(0);
334-
(blk.as_mut_ptr() as *mut c_void, blk)
355+
Ok((blk.as_mut_ptr() as *mut c_void, blk))
335356
}
336-
_ => (ptr::null_mut(), Vec::new())
357+
_ => Ok((ptr::null_mut(), Vec::new()))
337358
}
338359
}
339360

340-
fn make_dirp(d: Option<&OsString>) -> (*const u16, Vec<u16>) {
361+
fn make_dirp(d: Option<&OsString>) -> io::Result<(*const u16, Vec<u16>)> {
362+
341363
match d {
342364
Some(dir) => {
343-
let mut dir_str: Vec<u16> = dir.encode_wide().collect();
365+
let mut dir_str: Vec<u16> = try!(ensure_no_nuls(dir)).encode_wide().collect();
344366
dir_str.push(0);
345-
(dir_str.as_ptr(), dir_str)
367+
Ok((dir_str.as_ptr(), dir_str))
346368
},
347-
None => (ptr::null(), Vec::new())
369+
None => Ok((ptr::null(), Vec::new()))
348370
}
349371
}
350372

@@ -397,11 +419,12 @@ mod tests {
397419
#[test]
398420
fn test_make_command_line() {
399421
fn test_wrapper(prog: &str, args: &[&str]) -> String {
400-
String::from_utf16(
401-
&make_command_line(OsStr::new(prog),
402-
&args.iter()
403-
.map(|a| OsString::from(a))
404-
.collect::<Vec<OsString>>())).unwrap()
422+
let command_line = &make_command_line(OsStr::new(prog),
423+
&args.iter()
424+
.map(|a| OsString::from(a))
425+
.collect::<Vec<OsString>>())
426+
.unwrap();
427+
String::from_utf16(command_line).unwrap()
405428
}
406429

407430
assert_eq!(

0 commit comments

Comments
 (0)