Skip to content

Conversation

Elliot-Roberts
Copy link
Contributor

Fixes #39186
It's a small change but this is my first time contributing to open source so any feedback is very appreciated.

I also have some questions:
I documented that the setgroup call only happens when the parent is root. But this is potentially not the desired behavior, as described in #88716.
What more needs to happen decision-wise for that? Does #88716 remaining open mean that it is an approved and wanted change? I could fix that issue in this PR too if that would be ok.

@rust-highfive
Copy link
Contributor

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.

Please see the contribution instructions for more information.

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

r? @joshtriplett perhaps? This likely wants FCP, but I'm also not sure we want to make the guarantee this specific.

@joshtriplett joshtriplett added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Oct 26, 2021
@joshtriplett
Copy link
Member

I do think this warrants an FCP.

I think if we're going to have behavior this specific (and potentially unexpected), we should document it.

@rfcbot merge

@rfcbot
Copy link

rfcbot commented Oct 26, 2021

Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Oct 26, 2021
@Elliot-Roberts
Copy link
Contributor Author

Is the current behavior the one we want to keep? The specifics I wrote conflict with what #88716 wants.
Specifically, I say "This will also trigger a call to setgroups(0, NULL) in the child process if the parent is root ..."
Whereas implementing the change requested in #88716 would mean the call is triggered for any process that has the CAP_SETGID capability.

@Amanieu
Copy link
Member

Amanieu commented Nov 18, 2021

I agree that would should document this, however we should first resolve #88716 to decide the actual behavior we want to stabilize.

@rfcbot
Copy link

rfcbot commented Nov 18, 2021

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Nov 18, 2021
@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Nov 28, 2021
@rfcbot
Copy link

rfcbot commented Nov 28, 2021

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Dec 9, 2021
@JohnCSimon 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 Feb 6, 2022
@JohnCSimon
Copy link
Member

triage:
looks like #88716 is still waiting on resolution

@Elliot-Roberts
Copy link
Contributor Author

Closing in favor of #95982

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Document that std::os::unix::process::CommandExt.uid() triggers setgroups(0,0)
8 participants