win32unix `create_process`: duplicate std handles in `create_process` iff they're not inheritable by MisterDA · Pull Request #10807 · ocaml/ocaml

MisterDA · 2021-12-02T13:46:19Z

The current code makes the assumption that handles are not inheritable
(close-on-exec), and duplicates handles to mark them inheritable
(keep-on-exec) for the child process. That assumption is only
mentioned in #650, and not documented in the manual.

It's possible however that the calling code gives to create_process
handles that are already inheritable: either because the programmer
isn't aware of the security implications, or because the programmer
expects create_process to be a "simple" wrapper around WinAPI's
CreateProcess; in which case setting the handle inheritable would be
the responsibility of the calling code.

Converting the canonical example from Microsoft documentation [1]
without looking at win32unix's create_process would lead to this
problem.

In either case, the current create_process would needlessly duplicate
an already inheritable handle and leak the original handle in the
child process.

We can instead check whether the given handles are inheritable, and
only duplicate them if they're not. The calling code is still
responsible for closing the original handle.

xavierleroy

In either case, the current create_process would needlessly duplicate an already inheritable handle

Agreed.

and leak the original handle in the child process.

I would not call this "leaking": if the handle is marked as inheritable, it is intended to "leak" (i.e. be inherited) by the child process...

At any rate, this looks quite good to me. A suggestion for refactoring below. @dra27's opinion is awaited.

xavierleroy · 2021-12-16T17:40:04Z

otherlibs/win32unix/createprocess.c

