Fixes #55775 -- fixed regression in Command::exec's handling of PATH.

[alex]

This restores the previous behavior where if env_clear() or env_remove() was used, the parent's PATH would be consulted.

r? @alexcrichton

[alexcrichton]

Thanks for this! To confirm, it sounded like you took a look at glibc's source to start off with, is this what glibc actually does in terms of its exec method?

[alex]

Not really, it just calls getenv("PATH"): https://github.com/bminor/glibc/blob/master/posix/execvpe.c#L81-L182

Which, under the original code, would be the new env we just built. To be honest, I don't fully understand why this behavior occurs.

[alexcrichton]

Oh I think this is the operative line of code where if PATH is undefined it defaults to "/bin:/usr/bin"

That's... really weird. Let's discuss this a bit more on the issue

[alex]

Ooof, you're right. I think I should change this patch to match that behavior. Do you agree?

[alexcrichton]

Ok yeah I think it's probably best to basically continue to match glibc in this regard, so let's have the fallback of a missing PATH env var in the child's environment use /bin:/usr/bin as the PATH substitute.

I think we perhaps later may want to seriously look into what it would take to change up PATH handling to always look up the child's executable in the parent's PATH

[alex]

Done. If nothing else we're getting some more tests for Command::exec out of this 😂

[alexcrichton]

Heh, indeed!

[alex]

This should be good to go.

[alexcrichton]

@bors: r+

[bors]

📌 Commit 893f539 has been approved by alexcrichton

[kennytm]

@bors r-

The new tests likely failed on arm-android in the rollup #55912 (comment).

[alex]

Does android not have echo in /bin or /usr/bin?

[kennytm]

@alex iirc android doesn't have /bin nor /usr/bin (Maybe it's in /system/bin)

[ollie27]

It has echo in /system/bin. Android uses a different default search path:

https://github.com/aosp-mirror/platform_bionic/blob/b87e85aac399414aeacdc372df4f6900760c7aee/libc/bionic/exec.cpp#L132

https://github.com/aosp-mirror/platform_bionic/blob/b87e85aac399414aeacdc372df4f6900760c7aee/libc/include/paths.h#L50

[alex]

Ooof, thanks.

[alex]

Added an Android specific fallback path.

[rust-highfive]

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

travis_time:end:272558b7:start=1542116151346096217,finish=1542116152369822563,duration=1023726346
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#Pull-Requests-and-Security-Restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-5.0
---

[00:32:26] travis_fold:start:tidy
travis_time:start:tidy
tidy check
[00:32:27] tidy error: /checkout/src/libstd/sys/unix/process/process_unix.rs:24: line longer than 100 chars
[00:32:28] some tidy checks failed
[00:32:28] 
[00:32:28] 
[00:32:28] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor" "--quiet"
[00:32:28] 
[00:32:28] 
[00:32:28] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:32:28] Build completed unsuccessfully in 0:00:51
[00:32:28] Build completed unsuccessfully in 0:00:51
[00:32:28] Makefile:79: recipe for target 'tidy' failed
[00:32:28] make: *** [tidy] Error 1
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:11a0edc0
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
Tue Nov 13 14:08:30 UTC 2018
---
travis_time:end:34d0c4ae:start=1542118110824780071,finish=1542118110838037945,duration=13257874
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:024c5572
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:1d001e01
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

[ollie27]

musl uses a different set of paths again:

https://git.musl-libc.org/cgit/musl/tree/src/process/execvp.c#n21

Every libc seems to differ so If we're going to hardcode these paths we'll have to check every libc.

[alex]

• OpenBSD: /usr/bin:/bin:/usr/sbin:/sbin:/usr/X11R6/bin:/usr/local/bin
• FreeBSD: /sbin:/bin:/usr/sbin:/usr/bin:/usr/local/sbin:/usr/local/bin
• NetBSD: /usr/bin:/bin:/usr/pkg/bin:/usr/local/bin

Are there other libc's I ought to check? (It's worth noting that for some of these, the fallback-path varies based on compilation options. I selected the one I understand to be the standard choice.)

[alexcrichton]

Before we go too much further down this path, perhaps we should take a step back and see if this is the right approach?

The original PR was fixing #46775 which is largely just working with environ outside the environment lock. This is actually still an issue because we may read it outside a lock.

It seems to me like a simpler solution for #55775 (the regression here) is to revert #55359 and simply access environ in a lock (and restore it on problems with exec).

@alex does that fix sound right to you?

[alex]

I don't agree with this analysis.

a) I don't believe there's any cases where we're reading from the global env without a lock. In this patch we access the temporary env directly, which is fine because we own it, and we access the process env via the public std APIs, which are safe.

b) Adding a lock would not fix the issue I described in #46775 where we have a global reference that escapes the lifetime of the value it points at.

It's possible there's a different approach than the one I took that'd work, but I don't believe additional locking is necessary or sufficient.

[alexcrichton]

Hm isn't this line reading the environment without a lock? (at least in some situations)

For adding a lock, I'm thinking the code looks like this:

let _guard = acquire_env_lock();
let old_env = *sys::os::environ();
*sys::os::environ() = new_env;
let _reset_on_drop = on_drop(|| { *sys::os::environ() = old_env; });

// do the exec

We only temporarily change the entire environment for the block, but outside the block no other thread will see a visible change

[alex]

Hmm, accessing sys::os::environ() un-synchronized in the current code may be a bug/unsafe, but I think that's unrelated to what you're actually suggesting.

Your actual proposal isn't about the locking (though it's necessary for safety), it's about changing the code back to mutating the env, so we can just let libc do it's thing, and ensuring we restore the pointer if we fail. Is that right?

[alexcrichton]

Heh sure yeah! That's a good way to put it too. Basically we take the original code and update it to restore the old env on failure. That would fix your test cases I believe (because the env doesn't change across calls to exec if they fail.

Aftewards we'd then fix the orthogonal issue of "what if other threads are running around during exec" by throwing a lock around the whole operation.

[alex]

Yes, I think that'd work.

I won't have time to work on it for another few days (probably the weekend). If anyone else is rearing to work on this, you're welcome to steal it, otherwise I"ll get to this then.

[alexcrichton]

No worries, thanks again for helping out here @alex! I've opened a PR at #55939 for the fix, so I'm gonna close this in favor of that.