Skip to content

Update comments in do_exec and add assertion for fd flag #147656

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Update comments in do_exec and add assertion for fd flag #147656

wants to merge 1 commit into from

Conversation

chenyukang
Copy link
Member

@chenyukang chenyukang commented Oct 14, 2025 •
edited by rustbot
Loading

Fixes #147624
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Oct 14, 2025
@rustbot
Copy link
Collaborator

rustbot commented Oct 14, 2025

r? @ibraheemdev

rustbot has assigned @ibraheemdev.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer
chenyukang
chenyukang commented Oct 14, 2025
View reviewed changes
@joboet
Copy link
Member

joboet commented Oct 14, 2025

See #147624 (comment) for why I don't think this is necessary.
tgross35
tgross35 reviewed Oct 16, 2025
View reviewed changes
library/std/src/sys/process/unix/unix.rs Outdated
Comment on lines 290 to 293
// SAFETY: Dropping `stdio` (ChildPipes) is safe here since ChildPipes
// does not involve heap memory allocations
debug_assert!(size_of::<ChildPipes>() <= 24);
drop(stdio);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed with joboet that we shouldn't need this. If anything, you could do a debug_assert with F_GETFL that ensures F_CLOEXEC is set.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, I changed the PR to update outdated comments and add debug_assertion.
@chenyukang chenyukang force-pushed the yukang-fix-exec-file-issue branch from 3f065a5 to 30b8146 Compare October 17, 2025 03:53
@chenyukang chenyukang changed the title Fix file descriptors leak in do_exec Update comments in do_exec and add assertion for fd flag Oct 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue.

5 participants

@chenyukang @rustbot @joboet @tgross35 @ibraheemdev