Don't use posix_spawn_file_actions_addchdir_np on macOS. #80537

ehuss · 2020-12-30T23:18:34Z

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:

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, 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 #37868). I don't know if #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.

rust-highfive · 2020-12-30T23:18:38Z

r? @dtolnay

(rust-highfive has picked a reviewer for you, use r? to override)

tmiasko · 2020-12-31T13:08:02Z

The bug is in posix_spawnp implementation. After successful posix_spawn it calls stat on the specified executable path, which fails and breaks out of switch, and eventually returns ENOENT: https://opensource.apple.com/source/Libc/Libc-1353.100.2/sys/posix_spawn.c.auto.html

ehuss · 2020-12-31T14:38:46Z

Interesting, thanks for taking a look! Comments like "We hope that the race for a stat() is unimportant." are great. I guess another option in theory we could use posix_spawn directly and replicate the PATH search ourselves, although that seems unpleasant.

Also, to be clear, I'm fine with the "do nothing" option. It just seems like an easy footgun, and there doesn't seem to be a clear path forward for #37868, which if it used an absolute path I think would fix this.

I'm not sure I understood the discussion about canonicalize, because I would think it would be as simple as "if exe contains '/', join it with the current directory". I wouldn't think it would be necessary to use realpath, since the OS will handle all the other work. I know env::current_dir could fail, but I assume in that case (and exe isn't absolute), the call to spawn would fail, too.

A question is, are people willing to change the semantics of Command::current_dir (to make it consistent across platforms), or should we leave it in its confusing state, or should it be deprecated and replaced with something better?

tmiasko · 2021-01-05T09:16:57Z

The macOS bug is quite severe when combined with Command::current_dir. Using a workaround to avoid it feels well deserved in this case. The approached proposed in this PR, which fall backs to fork & exec and restores the previous behaviour looks fine to me. I would also add a regression test, e.g., in src/test/ui/command.

ehuss · 2021-01-08T21:32:25Z

OK, I have added a test. I think it should pass on all platforms, but I am not absolutely certain about some of the more unusual platforms (I tested linux, macos, and windows).

dtolnay · 2021-01-11T02:40:03Z

@bors r+

bors · 2021-01-11T02:40:04Z

📌 Commit 69a93fc86e7dc3e53b27d448de7e5e3e693d8c00 has been approved by dtolnay

bors · 2021-01-11T02:47:52Z

⌛ Testing commit 69a93fc86e7dc3e53b27d448de7e5e3e693d8c00 with merge 21fd9891c3033f3ded7aa9e56ed16de03ab7779d...

rust-log-analyzer · 2021-01-11T03:22:02Z

The job arm-android failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)


------------------------------------------
stderr:
------------------------------------------
cwd="/"
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Os { code: 30, kind: Other, message: "Read-only file system" }', /checkout/src/test/ui/command/command-current-dir.rs:23:30

------------------------------------------


---

Some tests failed in compiletest suite=ui mode=ui host=x86_64-unknown-linux-gnu target=arm-linux-androideabi


command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/compiletest" "--compile-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib" "--run-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/arm-linux-androideabi/lib" "--rustc-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "--src-base" "/checkout/src/test/ui" "--build-base" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui" "--stage-id" "stage2-arm-linux-androideabi" "--suite" "ui" "--mode" "ui" "--target" "arm-linux-androideabi" "--host" "x86_64-unknown-linux-gnu" "--llvm-filecheck" "/checkout/obj/build/x86_64-unknown-linux-gnu/llvm/build/bin/FileCheck" "--linker" "/android/ndk/arm-14/bin/arm-linux-androideabi-clang" "--host-rustcflags" "-Crpath -O -Cdebuginfo=0 -Zunstable-options  -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--target-rustcflags" "-Crpath -O -Cdebuginfo=0 -Zunstable-options  -Lnative=/checkout/obj/build/arm-linux-androideabi/native/rust-test-helpers" "--docck-python" "/usr/bin/python3" "--lldb-python" "/usr/bin/python3" "--llvm-version" "11.0.0-rust-1.51.0-nightly" "--llvm-components" "aarch64 aarch64asmparser aarch64codegen aarch64desc aarch64disassembler aarch64info aarch64utils aggressiveinstcombine all all-targets analysis arm armasmparser armcodegen armdesc armdisassembler arminfo armutils asmparser asmprinter avr avrasmparser avrcodegen avrdesc avrdisassembler avrinfo binaryformat bitreader bitstreamreader bitwriter cfguard codegen core coroutines coverage debuginfocodeview debuginfodwarf debuginfogsym debuginfomsf debuginfopdb demangle dlltooldriver dwarflinker engine executionengine extensions frontendopenmp fuzzmutate globalisel gtest gtest_main hexagon hexagonasmparser hexagoncodegen hexagondesc hexagondisassembler hexagoninfo instcombine instrumentation interpreter ipo irreader jitlink libdriver lineeditor linker lto mc mca mcdisassembler mcjit mcparser mips mipsasmparser mipscodegen mipsdesc mipsdisassembler mipsinfo mirparser msp430 msp430asmparser msp430codegen msp430desc msp430disassembler msp430info native nativecodegen nvptx nvptxcodegen nvptxdesc nvptxinfo objcarcopts object objectyaml option orcerror orcjit passes powerpc powerpcasmparser powerpccodegen powerpcdesc powerpcdisassembler powerpcinfo profiledata remarks riscv riscvasmparser riscvcodegen riscvdesc riscvdisassembler riscvinfo riscvutils runtimedyld scalaropts selectiondag sparc sparcasmparser sparccodegen sparcdesc sparcdisassembler sparcinfo support symbolize systemz systemzasmparser systemzcodegen systemzdesc systemzdisassembler systemzinfo tablegen target testingsupport textapi transformutils vectorize webassembly webassemblyasmparser webassemblycodegen webassemblydesc webassemblydisassembler webassemblyinfo windowsmanifest x86 x86asmparser x86codegen x86desc x86disassembler x86info xray" "--cc" "" "--cxx" "" "--cflags" "" "--remote-test-client" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/remote-test-client" "--adb-path" "adb" "--adb-test-dir" "/data/tmp/work" "--android-cross-path" "/android/ndk/arm-14" "--color" "always"


