Add Linux-specific pidfd process extensions (take 2) by voidc · Pull Request #81825 · rust-lang/rust

voidc · 2021-02-06T13:24:01Z

Continuation of #77168.
I addressed the following concerns from the original PR:

make CommandExt and ChildExt sealed traits
wrap file descriptors in PidFd struct representing ownership over the fd
add take_pidfd to take the fd out of Child
close fd when dropped

Tracking Issue: #82971

rust-highfive · 2021-02-06T13:24:04Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Mark-Simulacrum (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

library/std/src/sys/unix/process/process_unix.rs

nicokoch · 2021-02-08T08:09:58Z

library/std/src/sys/unix/process/process_unix.rs

Just checking for ENOSYS is sadly not enough here.
The syscall can also be blocked by seccomp (EPERM).

We have also found cases where new syscalls where randomly broken on some configurations (see the EOPNOTSUPP comment here. Though, I think we can ignore this case for now.

EPERM and EOPNOTSUPP might depend on the arguments of the clone3 call. It would be wrong to set HAS_CLONE3 to false if one of these errors occurred.

In that case the usual approach is to perform the probe separately with arguments that are known to fail with a specific error code, e.g. an invalid combination of flags leading to EINVAL. If that returns EPERM then we know the syscall is filtered. CLONE_SIGHAND | CLONE_CLEAR_SIGHAND could work.

Maybe we could just fallback to fork on EPERM without setting HAS_CLONE3.

That could be considered slightly suboptimal since it'll try again every time in that case, but we already have a preferred path through posix_spawn so this shouldn't be hit too frequently.

My concern was whether it would be possible for clone3 to succeed after having returned EPERM once. In case of ENOSYS we can be certain that any future calls will fail as well. For EPERM, this conclusion does potentially not hold since a seccomp filter could be defined in terms of arbitrary rules.

I don't think we need to account for arbitrary seccomp filters here. seccomp filters should produce ENOSYS, not EPERM.

library/std/src/sys/unix/process/process_unix.rs

library/std/src/os/linux/process.rs

library/std/src/sys/unix/process/process_unix.rs

Mark-Simulacrum · 2021-02-08T23:22:39Z

r? @m-ou-se or perhaps @joshtriplett

library/std/src/sys/unix/process/process_unix.rs

bors · 2021-02-13T06:06:46Z

☔ The latest upstream changes (presumably #82045) made this pull request unmergeable. Please resolve the merge conflicts.

voidc · 2021-03-02T17:37:39Z

@joshtriplett I think you were interested in the original PR. Would you mind taking a look at this?

bors · 2021-03-09T01:57:16Z

☔ The latest upstream changes (presumably #82877) made this pull request unmergeable. Please resolve the merge conflicts.

bors · 2021-07-28T00:13:18Z

📌 Commit f48caef6fc47df6acfb9d72f1a254e503e98b557 has been approved by joshtriplett

bors · 2021-07-28T00:15:59Z

⌛ Testing commit f48caef6fc47df6acfb9d72f1a254e503e98b557 with merge d1b9a4c984315ffa8365dbdfbdc5a331f5adcd8e...

bors · 2021-07-28T00:53:13Z

💔 Test failed - checks-actions

voidc · 2021-07-28T09:49:23Z

@Aaron1011 Thanks for tending to this issue! Unfortunately, I think the workaround in your commit is not enough to circumvent the LLVM bug. To my knowledge, the combination of i686, fat LTO, and opt-level=0 is already sufficient to trigger the assertion. While, on the i686-gnu target, I was able to work around the bug by ensuring opt-level > 0, the i686-gnu-nopt target disables optimizations by definition, triggering the assertion in every test using LTO. One option would be to add ignore-i686-unknown-linux-gnu to all of the tests listed below. However, landing this PR with the workaround would mean potentially breaking user's builds satisfying the three criteria mentioned above. The better option, of course, would be fixing the underlying LLVM bug. Since it appears like the bug is already fixed in the latest LLVM trunk (as you commented in #86758), maybe we could cherry-pick the commit resolving the issue?

List of failing tests

ui/abi/stack-probes-lto.rs
ui/debuginfo-lto.rs
ui/extern/issue-64655-allow-unwind-when-calling-panic-directly.rs#fat
ui/fat-lto.rs
ui/lto-many-codegen-units.rs
ui/lto-rustc-loads-linker-plugin.rs
ui/lto-still-runs-thread-dtors.rs#mir
ui/lto-still-runs-thread-dtors.rs#thir
ui/panic-runtime/lto-abort.rs
ui/panic-runtime/lto-unwind.rs
ui/sepcomp/sepcomp-lib-lto.rs

voidc · 2021-07-28T14:32:18Z

I went ahead and added -C opt-level=1 to the failing tests. However, there are more tests that specify -C lto.

List of tests

codegen/lto-removes-invokes.rs
codegen/sanitizer-memory-track-orgins.rs
codegen/sanitizer-recover.rs
debuginfo/cross-crate-type-uniquing.rs (*)
incremental/lto.rs
incremental/rlib-lto.rs
run-make-fulldeps/cdylib/Makefile
run-make-fulldeps/codegen-options-parsing/Makefile
run-make-fulldeps/cross-lang-lto/Makefile
run-make-fulldeps/issue-14500/Makefile
run-make-fulldeps/issue-64153/Makefile
run-make-fulldeps/lto-dylib-dep/Makefile
run-make-fulldeps/lto-no-link-whole-rlib/Makefile
run-make-fulldeps/lto-readonly-lib/Makefile
run-make-fulldeps/lto-smoke-c/Makefile
run-make-fulldeps/lto-smoke/Makefile
run-make-fulldeps/no-builtins-lto/Makefile
run-make-fulldeps/pgo-gen-lto/Makefile
run-make-fulldeps/reproducible-build-2/Makefile
run-make/wasm-custom-section/Makefile
run-make/wasm-export-all-symbols/Makefile
run-make/wasm-import-module/Makefile
run-make/wasm-panic-small/Makefile
run-make/wasm-stringify-ints-small/Makefile
run-make/wasm-symbols-different-module/Makefile
run-make/wasm-symbols-not-imported/Makefile
ui/abi/stack-probes-lto.rs (*)
ui/debuginfo-lto.rs (*)
ui/extern/issue-64655-allow-unwind-when-calling-panic-directly.rs (*)
ui/extern/issue-64655-extern-rust-must-allow-unwind.rs (*)
ui/fat-lto.rs (*)
ui-fulldeps/lto-syntax-extension.rs
ui/issues/issue-11154.rs (build-fail)
ui/issues/issue-44056.rs (only-x86_64)
ui/lto-and-no-bitcode-in-rlib.rs (build-fail)
ui/lto-duplicate-symbols.rs (build-fail)
ui/lto-many-codegen-units.rs (*)
ui/lto-rustc-loads-linker-plugin.rs (*)
ui/lto-still-runs-thread-dtors.rs (*)
ui/panic-runtime/lto-abort.rs (*)
ui/panic-runtime/lto-unwind.rs (*)
ui/sepcomp/sepcomp-lib-lto.rs (*)

(*): added -C opt-level=1

joshtriplett · 2021-07-30T03:16:19Z

#87610 may address this. Let's give it another try (with the workarounds removed) once that has gone in.

voidc · 2021-07-30T07:13:04Z

@joshtriplett I removed the workarounds.

Aaron1011 · 2021-07-31T20:22:38Z

#87610 has been merged, so let's try this again:

@bors r=joshtriplett

bors · 2021-07-31T20:22:39Z

📌 Commit 741176a38771b233bdd6f972929a3930682da169 has been approved by joshtriplett

bors · 2021-07-31T22:49:29Z

⌛ Testing commit 741176a38771b233bdd6f972929a3930682da169 with merge a2522c6c85a7388a80cb33edc2ce2631b0695f99...

bors · 2021-08-01T00:12:13Z

💔 Test failed - checks-actions

Aaron1011 · 2021-08-01T00:14:45Z

All of the non-Apple builders passes, so the LLVM issue is fixed 🎉

The failing tests need to be made Linux-only (or be adjusted to avoid libc::SYS_getpid on non-Linux Unix systems).

The test calls libc::getpid() in the pre_exec hook and asserts that the returned value is different from the PID of the parent. However, libc::getpid() returns the wrong value. Before version 2.25, glibc caches the PID of the current process with the goal of avoiding additional syscalls. The cached value is only updated when the wrapper functions for fork or clone are called. In PR rust-lang#81825 we switch to directly using the clone3 syscall. Thus, the cache is not updated and getpid returns the PID of the parent. source: https://man7.org/linux/man-pages/man2/getpid.2.html#NOTES

voidc · 2021-08-01T09:24:46Z

@Aaron1011 Okay, I fixed it! And thanks for seeing to the LLVM fix getting merged so fast! 😃

Aaron1011 · 2021-08-01T16:11:11Z

@bors r=joshtriplett

bors · 2021-08-01T16:11:12Z

📌 Commit 4a832d3 has been approved by joshtriplett

bors · 2021-08-01T16:45:52Z

⌛ Testing commit 4a832d3 with merge 4e21ef2...

bors · 2021-08-01T19:04:31Z

☀️ Test successful - checks-actions
Approved by: joshtriplett
Pushing 4e21ef2 to master...

Aaron1011 · 2021-08-01T19:06:12Z

@voidc: Thanks for all of your work getting this over the finish line!

voidc · 2021-08-01T19:08:53Z

🎉 🎉 Finally! Thanks for your patience @Aaron1011 and @joshtriplett during all of these 200+ comments and 15+ bors runs 😂

brauner · 2021-08-02T07:59:37Z

Thank you all for your work. Very happy to see upstream work make it into this project. :)

rust-highfive assigned Mark-Simulacrum Feb 6, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 6, 2021

This comment has been minimized.

Sign in to view

voidc commented Feb 6, 2021

View reviewed changes

library/std/src/sys/unix/process/process_unix.rs Outdated Show resolved Hide resolved

voidc commented Feb 6, 2021

View reviewed changes

library/std/src/sys/unix/process/process_unix.rs Outdated Show resolved Hide resolved

nicokoch reviewed Feb 7, 2021

View reviewed changes

library/std/src/sys/unix/process/process_unix.rs Outdated Show resolved Hide resolved

nicokoch reviewed Feb 7, 2021

View reviewed changes

library/std/src/sys/unix/process/process_unix.rs Outdated Show resolved Hide resolved

nicokoch reviewed Feb 8, 2021

View reviewed changes

library/std/src/sys/unix/process/process_unix.rs Outdated Show resolved Hide resolved

nicokoch reviewed Feb 8, 2021

View reviewed changes

library/std/src/sys/unix/process/process_unix.rs Outdated Show resolved Hide resolved

the8472 reviewed Feb 8, 2021

View reviewed changes

library/std/src/os/linux/process.rs Outdated Show resolved Hide resolved

library/std/src/sys/unix/process/process_unix.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

rust-highfive assigned m-ou-se and unassigned Mark-Simulacrum Feb 8, 2021

voidc force-pushed the pidfd branch from f2c0271 to ae00547 Compare February 10, 2021 14:39

brauner reviewed Feb 11, 2021

View reviewed changes

library/std/src/sys/unix/process/process_unix.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

voidc force-pushed the pidfd branch from daa5544 to 9abb6c8 Compare February 12, 2021 16:54

voidc force-pushed the pidfd branch from 9abb6c8 to 0714da4 Compare February 13, 2021 10:03

This comment has been minimized.

Sign in to view

JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 1, 2021

voidc force-pushed the pidfd branch from 25191e4 to 781cd6f Compare March 9, 2021 09:29

This comment has been minimized.

Sign in to view

voidc added 3 commits August 1, 2021 09:45

Add dummy FileDesc struct for doc target

2a4d012

Check whether clone3 syscall exists in pidfd test

4a832d3

voidc mentioned this pull request Aug 1, 2021

I/O safety. #87329

Merged

Aaron1011 mentioned this pull request Aug 2, 2021

Add pidfd-based implementation of tokio::process tokio-rs/tokio#4016

Closed

emilyalbini mentioned this pull request Aug 12, 2021

Fix command-create-pidfd test inside unprivileged Docker containers #87966

Merged

12101111 mentioned this pull request Oct 4, 2021

cargo 1.56 beta hang when run inside Gentoo's sandbox #89522

Closed

Aaron1011 mentioned this pull request Feb 9, 2022

Add pidfd-based implementation alexcrichton/wait-timeout#29

Open

Uh oh!

Conversation

voidc commented Feb 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rust-highfive commented Feb 6, 2021

Uh oh!

This comment has been minimized.

This comment has been minimized.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nicokoch Feb 8, 2021

Choose a reason for hiding this comment

Uh oh!

voidc Feb 8, 2021

Choose a reason for hiding this comment

Uh oh!

the8472 Feb 8, 2021

Choose a reason for hiding this comment

Uh oh!

voidc Feb 8, 2021

Choose a reason for hiding this comment

Uh oh!

the8472 Feb 8, 2021

Choose a reason for hiding this comment

Uh oh!

voidc Feb 8, 2021

Choose a reason for hiding this comment

Uh oh!

joshtriplett Mar 10, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

Mark-Simulacrum commented Feb 8, 2021

Uh oh!

Uh oh!

This comment has been minimized.

bors commented Feb 13, 2021

Uh oh!

This comment has been minimized.

voidc commented Mar 2, 2021

Uh oh!

bors commented Mar 9, 2021

Uh oh!

This comment has been minimized.

bors commented Jul 28, 2021

Uh oh!

bors commented Jul 28, 2021

Uh oh!

This comment has been minimized.

bors commented Jul 28, 2021

Uh oh!

voidc commented Jul 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

voidc commented Jul 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joshtriplett commented Jul 30, 2021

Uh oh!

voidc commented Jul 30, 2021

Uh oh!

Aaron1011 commented Jul 31, 2021

Uh oh!

bors commented Jul 31, 2021

Uh oh!

bors commented Jul 31, 2021

Uh oh!

This comment has been minimized.

bors commented Aug 1, 2021

Uh oh!

Aaron1011 commented Aug 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

voidc commented Feb 6, 2021 •

edited

Loading

voidc commented Jul 28, 2021 •

edited

Loading

voidc commented Jul 28, 2021 •

edited

Loading

Aaron1011 commented Aug 1, 2021 •

edited

Loading

voidc commented Aug 1, 2021 •

edited

Loading