+  if (! (handleInfo[0] & HANDLE_FLAG_INHERIT)) {
+    if (! DuplicateHandle(hp, fd0, hp, &(si.hStdInput),
+                          0, TRUE, DUPLICATE_SAME_ACCESS)) {
+      err = GetLastError(); goto ret1;
+    }
+  } else {
+      si.hStdInput = fd0;


Could we factor out this logic in a function? (Then we'd call the function 3 times for each of the 3 standard handles.) Something like:

GetHandleInformation

if inheritable, return original handle

otherwise, duplicate to an inheritable handle

Done in the third commit. The ensure_inheritable function must also return the information that we've duplicated the handle to close it after the child process is spawned.
Should I squash that last commit?

MisterDA · 2021-12-17T14:43:34Z

I would not call this "leaking": if the handle is marked as inheritable, it is intended to "leak" (i.e. be inherited) by the child process...

Good point, I've amended the commit message to just say that the child process has both the original handle and the duplicated handle.

smorimoto · 2022-01-30T03:02:56Z

Is there any update on this? There is no rush, but I'm just worried that it might have been overlooked.

dra27 · 2022-01-30T09:53:16Z

It’s on my list, but I don’t think this one’s slated for either 4.14 or 5.00.

MisterDA · 2022-02-15T16:04:41Z

I think this fixes a buggy situation I hadn't described, so I added a test. IMO it's worthwhile to have this in 4.14 if it's meant to last long.

Consider the following pipeline: writer | child | reader.

The problem with the child process having duplicated "leaked" stdin and stdout handles is that while it is running, it cannot signal to the writer that it should stop writing: the writer will not see a broken pipe as the child still has the leaked duplicated handle open. The same thing happens to the reader with the stdout of the child. Usually stdin/stdout are bound to the lifetime of the process, so this is not too much of a problem, but if the child itself spawns processes that should outlive it, the whole thing can become messy.

nojb

LGTM. I didn't quite follow the test, a brief comment explaining what it is checking may be useful.

nojb · 2022-02-15T16:15:06Z

otherlibs/win32unix/createprocess.c

  DWORD flags, err;
-  HANDLE hp;
+  HANDLE to_close[3] =
+      {INVALID_HANDLE_VALUE, INVALID_HANDLE_VALUE, INVALID_HANDLE_VALUE};


I wouldn't use an array here, it obfuscates matters. Let just call them to_close1, to_close2, to_close3 (and we can revert the above renaming fd1 -> fd0, etc).

Ok for the array. There's also the possibility of returning booleans or something to mark which fd should be closed.
I liked my renumbering better, but so be it :)

nojb · 2022-02-15T16:17:26Z

otherlibs/win32unix/createprocess.c


 static int win_has_console(void);

+static int ensure_inheritable(HANDLE h, HANDLE * hStd, HANDLE * to_close)


Can you add a brief comment above the function definition explaining what the function does and what each argument means?

Done, hope it's clearer.

nojb

LGTM. Thanks for the nice docs.

Just to confirm: the test runs roughly instantaneously in practice, right?

nojb · 2022-02-15T19:20:15Z

testsuite/tests/lib-unix/win-createprocess/test.ml

+
+  (* Check that the read time was determined solely by the lifetime of
+     the child process and _not_ the subchild. *)
+  assert (t' -. t <= float_of_int sub_child_lifetime)


Is it right that in practice t' - t will be much smaller than sub_child_lifetime (more like instantaneous) ?

MisterDA · 2022-02-15T19:55:06Z

Just to confirm: the test runs roughly instantaneously in practice, right?
Is it right that in practice t' - t will be much smaller than sub_child_lifetime (more like instantaneous) ?

Yes, t' -. t is instantaneous (with the patch) and bigger than sub_child_lifetime (EDIT: without the patch). I'm afraid that the subchild process still is part of a process group and that the test waits for its termination. Maybe I could lower its run time?

nojb · 2022-02-15T20:05:42Z

Just to confirm: the test runs roughly instantaneously in practice, right?
Is it right that in practice t' - t will be much smaller than sub_child_lifetime (more like instantaneous) ?

Yes, t' -. t is instantaneous (with the patch) and bigger than sub_child_lifetime.

I don't understand: bigger ? did you mean smaller ?

I'm afraid that the subchild process still is part of a process group and that the test waits for its termination. Maybe I could lower its run time?

Yes, 20 seconds is too long. Or perhaps we can kill the children once that the read in the parent is complete?

MisterDA · 2022-02-15T20:25:39Z

I don't understand: bigger ? did you mean smaller ?

yes, sorry, I meant bigger without the patch…

Yes, 20 seconds is too long. Or perhaps we can kill the children once that the read in the parent is complete?

Yes, that's sensible. Thanks for the comments, I'll implement this.

nojb · 2022-02-16T18:27:50Z

I don't understand: bigger ? did you mean smaller ?

yes, sorry, I meant bigger without the patch…

Yes, 20 seconds is too long. Or perhaps we can kill the children once that the read in the parent is complete?

Yes, that's sensible. Thanks for the comments, I'll implement this.

I understand that we are blocked because of #11021... what happens if you do exit 0 after the read in the parent is complete? Does it kill all the children as well?

MisterDA · 2022-02-28T15:56:34Z

what happens if you do exit 0 after the read in the parent is complete? Does it kill all the children as well?

I don't think it does. I've reduced the waiting time, maybe that's enough?

The current code makes the assumption that handles are not inheritable (close-on-exec), and duplicates handles to mark them inheritable (keep-on-exec) for the child process. That assumption is only mentioned in ocaml#650, and not documented in the manual. It's possible however that the calling code gives to create_process handles that are already inheritable: either because the programmer isn't aware of the security implications, or because the programmer expects create_process to be a "simple" wrapper around WinAPI's CreateProcess; in which case setting the handle inheritable would be the responsibility of the calling code. Converting the canonical example from Microsoft documentation [1] without looking at win32unix's create_process would lead to this problem. In either case, the current create_process would needlessly duplicate an already inheritable handle and the child process would inherit both the original and the duplicated handles. Consider the following pipeline: `writer | child | reader`. The problem with the child process having duplicated "leaked" stdin and stdout handles is that while it is running, it cannot signal to the writer that it should stop writing: the writer will not see a broken pipe as the child still has the leaked duplicated handle open. The same thing happens to the reader with the stdout of the child. Usually stdin/stdout are bound to the lifetime of the process, so this is not too much of a problem, but if the child itself spawns processes that should outlive it, the whole thing can become messy. We can instead check whether the given handles are inheritable, and only duplicate them if they're not. The calling code is still responsible for closing the original handle. [1]: https://docs.microsoft.com/en-us/windows/win32/procthread/creating-a-child-process-with-redirected-input-and-output

MisterDA · 2022-03-17T16:16:59Z

I don't see how to improve the test apart from reducing the timeout. Perhaps it's best to remove the test instead. Is there a way we can move forward with this patch?

nojb · 2022-03-17T16:31:24Z

I don't see how to improve the test apart from reducing the timeout. Perhaps it's best to remove the test instead. Is there a way we can move forward with this patch?

Hi, apologies for having dropped the ball on this one. This is good to merge from my side, and a 1s sleep timeout seems not too bad. @xavierleroy any objections to having this test in the CI?

xavierleroy · 2022-03-17T16:55:14Z

@xavierleroy any objections to having this test in the CI?

Timing-sensitive tests have been known to fail randomly because of hiccups in scheduling in virtual machines... Not sure how sensitive this test is to this kind of hiccups.

In testsuite/tests/lib-unix/common/cloexec.ml I tried to check directly in the executed program whether some file descriptors are still open. It seems disabled on Windows :-( for reasons I need to remember. But this may be a different way to check for FD leaks.

MisterDA · 2022-03-18T10:39:45Z

As a side note, Windows 10 introduced CompareObjectHandles which succeeds if two given handles point to the same kernel object. If we could iterate on all handles of a given process (1) we could compare it with a given list of handles to find duplicates, and potentially leaked handles, or make sure that some handles are closed.

It seems disabled on Windows :-( for reasons I need to remember.

From cloexec.ml:

Presumably this is because the OCaml runtime opens files, so that handles that have actually been closed at execution look open and make the test fail.

Maybe the runtime just re-uses handle numbers that we want to check whether they were closed?

I've written the same patch for Lwt and made other tests, but they rely more-or-less on the same property: without the patch the test will block (optionally fails with a timeout), and with it'll succeed as expected (optionally within the bounds of the timeout).

(1) from a short talk with a Windows guru, that seems to be difficult and a likely overkill for this case.

nojb · 2022-03-18T16:08:30Z

OK, let's merge this and if the test becomes a problem we can always disable it.

Et7f3 · 2023-01-12T03:13:23Z

I think I see issue not related directly to this PR (it was in the original code). The issue is a race-condition with handle better explained here https://devblogs.microsoft.com/oldnewthing/20111216-00/?p=8873

I see no use of UpdateProcThreadAttribute in the codebase so I guess it is subject to this race ? or maybe the runtime serialize the process creation ? Do we have a way to list all opened handle (to feed the UpdateProcThreadAttribute + 3 handles).

MisterDA mentioned this pull request Dec 6, 2021

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

Merged

xavierleroy reviewed Dec 16, 2021

View reviewed changes

MisterDA force-pushed the win32unix-create_process-check-if-inheritable-before-duplicating branch from 29c16a1 to 3d3e871 Compare December 17, 2021 14:40

MisterDA force-pushed the win32unix-create_process-check-if-inheritable-before-duplicating branch from 3d3e871 to 6a7778a Compare January 11, 2022 12:04

MisterDA force-pushed the win32unix-create_process-check-if-inheritable-before-duplicating branch from 6a7778a to d428c55 Compare February 15, 2022 16:00

nojb reviewed Feb 15, 2022

View reviewed changes

MisterDA force-pushed the win32unix-create_process-check-if-inheritable-before-duplicating branch from d428c55 to 32695ce Compare February 15, 2022 17:09

nojb approved these changes Feb 15, 2022

View reviewed changes

MisterDA force-pushed the win32unix-create_process-check-if-inheritable-before-duplicating branch 2 times, most recently from 72f5b20 to 867f8b9 Compare February 28, 2022 15:55

MisterDA added 2 commits March 17, 2022 17:13

win32unix: test if create_process leaks std handles

ee862a4

MisterDA force-pushed the win32unix-create_process-check-if-inheritable-before-duplicating branch from 867f8b9 to 961c184 Compare March 17, 2022 16:15

nojb merged commit 986c39b into ocaml:trunk Mar 18, 2022

MisterDA deleted the win32unix-create_process-check-if-inheritable-before-duplicating branch March 18, 2022 16:16


		static int win_has_console(void);

		static int ensure_inheritable(HANDLE h, HANDLE * hStd, HANDLE * to_close)

Conversation

MisterDA commented Dec 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xavierleroy left a comment

Choose a reason for hiding this comment

Uh oh!

xavierleroy Dec 16, 2021

Choose a reason for hiding this comment

Uh oh!

MisterDA Dec 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MisterDA commented Dec 17, 2021

Uh oh!

smorimoto commented Jan 30, 2022

Uh oh!

dra27 commented Jan 30, 2022

Uh oh!

MisterDA commented Feb 15, 2022

Uh oh!

nojb left a comment

Choose a reason for hiding this comment

Uh oh!

nojb Feb 15, 2022

Choose a reason for hiding this comment

Uh oh!

MisterDA Feb 15, 2022

Choose a reason for hiding this comment

Uh oh!

nojb Feb 15, 2022

Choose a reason for hiding this comment

Uh oh!

MisterDA Feb 15, 2022

Choose a reason for hiding this comment

Uh oh!

nojb left a comment

Choose a reason for hiding this comment

Uh oh!

nojb Feb 15, 2022

Choose a reason for hiding this comment

Uh oh!

MisterDA commented Feb 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nojb commented Feb 15, 2022

Uh oh!

MisterDA commented Feb 15, 2022

Uh oh!

nojb commented Feb 16, 2022

Uh oh!

MisterDA commented Feb 28, 2022

Uh oh!

MisterDA commented Mar 17, 2022

Uh oh!

nojb commented Mar 17, 2022

Uh oh!

xavierleroy commented Mar 17, 2022

Uh oh!

MisterDA commented Mar 18, 2022

Uh oh!

nojb commented Mar 18, 2022

Uh oh!

Et7f3 commented Jan 12, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

MisterDA commented Dec 2, 2021 •

edited

Loading

MisterDA Dec 17, 2021 •

edited

Loading

MisterDA commented Feb 15, 2022 •

edited

Loading