Skip to content

Preserve O_CLOEXEC when converting Unix.file_descr to a CRT fd on Windows#13921

Merged
dra27 merged 3 commits intoocaml:trunkfrom
dra27:more-cloexec
Apr 24, 2025
Merged

Preserve O_CLOEXEC when converting Unix.file_descr to a CRT fd on Windows#13921
dra27 merged 3 commits intoocaml:trunkfrom
dra27:more-cloexec

Conversation

@dra27
Copy link
Copy Markdown
Member

@dra27 dra27 commented Apr 1, 2025

Follow-on from #13879, which has been sporadically failing in Jenkins.

There are two kinds of failure which have been showing up:

  1. The test fails because of a Sys_error exception reporting "tmp.txt: Permission denied"
  2. The test fails because an fd which was expected to be closed appears to be open

I've investigated this further (in large part, because one of the bug-fixes in Relocatable OCaml made both these issues much more reproducible than they were before) and have determined:

  1. Is caused by Windows Defender (or other file system filters). The issue is that the VM is under considerable load during a testsuite run. The various open handles to tmp.txt are closed by L6 in:
    let () =
    for i = 2 to (Array.length Sys.argv) -1
    do
    process_and_close_fd (i - 1) Sys.argv.(i);
    done;
    (* For the execv version of the test, clean-up tmp.txt - for the
    Unix.create_process version, this is done by cloexec.ml *)
    if Sys.argv.(1) = "execv" then
    Sys.remove "tmp.txt"

    but by the time we reach the Sys.remove on L11, Windows Defender has opened the file ready for "scanning", but not actually done it. As a result, the Sys.remove fails.
  2. Amusingly, despite having switched to using CRT fds and Unix.execv in Restore lib-unix/common/cloexec.ml test on Windows #13879, we still end up seeing the same issue that caused the test to be disabled previously because of some missing code in caml_win32_CRT_fd_of_filedescr. The various mechanisms tested in cloexec.ml all correctly use SetHandleInformation or SECURITY_ATTRIBUTES, but when caml_win32_CRT_fd_of_filedescr converts them to CRT fds, we don't pass the O_NOINHERIT flag for close-on-exec handles. As a result all of the handles will be passed to the exec'd process - but the ones which are meant to be close-on-exec will just be invalid. However, what we are then sporadically seeing is that the Win32 handle value has been reused, so the CRT fd appears to be open (but it's in fact a different handle). This is what was being seen before when we were passing the Win32 handle values directly 🤦

Finally, while debugging this, I noted and have worked around an additional quirk of the way the exec family are implemented on Windows. Again, on an overloaded system, it's just possible that the exec'd program runs for quite some time, or even to completion, before the process which actually exec'd it (let's call it the "ancestor" rather than "parent", as it's not supposed to exist) has reached the _exit(0) call in the CRT. In this race condition, the handles may still be open when the Sys.remove is reached. That's obviously not a situation which can happen on a POSIX system, where the ancestor still running while the exec'd image is running is a major misimplementation!

This PR is therefore three commits:

  1. On Windows only, cloexec.ml creates a file lock.txt (with O_CLOEXEC) and locks the entire file for reading and writing. Again, on Windows only, fdstatus_main.ml opens this lock.txt and also attempts to lock the file for reading and writing. This will only succeed once the "ancestor" has completely terminated - in other words, fdstatus_main.ml is then able to block until cloexec.ml has definitely terminated.
  2. The second commit then deals with Windows Defender, et al. by opening tmp.txt with FILE_SHARE_DELETE and then adding an additional C stub which is used at the start of fdstatus_main.ml to open tmp.txt with FILE_FLAG_DELETE_ON_CLOSE. The Windows handle from this is then intentionally leaked - the point is that the handle will be automatically closed by Windows when the process exits and when Windows Defender has finally scanned it, the kernel will dutifully delete the file for us. Enough processes get held open in the process group while this happens that ocamltest waits around as normal for the test to conclude. This is a well-known problem on Windows, but this a less-common solution to it. The more common approach is to spin for up to a few seconds, retrying the deletion every 100ms or so. The solution I propose here is technically neater - the file is temporary, and we're using an (old and) actual Windows technology for dealing with the removal of temporary files, but also the value of the timeout we'd need for the spin is hard to determine - in one run on precheck, the delay is 10 seconds...!
  3. The final commit uses GetHandleInformation to determine if the Win32 handle has been marked as inheritable. If it is not, then we pass O_NOINHERIT to _open_osf_handle, which causes the CRT fd book-keeping to be corrected.

