New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

std: Properly handle interior NULs in std::process #31056

Merged
merged 1 commit into from Feb 3, 2016

Conversation

Projects
None yet
6 participants
@kamalmarhubi
Copy link
Contributor

kamalmarhubi commented Jan 20, 2016

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

Fixes #30858
Fixes #30862

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Jan 20, 2016

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @aturon (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@kamalmarhubi

This comment has been minimized.

Copy link
Contributor Author

kamalmarhubi commented Jan 20, 2016

@rust-highfive rust-highfive assigned alexcrichton and unassigned aturon Jan 20, 2016

@kamalmarhubi

This comment has been minimized.

Copy link
Contributor Author

kamalmarhubi commented Jan 20, 2016

I think both issues will manifest on Windows as well, but I made no changes there. The added regression tests will likely cause failures on Windows.

// to that point and report an error via a `Result` rather than by
// panicking.
#[derive(Debug, Clone)]
pub enum ValidatedCString {

This comment has been minimized.

@alexcrichton

alexcrichton Jan 20, 2016

Member

In the case that an invalid string is passed down, we don't necessarily need to store the original data, so perhaps this could just store a flag in Command as to whether something with a nul byte in it has been passed in? That way once we hit spawn we can just check the flag and return an error appropriately.

This comment has been minimized.

@kamalmarhubi

kamalmarhubi Jan 20, 2016

Author Contributor

I mostly kept it so the Debug impl on std::process::Command would display something. I can change this though.

This comment has been minimized.

@alexcrichton

alexcrichton Jan 20, 2016

Member

Ah that's a good point. The Debug impl would probably be fine to start including something like <string-with-nul> or something like that instead of the actual value, however. It seems relatively complicated to define a type like this just for a Debug implementation that's unlikely to ever be seen.

This comment has been minimized.

@kamalmarhubi

kamalmarhubi Jan 20, 2016

Author Contributor

Maybe (ab)use Option for this?

pub struct Command {
    program: Option<CString>,
    args: Vec<Optrion<ValidatedCString>>,
    env: Option<HashMap<Option<CString>, Option<ValidatedCString>>>,
    cwd: Option<Option<CString>>,
    uid: Option<uid_t>,
    gid: Option<gid_t>,
    session_leader: bool,
    saw_nul: bool,
}

The Debug impl could then use unwrap_or("<string-with-nul>").

This is a bit weird for env, as the hash map would lose context of how many vars were nulful. The cwd would be a bit unwieldy with the double Option...

This comment has been minimized.

@kamalmarhubi

kamalmarhubi Jan 21, 2016

Author Contributor

@alexcrichton any thoughts on how best to handle this?

This comment has been minimized.

@alexcrichton

alexcrichton Jan 21, 2016

Member

Oh I wouldn't change the internal structure of Command much, just add a flag indicating that something with a nul byte was passed in. The Debug implementation only shows the program and its arguments, so if we really want we can store CString::new("<string-with-nul>").unwrap() in those places, but other than that we don't need to track the information elsewhere.

This comment has been minimized.

@kamalmarhubi

kamalmarhubi Jan 21, 2016

Author Contributor

Ok, so that means dropping this whole ValidatedCString thing, right? Shall I close this and open an new PR skipping that altogether?

This comment has been minimized.

@kamalmarhubi

kamalmarhubi Jan 21, 2016

Author Contributor

I also don't have a good grasp on what Windows does here. I think my tests on std::process will fail there, but without seeing the failures I'm not sure what to do to fix it.

This comment has been minimized.

@alexcrichton

alexcrichton Jan 21, 2016

Member

Nah keeping this PR is fine (just pushing the changes to it).

I do think that the tests will fail on Windows as well, but it should be pretty straightforward (e.g. checking encode_wide() for zeros) and relatively the same implementation as Unix.

This comment has been minimized.

@kamalmarhubi

kamalmarhubi Jan 22, 2016

Author Contributor

I've changed the unix version. I'll try to make changes for Windows, but it'll be kind of fumbling in the dark.

pub fn get_program(&self) -> &OsString {
&self.program
}
pub fn args_iter(&self) -> slice::Iter<OsString> {

This comment has been minimized.

@alexcrichton

alexcrichton Jan 20, 2016

Member

Perhaps this (and unix) could just be:

pub fn arg(&self) -> &[OsString]

This comment has been minimized.

@kamalmarhubi

kamalmarhubi Jan 20, 2016

Author Contributor

I think the unix version would reqiure allocation of the slice, since the underlying storage isn't OsStr. This also conflicts with the naming of arg() for the setter.

This comment has been minimized.

@alexcrichton

alexcrichton Jan 20, 2016

Member

It may be best to delegate the Debug implementation to each platform's module in that case, we try to avoid differences in APIs wherever possible across the platforms.

This comment has been minimized.

@kamalmarhubi

kamalmarhubi Jan 20, 2016

Author Contributor

The public APIs are still the same. Do you mean avoid differences across the sys modules?

This comment has been minimized.

@alexcrichton

alexcrichton Jan 20, 2016

Member

Yes, and the public APIs are a little different b/c they're in theory returning impl Iterator<Item=OsString> but the actual type being returned is quite different

This comment has been minimized.

@kamalmarhubi

kamalmarhubi Jan 20, 2016

Author Contributor

Ok, I've delegated to impls in the platform modules.


#[test]
fn test_interior_nul_in_current_dir_does_not_panic() {
Command::new("echo").current_dir("has-some-\0\0s-inside");

This comment has been minimized.

@alexcrichton

alexcrichton Jan 20, 2016

Member

For this and the above tests can you assert that spawn will return an error?

This comment has been minimized.

@kamalmarhubi

kamalmarhubi Jan 20, 2016

Author Contributor

Done.

@brson

This comment has been minimized.

Copy link
Contributor

brson commented Jan 20, 2016

I marked this as a regression since it changes behavior, but I'm in favor.

@kamalmarhubi

This comment has been minimized.

Copy link
Contributor Author

kamalmarhubi commented Jan 21, 2016

Argh I missed the warnings and didn't realise they were errors in the build. Just pushed something that should pass. There are still a couple of open discussion points though.

}
fn init_env_map(&mut self) {
if self.env.is_none() {
self.env = Some(env::vars_os().collect());
// Will not add NULs to env: preexisting environment will not contain any.
self.env = Some(env::vars_os().map(|(k, v)| (k, v)).collect());

This comment has been minimized.

@alexcrichton

alexcrichton Jan 22, 2016

Member

I don't think that this needs to change, right?

This comment has been minimized.

@kamalmarhubi

kamalmarhubi Jan 22, 2016

Author Contributor

erm yeah that's a non-perfect refactor of an intermediate stage I had.... time to apply map id => id...

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jan 22, 2016

Looks good to me! I think the Windows implementation will just need to be tweaked and this should be good to go. If you want to test things out it and you're on Unix you may be able to use this helper script I have to at least ensure the standard library itself compiles, I suspect this is a situation where "when it compiles it works" :)

@kamalmarhubi

This comment has been minimized.

Copy link
Contributor Author

kamalmarhubi commented Jan 22, 2016

Thanks for the script!

@kamalmarhubi

This comment has been minimized.

Copy link
Contributor Author

kamalmarhubi commented Feb 2, 2016

Sorry for letting this go for so long. I kept putting off learning about OsStr on windows. This should be done now!

@@ -43,13 +43,25 @@ fn mk_key(s: &OsStr) -> OsString {
})
}

fn ensure_no_nuls<T: AsRef<OsStr>>(str: T) -> io::Result<T> {
let has_nul = {
let bytes = str.as_ref().as_inner().inner.as_inner();

This comment has been minimized.

@alexcrichton

alexcrichton Feb 3, 2016

Member

Ah can this actually use the .encode_wide() method? That's what'll be passed down to the OS anyway, and is a little more reliably than looking at the internal bytes.

This comment has been minimized.

@kamalmarhubi

kamalmarhubi Feb 3, 2016

Author Contributor

The environment block is built up using .extend(), and I wanted to avoid the extra allocation and copy of the keys and values that would be necessary to check the .encode_wide() output. See https://github.com/rust-lang/rust/pull/31056/files#diff-9a0a769432651d9c59644e0a8c7f887eR353

We can avoid the repeated allocation by reusing a buffer for the conversion, but we'd still pay the extra copy cost. I'm happy to do this however you prefer, though.

This comment has been minimized.

@kamalmarhubi

kamalmarhubi Feb 3, 2016

Author Contributor

Oh I see what you mean. Ignore above!

This comment has been minimized.

@kamalmarhubi

kamalmarhubi Feb 3, 2016

Author Contributor

Done!

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Feb 3, 2016

Thanks @kamalmarhubi! Looks good to me modulo one nit and I'd be fine sending to bors after.

@kamalmarhubi

This comment has been minimized.

Copy link
Contributor Author

kamalmarhubi commented Feb 3, 2016

Yay! I'm really excited to see what the bors experience is like, so I'll try and keep this moving. :-)

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Feb 3, 2016

Thanks! Can you squash the commits down into one as well? (sorry forgot to check that last time)

@kamalmarhubi kamalmarhubi force-pushed the kamalmarhubi:std-process-nul-chars branch from b3d4b4e to 59d070c Feb 3, 2016

@kamalmarhubi

This comment has been minimized.

Copy link
Contributor Author

kamalmarhubi commented Feb 3, 2016

Done, and rebased.

@kamalmarhubi kamalmarhubi changed the title std: Properly handle interior NULs in std::process on unix std: Properly handle interior NULs in std::process Feb 3, 2016

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Feb 3, 2016

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 3, 2016

⌛️ Testing commit 59d070c with merge 28c6780...

bors added a commit that referenced this pull request Feb 3, 2016

Auto merge of #31056 - kamalmarhubi:std-process-nul-chars, r=alexcric…
…hton

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

Fixes #30858
Fixes #30862
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 3, 2016

💔 Test failed - auto-win-gnu-64-nopt-t

std: Properly handle interior NULs in std::process
This reports an error at the point of calling `Command::spawn()` or one of
its equivalents.

Fixes #30858
Fixes #30862

@kamalmarhubi kamalmarhubi force-pushed the kamalmarhubi:std-process-nul-chars branch from 59d070c to 7c64bf1 Feb 3, 2016

@kamalmarhubi

This comment has been minimized.

Copy link
Contributor Author

kamalmarhubi commented Feb 3, 2016

Fixed test and squashed. Diff: kamalmarhubi/rust@59d070c...7c64bf1

@kamalmarhubi

This comment has been minimized.

Copy link
Contributor Author

kamalmarhubi commented Feb 3, 2016

Urg pushed the wrong thing.

@kamalmarhubi

This comment has been minimized.

Copy link
Contributor Author

kamalmarhubi commented Feb 3, 2016

Actually no, I pushed the right thing I just can't get the compare url to display as I'd like. Here's the diff:

diff --git a/src/libstd/sys/windows/process.rs b/src/libstd/sys/windows/process.rs
index 758044c..61cf28b 100644
--- a/src/libstd/sys/windows/process.rs
+++ b/src/libstd/sys/windows/process.rs
@@ -419,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!(
@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Feb 3, 2016

bors added a commit that referenced this pull request Feb 3, 2016

Auto merge of #31056 - kamalmarhubi:std-process-nul-chars, r=alexcric…
…hton

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

Fixes #30858
Fixes #30862
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 3, 2016

⌛️ Testing commit 7c64bf1 with merge 8fc73c7...

bors added a commit that referenced this pull request Feb 3, 2016

Auto merge of #31056 - kamalmarhubi:std-process-nul-chars, r=alexcric…
…hton

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

Fixes #30858
Fixes #30862

@bors bors merged commit 7c64bf1 into rust-lang:master Feb 3, 2016

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment