icculus / physfs

A portable, flexible file i/o abstraction.
https://icculus.org/physfs/
zlib License
560 stars 98 forks source link

Thread Sanitizer flags `__PHYSFS_platformGrabMutex` as having a data race #87

Open nicebyte opened 3 months ago

nicebyte commented 3 months ago

It appears that TSan gets tripped up by reading/writing the owner of the mutex in the function below:

int __PHYSFS_platformGrabMutex(void *mutex)
{
    PthreadMutex *m = (PthreadMutex *) mutex;
    pthread_t tid = pthread_self();
    if (m->owner != tid)
    {
        if (pthread_mutex_lock(&m->mutex) != 0)
            return 0;
        m->owner = tid;
    } /* if */

    m->count++;
    return 1;
} /* __PHYSFS_platformGrabMutex */

I've added this function to my suppressions file, but would be nice to do something about it? Technically, an atomic load/store is warranted here i believe.

icculus commented 3 months ago

I'll have to check when I'm not on a phone, but I think this was intended to deal with platforms that didn't offer recursive mutexes--which Mac OS X didn't at the time--but I think anything we care about does now and we should probably just remove this.

(Although strictly speaking, TSan is incorrect here: if we already hold the mutex, that value will match the current thread ID, otherwise we will wait for the lock. Short of memory corruption, it can't actually get a non-deterministic or racey result, afaict.)