Re: [PATCH] Fix cThread related race conditions

On 15/02/2023 17:17, Klaus Schmidinger wrote:
> On 02.02.23 21:56, Patrick Lerda wrote:
>> ...
>> diff --git a/thread.c b/thread.c
>> index 93eb8c0..21be7a4 100644
>> --- a/thread.c
>> +++ b/thread.c
>> @@ -312,13 +312,16 @@ bool cThread::Start(void)
>>                 cCondWait::SleepMs(THREAD_STOP_SLEEP);
>>           }
>>        if (!active) {
>> -        active = running = true;
>> -        if (pthread_create(&childTid, NULL, (void *(*) (void 
>> *))&StartThread, (void *)this) == 0) {
>> -           pthread_detach(childTid); // auto-reap
>> +        pthread_t localTid;
>> +        running = true;
>> +        if (pthread_create(&localTid, NULL, (void *(*) (void 
>> *))&StartThread, (void *)this) == 0) {
>> +           pthread_detach(localTid); // auto-reap
>> +           childTid = localTid;
>> +           active = true;
>>              }
>>           else {
>>              LOG_ERROR;
>> -           active = running = false;
>> +           running = false;
>>              return false;
>>              }
>>           }
>> @@ -339,11 +342,12 @@ bool cThread::Active(void)
>>        // performed but no signal is actually sent.
>>        //
>>        int err;
>> -     if ((err = pthread_kill(childTid, 0)) != 0) {
>> +     const pthread_t localTid = childTid;
>> +     if ((err = pthread_kill(localTid, 0)) != 0) {
>>           if (err != ESRCH)
>>              LOG_ERROR;
>> -        childTid = 0;
>> -        active = running = false;
>> +	if (active && childTid == localTid)
> 
> localTid was initialized to childTid 4 lines earlier, so what's with
> the "childTid == localTid" check here? Isn't this always true?
> 

The function pthread_kill() is not instantaneous and could pause the 
thread, this condition ensures that childTid was not updated in the 
meantime by a second thread.

>> +	  active = running = false;
>>           }
>>        else
>>           return true;
>> @@ -355,6 +359,7 @@ void cThread::Cancel(int WaitSeconds)
>>   {
>>     running = false;
>>     if (active && WaitSeconds > -1) {
>> +     const pthread_t localTid = childTid;
>>        if (WaitSeconds > 0) {
>>           for (time_t t0 = time(NULL) + WaitSeconds; time(NULL) < t0; 
>> ) {
>>               if (!Active())
>> @@ -363,9 +368,9 @@ void cThread::Cancel(int WaitSeconds)
>>               }
>>           esyslog("ERROR: %s thread %d won't end (waited %d seconds) - 
>> canceling it...", description ? description : "", childThreadId, 
>> WaitSeconds);
>>           }
>> -     pthread_cancel(childTid);
>> -     childTid = 0;
>> -     active = false;
>> +     pthread_cancel(localTid);
>> +     if (active && childTid == localTid)
> 
> Same here?
> 
> I see this happens with "address sanitizer". Is there an actual,
> reproducible, real world problem that this patch fixes?
> 

I had a few random crashes related to cThread, so it could happen. 
Anyway the sanitizer seems to increase the likelihood of these events.

>> +       active = false;
>>        }
>>   }
>>   diff --git a/thread.h b/thread.h
>> index 16c4bd7..06046ea 100644
>> --- a/thread.h
>> +++ b/thread.h
>> @@ -13,6 +13,7 @@
>>   #include <pthread.h>
>>   #include <stdio.h>
>>   #include <sys/types.h>
>> +#include <atomic>
>>     typedef pid_t tThreadId;
>>   @@ -56,7 +57,7 @@ class cRwLock {
>>   private:
>>     pthread_rwlock_t rwlock;
>>     int locked;
>> -  tThreadId writeLockThreadId;
>> +  std::atomic<tThreadId> writeLockThreadId;
>>   public:
>>     cRwLock(bool PreferWriter = false);
>>     ~cRwLock();
>> @@ -79,9 +80,11 @@ public:
>>   class cThread {
>>     friend class cThreadLock;
>>   private:
>> -  bool active;
>> -  bool running;
>> -  pthread_t childTid;
>> +  std::atomic_bool active;
>> +  std::atomic_bool running;
>> +  std::atomic<pthread_t> childTid;
>> +       ///< Assume that the content of childTid is valid when the 
>> class member
>> +       ///< active is set to true and undefined otherwise.
>>     tThreadId childThreadId;
>>     cMutex mutex;
>>     char *description;
> 
> Are the "atomics" really necessary?
> 

Atomic is mandatory, the compiler has to generate the proper op codes to 
ensure "atomic" operations at the thread level.

Patrick


> Klaus
> 
> 
> _______________________________________________
> vdr mailing list
> [email protected]
> https://www.linuxtv.org/cgi-bin/mailman/listinfo/vdr

_______________________________________________
vdr mailing list
[email protected]
https://www.linuxtv.org/cgi-bin/mailman/listinfo/vdr