Re: [vdr] [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 vdr@linuxtv.org https://www.linuxtv.org/cgi-bin/mailman/listinfo/vdr