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's current_dir behaves differently on Unix and Windows, with respect to relative exe paths #37868

Open
oconnor663 opened this issue Nov 18, 2016 · 13 comments
Labels
A-process Area: `std::process` and `std::env` C-feature-request Category: A feature request, i.e: not implemented / a PR. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@oconnor663
Copy link
Contributor

On Unix, relative paths to the exe are interpreted after cd'ing to the current_dir, but on Windows they're interpreted before. Here's an example test (1 test file, and 1 little binary that the test needs to build) that demonstrates this if you run cargo test on the two different platforms:

src/lib.rs

#[test]
fn execute_binary_from_another_dir() {
    // Run `cargo build` to make sure the associated `foo` binary gets built.
    std::process::Command::new("cargo").arg("build").status().unwrap();

    // Shell out to `foo`, just to confirm that this works fine.
    std::process::Command::new("target/debug/foo")
        .status()
        .unwrap();

    // Now try to shell out to `foo` again while also setting the current_dir.
    // This fails on Unix, because the path to the binary is resolved *after*
    // changing directories, but it works on Windows.
    std::process::Command::new("target/debug/foo")
        .current_dir("..")
        .status()
        .unwrap();
}

src/bin/foo.rs

fn main() {
    println!("foo!");
}

I ran into a similar difference when I tried this with Python's subprocess.run(..., cwd=...). It looks like it's an artifact of Windows supporting an lpCurrentDirectory arg natively, while Unix has to fork and then chdir in the child before it calls exec. I prefer the semantics that we're getting on Windows right now*, but this issue is mainly that they're not the same.

* My main reason for preferring the current-Windows-style "current_dir does not affect the exe location" semantics: Emulating the current-Unix-style only requires a Path::join, which always works. But going the other way requires a Path::canonicalize, which can fail.

@sfackler sfackler added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Nov 19, 2016
@alexcrichton
Copy link
Member

Interesting! I believe your diagnosis is spot on as well.

A possible fix to this would be to always pass an absolute path on Unix but that'd involve probing which would inevitably go wrong somewhere... I agree though that the Windows semantics make more sense to me here.

@oconnor663
Copy link
Contributor Author

@alexcrichton would it be possible to exec the new program with fexecve instead of execvp? I think would let us open the executable file before we change directories, and then execute it afterwards from its file descriptor. But it doesn't look like that's exposed in libc yet, and I don't know how portable it is?

@alexcrichton
Copy link
Member

mind blown

Whoa I had no idea that API even existed! The portability does worry me though, especially this clause from the man page:

On Linux, fexecve() is implemented using the proc(5) file system, so /proc needs to be mounted and available at the time of the call.

We've had instances before of Rust binaries running without /proc and random things in glibc break (like thread info stuff). Putting this on Command::new may be a bit hindering :(

@oconnor663
Copy link
Contributor Author

oconnor663 commented Dec 29, 2016

duct is currently working around this by doing a canonicalize under the covers. My reasoning here is that errors are acceptable, because in the situations that trigger this (relative program name, current_dir is in use) it's pretty much guaranteed that the invocation would fail otherwise anyway, unless the caller was actively prepared for the Unix semantics. But it's easier to say that as a higher level library, than it is as the stdlib.

@Mark-Simulacrum Mark-Simulacrum added the C-feature-request Category: A feature request, i.e: not implemented / a PR. label Jul 26, 2017
@dtolnay dtolnay added the I-needs-decision Issue: In need of a decision. label Nov 18, 2017
@rzhikharevich
Copy link

I've just ran into this problem (specifically, the Unix behavior being rather unintuitive and undocumented).

@alexcrichton could you please describe what's wrong with making an absolute path with canonicalize before forking?

@alexcrichton
Copy link
Member

@rzhikharevich I don't know of anything that could concretely go wrong right now, I'm just assuming that because this is low-level code used by tons of applications that the solution is bound to not be as simple as "just use canonicalize". In implementing a fix for this we basically just need to proceed with caution.

@oconnor663
Copy link
Contributor Author

I'll note that I've seen canonicalize fail before on a VirtualBox shared drive, because the filesystem itself didn't support it. In that case I think a fallback of manually traversing .. entries would actually have worked, but I wouldn't be surprised if that created other issues. Just spitballing, it's possible that a manual traversal would give a different answer than canonicalize would have -- since I think the filesystem is allowed to implement canonicalize however it likes -- which could then lead to really weird behavior if canonicalize was failing say 50% of the time.

Even though it's not the semantics I prefer, doing a Path::join on Windows so that we always get the Unix behavior might be simpler. And in the defense of that behavior, relative paths passed to the child process as command line arguments will of course interact with current_dir, so having the binary path interact too isn't totally inconsistent.

But either way, this would probably constitute a breaking change at this point. Or at least, we'd have to be super careful to check the ecosystem and see who's depending on these details. I might go ahead and open a PR to document the current behavior at least.

