Frequent crash in ws2_32 hooks (thread-safety issue)

Hello,

I would like to report a crash, with some technical details. I hope this is an appropriate channel to do that.

For context, I work for a game development company, and I recently discovered a large group of crashes in our crash reporting system -- some 25000 crashes over the course of about 6 months -- which I traced back to ReShade.

The crash primarily occurs in the ws2_32 hooks that ReShade installs into its host process.
Description of the symptoms:
 - on startup, our game creates some background threads that download various metadata from our servers
 - the background threads call through curl and openssl, and end up in ReShade's recv() or send() hook
 - the hook crashes trying to call a null function pointer

I looked at the hook code, and nothing obviously wrong stood out. Still, I suspected that this might be some sort of concurrency bug.
After some investigation, I discovered that the thread safety of static-local variables was explicitly disabled using the "/Zc:threadSafeInit-" compiler option in commit 054a4dbe7a1e27787f5618a69fe91dabb40ca01b, back in 2022. This is almost certainly the cause of the crashes I'm seeing.

I'm confident you already know how static-locals work, but let me summarize, just in case. Normally, static-locals are initialized the first time the function gets called. The C++ standard requires the initialization to be thread-safe, that is, it requires that even if the function is called from multiple threads at once, the initialization runs exactly one time, and no threads ever see the variable uninitialized. Disabling it means that if multiple threads call the function simultaneously, one of them might read the variable before it was written.
This explains the crash exactly: on one of the threads, the hook's trampoline is read as null.

Further evidence pointing towards this being the problem is the fact that the first release that includes the above commit is 5.3.0, which corresponds to the first version that I'm seeing among our crashes.
(For the sake of completeness, I have crash reports with the following DLL versions: 5.3.0.1556, 5.4.0.1572, 5.4.2.1604, 5.5.2.1676, 5.6.0.1690, 5.7.0.1711, 5.8.0.1726, 5.9.0.1750, 5.9.1.1754, 5.9.2.1758, 6.0.0.1814, 6.0.1.1823, 6.3.0.1901, 6.3.1.1914, 6.3.2.1920, 6.3.3.1924, 6.4.0.1958, 6.4.1.1968, 6.5.0.2003, 6.5.1.2011, 6.6.0.2075, 6.6.1.2079, 6.6.2.2083, 6.7.0.2122, 6.7.1.2134, 6.7.2.2146, 6.7.3.2150).

I would like to kindly request that either the above change be reverted, or some other solution be found for the thread-safety of the hooks.

Please let me know if there is any additional detail I can provide to help.

Many thanks,
Walter

Thank you so much debugging (and sorry for being the cause of crashes)! I imagine tracking it down to the obscure compiler flag was a journey, considering things would look correct code-wise ...

IIRC this had been primarily done to counteract overhead in the OpenGL hooks (which consist of many frequently called entry points with a similar pattern, but which are not expected to be initially called simultaneously from multiple threads, making this viable), but I clearly hadn't considered all other uses where it may break. Flipped it around in github.com/crosire/reshade/commit/697b00...fdbcb48c9ce848c6d54f now, so that the networking hooks are safe.

Cheers