Skip to content

[5.1] SR-11354: Foundation.Process inherits file descriptors into the child process #2608

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
Merged
merged 5 commits into from
Jan 16, 2020
Merged

[5.1] SR-11354: Foundation.Process inherits file descriptors into the child process #2608

merged 5 commits into from
Jan 16, 2020

Conversation

weissi
Copy link
Contributor

@weissi weissi commented Jan 14, 2020

This is a backport of #2542 & #2586 which together fix https://bugs.swift.org/browse/SR-11354 which is an important bug fix.

Same as #2607 but this one is for 5.1

weissi and others added 4 commits January 14, 2020 16:50
@weissi
do not inherit fds
dd86d19 
(cherry picked from commit f4eb86b)
(cherry picked from commit 7022366)
@millenomi
Cleanup:
93a0f0f 
 - POSIX_SPAWN_CLOEXEC_DEFAULT is a constant offered by the Darwin module. Import it conditionally that way.
 - Do not close Pipe/FileHandle-owned fds.

(cherry picked from commit eb23f1e)
(cherry picked from commit f250d32)
@weissi
more cleanup
861cb87 
(cherry picked from commit c6da1a0)
(cherry picked from commit e0e56a1)
@weissi
don't rely on getdtablesize()
8f19014 
Android and possibly other UNIX systems lack getdtablesize() so let's
try to use a better way of estimating the file descriptors we need to
close post-fork, pre-exec by listing /proc/self/fd. If that fails, we
fall back on getdtablesize or as a last resort a hard-coded 4096.

(cherry picked from commit 077775e)
@weissi weissi requested a review from kevints January 14, 2020 16:51
@weissi weissi mentioned this pull request Jan 14, 2020
Merged
@weissi
Copy link
Contributor Author

weissi commented Jan 14, 2020

@swift-ci please test

@kevints
Copy link
Contributor

kevints commented Jan 14, 2020

Thanks for the backport! I will review this once @millenomi has reviewed the 5.2 patch in #2607

@weissi
Copy link
Contributor Author

weissi commented Jan 14, 2020

@swift-ci please test

@spevans
Copy link
Contributor

spevans commented Jan 14, 2020

Would it be possible to squash these commits into one before merging just to keep the history a bit cleaner, (same with the other PR). Thanks.

@weissi
Copy link
Contributor Author

weissi commented Jan 14, 2020

@spevans our current backport rules say git cherry-pick -x the commits from the original PR(s) which is what I did here. Happy to squash too.

@weissi
Copy link
Contributor Author

weissi commented Jan 14, 2020

@swift-ci please test

@weissi
Copy link
Contributor Author

weissi commented Jan 16, 2020

@swift-ci please test

1 similar comment
@weissi
Copy link
Contributor Author

weissi commented Jan 16, 2020

@swift-ci please test

@weissi
Copy link
Contributor Author

weissi commented Jan 16, 2020

@kevints the 5.2 backport is merged

@kevints
Copy link
Contributor

kevints commented Jan 16, 2020

@swift-ci test and merge

@swift-ci swift-ci merged commit 43898c0 into swiftlang:swift-5.1-branch Jan 16, 2020
@spevans spevans mentioned this pull request Jan 24, 2020
Closed
@weissi weissi mentioned this pull request May 4, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kevints kevints Awaiting requested review from kevints

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@weissi @kevints @spevans @swift-ci @millenomi