Commit 507d7804 authored by Takashi Iwai's avatar Takashi Iwai Committed by Junio C Hamano

pager: don't use unsafe functions in signal handlers

Since the commit a3da8821 (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.

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.

This patch tries to reduce the calls of such functions in signal
handlers.  wait_for_signal() takes a flag and avoids the unsafe
calls.   Also, finish_command_in_signal() is introduced for the
same reason.  There the free() calls are removed, and only waits for
the children without whining at errors.

[*1*] https://bugzilla.opensuse.org/show_bug.cgi?id=942297
[*2*] http://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_04_03Signed-off-by: default avatarTakashi Iwai <tiwai@suse.de>
Reviewed-by: default avatarJeff King <peff@peff.net>
Signed-off-by: default avatarJunio C Hamano <gitster@pobox.com>
parent a17c56c0
......@@ -14,19 +14,29 @@
static const char *pager_argv[] = { NULL, NULL };
static struct child_process pager_process = CHILD_PROCESS_INIT;
static void wait_for_pager(void)
static void wait_for_pager(int in_signal)
{
fflush(stdout);
fflush(stderr);
if (!in_signal) {
fflush(stdout);
fflush(stderr);
}
/* signal EOF to pager */
close(1);
close(2);
finish_command(&pager_process);
if (in_signal)
finish_command_in_signal(&pager_process);
else
finish_command(&pager_process);
}
static void wait_for_pager_atexit(void)
{
wait_for_pager(0);
}
static void wait_for_pager_signal(int signo)
{
wait_for_pager();
wait_for_pager(1);
sigchain_pop(signo);
raise(signo);
}
......@@ -90,7 +100,7 @@ void setup_pager(void)
/* this makes sure that the parent terminates after the pager */
sigchain_push_common(wait_for_pager_signal);
atexit(wait_for_pager);
atexit(wait_for_pager_atexit);
}
int pager_in_use(void)
......
......@@ -18,26 +18,27 @@ struct child_to_clean {
static struct child_to_clean *children_to_clean;
static int installed_child_cleanup_handler;
static void cleanup_children(int sig)
static void cleanup_children(int sig, int in_signal)
{
while (children_to_clean) {
struct child_to_clean *p = children_to_clean;
children_to_clean = p->next;
kill(p->pid, sig);
free(p);
if (!in_signal)
free(p);
}
}
static void cleanup_children_on_signal(int sig)
{
cleanup_children(sig);
cleanup_children(sig, 1);
sigchain_pop(sig);
raise(sig);
}
static void cleanup_children_on_exit(void)
{
cleanup_children(SIGTERM);
cleanup_children(SIGTERM, 0);
}
static void mark_child_for_cleanup(pid_t pid)
......@@ -232,7 +233,7 @@ static inline void set_cloexec(int fd)
fcntl(fd, F_SETFD, flags | FD_CLOEXEC);
}
static int wait_or_whine(pid_t pid, const char *argv0)
static int wait_or_whine(pid_t pid, const char *argv0, int in_signal)
{
int status, code = -1;
pid_t waiting;
......@@ -240,6 +241,8 @@ static int wait_or_whine(pid_t pid, const char *argv0)
while ((waiting = waitpid(pid, &status, 0)) < 0 && errno == EINTR)
; /* nothing */
if (in_signal)
return 0;
if (waiting < 0) {
failed_errno = errno;
......@@ -450,7 +453,7 @@ int start_command(struct child_process *cmd)
* At this point we know that fork() succeeded, but execvp()
* failed. Errors have been reported to our stderr.
*/
wait_or_whine(cmd->pid, cmd->argv[0]);
wait_or_whine(cmd->pid, cmd->argv[0], 0);
failed_errno = errno;
cmd->pid = -1;
}
......@@ -549,12 +552,18 @@ int start_command(struct child_process *cmd)
int finish_command(struct child_process *cmd)
{
int ret = wait_or_whine(cmd->pid, cmd->argv[0]);
int ret = wait_or_whine(cmd->pid, cmd->argv[0], 0);
argv_array_clear(&cmd->args);
argv_array_clear(&cmd->env_array);
return ret;
}
int finish_command_in_signal(struct child_process *cmd)
{
return wait_or_whine(cmd->pid, cmd->argv[0], 1);
}
int run_command(struct child_process *cmd)
{
int code;
......@@ -785,7 +794,7 @@ int start_async(struct async *async)
int finish_async(struct async *async)
{
#ifdef NO_PTHREADS
return wait_or_whine(async->pid, "child process");
return wait_or_whine(async->pid, "child process", 0);
#else
void *ret = (void *)(intptr_t)(-1);
......
......@@ -50,6 +50,7 @@ void child_process_init(struct child_process *);
int start_command(struct child_process *);
int finish_command(struct child_process *);
int finish_command_in_signal(struct child_process *);
int run_command(struct child_process *);
extern const char *find_hook(const char *name);
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment