From: Claudio Jeker <cjeker@diehard.n-r-g.com>
Subject: Re: rpki-client: fix possible pthread deadlock on exit
To: David Gwynne <david@gwynne.id.au>
Cc: tech@openbsd.org
Date: Wed, 22 Jul 2026 06:57:43 +0200

On Wed, Jul 22, 2026 at 09:41:11AM +1000, David Gwynne wrote:
> 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.

Calling wakeup without the lock can lead to missed wakeups. Especially
with tsleep(9). A good example is dowait6 which was switched to
sleep_setup / sleep_finish because of this issue. OK in that case there
was no clean condition variable to work with and the code depended on the
KERNEL_LOCK for this.

I decided to call the pthread_cond_broadcast() with the lock because 
it is simpler to do that then grabbing both mutexes when quit is altered.
The result is the same. 

Also the code already uses pthread_cond_signal() with the mutex held for
similar reasons. IIRC people argued back when I added pthread support in
rpki-client that calling pthread_cond_signal() with the mutex held is the
proper way to do this (even though this results in a wakeup spin).
 
> 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);
> > 
> 

-- 
:wq Claudio