Undocumented panics in std::process on unix for strings with interior nuls

[kamalmarhubi]

All the strings are taken as AsRef<OsStr> and converted to CString with unwrapping: https://github.com/rust-lang/rust/blob/master/src/libstd/sys/unix/process.rs#L83-L85

An example for Command::new (playground):

use std::process::Command;

fn main() {
    Command::new("\0");
}

The panic should be documented, or the conversion unwrap should be delayed until a call to one of the methods that start the process so that the error can be returned in a Result.

Version:

$ rustc --version --verbose
rustc 1.5.0 (3d7cd77e4 2015-12-04)
binary: rustc
commit-hash: 3d7cd77e442ce34eaac8a176ae8be17669498ebc
commit-date: 2015-12-04
host: x86_64-unknown-linux-gnu
release: 1.5.0

[talchas]

This also occurs with arg(), but env() in fact doesn't panic /at all/ for \0 (just silently gives you an environ you probably don't intend), because of accidents of implementation, which seems very sketchy.

[kamalmarhubi]

Given that it's possible to delay the conversion until spawning, I think that is the leas surprising thing to do. The env vars can be checked at that point as well.

[kamalmarhubi]

That would also be similar to what happens if you attempt to create a file (playground):

use std::fs::File;

fn main() {
    println!("{:?}", File::create("/tmp/\0"));
}

prints

Err(Error { repr: Custom(Custom { kind: InvalidInput, error: StringError("path contained a null") }) })

[kamalmarhubi]

@talchas

This also occurs with arg(), but env() in fact doesn't panic /at all/ for \0 (just silently gives you an environ you probably don't intend), because of accidents of implementation, which seems very sketchy.

I decided the behaviour with env() is different enough to warrant its own bug. It isn't very good: http://is.gd/swL9IL

[alexcrichton]

triage: I-nominated

[kamalmarhubi]

I've got a patch in the works for this.

[alexcrichton]

triage: P-medium

The libs team discussed this in triage yesterday and the conclusion was that the defer-the-error strategy is fine for Command. We discussed briefly that this may be altering the contract of Command (e.g. changing panics to results), but the Result behavior seems more consistent with the std::fs APIs.