[Bug]: Race condition in coz.h

[GiuseppeCesarano]

What happened?

When using Coz, the user must insert COZ_PROGRESS or COZ_BEGIN/COZ_END macros into the codebase. Due to the use case of Coz itself, these macros are likely to end up in a multithreaded environment. This could result in a race condition during the initialization of the function pointer.

Here's an example:

#include <coz.h>
#include <thread>

using namespace std::chrono_literals;

void test() {
  for (size_t i{0}; i < 1000; ++i) {
    std::this_thread::sleep_for(1ms);
    COZ_PROGRESS;
  }
}

int main(int argc, char *argv[]) {
  std::thread a{test};
  std::thread b{test};

  a.join();
  b.join();
}

both the threads will have a call to _call_coz_get_counter

static coz_counter_t* _call_coz_get_counter(int type, const char* name) {
  static unsigned char _initialized = 0;
  static coz_get_counter_t fn; // The pointer to _coz_get_counter
  
  if(!_initialized) {
    if(dlsym) {
      // Locate the _coz_get_counter method
      void* p = dlsym(RTLD_DEFAULT, "_coz_get_counter");
  
      // Use memcpy to avoid pedantic GCC complaint about storing function pointer in void*
      memcpy(&fn, &p, sizeof(p));
    }
    
    _initialized = 1;
  }
  
  // Call the function, or return null if profiler is not found
  if(fn) return fn(type, name);
  else return 0;
}

And here is the race condition:

// Thread 1                                                            // Thread 2
if(!_initialized) {
  if(dlsym) {                                                          
  // PREEMPTED                                                            
                                                                       if(!_initialized) {
                                                                         if(dlsym) {
    void* p = dlsym(RTLD_DEFAULT, "_coz_get_counter");                     void* p = dlsym(RTLD_DEFAULT, "_coz_get_counter");
    memcpy(&fn, &p, sizeof(p));                                            memcpy(&fn, &p, sizeof(p));
    // ...                                                                 ...           

It occurs on the fn and _initialized variables.

Compiling the code with Clang's -fsanitize=thread also reveals these two points as race conditions.

To further support my case, I checked the examples in the benchmark folder and noticed a similar pattern is used in the histogram benchmark, validating this as an intended use case. In fact, compiling the histogram benchmark with -fsanitize=thread also shows the same race conditions.

By the nature of the check it should be very hard to hit the race condition and even if it happens the resulting memory should be fine, but being pedantic a race condition is UB.

What code did you run Coz on?

Example shown and histogram benchmark.

Coz Version

Built from code at commit 4615c29

OS Version

Linux 6.12.10-arch1-1 unknown

Anything else to add?

Wrapping the two variables in atomics should fix the race condition, and since there is low contention on the variables (they are written once), I don't think it will hurt performance.

Another solution could be to make those variables constant, but this will require a specific place where they are initialized.

I would gladly make a PR to fix the issue if desired.