Race condition in tdb_runtime_check_for_robust_mutexes()

[Jeremy Allison]

On Fri, Mar 25, 2016 at 06:10:24AM +0100, Ralph Boehme wrote:
> On Thu, Mar 24, 2016 at 11:34:11PM +0200, Uri Simchoni wrote:
> > On 03/24/2016 04:57 PM, Ralph Boehme wrote:
> > > Something like the attached patch. This can probably be enhanced by
> > > using pselect with a timeout and a signal mask to block/unblock
> > > SIGCHLD instead of sleep.
> > > 
> > ENOPATCH?
> 
> gna, attached.

Not too keen on this one. I really don't like the
playing with the signal handler SA_RESTART flags :-(.

Plus sleep(1) is *way* too long. Use usleep(100) (or
the select()-based alternative already used in tdb).

> -- 
> SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
> phone: +49-551-370000-0, fax: +49-551-370000-9
> AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
> http://www.sernet.de,mailto:kontakt@sernet.de

> From 091cfad07a7693cb88f76e46f576fd39978bdc6f Mon Sep 17 00:00:00 2001
> From: Ralph Boehme <slow at samba.org>
> Date: Thu, 24 Mar 2016 15:49:13 +0100
> Subject: [PATCH] WIP: only call waitpid() in the signal handler
> 
> ---
>  lib/tdb/common/mutex.c | 36 ++++++++++--------------------------
>  1 file changed, 10 insertions(+), 26 deletions(-)
> 
> diff --git a/lib/tdb/common/mutex.c b/lib/tdb/common/mutex.c
> index fae43d4..9556238 100644
> --- a/lib/tdb/common/mutex.c
> +++ b/lib/tdb/common/mutex.c
> @@ -723,9 +723,6 @@ static bool tdb_robust_mutex_setup_sigchild(void (*handler)(int),
>  	memset(&act, '\0', sizeof(act));
>  
>  	act.sa_handler = handler;
> -#ifdef SA_RESTART
> -	act.sa_flags = SA_RESTART;
> -#endif
>  	sigemptyset(&act.sa_mask);
>  	sigaddset(&act.sa_mask, SIGCHLD);
>  	sigaction(SIGCHLD, &act, &oldact);
> @@ -884,17 +881,11 @@ _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;
> -		}
> +		/*
> +		 * RACE: if the SIGCHLD is delivered now, *before* the
> +		 * sleep, we will unneededly sleep for one second.
> +		 */
> +		sleep(1);
>  	}
>  	tdb_robust_mutex_setup_sigchild(tdb_robust_mutext_old_handler, NULL);
>  
> @@ -927,19 +918,12 @@ _PUBLIC_ bool tdb_runtime_check_for_robust_mutexes(void)
>  
>  cleanup_child:
>  	while (tdb_robust_mutex_pid > 0) {
> -		pid_t pid;
> -
> +		/*
> +		 * RACE: if the SIGCHLD is delivered now, *before* the
> +		 * sleep, we will unneededly sleep for one second.
> +		 */
>  		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;
> -		}
> +		sleep();
>  	}
>  cleanup_sig_child:
>  	tdb_robust_mutex_setup_sigchild(tdb_robust_mutext_old_handler, NULL);
> -- 
> 2.5.0
>