Re: [PATCH] Fix cThread related race conditions

[Klaus Schmidinger <Klaus.Schmidinger@xxxxxxx>]

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?

> +	  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?

> +       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?

Klaus


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