Command::spawn has weird rules for finding binaries on Windows

de64fce
Opened by Oliver Middleton at 2024-03-25 16:15:41

The offending code is here which is apparently to have it search the child's %Path% for the binary to fix #15149. It has a few issues though.

  1. It is only triggered when the child's environment has been customised. This means Command::new("foo.exe").spawn() and Command::new("foo.exe").env("Thing", "").spawn() uses different rules for finding the binary which seems unexpected.
  2. It will replace any extension on the program name with .exe. This means Command::new("foo.bar").env("Thing", "").spawn() attempts to launch foo.exe first. Command::new("foo.bar").spawn() correctly tries to launch foo.bar as CreateProcess will not replace the extension.
  3. If the program name is an absolute path then this is still triggered but will just try that path for every item in the child's %Path%. For example Command::new(r"C:\foo.bar").env("Thing", "").spawn() looks for C:\foo.exe several times.
  4. The use of .to_str().unwrap() means it panic!s if the program name is not valid Unicode.
  5. Prehaps the biggest issue, but maybe it's itentional, is that none of the logic for finding binaries seems to be documented (std::process::Command).

An easy way to fix this is to just remove the hack so we just rely on the behaviour of CreateProcess on Windows which is a least documented. The behaviour is already very different between Windows and Linux and we should probably just accept that in order to get reliable results cross-platform it's best to use absolute paths.

  1. Personally I'd much rather we didn't have this hack and just accepted that Windows and Unix are different and document their differences.

    Peter Atashian at 2016-11-01 18:38:39

  2. @ollie27 these all sound like bugs to me rather than weird behavior. Removing this behavior is not an option as the are plenty of programs currently relying on this behavior. Would be great to fix the issues, however!

    Alex Crichton at 2016-11-02 16:55:20

  3. It would definitely be nicer if Command didn't try to be too smart here, but breaking existing usage would also suck. For sccache I contributed to the which crate, which will append an exe extension on Windows if necessary (but not replace an existing extension). One thing that the which crate doesn't implement is support for using the extensions from the PATHEXT environment variable, which on my Windows 10 machine does include .COM. (The Python which package does support this.)

    Ted Mielczarek at 2018-02-01 14:10:08

  4. Another issue is that .spawn() assumes that ".exe" is the only extension that Windows' executables have: https://github.com/rust-lang/rust/blob/1.24.0/src/libstd/sys/windows/process.rs#L154 This ignores ".cmd", ".com", etc

    Ron Waldon-Howe at 2018-02-25 11:09:15

  5. Another issue is that .spawn() assumes that ".exe" is the only extension that Windows' executables have

    I just found in https://github.com/rust-lang/rust/issues/50870#issuecomment-390309938 that it's consistent with CreateProcess to only try ".exe".

    Josh Stone at 2018-05-18 19:38:08

  6. I just found a documentation about how file extensions work in Windows, and it might help since CMD finds the application to open a file with %PATHEXT% using this information (I found this on Stack Overflow answer, but it may be wrong.).

    Sung Jeon at 2020-03-06 14:31:53

  7. One thing that the which crate doesn't implement is support for using the extensions from the PATHEXT environment variable, which on my Windows 10 machine does include .COM.

    By the way @luser, after reading through this thread I found that in the interim the which crate has gained support for other extensions through PATHEXT.

    Adam Perry at 2020-08-23 01:10:22

  8. The particular code in question has been replaced as a side effect of #87704. Most notably the behaviour is now much more consistent.

    So I think this can be closed.

    Chris Denton at 2021-12-02 18:16:22

  9. Command::spawn cannot launch *.cmd file located in PATH if you don't specify extension.

    E.g. launching VSCode doesn't work:

    Command::new("code").spawn().unwrap(); // Error { kind: NotFound, message: "program not found" }

    CMD and PowerShell launch code without extension just fine. I would expect Rust's Command behaviour to match CMD and PowerShell.

    In Rust you need to specify extension Command::new("code.cmd") or launch with CMD Command::new("cmd").arg("/C").arg("code").

    kryptan at 2023-08-26 19:46:04

  10. If some of you weren't aware, it might help to know that the which crate has logic for handling this without requiring you to append .cmd, .com, .bat, .exe, etc. This is based on the PATHEXT environment variable on Windows, which you don't have to set it yourself.

    See https://github.com/harryfei/which-rs/blob/e5ec71143965a7c7751c853fb229cb973bfcaa13/src/finder.rs#L158-L162

    Hope that helps others like it helped me!

    Jorge Israel Peña at 2023-08-26 21:46:38

  11. This thread was about buggy behaviour that has since been fixed (aside from improving the documentation).

    The fact that Command is a thin wrapper around CreateProcessW is intentional. If you would like more shell-like behaviour then using a crate is definitely the preferred method.

    I opened #94743 to discuss running scripts, etc, from Command but there was a distinct lack of enthusiasm for it. I'd suggest moving conversation there if there's a strong argument for it. Or open an API Change Proposal (ACP) on the library team's repo.

    Chris Denton at 2023-08-26 21:57:18

  12. Perhaps helpful - https://github.com/cli/safeexec

    tison at 2024-03-24 07:10:43

  13. This thread was about buggy behaviour that has since been fixed (aside from improving the documentation).

    The fact that Command is a thin wrapper around CreateProcessW is intentional. If you would like more shell-like behaviour then using a crate is definitely the preferred method.

    I opened #94743 to discuss running scripts, etc, from Command but there was a distinct lack of enthusiasm for it. I'd suggest moving conversation there if there's a strong argument for it. Or open an API Change Proposal (ACP) on the library team's repo.

    Adding more rustdocs to clarify this sounds like a good idea. Maybe someone who regularly works with this on Windows can put up a PR?

    Martin Pool at 2024-03-25 16:15:41