Detached thread error in helgrind

[hebrerillo]

Hello everyone!

I am implementing a detached thread using pthreads. After I run the code under helgrind, it reports a race condition and I think it might be a false positive.

However, I want to be sure that I am not using detached pthreads the wrong way.

Here is a code example:

#include <stdio.h>
#include <pthread.h>
#include <unistd.h>

static pthread_mutex_t mutex;
static pthread_cond_t cond;
static int finished;
static int shared;

static void* runThread(void *args)
{
    pthread_mutex_lock(&mutex);
    usleep(1000 * 100);
    shared--;
    finished = 1;
    pthread_cond_broadcast(&cond);
    pthread_mutex_unlock(&mutex);
    pthread_exit(NULL);
    return NULL;
}

static void example()
{
    pthread_t thread;
    finished = 0;
    shared = 0;

    pthread_mutex_init(&mutex, NULL);
    pthread_cond_init(&cond, NULL);

    pthread_create(&thread, NULL, &runThread, NULL);
    pthread_detach(thread);

    pthread_mutex_lock(&mutex);
    shared++;
    while (finished == 0)
    {
        pthread_cond_wait(&cond, &mutex); //Wait for the detached thread to finish.
    }
    pthread_mutex_unlock(&mutex);

    pthread_cond_destroy(&cond);
    pthread_mutex_destroy(&mutex); //If I remove this instruction, there are no race conditions.
}

int main()
{
    for (size_t i = 0; i < 90; i++)
        example();
    return 0;
}

Basically, there is a shared integer variable called 'int shared'. The detached thread decrements it and the main thread increments it.

The main thread has to wait for the detached thread to finish by waiting in a condition variable until finish is true.

And this is the race condition reported by helgrind:

==3262== Possible data race during read of size 1 at 0x10C048 by thread #1
==3262== Locks held: none
==3262==    at 0x4844071: my_memcmp (hg_intercepts.c:226)
==3262==    by 0x484431F: mutex_destroy_WRK (hg_intercepts.c:882)
==3262==    by 0x4848CD3: pthread_mutex_destroy (hg_intercepts.c:905)
==3262==    by 0x109401: example (main.c:42)
==3262==    by 0x109438: main (main.c:48)
==3262== 
==3262== This conflicts with a previous write of size 4 by thread #81
==3262== Locks held: none
==3262==    at 0x48797D1: __pthread_mutex_unlock_usercnt (pthread_mutex_unlock.c:52)
==3262==    by 0x48797D1: pthread_mutex_unlock (pthread_mutex_unlock.c:357)
==3262==    by 0x4844B31: mutex_unlock_WRK (hg_intercepts.c:1182)
==3262==    by 0x4848D0F: pthread_mutex_unlock (hg_intercepts.c:1200)
==3262==    by 0x10931F: runThread (main.c:17)
==3262==    by 0x48475C2: mythread_wrapper (hg_intercepts.c:406)
==3262==    by 0x4875608: start_thread (pthread_create.c:477)
==3262==    by 0x49AF132: clone (clone.S:95)
==3262==  Address 0x10c048 is 8 bytes inside data symbol "mutex"

To get this error, I have to run helgrind at least twice. The first time I run helgrind I get no errors at all.

If I remove 'pthread_mutex_destroy(&mutex)', helgrind reports no errors at all.

Do you think I am using the detached thread in a wrong way? or is it that helgrind is reporting a false positive?

Thank you!

[kigar64551]

With a "detached" thread, you have no guarantees on when that thread actually terminates. That's unlike a pthread_join(), where you can be sure that the thread has actually terminated when the pthread_join() function returns.

So, I think, in your example() function, the invocations of pthread_cond_destroy() and pthread_mutex_destroy() are not guaranteed to only happen after the thread already has terminated. The thread could remain alive after line #17 for an undefined amount of time before it actually terminates! Your example() function does not explicitly wait for the threads to terminate! I think this should be "okay" here, because the thread won't access the shared pthread_mutex_t and pthread_cond_t again after line #16 and line #17. And because example() cannot proceed beyond the while loop before the thread has completed line #17.

But, anyways, why not use a normal pthread_join() in your example() function, before destroying the shared resources?

________


As an aside, you should check the return value of every pthread_*() invocation for possible errors!

[hebrerillo]

Hello Kigar!

Thank you so much for your help!

My original code is big and complex and makes use of detached threads, that is why I posted that example code and removed the checks of returned values of pthread_* to make it as simple as possible.

I think, in your example() function, the invocations of pthread_cond_destroy() and pthread_mutex_destroy() are not guaranteed to only happen after the thread already has terminated

Yes, they are guaranteed to happen after the detached thread releases 'mutex'. When the main thread is notified on line 38 and 'pthread_cond_wait' returns, the 'mutex' is locked, and that will block the main thread, because the detached thread has 'mutex' already locked on line 12.

That is, the main thread will block until the detached thread releases 'mutex' on line 17.

By the way, I posted the wrong output of helgrind. The read data race error is on line 43, not 42.

