Skip to content

Only use clone3 when needed for pidfd #89924

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom
Merged
merged 3 commits into from
Oct 16, 2021
Merged

Only use clone3 when needed for pidfd #89924

merged 3 commits into from
Oct 16, 2021

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.

r? @Mark-Simulacrum

@cuviper
Only use clone3 when needed for pidfd
de2e483 
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`.
@rust-highfive
Copy link
Contributor

⚠️ Warning ⚠️

  • Pull requests are usually filed against the master branch for this repo, but this one is against beta. Please double check that you specified the right target!

@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

cc @joshtriplett

This patch seems reasonable to me -- should be minimal and work around the issues with the sandbox tool for 1.56. I think we can likely just not do this for 1.57 given that it appears to be ultimately a bug in the sandbox tooling (that could trigger with other code as well, not specific to Rust).

@bors r+ rollup=never

@bors
Copy link
Collaborator

bors commented Oct 15, 2021

📌 Commit de2e483 has been approved by Mark-Simulacrum

@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 Oct 15, 2021
joshtriplett
joshtriplett reviewed Oct 15, 2021
View reviewed changes
@joshtriplett
Copy link
Member

I posted one suggestion to generalize the explanatory comment, but otherwise this LGTM as a workaround.

@cuviper cuviper mentioned this pull request Oct 15, 2021
Closed
@the8472
Copy link
Member

the8472 commented Oct 15, 2021

The comment after the clone3, above the fork call needs to be updated too.

@cuviper
Copy link
Member Author

cuviper commented Oct 15, 2021

Update the comment that @the8472 pointed out.

@bors r=Mark-Simulacrum

@bors
Copy link
Collaborator

bors commented Oct 15, 2021

📌 Commit ad6e5e1 has been approved by Mark-Simulacrum

@bors
Copy link
Collaborator

bors commented Oct 15, 2021

⌛ Testing commit ad6e5e1 with merge 1135eff91c55a5ba56fc955647652883baec4b53...

@cuviper @joshtriplett
Also note tool expectations of fork vs clone3
74ef530 
Co-authored-by: Josh Triplett <josh@joshtriplett.org>
@nbdd0121
Copy link
Contributor

Should this land on nightly as well?

@cuviper
Copy link
Member Author

cuviper commented Oct 15, 2021

@nbdd0121 Yes, we should, now that it seems less temporary of a thing.

I'm not sure of the right way to kick out the current test to get the new comment, but let's try:
@bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 15, 2021
@cuviper
Copy link
Member Author

cuviper commented Oct 15, 2021

@bors r=Mark-Simulacrum

@bors
Copy link
Collaborator

bors commented Oct 15, 2021

📌 Commit 74ef530 has been approved by Mark-Simulacrum

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 15, 2021
@rust-log-analyzer

This comment has been minimized.

# to view
@cuviper cuviper mentioned this pull request Oct 15, 2021
Merged
@bors
Copy link
Collaborator

bors commented Oct 16, 2021

⌛ Testing commit 74ef530 with merge 92b7da2cb7f50a496698996dd34a8dcadbd1b3cc...

@bors
Copy link
Collaborator

bors commented Oct 16, 2021

💥 Test timed out

@bors 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 Oct 16, 2021
@jackh726
Copy link
Member

@bors retry

seems stuck

@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 Oct 16, 2021
@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

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

@bors
Copy link
Collaborator

bors commented Oct 16, 2021

⌛ Testing commit 74ef530 with merge 16c2ad090fb360368b60cf606e41871d4f40c2a1...

@bors
Copy link
Collaborator

bors commented Oct 16, 2021

💥 Test timed out

@bors 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 Oct 16, 2021
@ehuss
Copy link
Contributor

ehuss commented Oct 16, 2021

@bors retry

timeout on dist-x86_64-musl, no logs

@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 Oct 16, 2021
@bors
Copy link
Collaborator

bors commented Oct 16, 2021

⌛ Testing commit 74ef530 with merge 7eda943...

@rust-log-analyzer

This comment has been minimized.

# to view
@bors
Copy link
Collaborator

bors commented Oct 16, 2021

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing 7eda943 to beta...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 16, 2021
@bors bors merged commit 7eda943 into rust-lang:beta Oct 16, 2021
@rustbot rustbot added this to the 1.56.0 milestone Oct 16, 2021
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Nov 10, 2021
@matthiaskrgr
Rollup merge of rust-lang#89930 - cuviper:avoid-clone3, r=joshtriplett
a09115f 
Only use `clone3` when needed for pidfd

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`.

This is a re-application of beta rust-lang#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`
@cuviper cuviper deleted the beta-clone3 branch April 12, 2022 17:55
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@joshtriplett joshtriplett joshtriplett left review comments

Assignees

@Mark-Simulacrum Mark-Simulacrum

Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.56.0
Development

Successfully merging this pull request may close these issues.