Re: glibc mutex deadlock in signal handler

From: Jeff King <peff@peff.net>
Date: Sep 4, 2015, 13:04 UTC
Message-ID: <20150904130448.GB25501@sigill.intra.peff.net>
In-Reply-To: <s5hzj12jzlu.wl-tiwai@suse.de>

On Fri, Sep 04, 2015 at 11:35:57AM +0200, Takashi Iwai wrote:

Show 6 quoted lines

> > Hmm, is there is any reason to just pass an "in_signal" flag to
> > wait_for_pager(), to avoid duplicating the logic?
> 
> Just because wait_for_pager() itself is an atexit hook that can't take
> an argument, so we'd need to split to a new function.  I don't mind
> either way.  The revised patch is below.

Ah, right. That's unfortunate, but I think I prefer adding the extra wrapper to duplicating the contents of the function.

> -- 8< --
> From: Takashi Iwai <tiwai@suse.de>
> Subject: [PATCH v2] pager: don't use unsafe functions in signal handlers
> [...]

This looks good to me. Do you plan on fixing any of the other handlers (you don't have to; I just want to know if somebody is planning to work on it).

The pattern of atexit and signal handlers is repeated in several places, and it seems like we will have to add the same in_signal boilerplate in each instance. I wonder if we should provide a global "register_cleanup" that takes a "void (*func)(int in_signal))" function pointer, and:

  1. Adds it to a list (ideally in a way that is atomic if we get
     interrupted while adding to the list).

  2. If not already run, registers an atexit() handler and
     sigchain_push_common for a meta-handler which runs through the list
     and runs each handler.

It's not a _huge_ amount of boilerplate code we'd be saving, but at least conforming to the "in_signal" function template would make people think twice about what they're doing inside the cleanup function. :)

-Peff