==3118== Possible data race during read of size 1 at 0x10C048 by thread #1
==3118== Locks held: none
==3118==    at 0x4844071: my_memcmp (hg_intercepts.c:226)
==3118==    by 0x484431F: mutex_destroy_WRK (hg_intercepts.c:882)
==3118==    by 0x4848EE3: pthread_mutex_destroy (hg_intercepts.c:905)
==3118==    by 0x109401: example (main.c:43)
==3118==    by 0x109438: main (main.c:49)
==3118== 
==3118== This conflicts with a previous write of size 4 by thread #6
==3118== Locks held: none
==3118==    at 0x48797D1: __pthread_mutex_unlock_usercnt (pthread_mutex_unlock.c:52)
==3118==    by 0x48797D1: pthread_mutex_unlock (pthread_mutex_unlock.c:357)
==3118==    by 0x4844D41: mutex_unlock_WRK (hg_intercepts.c:1182)
==3118==    by 0x4848F2E: pthread_mutex_unlock (hg_intercepts.c:1200)
==3118==    by 0x10931F: runThread (main.c:17)
==3118==    by 0x48477D2: mythread_wrapper (hg_intercepts.c:406)
==3118==    by 0x4875608: start_thread (pthread_create.c:477)
==3118==    by 0x49AF132: clone (clone.S:95)
==3118==  Address 0x10c048 is 8 bytes inside data symbol "mutex"

The thing is that if you decrese the number of loops on line 48 to 4 or 5 loops (or remove the for-loop), helgrind reports no errors at all.

Is not that weird??? Don't you think this could be a 'helgrind' bug??

Anyway, thank you so much for your response!

[kigar64551]

Yeah, could be a "false positive".

On the other hand, think about this: The function pthread_mutex_unlock() is not just a single "atomic" CPU instruction; it is a complex function implemented in libpthread. All we really know is that, at some point in pthread_mutex_unlock(), the mutex will be marked as "not currently locked", so that the other thread can proceed. But, does pthread_mutex_unlock() return immediately after that point? Is it guaranteed that pthread_mutex_unlock() won't access the pthread_mutex_t struct after that point, before returning? I don't think so, because this is an implementation detail that we should not make any assumptions about. Better assume the "worst" case 😏

In other words, your example() function does not wait until pthread_mutex_unlock() actually has fully returned inside of the runThread() function; it only waits until the runThread() function has (at least) begun its pthread_mutex_unlock() operation.

Surely, any kind of "concurrent" or "interleaved" pthread_mutex_lock() or pthread_mutex_unlock() invocations on the same pthread_mutex_t must always be safe, assuming that the pthread_mutex_t was properly initialized. Meanwhile, destorying the pthread_mutex_t while an pthread_mutex_unlock() still might be in progress probably results in undefined behavior!

I think it's a good guideline to only destroy "shared" resources after all (other) threads that might access those resources have terminated.

________

EDIT: Is it really necessary to create a new pthread_mutex_t and pthread_cond_t for each thread? You could simply create those once and re-use them. This eliminates the need to destroy them (except when your program terminates). Note that your variables are declared static, so you can only have one mutex and cond at a time anyways. Doesn't make much sense to destroy + re-create them continuously...

EDIT²: If you want to be sure that multiple threads have passed a certain point, consider using a pthread_barrier:
https://www.daemon-systems.org/man/pthread_barrier.3.html

(But, of course, this creates the new problem of when to call the pthread_barrier_destroy function 😏)

[kbw]

Lots of assumptions in that code:
1. while (finished == 0) will be true
2. pthread_mutex_destroy(&mutex); runs after runThread has completed.
3. mutex and cond are initialized when needed.
...

These aren't necessarily true.

[kigar64551]

1. while (finished == 0) will be true

This is a fairly standard pattern to safely and efficiently wait until a specific condition is met (e.g. that a "shared" variable contains a specific value), with the help of a pthread mutex and a pthread conditional variable:

pthread_mutex_lock(&mutex);
while (shared_variable != EXPECTED_VALUE)
{
    pthread_cond_wait(&cond, &mutex); // <-- unlocks the mutex while waiting; re-locks the mutex before proceeding
}
pthread_mutex_unlock(&mutex);

...provided that any thread which wants to accesses the "shared" variable will acquire the mutex before doing so (and will release the mutex afterwards). And that any thread, which has modified the value of the "shared" variable, will signal the conditional variable.

In this regard, I think hebrerillo's code looks correct.

[dhayden]

If line 34 executes before line 12, then finished will never be set.

[kigar64551]

If line 34 executes before line 12, then finished will never be set.

finished is initialized to zero in the "main" thread in L#25, before the other thread is even created in L#31.

So, nothing in runThread() can happen before the "main" thread has completed L#31.

The "main" thread then loops in #36 ff. until finished becomes non-zero. This can only happen when the other thread executes L#15.

While finished still is zero, the "main" thread sleeps, using pthread_cond_wait(), until the other threads signals the condition in L#16.

All access to the "shared" variable finished is properly protected by a mutex.


Note: A thread must "own" the mutex when calling pthread_cond_wait(); however, if the call to pthread_cond_wait() blocks, then the mutex is automatically un-locked while the thread is sleeping; the mutex is re-locked automatically as soon as pthread_cond_wait() returns.