Data race in init_shuffle_implementation

[jbms]

There appears to be a data race in the init_shuffle_implementation function in shuffle.c (identified by ThreadSanitizer).

The problematic lines are:

  if (!implementation_initialized) {
    /* Initialize the implementation. */
    host_implementation = get_shuffle_implementation();

    /*  Set the flag indicating the implementation has been initialized. */
    implementation_initialized = 1;
  }

The issue is that the compiler is free to reorder the writes to host_implementation and implementation_initialized, and likewise could reorder the subsequent read from host_implementation to before the read from implementation_initialized, since no memory synchronization is specified.

The solution is to make implementation_initialized an atomic variable (e.g. using C11 _Atomic), or an equivalent compiler-specific builtin if you don't want to depend on C11.

[FrancescAlted]

Thanks for bring this to our attention. Unfortunately, we cannot use C11 (and not even C99) for C-Blosc as we try to keep it compatible with VS2010, which is necessary for different reasons. I wonder if you can provide a fix (PR) that does not depend on C11/C99.

[wenjuno]

I think using atomic solves only a part of the problem. The host_implementation is a struct so cannot be assigned atomically, so that multiple threads can potentially write to it simultaneously which is a data race. I think we need a mutex to guard this block of code, and we don't have to make implementation_initialized atomic in that case.
If we concerned about the regression of added mutex locking/unlocking, how about making host_implementation and implementation_initialized thread local variables? That should also make the codes simpler.

[jbms]

The canonical approach would be to use pthread_once, but that would require providing a Win32 implementation. That would be easy to do using the Windows One-time initialization APIs (https://docs.microsoft.com/en-us/windows/desktop/sync/one-time-initialization), but those are only available starting with Windows Vista and Windows Server 2008. Does blosc need to work on older versions of Windows? If it does, I think pthread_once could be implemented for MSVC by taking advantage of the acquire/release semantics MSVC gives accesses to volatile variables (https://docs.microsoft.com/en-us/cpp/build/reference/volatile-volatile-keyword-interpretation?view=vs-2017). However, that is a bit tricky because specifying /volatile:iso would break it --- I'm not sure if that flag affects C or only C++.

I'm not sure if the multiple writes to the host_implementation are a problem in practice, because the writes will all be the same value. In any case it makes sense to avoid that by using pthread_once.

[FrancescAlted]

I'd say that keeping compatibility with Windows 7 should be more than enough?