failed to run: /checkout/obj/build/bootstrap/debug/bootstrap --stage 2 test --host= --target arm-linux-androideabi
Build completed unsuccessfully in 0:30:16

bors · 2021-01-11T03:24:10Z

💔 Test failed - checks-actions

remote-test-server does not set the current_dir, and leaves it as `/`.

ehuss · 2021-01-17T23:17:58Z

I pushed a small fix to make the test work on Android. An alternative fix would be to change remote-test-server to set the current_dir when it spawns a test, but I figure just updating the test would make it more robust.

@bors r=dtolnay

bors · 2021-01-17T23:18:00Z

📌 Commit 6e467b7 has been approved by dtolnay

bors · 2021-01-17T23:44:51Z

⌛ Testing commit 6e467b7 with merge c4df63f...

bors · 2021-01-18T02:43:11Z

☀️ Test successful - checks-actions
Approved by: dtolnay
Pushing c4df63f to master...

This is a complete rewrite `posix/runProcess.c`. There are a few goals of this rewrite: * fix a long-standing and serious bug in the `execvpe` fallback path, which uses non-reentrant functions after `fork`ing. This is of course undefined behavior and has been causing failures under Darwin's Rosetta binary translation engine (see GHC #19994). * eliminate code duplication in the `fork/exec` implementation. * introduce support for `posix_spawn`, allowing us to unload a significant amount of complexity in some cases. This is particularly desireable as the cost of `fork` has increased considerably in some cases on recent Darwin releases (namely when `MAP_JIT` mappings are used; see [1]) While `posix_spawn` is often a win, there are unfortunately several cases where it cannot be used: * `posix_spawn_file_actions_addchdir_np` is broken on Darwin * `POSIX_SPAWN_SETSID` is only supported on mac 10.15 and later, but doesn't return a proper error code when not supported * the originally-specified semantics of `posix_spawn_file_actions_adddup2` are unsafe and have been amended (see [3]) but not all implementations have caught up (musl has [4], glibc did later [5], Darwin seemingly hasn't) there appears to be no support at all for setuid and setgid * `spawn` is significantly slower than fork on some Darwin releases (see [6]) To address this we first try using `posix_spawn`, falling back on `fork/exec` if we encounter a case which the former cannot handle. [1]: libuv/libuv#3064 [2]: https://www.austingroupbugs.net/view.php?id=411 [3]: rust-lang/rust#80537 [4]: https://git.musl-libc.org/cgit/musl/commit/?id=6fc6ca1a323bc0b6b9e9cdc8fa72221ae18fe206 [5]: https://sourceware.org/bugzilla/show_bug.cgi?id=23640 [6]: https://discuss.python.org/t/multiprocessing-spawn-default-on-macos-since-python-3-8-is-slower-than-fork-method/5910/4

Currently, on macOS, Rust never uses the fast posix_spawn path if a directory change is requested due to a bug in Apple's libc. However, the bug is only triggered if the program is a relative path. This PR makes it so that the fast path continues to work if the program is an absolute path or a lone filename. This was an alternative proposed in rust-lang#80537 (comment), and it makes a measurable performance difference in some of my code that spawns thousands of processes.

…howers Use posix_spawn for absolute paths on macOS Currently, on macOS, Rust never uses the fast posix_spawn path if a directory change is requested, due to a bug in Apple's libc. However, the bug is only triggered if the program is a relative path. This PR makes it so that the fast path continues to work if the program is an absolute path or a lone filename. This was an alternative proposed in rust-lang#80537 (comment), and it makes a measurable performance difference in some of my code that spawns thousands of processes.

Currently, on macOS, Rust never uses the fast posix_spawn path if a directory change is requested due to a bug in Apple's libc. However, the bug is only triggered if the program is a relative path. This PR makes it so that the fast path continues to work if the program is an absolute path or a lone filename. This was an alternative proposed in rust-lang/rust#80537 (comment), and it makes a measurable performance difference in some of my code that spawns thousands of processes.

Use posix_spawn for absolute paths on macOS Currently, on macOS, Rust never uses the fast posix_spawn path if a directory change is requested, due to a bug in Apple's libc. However, the bug is only triggered if the program is a relative path. This PR makes it so that the fast path continues to work if the program is an absolute path or a lone filename. This was an alternative proposed in rust-lang/rust#80537 (comment), and it makes a measurable performance difference in some of my code that spawns thousands of processes.

rust-highfive assigned dtolnay Dec 30, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 30, 2020

tmiasko mentioned this pull request Jan 8, 2021

Issue with std::process::Command on macOS. #80819

Closed

mmulet mentioned this pull request Jan 8, 2021

Can't compile sixel-sys on windows or macOS (works fine in linux) AdnoC/sixel-sys#1

Open

ehuss force-pushed the macos-posix-spawn-chdir branch 4 times, most recently from a6a605b to 69a93fc Compare January 8, 2021 21:31

dtolnay approved these changes Jan 11, 2021

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 11, 2021

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 11, 2021

tdyas mentioned this pull request Jan 12, 2021

Failure to launch process with a materialized arg0 on rust 1.49.0 and OSX pantsbuild/pants#11406

Closed

ehuss added 3 commits January 17, 2021 09:51

Don't use posix_spawn_file_actions_addchdir_np on macOS.

a938725

Add test for Command::current_dir behavior.

fcbcc97

Fix test to work with remote-test-server.

6e467b7

remote-test-server does not set the current_dir, and leaves it as `/`.

ehuss force-pushed the macos-posix-spawn-chdir branch from 69a93fc to 6e467b7 Compare January 17, 2021 22:03

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 17, 2021

bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 18, 2021

bors merged commit c4df63f into rust-lang:master Jan 18, 2021

rustbot added this to the 1.51.0 milestone Jan 18, 2021

bgamari mentioned this pull request Jun 28, 2021

Refactoring of POSIX process logic haskell/process#208

Merged

sunshowers mentioned this pull request Aug 20, 2022

Use posix_spawn for absolute paths on macOS #100786

Merged

rgrinberg mentioned this pull request Nov 30, 2022

Use posix_spawn on macOS 12 janestreet/spawn#47

Draft

PodTheCoder mentioned this pull request Oct 31, 2023

Errors while setting up on MacOS PodTheCoder/edge_staking_gui#6

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't use posix_spawn_file_actions_addchdir_np on macOS. #80537

Don't use posix_spawn_file_actions_addchdir_np on macOS. #80537

ehuss commented Dec 30, 2020 •

edited

Loading

rust-highfive commented Dec 30, 2020

tmiasko commented Dec 31, 2020 •

edited

Loading

ehuss commented Dec 31, 2020

tmiasko commented Jan 5, 2021

ehuss commented Jan 8, 2021

dtolnay commented Jan 11, 2021

bors commented Jan 11, 2021

bors commented Jan 11, 2021

rust-log-analyzer commented Jan 11, 2021

bors commented Jan 11, 2021

ehuss commented Jan 17, 2021

bors commented Jan 17, 2021

bors commented Jan 17, 2021

bors commented Jan 18, 2021

Don't use posix_spawn_file_actions_addchdir_np on macOS. #80537

Don't use posix_spawn_file_actions_addchdir_np on macOS. #80537

Conversation

ehuss commented Dec 30, 2020 • edited Loading

rust-highfive commented Dec 30, 2020

tmiasko commented Dec 31, 2020 • edited Loading

ehuss commented Dec 31, 2020

tmiasko commented Jan 5, 2021

ehuss commented Jan 8, 2021

dtolnay commented Jan 11, 2021

bors commented Jan 11, 2021

bors commented Jan 11, 2021

rust-log-analyzer commented Jan 11, 2021

bors commented Jan 11, 2021

ehuss commented Jan 17, 2021

bors commented Jan 17, 2021

bors commented Jan 17, 2021

bors commented Jan 18, 2021

ehuss commented Dec 30, 2020 •

edited

Loading

tmiasko commented Dec 31, 2020 •

edited

Loading