On Fri, Sep 04, 2015 at 07:52:21AM +0200, Takashi Iwai wrote:
> -- 8< --
> From: Takashi Iwai <[email protected]>
> 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.
> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
