Race condition in tdb_runtime_check_for_robust_mutexes()

[Jeremy Allison]

On Sun, Mar 27, 2016 at 09:34:38AM +0200, Ralph Boehme wrote:
> 
> > but to be honest I'd much prefer it if you moved those calls outside
> > and back to just before/after the calls to
> > tdb_robust_mutex_setup_sigchild(), and leave that call as-is. That
> > way I can *see* they're there at the right point in the code.
> 
> attached. :)

Thanks Ralph, I can actually understand your code
changes now :-).

Pushed !

> From 40cef032dab216e7098bf08652f40be004594f9a Mon Sep 17 00:00:00 2001
> From: Ralph Boehme <slow at samba.org>
> Date: Sat, 26 Mar 2016 12:43:55 +0100
> Subject: [PATCH] tdb: avoid a race condition when checking for robust mutexes
> 
> This fixes a race between calling waitpid() in two places (SIGCHLD the
> signal handler and the rendezvous code when waiting for the child to
> terminate), by
> 
> - blocking SIGCHLD before installing our signal handler
> 
> - in the rendezvous code call sigssuspend() which unblocks SIGCHLD and
>   suspends the thread and waits for signal delivery
> 
> BUG: https://bugzilla.samba.org/show_bug.cgi?id=11808
> 
> Signed-off-by: Ralph Boehme <slow at samba.org>
> ---
>  lib/tdb/common/mutex.c | 45 ++++++++++++++++++++++++---------------------
>  1 file changed, 24 insertions(+), 21 deletions(-)
> 
> diff --git a/lib/tdb/common/mutex.c b/lib/tdb/common/mutex.c
> index fae43d4..1f9ea22 100644
> --- a/lib/tdb/common/mutex.c
> +++ b/lib/tdb/common/mutex.c
> @@ -777,6 +777,7 @@ _PUBLIC_ bool tdb_runtime_check_for_robust_mutexes(void)
>  	bool ok;
>  	int status;
>  	static bool initialized;
> +	sigset_t mask, old_mask, suspend_mask;
>  
>  	if (initialized) {
>  		return tdb_mutex_locking_cached;
> @@ -828,6 +829,19 @@ _PUBLIC_ bool tdb_runtime_check_for_robust_mutexes(void)
>  		goto cleanup_ma;
>  	}
>  
> +	/*
> +	 * Block SIGCHLD so we can atomically wait for it later with
> +	 * sigsuspend()
> +	 */
> +	sigemptyset(&mask);
> +	sigaddset(&mask, SIGCHLD);
> +	ret = pthread_sigmask(SIG_BLOCK, &mask, &old_mask);
> +	if (ret != 0) {
> +		goto cleanup_ma;
> +	}
> +	suspend_mask = old_mask;
> +	sigdelset(&suspend_mask, SIGCHLD);
> +
>  	if (tdb_robust_mutex_setup_sigchild(tdb_robust_mutex_handler,
>  			&tdb_robust_mutext_old_handler) == false) {
>  		goto cleanup_ma;
> @@ -884,16 +898,9 @@ _PUBLIC_ bool tdb_runtime_check_for_robust_mutexes(void)
>  	}
>  
>  	while (tdb_robust_mutex_pid > 0) {
> -		pid_t pid;
> -
> -		errno = 0;
> -		pid = waitpid(tdb_robust_mutex_pid, &status, 0);
> -		if (pid == tdb_robust_mutex_pid) {
> -			tdb_robust_mutex_pid = -1;
> -			break;
> -		}
> -		if (pid == -1 && errno != EINTR) {
> -			goto cleanup_child;
> +		ret = sigsuspend(&suspend_mask);
> +		if (ret != -1 || errno != EINTR) {
> +			abort();
>  		}
>  	}
>  	tdb_robust_mutex_setup_sigchild(tdb_robust_mutext_old_handler, NULL);
> @@ -927,22 +934,18 @@ _PUBLIC_ bool tdb_runtime_check_for_robust_mutexes(void)
>  
>  cleanup_child:
>  	while (tdb_robust_mutex_pid > 0) {
> -		pid_t pid;
> -
>  		kill(tdb_robust_mutex_pid, SIGKILL);
> -
> -		errno = 0;
> -		pid = waitpid(tdb_robust_mutex_pid, &status, 0);
> -		if (pid == tdb_robust_mutex_pid) {
> -			tdb_robust_mutex_pid = -1;
> -			break;
> -		}
> -		if (pid == -1 && errno != EINTR) {
> -			break;
> +		ret = sigsuspend(&suspend_mask);
> +		if (ret != -1 || errno != EINTR) {
> +			abort();
>  		}
>  	}
>  cleanup_sig_child:
>  	tdb_robust_mutex_setup_sigchild(tdb_robust_mutext_old_handler, NULL);
> +	ret = pthread_sigmask(SIG_SETMASK, &old_mask, NULL);
> +	if (ret != 0) {
> +		abort();
> +	}
>  cleanup_m:
>  	pthread_mutex_destroy(m);
>  cleanup_ma:
> -- 
> 2.5.0
>