[PATCH] Fix cThread related race conditions - vdr

2 Feb 2023

This change fixes the following issue:
==15457==ERROR: AddressSanitizer: SEGV on unknown address 0x0000000002d0 (pc 0x7fd2f4301710 bp 0x7fd2b5552a30 sp 0x7fd2b5552988 T228)
==15457==The signal is caused by a READ memory access.
==15457==Hint: address points to the zero page.
    #0 0x7fd2f4301710 in pthread_detach (/lib64/libc.so.6+0x8b710)
    #1 0xdb4430 in cThread::Start() vdr-2.6.3/thread.c:317
    #2 0x7fd2e5ea8fc4 in cIptvStreamer::Open() vdr-2.6.3/PLUGINS/src/iptv/streamer.c:66
    #3 0x7fd2e5e1e8c2 in cIptvDevice::OpenDvr() vdr-2.6.3/PLUGINS/src/iptv/device.c:342
    #4 0x634903 in cDevice::Action() vdr-2.6.3/device.c:1714
    #5 0xdb644a in cThread::StartThread(cThread*) vdr-2.6.3/thread.c:293
    #6 0x7fd2f4300729  (/lib64/libc.so.6+0x8a729)
    #7 0x7fd2f437d0bb  (/lib64/libc.so.6+0x1070bb)
AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV (/lib64/libc.so.6+0x8b710) in pthread_detach
Thread T228 (device 5 receiv) created by T222 (cLiveStreamer s) here:
    #0 0x7fd2f52d4716 in pthread_create (/usr/lib64/libasan.so.6+0x59716)
    #1 0xdb437b in cThread::Start() vdr-2.6.3/thread.c:316
    #2 0x63a3ad in cDevice::AttachReceiver(cReceiver*) vdr-2.6.3/device.c:1844
    #3 0x7fd2e8162c56 in cDummyReceiver::Create(cDevice*) vdr-2.6.3/PLUGINS/src/vnsiserver/videoinput.c:472
    #4 0x7fd2e816dbc2 in cVideoInput::Open(cChannel const*, int, cVideoBuffer*) vdr-2.6.3/PLUGINS/src/vnsiserver/videoinput.c:530
    #5 0x7fd2e80d78d3 in cLiveStreamer::Action() vdr-2.6.3/PLUGINS/src/vnsiserver/streamer.c:318
    #6 0xdb644a in cThread::StartThread(cThread*) vdr-2.6.3/thread.c:293
    #7 0x7fd2f4300729  (/lib64/libc.so.6+0x8a729)
---
 thread.c | 25 +++++++++++++++----------
 thread.h | 11 +++++++----
 2 files changed, 22 insertions(+), 14 deletions(-)

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)
+	  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)
+       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;
-- 
2.39.1



    