Skip to content
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::process::Command sometimes ignores PATH env variable on Windows #122660

Open
jpikl opened this issue Mar 17, 2024 · 5 comments
Open

std::process::Command sometimes ignores PATH env variable on Windows #122660

jpikl opened this issue Mar 17, 2024 · 5 comments
Labels
C-bug Category: This is a bug. O-windows Operating system: Windows T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@jpikl
Copy link

jpikl commented Mar 17, 2024

I'm building a tool which spawns a shell process. Here's a very simplified version:

use std::process::{Command, Stdio};

fn main() {
    Command::new("bash")
        .arg("-c")
        .arg("uname")
        .stdin(Stdio::null())
        .spawn()
        .unwrap()
        .wait()
        .unwrap();
}

On my Windows system, there are 2 bash versions:

  • One provided by WSL - C:\Windows\System32\bash.exe
  • MinGW version from Git for Windows - C:\Program Files\Git\bin\bash.exe

The PATH environment variable is configured as PATH=C:\Program Files\Git\bin\;C:\Windows\System32\;..., so the MinGW version should have precedence. And when I run the following from cmd.exe:

bash -c uname

it produces, the expected output:

MINGW64_NT-10.0-19045

Yet, the Rust example will ignore the PATH, use the WSL instead and output:

Linux

The funny thing is that if I modify the example and just add some random environment variable:

use std::process::{Command, Stdio};

fn main() {
    Command::new("bash")
        .env("A", "B") // <-- This !!!!
        .arg("-c")
        .arg("uname")
        .stdin(Stdio::null())
        .spawn()
        .unwrap()
        .wait()
        .unwrap();
}

then the Rust example will use the expected MinGW version and produce:

MINGW64_NT-10.0-19045

I have investigated a little bit and I think the problem lies in windows search path implementation

When std::process::Command::env is set, the 1. Child paths branch searches the PATH as first and it works as expected.

Otherwise, the 3 & 4. System paths part of code finds the bash executable under C:\Windows\System32 which produces the unexpected behavior.

I think we could fix this by searching 5. Parent paths before 3 & 4. System paths but I'm not sure if this change could negatively impact something else.


Tested on

rustc --version
rustc 1.76.0 (07dca489a 2024-02-04)

rustc +nightly --version
rustc 1.78.0-nightly (c67326b06 2024-03-15)
@jpikl jpikl added the C-bug Category: This is a bug. label Mar 17, 2024
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Mar 17, 2024
@jieyouxu jieyouxu added O-windows Operating system: Windows T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Mar 17, 2024
@ChrisDenton
Copy link
Member

Ooph yeah, this is semi-intentional but for historic reasons. So please bare with me while I give a history lesson 🙂

The search order in CreateProcessW (the underlying API) searches the current directory and system directories before PATH.

At some point some code was added to try to mimic Linux's search order but it was super buggy, Maybe it worked originally but broke over time due to later refactoring that went untested. This lead to (amongst other things) the behaviour you've seen.

When I came along, it was to fix a security issue. We didn't want to ever search the current directory so I rewrote the search manually. I opted to (mostly) preserve the existing behaviour except for removing the current directory because I wanted to avoid too many technically breaking changes at once. I did admittedly fix some bugs though.


The issue now is deciding which behaviour we want to use consistently. This may involve breaking someone's workflow. Our options are:

  • Just search the PATH environment variable. No more, no less. This is my preference.
  • Try to mimic the Linux behaviour again. The only difference with the first bullet point is that Linux APIs search the child's PATH whereas Windows APIs, by default, searches the current process' PATH. Obviously this only matters when the two PATHs differ.
  • We could try to do both (which is closest to the current behaviour). That is search the child's PATH first then fallback to the current process if not found.

Additionally there's the question of what to do about the system paths that CreateProcessW API searches and I preserved in the rewrite. We could just remove them (again this is my preference) but I'm not sure if that would break anyone's expectations.

In any case this is ultimately an API question so tagging appropriately.

