Re: glibc mutex deadlock in signal handler

From: Jeff King <peff@peff.net>
Date: Sep 4, 2015, 09:23 UTC
Message-ID: <20150904092355.GA524@sigill.intra.peff.net>
In-Reply-To: <s5hy4gmvii2.wl-tiwai@suse.de>

On Fri, Sep 04, 2015 at 07:52:21AM +0200, Takashi Iwai wrote:

Show 18 quoted lines

> -- 8< --
> From: Takashi Iwai <tiwai@suse.de>
> Subject: [PATCH] pager: don't use unsafe functions in signal handlers
> 
> Since the commit [a3da8821208d: pager: do wait_for_pager on signal
> death], we call wait_for_pager() in the pager's signal handler.  The
> recent bug report revealed that this causes a deadlock in glibc at
> aborting "git log" [*1].  When this happens, git process is left
> unterminated, and it can't be killed by SIGTERM but only by SIGKILL.
> 
> The problem is that wait_for_pager() function does more than waiting
> for pager process's termination, but it does cleanups and printing
> errors.  Unfortunately, the functions that may be used in a signal
> handler are very limited [*2].  Particularly, malloc(), free() and the
> variants can't be used in a signal handler because they take a mutex
> internally in glibc.  This was the cause of the deadlock above.  Other
> than the direct calls of malloc/free, many functions calling
> malloc/free can't be used.  strerror() is such one, either.

I think this approach is the only real solution here (and I agree it is a real-world problem). Unfortunately, it is the tip of the iceberg. Looking at other signal handlers, there are lots of other potential problems. For example, here are the first few I found by grepping:

  - clone.c:remove_junk uses strbufs; these are doing useful work, and
    can't just be skipped if we are in a signal handler

  - fetch calls transport_unlock_pack, which has a free (which can be
    skipped)

  - repack uses remove_temporary_files, which uses a strbuf

and so on.

> Also the usage of fflush() and printf() in a signal handler is bad,
> although it seems working so far.  In a safer side, we should avoid
> them, too.

I'd be surprised if they are safe; stdio definitely involves locking.

Perhaps we should reconsider whether f4c3edc (vreportf: avoid intermediate buffer, 2015-08-11) is a good idea. Note that snprintf is not on the list of safe functions, but I imagine that in practice it is fine. Though just avoiding error()/warning() in signal handlers might be a more practical solution anyway.

Show 16 quoted lines

> diff --git a/pager.c b/pager.c
> index 27d4c8a17aa1..12d17af73745 100644
> --- a/pager.c
> +++ b/pager.c
> @@ -26,7 +26,10 @@ static void wait_for_pager(void)
>  
>  static void wait_for_pager_signal(int signo)
>  {
> -	wait_for_pager();
> +	/* signal EOF to pager */
> +	close(1);
> +	close(2);
> +	finish_command_in_signal(&pager_process);
>  	sigchain_pop(signo);
>  	raise(signo);
>  }

Hmm, is there is any reason to just pass an "in_signal" flag to wait_for_pager(), to avoid duplicating the logic?

The rest of the patch looks pretty straightforward.

-Peff