Re: [PATCH] Fix cThread related race conditions

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

_______________________________________________
vdr mailing list
vdr@xxxxxxxxxxx
https://www.linuxtv.org/cgi-bin/mailman/listinfo/vdr



[Index of Archives]     [Linux Media]     [Asterisk]     [DCCP]     [Netdev]     [Xorg]     [Util Linux NG]     [Xfree86]     [Big List of Linux Books]     [Fedora Users]     [Fedora Women]     [ALSA Devel]     [Linux USB]

  Powered by Linux