From: Theo Buehler <tb@theobuehler.org>
Subject: Re: rpki-client: fix possible pthread deadlock on exit
To: tech@openbsd.org
Date: Tue, 21 Jul 2026 13:36:46 +0200

On Tue, Jul 21, 2026 at 10:55:05AM +0200, Claudio Jeker 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.

This all makes sense and looks correct to me.

ok tb

> -- 
> :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);
>