Note that while the fix to caml_win32_CRT_fd_of_filedescr is a bug-fix, it is not as critical a bug-fix as it might look on first glance. Handles marked as close-on-exec were not being passed to any exec'd processes - the issue is that the Win32 handle values were being passed in fds, but the descriptors fail on first use. This combined slightly unfortunately in the cloexec.ml test with the fact that Windows uses HANDLE values for multiple things and meant that the fdstatus_main.ml might re-use the value of a HANDLE with the result that an fd appeared open - but it would not be a handle to the same thing as in cloexec.ml (i.e. the bug is book-keeping/correctness and not security). That said, Jenkins is relatively frequently suffering from this, so it'd be good to fix the test (again)...

dra27 added 2 commits April 1, 2025 14:35
On slower machines, it's possible for the main part of the test (in
fdstatus_main.ml) to complete before the cloexec.ml has actually
terminated as part of `Unix.execv` (this is an artefact of how the
underlying CRT `execv` call is implemented on Windows).

Strictly on native Windows, cloexec.ml creates an additional dummy file
and takes a write lock on it (with `Unix.lockf`). Before running, if
this file exists, fdstatus_main.ml will also try to take a write lock on
it, providing a way of fdstatus_main.ml being sure that its ancestor has
fully terminated without actually having to know its PID.
Particularly on slow VMs, on Windows it's possible to end up seeing a
"permission denied" error trying to delete tmp.txt at the end of the
test. The issue is that Windows Defender (or other virus scanners) are
backlogged during a testsuite run scanning executables which are being
compiled. When tmp.txt is closed, it gets added to Windows Defender's
scan queue, but it can take long enough that it hasn't actually closed
the file by the time fdstatus_main.ml tries to delete it.

The solution here is instead to open tmp.txt with O_SHARE_DELETE and
then use an additional C stub to open the file with
FILE_FLAG_DELETE_ON_CLOSE. This way, when fdstatus_main.ml terminates,
the file is automatically by Windows "when it's ready". ocamltest
merrily waits for the entire process group to terminate, as it usually
does.
@dra27
Copy link
Copy Markdown
Member Author

dra27 commented Apr 1, 2025

See also precheck#1038. If any one of these Windows tests fails this time, I may actually sob at my desk......

@nojb
Copy link
Copy Markdown
Contributor

nojb commented Apr 1, 2025

Incidentally, 1. looks very similar to ocaml/dune#11437.

@dra27
Copy link
Copy Markdown
Member Author

dra27 commented Apr 1, 2025

There may be something else interesting to dig into "at some point":

mingw64 on AppVeyor:

 ... testing 'channel_of.ml' => passed (wall clock: 0.26s)
 ... testing 'cloexec.ml' => passed (wall clock: 6.49s)
 ... testing 'dup.ml' => passed (wall clock: 0.22s)

msvc64 on precheck:

 ... testing 'channel_of.ml' => passed (wall clock: 3.50s)
 ... testing 'cloexec.ml' => passed (wall clock: 18.93s)
 ... testing 'dup.ml' => passed (wall clock: 3.96s)

mingw64 on precheck:

 ... testing 'channel_of.ml' => passed (wall clock: 5.51s)
 ... testing 'cloexec.ml' => passed (wall clock: 20.69s)
 ... testing 'dup.ml' => passed (wall clock: 3.17s)

msvc32 on precheck:

 ... testing 'channel_of.ml' => passed (wall clock: 0.67s)
 ... testing 'cloexec.ml' => passed (wall clock: 12.31s)
 ... testing 'dup.ml' => passed (wall clock: 0.65s)

mingw32 on precheck:

 ... testing 'channel_of.ml' => passed (wall clock: 0.34s)
 ... testing 'cloexec.ml' => passed (wall clock: 10.82s)
 ... testing 'dup.ml' => passed (wall clock: 0.37s)

Copy link
Copy Markdown
Contributor

@nojb nojb left a comment

Choose a reason for hiding this comment

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

LGTM

Tricky code !

@nojb
Copy link
Copy Markdown
Contributor

nojb commented Apr 15, 2025

@dra27 Feel free to merge if you are happy with the current state of the PR.

On Windows, the Unix library performs most operations on files using the
Windows API directly, but the Unix.file_descr structure has the option
for using an underlying CRT fd. In particular, these are needed for the
{in,out}_channel_of_descr functions.

For some reason, the underlying _open_osfhandle call doesn't check for
inheritability of the OS handle itself (it's slightly odd, because it
does check other properties of it). By doing this ourselves, the
resulting CRT fd now correctly propagates O_CLOEXEC.
@dra27
Copy link
Copy Markdown
Member Author

dra27 commented Apr 24, 2025

Thanks, @nojb - sorry for taking quite so long to push the revised Changes entry!!

@dra27 dra27 merged commit 23852cc into ocaml:trunk Apr 24, 2025
22 checks passed
@dra27 dra27 deleted the more-cloexec branch April 24, 2025 18:17
dra27 added a commit that referenced this pull request Apr 24, 2025
Preserve `O_CLOEXEC` when converting `Unix.file_descr` to a CRT fd on Windows

(cherry picked from commit 23852cc)
@dra27 dra27 mentioned this pull request May 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants