8000 fix: ensure `/dev/null` fd is closed on failure by codebytere · Pull Request #47525 · electron/electron · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

fix: ensure /dev/null fd is closed on failure #47525

New issue 8000

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
Merged
merged 2 commits into from
Jun 24, 2025
Merged

fix: ensure /dev/null fd is closed on failure #47525

merged 2 commits into from
Jun 24, 2025
+3 −0

Conversation

codebytere
Copy link
Member

Description of Change

Closes #47515.

Previously we were opening a handle to /dev/null and never remapping it, so it would be leaked as we never explicitly closed it. This fixes that.

Upstream in this case just called _exit which terminates the process and closes open file descriptors belonging to the process, which s why they didn't have this issue.

Checklist

Release Notes

Notes: Fixed an issue where utility processes could leak file handles.

@codebytere codebytere requested review from deepak1556 and nikwen June 23, 2025 13:16
@codebytere codebytere added semver/patch backwards-compatible bug fixes target/35-x-y PR should also be added to the "35-x-y" branch. target/36-x-y PR should also be added to the "36-x-y" branch. target/37-x-y PR should also be added to the "37-x-y" branch. labels Jun 23, 2025
@electron-cation electron-cation bot added the new-pr 🌱 PR opened recently label Jun 23, 2025
@deepak1556
Copy link
Member

Maybe we can break early for stdin (avoids unnecessary file calls) since we know that we don't support the ignore or pipe option for this handle ?

for (const auto& [io_handle, io_type] : stdio) {
  if (io_handle == IOHandle::STDIN) {
     continue;
  }

@codebytere codebytere force-pushed the fix-fd-utilityprocess-leak branch from e9ac738 to c56db6a Compare June 23, 2025 13:45
@codebytere codebytere force-pushed the fix-fd-utilityprocess-leak branch from c56db6a to e0d109a Compare June 23, 2025 13:45
deepak1556
deepak1556 reviewed Jun 23, 2025
View reviewed changes
nikwen
nikwen approved these changes Jun 23, 2025
View reviewed changes
Copy link
Member
@nikwen nikwen left a comment

Choose a reason for hiding this comment

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

Thank you both! Appreciate it.

ckerr
ckerr reviewed Jun 23, 2025
View reviewed changes
@codebytere codebytere force-pushed the fix-fd-utilityprocess-leak branch from 9ba5a32 to da59711 Compare June 23, 2025 17:17
@codebytere codebytere requested a review from deepak1556 June 23, 2025 17:17
@electron-cation electron-cation bot removed the new-pr 🌱 PR opened recently label Jun 24, 2025
@codebytere codebytere merged commit 51fbc96 into main Jun 24, 2025
101 of 104 checks passed
@codebytere codebytere deleted the fix-fd-utilityprocess-leak branch June 24, 2025 15:44
@release-clerk
Copy link
release-clerk bot commented Jun 24, 2025

Release Notes Persisted

Fixed an issue where utility processes could leak file handles.

This was referenced Jun 24, 2025
Merged
Merged
Merged
@trop
Copy link
Contributor
trop bot commented Jun 24, 2025

I have automatically backported this PR to "36-x-y", please check out #47541

@trop trop bot added in-flight/36-x-y and removed target/36-x-y PR should also be added to the "36-x-y" branch. labels Jun 24, 2025
@trop
Copy link
Contributor
trop bot commented Jun 24, 2025

I have automatically backported this PR to "35-x-y", please check out #47542

@trop
Copy link
Contributor
trop bot commented Jun 24, 2025

I have automatically backported this PR to "37-x-y", please check out #47543

@trop trop bot added in-flight/35-x-y in-flight/37-x-y merged/36-x-y PR was merged to the "36-x-y" branch. merged/37-x-y PR was merged to the "37-x-y" branch. merged/35-x-y PR was merged to the "35-x-y" branch. and removed target/35-x-y PR should also be added to the "35-x-y" branch. target/37-x-y PR should also be added to the "37-x-y" branch. in-flight/36-x-y in-flight/37-x-y in-flight/35-x-y labels Jun 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ckerr ckerr ckerr approved these changes

@deepak1556 deepak1556 deepak1556 approved these changes

@nikwen nikwen nikwen approved these changes

Assignees
No one assigned
Labels
merged/35-x-y PR was merged to the "35-x-y" branch. merged/36-x-y PR was merged to the "36-x-y" branch. merged/37-x-y PR was merged to the "37-x-y" branch. semver/patch backwards-compatible bug fixes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Utility process leaks file handles
4 participants
@codebytere @deepak1556 @ckerr @nikwen
0