@retep998
Copy link
Member

We should never be using canonicalize on Windows unless we want to resolve symbolic links. If we just want to make a relative path absolute we should be using GetFullPathNameW.

@oconnor663
Copy link
Contributor Author

@alexcrichton @retep998 @rzhikharevich I just added a docs PR above. Please take a look if you have time.

bors added a commit that referenced this issue Aug 28, 2018
document the platform-specific behavior of Command::current_dir

See also #37868.

Here's my initial wording:

> Note that if the program path is relative (e.g. `"./script.sh"`), the interaction between that path and `current_dir` varies across platforms. Windows currently ignores `current_dir` when locating the program, but Unix-like systems interpret the program path relative to `current_dir`. These implementation details aren't considered stable, and it's recommended to call `canonicalize` to get an absolute program path instead of using relative paths and `current_dir` together.

I'd like to get feedback on:

- _Should_ we consider those details stable? It might be disruptive to change them, regardless of what I can get away with claiming in docs :)
- Is `canonicalize` an appropriate recommendation? As discussed in #37868 above, there are reasons it's not called automatically in the `Command` implementation.
@benesch
Copy link
Contributor

benesch commented Oct 21, 2019

Just stumbled across this myself. Or rather ran rather headfirst into it. The latest macOS makes this situation even worse, in that an invocation like

Command::new("./script").current_dir("some_dir").status().unwrap()

will both a) successfully spawn the process some_dir/script (assuming said file exists) and b) panic with an ENOENT error. This actually might be a Rust bug; it's not clear to me whether the ENOENT is something macOS's posix_spawn function is incorrectly generating or whether Rust is doing something that generates an ENOENT after calling posix_spawn.

What are our options here? Is there any way we can disallow current_dir when the command is relative? The current behavior is a massive footgun, IMO.

benesch added a commit to benesch/rust-rdkafka that referenced this issue Oct 21, 2019
std::process::Command is known to be unpredictable when a relative path
is used together with `current_dir`, and outright fails on macOS
Catalina [0]. Absolutize paths before passing them to
std::process::Command to stay within the documented constraints of the
API.

[0]: rust-lang/rust#37868
ghost pushed a commit to vectordotdev/rust-rdkafka that referenced this issue Oct 22, 2019
std::process::Command is known to be unpredictable when a relative path
is used together with `current_dir`, and outright fails on macOS
Catalina [0]. Absolutize paths before passing them to
std::process::Command to stay within the documented constraints of the
API.

[0]: rust-lang/rust#37868
ghost pushed a commit to vectordotdev/rust-rdkafka that referenced this issue Oct 22, 2019
* Avoid use of undefined std::process::Command behavior

std::process::Command is known to be unpredictable when a relative path
is used together with `current_dir`, and outright fails on macOS
Catalina [0]. Absolutize paths before passing them to
std::process::Command to stay within the documented constraints of the
API.

[0]: rust-lang/rust#37868

* Revert "Fix build on macOS Catalina (#1)"

This reverts commit 9cb0245.

Signed-off-by: Alexander Rodin <[email protected]>
benesch added a commit to benesch/rust-rdkafka that referenced this issue Oct 22, 2019
std::process::Command is known to be unpredictable when a relative path
is used together with `current_dir`, and outright fails on macOS
Catalina [0]. Absolutize paths before passing them to
std::process::Command to stay within the documented constraints of the
API.

[0]: rust-lang/rust#37868
tuzz added a commit to tuzz/ipasir-sys that referenced this issue Jul 8, 2020
- Fix the build on MacOS Catalina. This was failing due to `current_dir`
  not working as expected, see: rust-lang/rust#37868

- Use dynamic linking the C++ standard library and infer its name in the
  same way as the cc crate. This supersedes the messy steps around
  symlinking the g++ compiler and standard library.
bors added a commit to rust-lang-ci/rust that referenced this issue Jan 17, 2021
Don't use posix_spawn_file_actions_addchdir_np on macOS.

There is a bug on macOS where using `posix_spawn_file_actions_addchdir_np` with a relative executable path will cause `posix_spawnp` to return ENOENT, even though it successfully spawned the process in the given directory.

`posix_spawn_file_actions_addchdir_np` was introduced in macOS 10.15 first released in Oct 2019.  I have tested macOS 10.15.7 and 11.0.1.

Example offending program:

```rust
use std::fs;
use std::os::unix::fs::PermissionsExt;
use std::process::*;

fn main() {
    fs::create_dir_all("bar").unwrap();
    fs::create_dir_all("foo").unwrap();
    fs::write("foo/foo.sh", "#!/bin/sh\necho hello ${PWD}\n").unwrap();
    let perms = fs::Permissions::from_mode(0o755);
    fs::set_permissions("foo/foo.sh", perms).unwrap();
    let c = Command::new("../foo/foo.sh").current_dir("bar").spawn();
    eprintln!("{:?}", c);
}
```

