Skip to content
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

Windows: make create_process duplicate non-inheritable std handles #909

Merged
merged 3 commits into from
Apr 12, 2022
Merged

Windows: make create_process duplicate non-inheritable std handles #909

merged 3 commits into from
Apr 12, 2022

Conversation

MisterDA
Copy link
Contributor

@MisterDA MisterDA commented Dec 6, 2021 •
edited
Loading

Unix.create_process currently always duplicates the handles given to
the child process to make sure they're inheritable (keep-on-exec). I
have opened the PR 10807 on OCaml to duplicate the handles only if
they're not inheritable (close-on-exec).

This PR mimic this behaviour in Lwt for consistency (assuming the
OCaml PR will be accepted).

See the following PRs:

@MisterDA MisterDA marked this pull request as draft December 20, 2021 11:56
@MisterDA MisterDA force-pushed the windows-create-process-inheritable-handles branch from c8a0986 to 57b43d4 Compare February 8, 2022 17:20
@MisterDA MisterDA force-pushed the windows-create-process-inheritable-handles branch 3 times, most recently from 1c807dd to affeedf Compare February 28, 2022 17:58
@MisterDA
Copy link
Contributor Author

MisterDA commented Mar 1, 2022

I've just expanded a bit the scope of this PR by switching all internal pipes used in Lwt_process to close-on-exec.

@MisterDA MisterDA force-pushed the windows-create-process-inheritable-handles branch from affeedf to 5ebe9e2 Compare March 18, 2022 16:22
@MisterDA
Copy link
Contributor Author

MisterDA commented Mar 18, 2022 •
edited
Loading

The patch just got merged in ocaml/ocaml! I think this is ready to go right into Lwt.
The good security model is to always keep file descriptors close-on-exec. On Linux this works fine because we can dup2 just after the fork, which removes the close-on-exec flag. On Windows, we have to either unset the flag or duplicate the file descriptor to unset the flag just before calling CreateProcess.
On Windows, Unix.create_process used to duplicate the file descriptor. However, if the programmer isn't aware of that, and gives a inheritable (keep-on-exec) handle to Unix.create_process, the parent process ends up with a duplicated handle and the child process with a leaked handle. This can cause the parent process to block on itself, or block for a child process, and all kinds of problems follow. To prevent that, we now check whether is the file descriptor is already inheritable or not, and only duplicate it if it's not.
Lwt on Windows didn't do this duplication, so it was exhibiting a different behaviour than win32unix, prone to confuse even relentless programmers. This PR brings Lwt on par with win32unix, with the above bugfix.
As a new change, I've also switched all of Lwt internal pipes to close-on-exec. With my previous patches, this makes the change portable all OCaml version, fixes concurrency and security issues, I've added tests, and it won't change what a sane user would expect (an insane user might loop over all files descriptors of a running process and see slightly fewer leaked fds after this patch).

@MisterDA MisterDA marked this pull request as ready for review March 21, 2022 09:55
@MisterDA @smorimoto
More testing of Lwt_process
e9c6f0a
@MisterDA @smorimoto
Windows: make create_process duplicate non-inheritable std handles
1b071ad 
Unix.create_process duplicates the handles given to the child process
if they're not inheritable. Mimic this behaviour in Lwt for
consistency.

See the following PRs:
- [Unix library: better API for "close-on-exec" over file descriptors ocsigen#650](ocaml/ocaml#650);
- [win32unix create_process: duplicate std handles in create_process iff they're not inheritable #10807](ocaml/ocaml#650).
@MisterDA @smorimoto
Set all pipes to cloexec
9de01c9 
This is done to prevent possible race conditions, and for consistency.
The spawn function is now responsible for ensuring that file
descriptors can be inherited by subprocesses (this is always the case
on unix when using dup2, but can be trickier on Windows).

We can also remove to_close file descriptors in Lwt_process spawn,
because the file descriptors are the parent's end of the pipes which
must be closed in the child; however switching the pipes to cloexec
means they won't be inherited by the child at all.
@smorimoto smorimoto force-pushed the windows-create-process-inheritable-handles branch from 5ebe9e2 to 9de01c9 Compare April 3, 2022 13:15
raphael-proust
raphael-proust approved these changes Apr 12, 2022
View reviewed changes
Copy link
Collaborator

@raphael-proust raphael-proust left a comment

Choose a reason for hiding this comment

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

Once again I can't test the windows side of things, but the code LGTM. Thank you for the added tests.

@raphael-proust raphael-proust merged commit 9283ec9 into ocsigen:master Apr 12, 2022
@MisterDA MisterDA deleted the windows-create-process-inheritable-handles branch April 12, 2022 13:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@raphael-proust raphael-proust raphael-proust approved these changes

@smorimoto smorimoto Awaiting requested review from smorimoto

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

Successfully merging this pull request may close these issues.
2 participants
@MisterDA @raphael-proust