Skip to content

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

Merged
codebytere merged 2 commits intomainfrom
fix-fd-utilityprocess-leak
Jun 24, 2025
Merged

fix: ensure /dev/null fd is closed on failure#47525
codebytere merged 2 commits intomainfrom
fix-fd-utilityprocess-leak

Conversation

@codebytere
Copy link
Member

@codebytere codebytere commented Jun 23, 2025

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

deepak1556 commented Jun 23, 2025

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
kigh-ota pushed a commit to kigh-ota/electron that referenced this pull request Sep 30, 2025
@codebytere @deepak1556 @kigh-ota
fix: ensure /dev/null fd is closed on failure (electron#47525)
451194e 
* fix: ensure /dev/null fd is closed on failure

* chore: ignore closehandle for windows

---------

Co-authored-by: Robo <hop2deep@gmail.com>
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