This prints:

```
Err(Os { code: 2, kind: NotFound, message: "No such file or directory" })
hello /Users/eric/Temp/bar
```

I wanted to open this PR to get some feedback on possible solutions.  Alternatives:
* Do nothing.
* Document the bug.
* Try to detect if the executable is a relative path on macOS, and avoid using `posix_spawn_file_actions_addchdir_np` only in that case.

I looked at the [XNU source code](https://opensource.apple.com/source/xnu/xnu-6153.141.1/bsd/kern/kern_exec.c.auto.html), but I didn't see anything obvious that would explain the behavior.  The actual chdir succeeds, it is something else further down that fails, but I couldn't see where.

EDIT: I forgot to mention, relative exe paths with `current_dir` in general are discouraged (see rust-lang#37868).  I don't know if rust-lang#37868 is fixable, since normalizing it would change the semantics for some platforms. Another option is to convert the executable to an absolute path with something like joining the cwd with the new cwd and the executable, but I'm uncertain about that.
bors added a commit to rust-lang/cargo that referenced this issue Jun 10, 2021
Fix rustc/rustdoc config values to be config-relative.

The `rustc`, `rustdoc`, `rustc_wrapper`, and `rustc_workspace_wrapper` config values (in the `[build]` table) were being interpreted literally. This caused a problem if you used a relative path like `foo/rustc`.  This would be interpreted as a relative path from whatever cwd cargo launches rustc from, which changes for different scenarios, making it essentially unusuable (since crates.io dependencies wouldn't be buildable).

Additionally, due to rust-lang/rust#37868, it is a bad idea to use relative paths.

This changes it so that those paths are config-relative.  Bare names (like "my-rustc-program") still use PATH as before.

This also includes a commit to centralize the rustc-wrapper program used by several tests so that it isn't built multiple times (and to allow several tests to work on windows).

Fixes #8202
@m-ou-se
Copy link
Member

m-ou-se commented Aug 11, 2021

This needs a proposal for moving forward, so we can provide feedback on the proposed solution.

@ChrisDenton Since you're working on the Windows-specific code around environment variables and Command recently, I'd like to hear your input on it, if you have an opinion on this issue.

@m-ou-se m-ou-se removed the I-needs-decision Issue: In need of a decision. label Aug 11, 2021
@ChrisDenton
Copy link
Member

Hm, I don't feel too strongly on this but I think resolving using the parent's current directory is more intuitive to me. In Windows the executable must be found before a new process is created which is unlike a fork-exec model that sets up the environment before locating the executable. Perhaps this accounts for my personal expectations.

That said, using the child's path should not be too hard to implement on the Windows side but if that were to happen I think it'd be great if the documentation for Command::new made this clear. It's easy to miss if it's only documented in Command::current_dir and I do think this would surprising behaviour.

Of course if *nix users would be happy with using the parent's current directory then I'd be happy too. Or if neither can be changed at this point then that's frustrating but understandable. In the future there could perhaps be an API to change how relative paths and plain file names are resolved but that's probably drifting off topic.

tl;dr I'd vote for using the parent's current directory but I'd not object if the decision goes another way.

@oconnor663
Copy link
Contributor Author

oconnor663 commented Aug 11, 2021

I also prefer the Windows behavior here, though I'm not a Windows user myself. For example, imagine that both ./run.sh and ./foo/run.sh exist. Then we could write this:

let mut my_command = Command::new("./run.sh");
my_command.status().unwrap();  // the exe in this directory
my_command.current_dir("foo");
my_command.status().unwrap();  // the same exe on Windows, or the other one on Unix

I think "the same exe" is much less surprising, given that it's the same Command object and that the command path inside is otherwise immutable.

As far as what we can do about it, gosh I don't know. Even if we were willing to break some existing code on Unix, is there an implementation we'd be comfortable with that would give us Windows-like behavior? Calling realpath or similar introduces new error cases, which could conceivably break programs that aren't using relative paths at all. I don't know the standard filesystem APIs well enough to know what all our options are here.

ishbosamiya added a commit to ishbosamiya/rt_rust that referenced this issue Nov 14, 2021
std::process::Command::current_dir() works differently on Windows and
Unix. On Unix, the current directory is set before spawning the child
process which can lead to incorrect file path for child process but on
Windows it works as expected, sets the current working directory after
spawning the child process. A simple workaround for this is to use
absolute paths.

See rust-lang/rust#37868 for more details.
@ChrisDenton ChrisDenton added the A-process Area: `std::process` and `std::env` label Jan 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-process Area: `std::process` and `std::env` C-feature-request Category: A feature request, i.e: not implemented / a PR. 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

10 participants