@ChrisDenton ChrisDenton added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed T-libs Relevant to the library team, which will review and decide on the PR/issue. needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Mar 17, 2024
@ChrisDenton
Copy link
Member

ChrisDenton commented Mar 18, 2024

In case the above wasn't entirely clear I'll try to explictly list the current state and future options.

Current state:

  • child's PATH environment variable (but only if the child's environment is not inherited)
  • application's path
  • system directories
  • PATH environment variable

Option 1:

  • PATH only

Option 2:

  • application's path
  • PATH

Option 3:

  • application's path
  • system directories
  • PATH

Option 4:

  • child's PATH

Option 5:

  • child's PATH
  • PATH

Option 6:

  • application's path
  • child's PATH
  • PATH

Option 7:

  • application's path
  • system directories
  • child's PATH
  • PATH

@jpikl
Copy link
Author

jpikl commented Mar 20, 2024

Additionally there's the question of what to do about the system paths that CreateProcessW API searches and I preserved in the rewrite. We could just remove them (again this is my preference) but I'm not sure if that would break anyone's expectations.

By default, the system path %SystemRoot%\System32 is usually the first entry in the PATH env variable. So, we could theoretically just search PATH, as you have suggested. But, since the PATH can be modified, it might be safer to also search the system path afterwards as a fallback.

Searching PATH before the system directory seems to me like a better approach because it gives users better control over the search order (see my use case with C:\Program Files\Git\bin\bash.exe vs C:\Windows\System32\bash.exe). I'm not sure if this can have some security implications, though.

Regarding the options, it would be convenient (for me, as Rust developer) if the std::process::Command behavior was consistent across platforms (unless there is some good reason for it not to be).

This seems like a situation where we will definitely break someone's workflow, no matter what approach we choose.

@LunarLambda
Copy link

LunarLambda commented Apr 5, 2024

Okay I kinda fell down a rabbithole with this, apologies in advance.

I think Windows should probably follow the Unix-ish variant of using the child's PATH (if set by env, since Windows lacks a fork-like mechanism that can modify PATH before exec), else the parent's one.

However:

  1. Unix has execvp, which searches PATH, and if PATH is not set, the behaviour is implementation defined.
    On Linux (and other glibc systems) this means calling confstr(_SC_PATH) which returns some path which MAY (but as of glibc 2.24, does not) include the current directory (.)
    It will also try to run the file with sh if executing it as a binary file fails. This is also defined by POSIX.
    Rust doesn't use this, however it roughly approximates what Windows does. (In different order, however.)
  2. Further complicating this, the variants of exec that don't search path (e.g. execve) CAN execute things in the current directory (of the child!). (The path argument works as-if passed to open)

I think PATH-only is a reasonable default that is relatively easy to get consistent across platforms, especially since Rust provides ways to query both the executable directory, and the working directory, so the behaviour can be implemented by the user, should they need it (Although adding them to PATH manually is maybe a little clunky.)

In a perfect world, what I would want is:

  1. Only search PATH (child's if applicable, else parent's), consistently on all platforms
  2. A method on std::os::windows::process::CommandExt to opt into the regular, pass-it-straight-to-CreateProcess behaviour, i.e. fn use_system_search_path(&mut self, use: bool) -> &mut Command. Adding this to Command to have the same apply to execve on Unix doesn't seem worth it, as it restricts relative paths to only the current directory, and std wants as little platform-specificity in its APIs as possible outside of OS-specific extension traits.

Needless to say, all of this is a pretty big mess. I'm advocating for following the principle of least surprise here as much as possible. Notably, it makes "check the value of PATH" be the correct first troubleshooting step on all platforms.

@alecmocatta
Copy link
Contributor

On GitHub actions ubuntu runners we had Command::new("bash")….spawn().unwrap() failing, despite it being in PATH and succeeding when invoked from the shell. cc actions/runner#1328 (comment) and #37519.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. O-windows Operating system: Windows T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

6 participants