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

Only use clone3 when needed for pidfd #89930

Merged
merged 4 commits into from
Nov 11, 2021
Merged

Conversation

cuviper
Copy link
Member

@cuviper cuviper commented Oct 15, 2021

In #89522 we learned that clone3 is interacting poorly with Gentoo's
sandbox tool. We only need that for the unstable pidfd extensions, so
otherwise avoid that and use a normal fork.

This is a re-application of beta #89924, now that we're aware that we need
more than just a temporary release fix. I also reverted 12fbabd, as
that was just fallout from using clone3 instead of fork.

r? @Mark-Simulacrum
cc @joshtriplett

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 15, 2021
@Mark-Simulacrum
Copy link
Member

r? @joshtriplett for the master version of this -- I saw some followup discussion and you seem like a potentially better reviewer for the long(er)-term fix.

@brauner
Copy link

brauner commented Oct 20, 2021

In #89522 we learned that clone3 is interacting poorly with Gentoo's sandbox tool. We only need that for the unstable pidfd extensions, so otherwise avoid that and use a normal fork.

This is a re-application of beta #89924, now that we're aware that we need more than just a temporary release fix. I also reverted 12fbabd, as that was just fallout from using clone3 instead of fork.

r? @Mark-Simulacrum cc @joshtriplett

In contexts where you call clone3() only to get at a pidfd and you want to avoid calling both clone() and clone3() you can simply call:

pid_t pid = fork();
if (pid)
         int pidfd = pidfd_open(pid, 0);

which let's you circumvent all of the problems that @joshtriplett is currently discussing in another thread (Not saying that this shouldn't be fixed in the relevant libcs! :)).

@bjorn3
Copy link
Member

bjorn3 commented Oct 20, 2021

That is not race free. The forked process may have been killed in between. I think in the cases where pidfd should be used, it is very important that it is race free to acquire the pidfd.

@cuviper
Copy link
Member Author

cuviper commented Oct 20, 2021

The forked process may have been killed in between.

The zombie will remain unless you've otherwise consumed or ignored SIGCHLD.
But let's move this to #82971, because I'm not trying to solve the whole issue in this PR.

@joshtriplett
Copy link
Member

The zombie will remain unless you've otherwise consumed or ignored SIGCHLD.

Right, but Rust internals cant' know if that's the case.

But let's move this to #82971, because I'm not trying to solve the whole issue in this PR.

👍

@brauner
Copy link

brauner commented Oct 20, 2021

The zombie will remain unless you've otherwise consumed or ignored SIGCHLD.

Right, but Rust internals cant' know if that's the case.

I think it should be possible to make this race-free? Rust must be in control of the actual fork() call?

@cuviper
Copy link
Member Author

cuviper commented Oct 20, 2021

Rust must be in control of the actual fork() call?

Yes, but we don't know the disposition of SIGCHLD, or whether another thread is calling wait. I suspect other parts of the Command API also won't work right if you mess with that stuff though, so maybe we can write that off.

@apiraino apiraino added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Oct 28, 2021
@cuviper
Copy link
Member Author

cuviper commented Nov 1, 2021

@joshtriplett ping?

We'll need to backport this for 1.57 too.

@cuviper cuviper added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Nov 1, 2021
cuviper and others added 4 commits November 5, 2021 14:48
In rust-lang#89522 we learned that `clone3` is interacting poorly with Gentoo's
`sandbox` tool. We only need that for the unstable pidfd extensions, so
otherwise avoid that and use a normal `fork`.
Co-authored-by: Josh Triplett <josh@joshtriplett.org>
This reverts commit 12fbabd.

It was only needed because of using raw `clone3` instead of `fork`, but
we only do that now when a pidfd is requested.
@joshtriplett
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Nov 10, 2021

📌 Commit e96a0a8 has been approved by joshtriplett

@bors 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 Nov 10, 2021
@joshtriplett joshtriplett added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Nov 10, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 10, 2021
…askrgr

Rollup of 3 pull requests

Successful merges:

 - rust-lang#89930 (Only use `clone3` when needed for pidfd)
 - rust-lang#90736 (adjust documented inline-asm register constraints)
 - rust-lang#90783 (Update Miri)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit a09115f into rust-lang:master Nov 11, 2021
@rustbot rustbot added this to the 1.58.0 milestone Nov 11, 2021
@cuviper cuviper mentioned this pull request Nov 16, 2021
@cuviper cuviper removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Nov 16, 2021
@cuviper cuviper modified the milestones: 1.58.0, 1.57.0 Nov 16, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 19, 2021
[beta] backports

-  Fix assertion failures in OwnedHandle with windows_subsystem. rust-lang#88798
-  Ensure that pushing empty path works as before on verbatim paths rust-lang#89665
-  Feature gate + make must_not_suspend allow-by-default rust-lang#89826
-  Only use clone3 when needed for pidfd rust-lang#89930
-  Fix documentation header sizes rust-lang#90186
-  Fixes incorrect handling of ADT's drop requirements rust-lang#90218
-  Fix ICE when forgetting to Box a parameter to a Self::func call rust-lang#90221
-  Prevent duplicate caller bounds candidates by exposing default substs in Unevaluated rust-lang#90266
-  Update odht crate to 0.3.1 (big-endian bugfix) rust-lang#90403
-  rustdoc: Go back to loading all external crates unconditionally rust-lang#90489
-  Split doc_cfg and doc_auto_cfg features rust-lang#90502
-  Apply adjustments for field expression even if inaccessible rust-lang#90508
-  Warn for variables that are no longer captured rust-lang#90597
-  Properly register text_direction_codepoint_in_comment lint. rust-lang#90626
-  CI: Use ubuntu image to download openssl, curl sources, cacert.pem for x86 dist builds rust-lang#90457
-  Android is not GNU rust-lang#90834
-  Update llvm submodule rust-lang#90954

Additionally, this bumps the stage 0 compiler from beta to stable 1.56.1.

r? `@Mark-Simulacrum`
@cuviper cuviper deleted the avoid-clone3 branch December 8, 2021 07:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants