Re: glibc mutex deadlock in signal handler

From: Jeff King <peff@peff.net>
Date: Sep 5, 2015, 08:59 UTC
Message-ID: <20150905085934.GA31358@sigill.intra.peff.net>
In-Reply-To: <xmqq7fo5g85x.fsf@gitster.mtv.corp.google.com>

On Fri, Sep 04, 2015 at 02:56:58PM -0700, Junio C Hamano wrote:

Show 12 quoted lines

> Jeff King <peff@peff.net> writes:
> 
> > 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.
> 
> I had exactly the same thought when I read the initial report here.
> 
> I wish we can just do "if (in_signal) return;" at the beginning of
> vreportf(), but we would not want a global variable there, so... ;-)

Why not? I mean, sure it's gross. But it actually seems like a pretty simple fix that doesn't have to hurt other callers (or involve passing an "in_signal" through the stack). We could even fallback to snprintf() into a fixed-sized buffer, or some other degraded mode.

> Further, I briefly hoped that avoiding error/warning in the signal
> handler codepath would allow us to be more lax around allocations,
> but I suspect that it unfortunately would not help us that much, as
> we may be calling these functions in low memory situations.

I'm not sure the low-memory thing isn't a red herring. Sure, we call die() when malloc fails. But only with a tiny string. Something like the robust_buf patch I posted would handle that just fine.

The real danger of signal handlers is that you don't get to say "oh, malloc failed, so let's fallback to some degraded mode". You just get deadlocked in a futex and never return. :)

> So let's queue Takashi's patch as-is for now and look at other
> signal codepaths.

Sounds like a good first step, unless we are going to do refactoring that Takashi's patch could take advantage of (either a global in_signal, or some register_cleanup() infrastructure).

-Peff