Regression in Command::exec's PATH resolution

[euank]

Using env_clear() + exec() currently exhibits different behaviour on nightly and stable.

I believe the first nightly to regress was 2018-11-02.

A minimal reproduction is the following code:

use std::process::Command;
use std::os::unix::process::CommandExt;

fn main() {
    let mut cmd = Command::new("true");
    cmd.env_clear();
    let err = cmd.exec();
    println!("error: {}", err);
}

On stable, it prints no error. On nightly, it prints error: No such file or directory (os error 2).

Using strace -e trace=execve, we can see the difference:

# stable
execve("/bin/true", ["true"], 0x7fedf1821010 /* 0 vars */) = 0
# nightly
execve("true", ["true"], 0x561a58edbb80 /* 0 vars */) = -1 ENOENT (No such file or directory)

Meta

$ rustc --version --verbose
rustc 1.32.0-nightly (15d770400 2018-11-06)
binary: rustc
commit-hash: 15d770400eed9018f18bddf83dd65cb7789280a5
commit-date: 2018-11-06
host: x86_64-unknown-linux-gnu
release: 1.32.0-nightly
LLVM version: 8.0

[phansch]

Probably due to #55359

[Mark-Simulacrum]

Cc @rust-lang/libs @alexcrichton

[alexcrichton]

Thanks for the report! I'm... not actually sure how this ever passed on stable, it wasn't really ever intended to. #55359 is almost for sure the cause of this, though

cc @alex too

[alex]

I'm not sure I understand what the desired behavior is. We look up $PATH in the child process's environment, that's the decided on behavior. If the child is modifying unrelated environment variables, $PATH is the same as the parent's, and if $PATH itself is modified we use that. Is the desired behavior here that if env is cleared then we should look at the parent's $PATH instead? And does this apply to env_remove as well?

I'm concerned that this behavior is awfully complicated, and not consistent with any platform APIs. That said, I'm happy to implement this if we can clearly articulate what the desired behavior is.

[Mark-Simulacrum]

I believe we've backported the relevant PR to beta as well so marking as regression from stable to beta

[alex]

Beta backport hasn't landed yet, #55611.

[alexcrichton]

@alex there's not really a "desired behavior" in this case, at this point it's just whack-a-mole of trying to rationalize with what we have so far.

The original implementation happened to have the side effect of using the child's PATH to do the binary lookup. This was then duplicated on Windows. That was probably a mistake. The ship has sailed, however, and now we need to make sure bugs are fixed and hopefully not break too much.

[alex]

Ok. I'll try to get a patch for this by this evening.

[euank]

And does this apply to env_remove as well?

Yes, env_remove also technically exhibits the same regression, though it seems somewhat less likely to me that someone might accidentally remove PATH via env_remove("PATH") than via env_clear().

Perhaps it could be argued that the env_remove case is unlikely to impact anyone... though, with a github search, I found one use ofenv_remove that looks like it would break.

I know that case uses spawn, not exec, but after checking, it turns out spawn exhibits the same change in behaviour, presumably because it uses do_exec under the hood.

I agree the new behaviour makes more sense, but I think backwards compatibility within 1.0 is more important if it's reasonably achievable.

Thank you for the quick response by the way!

[alex]

PR is up at #55793

[alexcrichton]

@euank can you elaborate a bit more on your use case for this? After browsing glibc's source in #55793 it looks like this would only have previously worked for binaries in /bin or /usr/bin (because that's hardcoded in glibc). Is that the behavior you're seeing as well?

[euank]

can you elaborate a bit more on your use case for this?

I wouldn't say that what I have is a use-case per-se, it's simply code that happened to work with env_clear and then eventually broke on nightly. I don't mind setting my own PATH in my code if needed -- env_clear was meant as a shorthand to remove a bunch of other variables, not to do anything PATH related.

My reason for reporting this issue is because it's a regression which might impact other users.
If we still want to make a decision to keep the new behavior, at least it will be a conscious decision to to potentially break some users.

looks like this would only have previously worked for binaries in /bin or /usr/bin

I didn't realize that, but indeed the commands I'm running are in /usr/bin.

[alexcrichton]

Ok thanks for the info! Just wanted to confirm at least, and the report is still very much appreciated!

[alexcrichton]

The backport of the fix for this (#55939) is posted at #55978