From: David Gwynne <david@gwynne.id.au>
Subject: Re: rpki-client: fix possible pthread deadlock on exit
To: tech@openbsd.org
Date: Wed, 22 Jul 2026 09:41:11 +1000

I agree with Philip, it should be safe to pthread_cond_signal or 
pthread_cond_broadcast without holding the mutex, just like it's safe 
(and often desirable) to call wakeup(9) in the kernel without the lock 
the waiting thread is sleeping on.

On 22/07/2026 05:22, Philip Guenther wrote:
> Hmm, as long as the Boolean being checked (quit?) is both checked and 
> changed with the mutex held, there should be no race. How did the hung 
> thread see quit not true before waiting but the signaling thread set 
> or see it as true before signaling?  Smells like quit is being set 
> outside the mutex…
>
> (Holding the mutex while signaling is fine of course, but feels like 
> it should have been held even earlier)
>
> Philip
>
> On Tuesday, July 21, 2026, Claudio Jeker <cjeker@diehard.n-r-g.com> wrote:
>
>     The interaction between quit and the pthread_cond_broadcast calls
>     in the
>     exit path are not properly sequenced. Calling the cond_broadcast
>     without
>     holding its mutex can lead to a case where the broadcast is lost
>     and so
>     that thread would not wake up. The process would then hang forever in
>     pthread_join().
>
>     Grab the mutex before sending the pthread_cond_broadcast() fixes this
>     issue since now the receiver is either properly parked or it will
>     see that quit is true and exit before calling pthread_cond_wait().
>
>     Also do an early exit from the event loop if quit is true. There is
>     no need work through the full event loop in that case.
>     -- 
>     :wq Claudio
>
>     Index: parser.c
>     ===================================================================
>     RCS file: /cvs/src/usr.sbin/rpki-client/parser.c,v
>     diff -u -p -r1.182 parser.c
>     --- parser.c    26 Jun 2026 08:05:22 -0000      1.182
>     +++ parser.c    9 Jul 2026 20:44:10 -0000
>     @@ -1072,6 +1072,12 @@ parse_worker(void *arg)
>                                     errx(1, "pthread_cond_wait: %s",
>                                         strerror(error));
>                     }
>     +               if (quit) {
>     +                       if ((error =
>     pthread_mutex_unlock(&globalq_mtx)) != 0)
>     +                               errx(1, "pthread_mutex_unlock: %s",
>     +                                   strerror(error));
>     +                       break;
>     +               }
>                     n = 0;
>                     while ((entp = TAILQ_FIRST(&globalq)) != NULL) {
>                             TAILQ_REMOVE(&globalq, entp, entries);
>     @@ -1141,6 +1147,8 @@ parse_writer(void *arg)
>                             if (error != 0)
>                                     errx(1, "pthread_mutex_lock: %s",
>                                         strerror(error));
>     +                       if (quit)
>     +                               break;
>                     }
>
>                     if (msgbuf_queuelen(myq) > 0) {
>     @@ -1279,13 +1287,19 @@ proc_parser(int fd, int nthreads)
>             }
>
>             /* signal all threads */
>     +       if ((error = pthread_mutex_lock(&globalq_mtx)) != 0)
>     +               errx(1, "pthread_mutex_lock: %s", strerror(error));
>     +       if ((error = pthread_mutex_lock(&globalmsgq_mtx)) != 0)
>     +               errx(1, "pthread_mutex_lock: %s", strerror(error));
>     +
>             if ((error = pthread_cond_broadcast(&globalq_cond)) != 0)
>                     errx(1, "pthread_cond_broadcast: %s",
>     strerror(error));
>             if ((error = pthread_cond_broadcast(&globalmsgq_cond)) != 0)
>                     errx(1, "pthread_cond_broadcast: %s",
>     strerror(error));
>
>     -       if ((error = pthread_mutex_lock(&globalq_mtx)) != 0)
>     -               errx(1, "pthread_mutex_lock: %s", strerror(error));
>     +       if ((error = pthread_mutex_unlock(&globalmsgq_mtx)) != 0)
>     +               errx(1, "pthread_mutex_unlock: %s", strerror(error));
>     +
>             while ((entp = TAILQ_FIRST(&globalq)) != NULL) {
>                     TAILQ_REMOVE(&globalq, entp, entries);
>                     entity_free(entp);
>