Fix concurrent init/deinit race with a lifecycle mutex and refcounting

[crvineeth97]

Summary

cpuinfo_deinitialize() (added in #387) introduced data races when multiple independent consumers initialize and deinitialize cpuinfo concurrently. Libraries such as tflite, ruy, and pthreadpool all call deinitialize without coordinating with one another.

This PR fixes the root cause.

The problem

The previous lifecycle had two race conditions:

1. Non-once-safe guards. cpuinfo_deinitialize() reset the pthread_once_t / INIT_ONCE guards back to their static initializers so the next cpuinfo_initialize() would re-run. Writing to a pthread_once_t/INIT_ONCE while another thread may be reading it is undefined behavior and defeats the "run exactly once" guarantee, enabling double-init / double-free.

2. No reference counting. The first deinitialize() freed the shared global state unconditionally, even while other consumers were still using it. A thread reading cpuinfo_get_*() would then hit freed state and abort.

The fix

• Replace the two pthread_once/INIT_ONCE guards with a single static lifecycle mutex. The mutex serializes the init/deinit sections, so the platform once-guards are no longer needed.
• Add an unsigned int reference count. cpuinfo_initialize() runs the platform init only for the first consumer and increments the count on success; cpuinfo_deinitialize() decrements and only frees the shared state once the last consumer has released it.

This keeps shared globals alive until the final consumer is done, so concurrent init/deinit across independent users is safe.

Tests

• deinitialize_balances_initialize - verifies that a deinitialize() after two initialize() calls does not tear down state still held by another reference.
• concurrent_deinitialize_does_not_disturb_other_consumers - a multithreaded stress test modeling the exact reported scenario: one long-lived consumer continuously using cpuinfo while several other threads concurrently init/deinit. Before this fix the holder aborts on freed state; after it, the run completes cleanly.

[crvineeth97]

@GregoryComer @georgthegreat @malfet Requesting a review

[georgthegreat]

I would suggest making this behavior an opt-in.

Why ORT (which is the only library that poorly supports static linking) should cause a disruption to every other program?
Their load / unload case is somewhat weird.

[georgthegreat]

At the moment the code is susceptible to a race condition if cpuinfo initialization takes place in pre-main (which does not look impossible to me).

[georgthegreat]

There is nothing wrong with global variables being left upon program exit. Valgrind properly handles this, memory sanitizers simply does not report it by default.

I think the original problem regards ORT only and should be fixed accordingly.

[crvineeth97]

Thanks @georgthegreat, that's a fair point. I've updated the PR to make the deinit behavior opt-in so it doesn't affect any existing consumers. For what it's worth, even the opt-in path uses a constant-initialized lock, so it should be safe to acquire from a pre-main initializer.

I agree that for process exit, leaked globals are cleaned up by the OS and memory sanitizers handle this well. The motivating case is different thought, at least on Windows, it's a dynamic DLL unload mid-process. ORT is often loaded as a DLL inside another DLL and then FreeLibrary'd while the process keeps running. When that happens, cpuinfo's globals are never released and get flagged as a memory leak.

@GregoryComer @fbarchard, I noticed there don't seem to be any tests actually running in the CI pipeline. Not sure if I'm missing something here. I've added a simple ctest that exercises the deinit path. Please let me know if that should be removed

[snnn]

There is nothing wrong with global variables being left upon program exit.

It is a bit arguable. In ONNX Runtime's case, if you don't destroy the global OrtEnv object correctly , it may lead to a crash on exit. This is because C++ objects' destruction order on Windows is different than other platforms. A wrong destruction order could lead to use-after-free issues, which could cause crashes. But, let's say, if you have a global object whose destructor depends on nothing else but the standard C/C++ runtime, in most cases it should be fine. Otherwise you will need to be cautious on that. I am indicating this change should be taken or not. But, I was about to say, the new code does leak something: the mutex you are adding. On some platforms, for example macOS and Android and Linux libc++, the std::mutex's destructor is not a no-op. It's hard to tell whether calling pthread_mutex_destroy is necessary, because it is very platform dependent and even OS version dependent . I just wanted to point out that the new code is introducing another potential memory leak. The easiest way to implement Singleton pattern is to use C++'s function local static, which is also faster than most other choices. But this is C code. Similarly, if you can use std::mutex, use std::mutex. Otherwise you will have to deal with the platform differences, and that is much more complicated and could lead to endless debates. Like, what's wrong if I don't call pthread_mutex_destroy at all?

[georgthegreat]

Yep, a lot of fuss here is caused by the lack of C++ runtime.
In C++ one would just use magic static.

[crvineeth97]

Thanks @snnn, I agree std::mutex would be the clean approach. Unfortunately, we have to deal with C code here. A couple of clarifications: the code uses a static-init pthread_mutex_t, where the destroy is effectively a no-op on mainstream libcs, and SRWLOCK has no cleanup API and allocates no resources. The lifecycle lock has to outlive the init/deinit cycles so it cannot be destroyed. A use-after-destroy is worse than leaving a single fixed-size static around. It also doesn't accumulate across DLL load/unload and doesn't get flagged by leak checkers.

[crvineeth97]

@malfet Rebased to latest main which should fix the cmake-uwp build.

I noticed there don't seem to be any tests actually running in the CI pipeline. Not sure if I'm missing something here. I've added a simple ctest that exercises the deinit path. Please let me know if that should be removed

Should the new build I added run the tests?

[crvineeth97]

@malfet @GregoryComer Requesting your reviews

[malfet]

This is effectively a roll-back of #387 , because all of the changes you are adding as well as test are guarded behind #ifdefs which are off by default. Let's just revert the original change and you can propose a new one, with clear init/deinit routines and tests, which are enabled by default