Fix data race on Windows file descriptors

[OlivierNicole]

On Windows, the function win_CRT_fd_of_filedescr is called by several functions of the Unix module, to wit:

• Unix.in_channel_of_descr
• Unix.out_channel_of_descr
• Unix.fsync
• Unix.dup2
• Unix.isatty

This function can cause races on the crt_fd field of the filedescr structure. This PR removes the data race itself by making the field atomic.

Currently not equipped with a Windows machine, I have not been able to test these changes.

Not fixed by this PR

This PR does not change the fact that concurrent calls to to this function can call the Windows API function _open_osfhandle several times on the same handle (as doing so would require some form of locking). Microsoft's documentation is unclear about the possible consequences of this (no consequences, file descriptor leaks, or error).

In addition, concurrent calls to win_CRT_fd_of_filedescr and Unix.close can still lead to fd leaks.

[xavierleroy]

concurrent calls to to this function can call the Windows API function _open_osfhandle several times on the same handle

This defeats the purpose of caching the result of _open_osfhandle in the crt_fd field of struct filedescr...

(as doing so would require some form of locking).

Indeed, it might be safer and easier to put a CRITICAL_SECTION in each struct filedescr and use it to protect crt_fd.

But you can also get it right with only atomic instructions + some busy-waiting: the thread that needs to call _open_osfhandle puts a special GETTING_CRT_FD value in crt_fd before the call, and other threads spin while crt_fd is GETTING_CRT_FD.

[OlivierNicole]

Indeed, it might be safer and easier to put a CRITICAL_SECTION in each struct filedescr and use it to protect crt_fd.

But you can also get it right with only atomic instructions + some busy-waiting: the thread that needs to call _open_osfhandle puts a special GETTING_CRT_FD value in crt_fd before the call, and other threads spin while crt_fd is GETTING_CRT_FD.

I propose such a busy-waiting implementation in the updated version. It wasn't clear to me how to implement it with a CRITICAL_SECTION, more precisely, when the CRITICAL_SECTION could be finalized, since blocking calls are forbidden in GC finalization, and maybe that includes DeleteCriticalSection?

Again, I haven't been able to test this, not having a Windows system at the moment.

[xavierleroy]

I took the liberty to push directly to your branch:

• a small change to your implementation that uses the SPIN_WAIT macro to spin more carefully;
• a test for the race condition that fails most of the time in the current trunk but seems to work OK with the new implementation.

I tested the code on the Mingw-64 system used for the Jenkins CI. It builds and passes the test suite.

Code reviews welcome, as always.

[OlivierNicole]

Thank you for this. I rebased the PR on trunk after the recent merge of #10926 and ensured that our functions follow the new naming policy.

Your improvements and tests look good to me.

[xavierleroy]

@dra27 @damiendoligez @sadiqj @ctk21 : could we get a quick review for this code? Thanks.

[dra27]

Sorry for the delay - partly because of faffing around with other things, but mainly because there was something I didn't like about this and I hadn't been able to put my finger on it. I now have - I think this would be fine, but I wonder if we can avoid the introduction of a new function?

Any uses of CRT_fd_val as an rvalue in user code I believe start to get warnings about _Atomic - or, if not, should we actually remove the macro, on the basis that it's no longer a safe way to access that field?

Entire review is a suggestion only - the PR could be merged as-is.

[sadiqj]

Agree with @dra27 that it's weird to have functions and the macros. I take the opposite view on the solution though, it might be nicer to have functions to retrieve/set which deal with the atomics and eliminate the macro entirely? At the moment the atomics are strewn across every use and may be forgotten in future uses of CRT_fd_val.

[sadiqj]

(This applies to all the relaxed stores in this change)

What's the rationale behind relaxed atomics? I see at least here we're doing some non-atomic stores before which makes me wonder.

[OlivierNicole]

What's the rationale behind relaxed atomics? I see at least here we're doing some non-atomic stores before which makes me wonder.

I was mistaken, I thought that for initializing writes we still needed relaxed atomics, but it appears that plain stores are fine.

Regarding macros vs functions: I tend to like more the idea of removing the CRT_fd_val macro entirely, keeping only functions, as macros used for both l- and rvalues have caused problems before. But I am fine with either way.

[xavierleroy]

Like @OlivierNicole, I'd rather unexport the CRT_fd_val macro and export only the getter function caml_win32_CRT_fd_of_filedescr.

Re "non-atomic" stores, you mean syntactic assignments a = ... instead of atomic_store(&a, ...), right? I understand both are equivalent, but please correct me if I'm wrong.

Re relaxed atomics, I think what we need here is sequential consistency for each crt_fd of each struct filedescr, but no consistency guarantees between several memory locations. In which case relaxed atomics might be enough. But performance is not critical and SC atomics are so much easier to reason about...

[OlivierNicole]

I unexported the CRT_fd_val macro, but kept the exports of caml_win32_CRT_fd_of_filedescr and caml_win32_get_CRT_fd (since both are used in other files, such as close_win32.c.

Regarding relaxed atomics, in my understanding since these are initializing writes, they will go through a fence before another thread has a chance to access the custom block, so there should be no difference between them and plain stores.

[OlivierNicole]

If a is a l-value of _Atomic-qualified type, even plain assignments a = ... are atomic (at the seq_cst consistency level, if I'm not mistaken). There is no such thing as a non-atomic write to an _Atomic-qualified l-value. But an atomic_store_explicit(..., memory_order_relaxed) can be used to produce a plain processor store.

I wasn't aware of this. In that case, I think I should revert to using atomic_store_explicit(..., memory_order_relaxed) for the initializing writes in caml_win32_alloc_handle, caml_win32_alloc_socket, caml_unix_filedescr_of_channel and caml_unix_filedescr_of_fd as atomic stores are not necessary there.

[xavierleroy]

I think I should revert to using atomic_store_explicit(..., memory_order_relaxed) for the initializing writes in caml_win32_alloc_handle, caml_win32_alloc_socket, caml_unix_filedescr_of_channel and caml_unix_filedescr_of_fd as atomic stores are not necessary there.

Do you really think the performance difference matters? If not, let's keep the code as simple as possible, i.e. with SC atomics everywhere. If so, I think memory_order_relaxed could be used for all accesses to crt_fd, on the basis that it's still SC per location. But we need confirmation from a memory model expert.

[xavierleroy]

Rebased to fix the conflict and restart CI.

[xavierleroy]

The code looks good to me in its current form. It's likely that the SC atomics are too strong, but do not really hurt.

[dra27]

Good to go

[sadiqj]

Thanks @OlivierNicole

[dra27]

Cherry-picked to 5.0 along with a commit fixing the mingw-w64 build.