{"thread":{"id":"40261","subject":"glibc mutex deadlock in signal handler","startedAt":"2015-09-03T11:00:53Z","lastAt":"2015-09-05T08:59:34Z","messageCount":11,"participants":["Takashi Iwai","Junio C Hamano","Andreas Schwab","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"269309","messageId":"s5hfv2vn4wq.wl-tiwai@suse.de","threadId":"40261","inReplyTo":null,"subject":"glibc mutex deadlock in signal handler","fromName":"Takashi Iwai","fromEmail":"tiwai@suse.de","sentAt":"2015-09-03T11:00:53Z","receivedAt":"2015-09-03T11:00:53Z","isPatch":false,"sender":{"key":"tiwai@suse.de","avatar":"https://avatars.githubusercontent.com/u/306482?v=4"},"body":"Hi,\n\nwe've got a bug report of git-log stall in below:\n  https://bugzilla.opensuse.org/show_bug.cgi?id=942297\n\nIn short, git-log is left unterminated sometimes when pager is\naborted.  When this happens, git process can't be killed by SIGTERM,\nbut only by SIGKILL.  And, further investigation revealed the possible\nmutex deadlock used in glibc *alloc()/free().\n\nI tried to reproduce this by adding a fault injection in glibc code,\nand actually got the similar stack trace as reported.  The problem is\nthat glibc malloc (in this case realloc() and free()) takes a private\nmutex.  Thus calling any function does *alloc() or free() in a signal\nhandler may deadlock.  In the case of git, it was free() calls in\nvarious cleanup codes and the printf() / strerror() invocations.\n\nAlso, basically it's not safe to call fflush() or raise(), either.\nBut they seem to work practically on the current systems.\n\nBelow is a band-aid patch I tested and confirmed to work in the\nfault-injection case.  But, some unsafe calls mentioned in the above\nare left.  If we want to be in a safer side, we should really limit\nthe things to do in a signal handler, e.g. only calling close() and\ndoing waitpid(), I suppose.\n\nAny better ideas?\n\n\nthanks,\n\nTakashi\n\n---\ndiff --git a/pager.c b/pager.c\nindex 27d4c8a17aa1..57dda0142fa9 100644\n--- a/pager.c\n+++ b/pager.c\n@@ -14,19 +14,25 @@\n static const char *pager_argv[] = { NULL, NULL };\n static struct child_process pager_process = CHILD_PROCESS_INIT;\n \n-static void wait_for_pager(void)\n+static void flush_pager(void)\n {\n \tfflush(stdout);\n \tfflush(stderr);\n \t/* signal EOF to pager */\n \tclose(1);\n \tclose(2);\n+}\n+\n+static void wait_for_pager(void)\n+{\n+\tflush_pager();\n \tfinish_command(&pager_process);\n }\n \n static void wait_for_pager_signal(int signo)\n {\n-\twait_for_pager();\n+\tflush_pager();\n+\tfinish_command_in_signal(&pager_process);\n \tsigchain_pop(signo);\n \traise(signo);\n }\ndiff --git a/run-command.c b/run-command.c\nindex 3277cf797ed4..a8cdc8f32944 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -18,26 +18,27 @@ struct child_to_clean {\n static struct child_to_clean *children_to_clean;\n static int installed_child_cleanup_handler;\n \n-static void cleanup_children(int sig)\n+static void cleanup_children(int sig, int in_signal)\n {\n \twhile (children_to_clean) {\n \t\tstruct child_to_clean *p = children_to_clean;\n \t\tchildren_to_clean = p->next;\n \t\tkill(p->pid, sig);\n-\t\tfree(p);\n+\t\tif (!in_signal)\n+\t\t\tfree(p);\n \t}\n }\n \n static void cleanup_children_on_signal(int sig)\n {\n-\tcleanup_children(sig);\n+\tcleanup_children(sig, 1);\n \tsigchain_pop(sig);\n \traise(sig);\n }\n \n static void cleanup_children_on_exit(void)\n {\n-\tcleanup_children(SIGTERM);\n+\tcleanup_children(SIGTERM, 0);\n }\n \n static void mark_child_for_cleanup(pid_t pid)\n@@ -220,7 +221,7 @@ static inline void set_cloexec(int fd)\n \t\tfcntl(fd, F_SETFD, flags | FD_CLOEXEC);\n }\n \n-static int wait_or_whine(pid_t pid, const char *argv0)\n+static int wait_or_whine(pid_t pid, const char *argv0, int in_signal)\n {\n \tint status, code = -1;\n \tpid_t waiting;\n@@ -231,13 +232,17 @@ static int wait_or_whine(pid_t pid, const char *argv0)\n \n \tif (waiting < 0) {\n \t\tfailed_errno = errno;\n-\t\terror(\"waitpid for %s failed: %s\", argv0, strerror(errno));\n+\t\tif (!in_signal)\n+\t\t\terror(\"waitpid for %s failed: %s\", argv0,\n+\t\t\t      strerror(errno));\n \t} else if (waiting != pid) {\n-\t\terror(\"waitpid is confused (%s)\", argv0);\n+\t\tif (!in_signal)\n+\t\t\terror(\"waitpid is confused (%s)\", argv0);\n \t} else if (WIFSIGNALED(status)) {\n \t\tcode = WTERMSIG(status);\n \t\tif (code != SIGINT && code != SIGQUIT)\n-\t\t\terror(\"%s died of signal %d\", argv0, code);\n+\t\t\tif (!in_signal)\n+\t\t\t\terror(\"%s died of signal %d\", argv0, code);\n \t\t/*\n \t\t * This return value is chosen so that code & 0xff\n \t\t * mimics the exit code that a POSIX shell would report for\n@@ -254,10 +259,12 @@ static int wait_or_whine(pid_t pid, const char *argv0)\n \t\t\tfailed_errno = ENOENT;\n \t\t}\n \t} else {\n-\t\terror(\"waitpid is confused (%s)\", argv0);\n+\t\tif (!in_signal)\n+\t\t\terror(\"waitpid is confused (%s)\", argv0);\n \t}\n \n-\tclear_child_for_cleanup(pid);\n+\tif (!in_signal)\n+\t\tclear_child_for_cleanup(pid);\n \n \terrno = failed_errno;\n \treturn code;\n@@ -437,7 +444,7 @@ fail_pipe:\n \t\t * At this point we know that fork() succeeded, but execvp()\n \t\t * failed. Errors have been reported to our stderr.\n \t\t */\n-\t\twait_or_whine(cmd->pid, cmd->argv[0]);\n+\t\twait_or_whine(cmd->pid, cmd->argv[0], 0);\n \t\tfailed_errno = errno;\n \t\tcmd->pid = -1;\n \t}\n@@ -536,12 +543,18 @@ fail_pipe:\n \n int finish_command(struct child_process *cmd)\n {\n-\tint ret = wait_or_whine(cmd->pid, cmd->argv[0]);\n+\tint ret = wait_or_whine(cmd->pid, cmd->argv[0], 0);\n \targv_array_clear(&cmd->args);\n \targv_array_clear(&cmd->env_array);\n \treturn ret;\n }\n \n+int finish_command_in_signal(struct child_process *cmd)\n+{\n+\treturn wait_or_whine(cmd->pid, cmd->argv[0], 1);\n+}\n+\n+\n int run_command(struct child_process *cmd)\n {\n \tint code;\n@@ -772,7 +785,7 @@ error:\n int finish_async(struct async *async)\n {\n #ifdef NO_PTHREADS\n-\treturn wait_or_whine(async->pid, \"child process\");\n+\treturn wait_or_whine(async->pid, \"child process\", 0);\n #else\n \tvoid *ret = (void *)(intptr_t)(-1);\n \ndiff --git a/run-command.h b/run-command.h\nindex 5b4425a3cbe1..275d35c442ac 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -50,6 +50,7 @@ void child_process_init(struct child_process *);\n \n int start_command(struct child_process *);\n int finish_command(struct child_process *);\n+int finish_command_in_signal(struct child_process *);\n int run_command(struct child_process *);\n \n /*\n"},{"id":"269347","messageId":"xmqqvbbrjrs9.fsf@gitster.mtv.corp.google.com","threadId":"40261","inReplyTo":"s5hfv2vn4wq.wl-tiwai@suse.de","subject":"Re: glibc mutex deadlock in signal handler","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-03T18:12:38Z","receivedAt":"2015-09-03T18:12:38Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Takashi Iwai <tiwai@suse.de> writes:\n\n> we've got a bug report of git-log stall in below:\n>   https://bugzilla.opensuse.org/show_bug.cgi?id=942297\n>\n> In short, git-log is left unterminated sometimes when pager is\n> aborted.  When this happens, git process can't be killed by SIGTERM,\n> but only by SIGKILL.  And, further investigation revealed the possible\n> mutex deadlock used in glibc *alloc()/free().\n>\n> I tried to reproduce this by adding a fault injection in glibc code,\n> and actually got the similar stack trace as reported.  The problem is\n> that glibc malloc (in this case realloc() and free()) takes a private\n> mutex.  Thus calling any function does *alloc() or free() in a signal\n> handler may deadlock.  In the case of git, it was free() calls in\n> various cleanup codes and the printf() / strerror() invocations.\n>\n> Also, basically it's not safe to call fflush() or raise(), either.\n> But they seem to work practically on the current systems.\n>\n> Below is a band-aid patch I tested and confirmed to work in the\n> fault-injection case.  But, some unsafe calls mentioned in the above\n> are left.  If we want to be in a safer side, we should really limit\n> the things to do in a signal handler, e.g. only calling close() and\n> doing waitpid(), I suppose.\n>\n> Any better ideas?\n\nSorry, but I am not with better ideas (at least offhand right now).\n\nEssentially the idea seems to be to avoid calling allocation-related\nfunctions in the signal handler codepath that is used for cleaning\nthings up.  Not calling free() in the codepaths is perfectly fine as\nwe know we will be soon exiting anyway.  Not being able to call\nerror() and strerror() to report funnies (other than the fact that\nwe were interrupted) is somewhat sad, though.\n\n\n\n\n> diff --git a/pager.c b/pager.c\n> index 27d4c8a17aa1..57dda0142fa9 100644\n> --- a/pager.c\n> +++ b/pager.c\n> @@ -14,19 +14,25 @@\n>  static const char *pager_argv[] = { NULL, NULL };\n>  static struct child_process pager_process = CHILD_PROCESS_INIT;\n>  \n> -static void wait_for_pager(void)\n> +static void flush_pager(void)\n>  {\n>  \tfflush(stdout);\n>  \tfflush(stderr);\n>  \t/* signal EOF to pager */\n>  \tclose(1);\n>  \tclose(2);\n> +}\n> +\n> +static void wait_for_pager(void)\n> +{\n> +\tflush_pager();\n>  \tfinish_command(&pager_process);\n>  }\n>  \n>  static void wait_for_pager_signal(int signo)\n>  {\n> -\twait_for_pager();\n> +\tflush_pager();\n> +\tfinish_command_in_signal(&pager_process);\n>  \tsigchain_pop(signo);\n>  \traise(signo);\n>  }\n> diff --git a/run-command.c b/run-command.c\n> index 3277cf797ed4..a8cdc8f32944 100644\n> --- a/run-command.c\n> +++ b/run-command.c\n> @@ -18,26 +18,27 @@ struct child_to_clean {\n>  static struct child_to_clean *children_to_clean;\n>  static int installed_child_cleanup_handler;\n>  \n> -static void cleanup_children(int sig)\n> +static void cleanup_children(int sig, int in_signal)\n>  {\n>  \twhile (children_to_clean) {\n>  \t\tstruct child_to_clean *p = children_to_clean;\n>  \t\tchildren_to_clean = p->next;\n>  \t\tkill(p->pid, sig);\n> -\t\tfree(p);\n> +\t\tif (!in_signal)\n> +\t\t\tfree(p);\n>  \t}\n>  }\n>  \n>  static void cleanup_children_on_signal(int sig)\n>  {\n> -\tcleanup_children(sig);\n> +\tcleanup_children(sig, 1);\n>  \tsigchain_pop(sig);\n>  \traise(sig);\n>  }\n>  \n>  static void cleanup_children_on_exit(void)\n>  {\n> -\tcleanup_children(SIGTERM);\n> +\tcleanup_children(SIGTERM, 0);\n>  }\n>  \n>  static void mark_child_for_cleanup(pid_t pid)\n> @@ -220,7 +221,7 @@ static inline void set_cloexec(int fd)\n>  \t\tfcntl(fd, F_SETFD, flags | FD_CLOEXEC);\n>  }\n>  \n> -static int wait_or_whine(pid_t pid, const char *argv0)\n> +static int wait_or_whine(pid_t pid, const char *argv0, int in_signal)\n>  {\n>  \tint status, code = -1;\n>  \tpid_t waiting;\n> @@ -231,13 +232,17 @@ static int wait_or_whine(pid_t pid, const char *argv0)\n>  \n>  \tif (waiting < 0) {\n>  \t\tfailed_errno = errno;\n> -\t\terror(\"waitpid for %s failed: %s\", argv0, strerror(errno));\n> +\t\tif (!in_signal)\n> +\t\t\terror(\"waitpid for %s failed: %s\", argv0,\n> +\t\t\t      strerror(errno));\n>  \t} else if (waiting != pid) {\n> -\t\terror(\"waitpid is confused (%s)\", argv0);\n> +\t\tif (!in_signal)\n> +\t\t\terror(\"waitpid is confused (%s)\", argv0);\n>  \t} else if (WIFSIGNALED(status)) {\n>  \t\tcode = WTERMSIG(status);\n>  \t\tif (code != SIGINT && code != SIGQUIT)\n> -\t\t\terror(\"%s died of signal %d\", argv0, code);\n> +\t\t\tif (!in_signal)\n> +\t\t\t\terror(\"%s died of signal %d\", argv0, code);\n>  \t\t/*\n>  \t\t * This return value is chosen so that code & 0xff\n>  \t\t * mimics the exit code that a POSIX shell would report for\n> @@ -254,10 +259,12 @@ static int wait_or_whine(pid_t pid, const char *argv0)\n>  \t\t\tfailed_errno = ENOENT;\n>  \t\t}\n>  \t} else {\n> -\t\terror(\"waitpid is confused (%s)\", argv0);\n> +\t\tif (!in_signal)\n> +\t\t\terror(\"waitpid is confused (%s)\", argv0);\n>  \t}\n>  \n> -\tclear_child_for_cleanup(pid);\n> +\tif (!in_signal)\n> +\t\tclear_child_for_cleanup(pid);\n>  \n>  \terrno = failed_errno;\n>  \treturn code;\n> @@ -437,7 +444,7 @@ fail_pipe:\n>  \t\t * At this point we know that fork() succeeded, but execvp()\n>  \t\t * failed. Errors have been reported to our stderr.\n>  \t\t */\n> -\t\twait_or_whine(cmd->pid, cmd->argv[0]);\n> +\t\twait_or_whine(cmd->pid, cmd->argv[0], 0);\n>  \t\tfailed_errno = errno;\n>  \t\tcmd->pid = -1;\n>  \t}\n> @@ -536,12 +543,18 @@ fail_pipe:\n>  \n>  int finish_command(struct child_process *cmd)\n>  {\n> -\tint ret = wait_or_whine(cmd->pid, cmd->argv[0]);\n> +\tint ret = wait_or_whine(cmd->pid, cmd->argv[0], 0);\n>  \targv_array_clear(&cmd->args);\n>  \targv_array_clear(&cmd->env_array);\n>  \treturn ret;\n>  }\n>  \n> +int finish_command_in_signal(struct child_process *cmd)\n> +{\n> +\treturn wait_or_whine(cmd->pid, cmd->argv[0], 1);\n> +}\n> +\n> +\n>  int run_command(struct child_process *cmd)\n>  {\n>  \tint code;\n> @@ -772,7 +785,7 @@ error:\n>  int finish_async(struct async *async)\n>  {\n>  #ifdef NO_PTHREADS\n> -\treturn wait_or_whine(async->pid, \"child process\");\n> +\treturn wait_or_whine(async->pid, \"child process\", 0);\n>  #else\n>  \tvoid *ret = (void *)(intptr_t)(-1);\n>  \n> diff --git a/run-command.h b/run-command.h\n> index 5b4425a3cbe1..275d35c442ac 100644\n> --- a/run-command.h\n> +++ b/run-command.h\n> @@ -50,6 +50,7 @@ void child_process_init(struct child_process *);\n>  \n>  int start_command(struct child_process *);\n>  int finish_command(struct child_process *);\n> +int finish_command_in_signal(struct child_process *);\n>  int run_command(struct child_process *);\n>  \n>  /*\n"},{"id":"269353","messageId":"s5h7fo7wb3e.wl-tiwai@suse.de","threadId":"40261","inReplyTo":"xmqqvbbrjrs9.fsf@gitster.mtv.corp.google.com","subject":"Re: glibc mutex deadlock in signal handler","fromName":"Takashi Iwai","fromEmail":"tiwai@suse.de","sentAt":"2015-09-03T19:34:45Z","receivedAt":"2015-09-03T19:34:45Z","isPatch":false,"sender":{"key":"tiwai@suse.de","avatar":"https://avatars.githubusercontent.com/u/306482?v=4"},"body":"On Thu, 03 Sep 2015 20:12:38 +0200,\nJunio C Hamano wrote:\n> \n> Takashi Iwai <tiwai@suse.de> writes:\n> \n> > we've got a bug report of git-log stall in below:\n> >   https://bugzilla.opensuse.org/show_bug.cgi?id=942297\n> >\n> > In short, git-log is left unterminated sometimes when pager is\n> > aborted.  When this happens, git process can't be killed by SIGTERM,\n> > but only by SIGKILL.  And, further investigation revealed the possible\n> > mutex deadlock used in glibc *alloc()/free().\n> >\n> > I tried to reproduce this by adding a fault injection in glibc code,\n> > and actually got the similar stack trace as reported.  The problem is\n> > that glibc malloc (in this case realloc() and free()) takes a private\n> > mutex.  Thus calling any function does *alloc() or free() in a signal\n> > handler may deadlock.  In the case of git, it was free() calls in\n> > various cleanup codes and the printf() / strerror() invocations.\n> >\n> > Also, basically it's not safe to call fflush() or raise(), either.\n> > But they seem to work practically on the current systems.\n> >\n> > Below is a band-aid patch I tested and confirmed to work in the\n> > fault-injection case.  But, some unsafe calls mentioned in the above\n> > are left.  If we want to be in a safer side, we should really limit\n> > the things to do in a signal handler, e.g. only calling close() and\n> > doing waitpid(), I suppose.\n> >\n> > Any better ideas?\n> \n> Sorry, but I am not with better ideas (at least offhand right now).\n> \n> Essentially the idea seems to be to avoid calling allocation-related\n> functions in the signal handler codepath that is used for cleaning\n> things up.  Not calling free() in the codepaths is perfectly fine as\n> we know we will be soon exiting anyway.  Not being able to call\n> error() and strerror() to report funnies (other than the fact that\n> we were interrupted) is somewhat sad, though.\n\nReading signal(7) again, I correct some of my statement: raise() is\nsafe to use.  But fflush() still isn't.  Maybe fflush() should be\navoided but just call only close().\n\nRegarding the error message: write() is still safe to use, so it is\npossible in some level.  strerror() seems invoking malloc(), judging\nfrom the stack trace in bugzilla, so this needs to be avoided.\nprintf() is a blackbox, so it's hard to tell.  That is, in the worst\ncase, we can just call write() directly for serious errors, if any.\n\n\nTakashi\n\n> \n> \n> \n> \n> > diff --git a/pager.c b/pager.c\n> > index 27d4c8a17aa1..57dda0142fa9 100644\n> > --- a/pager.c\n> > +++ b/pager.c\n> > @@ -14,19 +14,25 @@\n> >  static const char *pager_argv[] = { NULL, NULL };\n> >  static struct child_process pager_process = CHILD_PROCESS_INIT;\n> >  \n> > -static void wait_for_pager(void)\n> > +static void flush_pager(void)\n> >  {\n> >  \tfflush(stdout);\n> >  \tfflush(stderr);\n> >  \t/* signal EOF to pager */\n> >  \tclose(1);\n> >  \tclose(2);\n> > +}\n> > +\n> > +static void wait_for_pager(void)\n> > +{\n> > +\tflush_pager();\n> >  \tfinish_command(&pager_process);\n> >  }\n> >  \n> >  static void wait_for_pager_signal(int signo)\n> >  {\n> > -\twait_for_pager();\n> > +\tflush_pager();\n> > +\tfinish_command_in_signal(&pager_process);\n> >  \tsigchain_pop(signo);\n> >  \traise(signo);\n> >  }\n> > diff --git a/run-command.c b/run-command.c\n> > index 3277cf797ed4..a8cdc8f32944 100644\n> > --- a/run-command.c\n> > +++ b/run-command.c\n> > @@ -18,26 +18,27 @@ struct child_to_clean {\n> >  static struct child_to_clean *children_to_clean;\n> >  static int installed_child_cleanup_handler;\n> >  \n> > -static void cleanup_children(int sig)\n> > +static void cleanup_children(int sig, int in_signal)\n> >  {\n> >  \twhile (children_to_clean) {\n> >  \t\tstruct child_to_clean *p = children_to_clean;\n> >  \t\tchildren_to_clean = p->next;\n> >  \t\tkill(p->pid, sig);\n> > -\t\tfree(p);\n> > +\t\tif (!in_signal)\n> > +\t\t\tfree(p);\n> >  \t}\n> >  }\n> >  \n> >  static void cleanup_children_on_signal(int sig)\n> >  {\n> > -\tcleanup_children(sig);\n> > +\tcleanup_children(sig, 1);\n> >  \tsigchain_pop(sig);\n> >  \traise(sig);\n> >  }\n> >  \n> >  static void cleanup_children_on_exit(void)\n> >  {\n> > -\tcleanup_children(SIGTERM);\n> > +\tcleanup_children(SIGTERM, 0);\n> >  }\n> >  \n> >  static void mark_child_for_cleanup(pid_t pid)\n> > @@ -220,7 +221,7 @@ static inline void set_cloexec(int fd)\n> >  \t\tfcntl(fd, F_SETFD, flags | FD_CLOEXEC);\n> >  }\n> >  \n> > -static int wait_or_whine(pid_t pid, const char *argv0)\n> > +static int wait_or_whine(pid_t pid, const char *argv0, int in_signal)\n> >  {\n> >  \tint status, code = -1;\n> >  \tpid_t waiting;\n> > @@ -231,13 +232,17 @@ static int wait_or_whine(pid_t pid, const char *argv0)\n> >  \n> >  \tif (waiting < 0) {\n> >  \t\tfailed_errno = errno;\n> > -\t\terror(\"waitpid for %s failed: %s\", argv0, strerror(errno));\n> > +\t\tif (!in_signal)\n> > +\t\t\terror(\"waitpid for %s failed: %s\", argv0,\n> > +\t\t\t      strerror(errno));\n> >  \t} else if (waiting != pid) {\n> > -\t\terror(\"waitpid is confused (%s)\", argv0);\n> > +\t\tif (!in_signal)\n> > +\t\t\terror(\"waitpid is confused (%s)\", argv0);\n> >  \t} else if (WIFSIGNALED(status)) {\n> >  \t\tcode = WTERMSIG(status);\n> >  \t\tif (code != SIGINT && code != SIGQUIT)\n> > -\t\t\terror(\"%s died of signal %d\", argv0, code);\n> > +\t\t\tif (!in_signal)\n> > +\t\t\t\terror(\"%s died of signal %d\", argv0, code);\n> >  \t\t/*\n> >  \t\t * This return value is chosen so that code & 0xff\n> >  \t\t * mimics the exit code that a POSIX shell would report for\n> > @@ -254,10 +259,12 @@ static int wait_or_whine(pid_t pid, const char *argv0)\n> >  \t\t\tfailed_errno = ENOENT;\n> >  \t\t}\n> >  \t} else {\n> > -\t\terror(\"waitpid is confused (%s)\", argv0);\n> > +\t\tif (!in_signal)\n> > +\t\t\terror(\"waitpid is confused (%s)\", argv0);\n> >  \t}\n> >  \n> > -\tclear_child_for_cleanup(pid);\n> > +\tif (!in_signal)\n> > +\t\tclear_child_for_cleanup(pid);\n> >  \n> >  \terrno = failed_errno;\n> >  \treturn code;\n> > @@ -437,7 +444,7 @@ fail_pipe:\n> >  \t\t * At this point we know that fork() succeeded, but execvp()\n> >  \t\t * failed. Errors have been reported to our stderr.\n> >  \t\t */\n> > -\t\twait_or_whine(cmd->pid, cmd->argv[0]);\n> > +\t\twait_or_whine(cmd->pid, cmd->argv[0], 0);\n> >  \t\tfailed_errno = errno;\n> >  \t\tcmd->pid = -1;\n> >  \t}\n> > @@ -536,12 +543,18 @@ fail_pipe:\n> >  \n> >  int finish_command(struct child_process *cmd)\n> >  {\n> > -\tint ret = wait_or_whine(cmd->pid, cmd->argv[0]);\n> > +\tint ret = wait_or_whine(cmd->pid, cmd->argv[0], 0);\n> >  \targv_array_clear(&cmd->args);\n> >  \targv_array_clear(&cmd->env_array);\n> >  \treturn ret;\n> >  }\n> >  \n> > +int finish_command_in_signal(struct child_process *cmd)\n> > +{\n> > +\treturn wait_or_whine(cmd->pid, cmd->argv[0], 1);\n> > +}\n> > +\n> > +\n> >  int run_command(struct child_process *cmd)\n> >  {\n> >  \tint code;\n> > @@ -772,7 +785,7 @@ error:\n> >  int finish_async(struct async *async)\n> >  {\n> >  #ifdef NO_PTHREADS\n> > -\treturn wait_or_whine(async->pid, \"child process\");\n> > +\treturn wait_or_whine(async->pid, \"child process\", 0);\n> >  #else\n> >  \tvoid *ret = (void *)(intptr_t)(-1);\n> >  \n> > diff --git a/run-command.h b/run-command.h\n> > index 5b4425a3cbe1..275d35c442ac 100644\n> > --- a/run-command.h\n> > +++ b/run-command.h\n> > @@ -50,6 +50,7 @@ void child_process_init(struct child_process *);\n> >  \n> >  int start_command(struct child_process *);\n> >  int finish_command(struct child_process *);\n> > +int finish_command_in_signal(struct child_process *);\n> >  int run_command(struct child_process *);\n> >  \n> >  /*\n> \n"},{"id":"269369","messageId":"87y4gn5ijr.fsf@igel.home","threadId":"40261","inReplyTo":"s5h7fo7wb3e.wl-tiwai@suse.de","subject":"Re: glibc mutex deadlock in signal handler","fromName":"Andreas Schwab","fromEmail":"schwab@linux-m68k.org","sentAt":"2015-09-03T20:55:52Z","receivedAt":"2015-09-03T20:55:52Z","isPatch":false,"sender":{"key":"schwab@linux-m68k.org","avatar":"https://avatars.githubusercontent.com/u/2175493?v=4"},"body":"See\n<http://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_04_03>\nfor the complete list of functions you may safely call from a signal\nhandler.\n\nAndreas.\n\n-- \nAndreas Schwab, schwab@linux-m68k.org\nGPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5\n\"And now for something completely different.\"\n"},{"id":"269399","messageId":"s5hy4gmvii2.wl-tiwai@suse.de","threadId":"40261","inReplyTo":"87y4gn5ijr.fsf@igel.home","subject":"Re: glibc mutex deadlock in signal handler","fromName":"Takashi Iwai","fromEmail":"tiwai@suse.de","sentAt":"2015-09-04T05:52:21Z","receivedAt":"2015-09-04T05:52:21Z","isPatch":false,"sender":{"key":"tiwai@suse.de","avatar":"https://avatars.githubusercontent.com/u/306482?v=4"},"body":"On Thu, 03 Sep 2015 22:55:52 +0200,\nAndreas Schwab wrote:\n> \n> See\n> <http://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_04_03>\n> for the complete list of functions you may safely call from a signal\n> handler.\n\nThanks, this looks more comprehensive.\nFWIW, below is the updated patch with a proper change log.\n\n\nTakashi\n\n-- 8< --\nFrom: Takashi Iwai <tiwai@suse.de>\nSubject: [PATCH] pager: don't use unsafe functions in signal handlers\n\nSince the commit [a3da8821208d: pager: do wait_for_pager on signal\ndeath], we call wait_for_pager() in the pager's signal handler.  The\nrecent bug report revealed that this causes a deadlock in glibc at\naborting \"git log\" [*1].  When this happens, git process is left\nunterminated, and it can't be killed by SIGTERM but only by SIGKILL.\n\nThe problem is that wait_for_pager() function does more than waiting\nfor pager process's termination, but it does cleanups and printing\nerrors.  Unfortunately, the functions that may be used in a signal\nhandler are very limited [*2].  Particularly, malloc(), free() and the\nvariants can't be used in a signal handler because they take a mutex\ninternally in glibc.  This was the cause of the deadlock above.  Other\nthan the direct calls of malloc/free, many functions calling\nmalloc/free can't be used.  strerror() is such one, either.\n\nAlso the usage of fflush() and printf() in a signal handler is bad,\nalthough it seems working so far.  In a safer side, we should avoid\nthem, too.\n\nThis patch tries to reduce the calls of such functions in signal\nhandlers.  wait_for_pager_signal() is now open-coded not to call\nunsafe functions, and finish_command_in_signal() is introduced for the\nsame reason.  There the free() calls are removed, and only waits for\nthe children without whining at errors.\n\n[*1] https://bugzilla.opensuse.org/show_bug.cgi?id=942297\n[*2] http://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_04_03\n\nSigned-off-by: Takashi Iwai <tiwai@suse.de>\n---\n pager.c       |  5 ++++-\n run-command.c | 25 +++++++++++++++++--------\n run-command.h |  1 +\n 3 files changed, 22 insertions(+), 9 deletions(-)\n\ndiff --git a/pager.c b/pager.c\nindex 27d4c8a17aa1..12d17af73745 100644\n--- a/pager.c\n+++ b/pager.c\n@@ -26,7 +26,10 @@ static void wait_for_pager(void)\n \n static void wait_for_pager_signal(int signo)\n {\n-\twait_for_pager();\n+\t/* signal EOF to pager */\n+\tclose(1);\n+\tclose(2);\n+\tfinish_command_in_signal(&pager_process);\n \tsigchain_pop(signo);\n \traise(signo);\n }\ndiff --git a/run-command.c b/run-command.c\nindex 3277cf797ed4..e09275bd9e36 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -18,26 +18,27 @@ struct child_to_clean {\n static struct child_to_clean *children_to_clean;\n static int installed_child_cleanup_handler;\n \n-static void cleanup_children(int sig)\n+static void cleanup_children(int sig, int in_signal)\n {\n \twhile (children_to_clean) {\n \t\tstruct child_to_clean *p = children_to_clean;\n \t\tchildren_to_clean = p->next;\n \t\tkill(p->pid, sig);\n-\t\tfree(p);\n+\t\tif (!in_signal)\n+\t\t\tfree(p);\n \t}\n }\n \n static void cleanup_children_on_signal(int sig)\n {\n-\tcleanup_children(sig);\n+\tcleanup_children(sig, 1);\n \tsigchain_pop(sig);\n \traise(sig);\n }\n \n static void cleanup_children_on_exit(void)\n {\n-\tcleanup_children(SIGTERM);\n+\tcleanup_children(SIGTERM, 0);\n }\n \n static void mark_child_for_cleanup(pid_t pid)\n@@ -220,7 +221,7 @@ static inline void set_cloexec(int fd)\n \t\tfcntl(fd, F_SETFD, flags | FD_CLOEXEC);\n }\n \n-static int wait_or_whine(pid_t pid, const char *argv0)\n+static int wait_or_whine(pid_t pid, const char *argv0, int in_signal)\n {\n \tint status, code = -1;\n \tpid_t waiting;\n@@ -228,6 +229,8 @@ static int wait_or_whine(pid_t pid, const char *argv0)\n \n \twhile ((waiting = waitpid(pid, &status, 0)) < 0 && errno == EINTR)\n \t\t;\t/* nothing */\n+\tif (in_signal)\n+\t\treturn 0;\n \n \tif (waiting < 0) {\n \t\tfailed_errno = errno;\n@@ -437,7 +440,7 @@ fail_pipe:\n \t\t * At this point we know that fork() succeeded, but execvp()\n \t\t * failed. Errors have been reported to our stderr.\n \t\t */\n-\t\twait_or_whine(cmd->pid, cmd->argv[0]);\n+\t\twait_or_whine(cmd->pid, cmd->argv[0], 0);\n \t\tfailed_errno = errno;\n \t\tcmd->pid = -1;\n \t}\n@@ -536,12 +539,18 @@ fail_pipe:\n \n int finish_command(struct child_process *cmd)\n {\n-\tint ret = wait_or_whine(cmd->pid, cmd->argv[0]);\n+\tint ret = wait_or_whine(cmd->pid, cmd->argv[0], 0);\n \targv_array_clear(&cmd->args);\n \targv_array_clear(&cmd->env_array);\n \treturn ret;\n }\n \n+int finish_command_in_signal(struct child_process *cmd)\n+{\n+\treturn wait_or_whine(cmd->pid, cmd->argv[0], 1);\n+}\n+\n+\n int run_command(struct child_process *cmd)\n {\n \tint code;\n@@ -772,7 +781,7 @@ error:\n int finish_async(struct async *async)\n {\n #ifdef NO_PTHREADS\n-\treturn wait_or_whine(async->pid, \"child process\");\n+\treturn wait_or_whine(async->pid, \"child process\", 0);\n #else\n \tvoid *ret = (void *)(intptr_t)(-1);\n \ndiff --git a/run-command.h b/run-command.h\nindex 5b4425a3cbe1..275d35c442ac 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -50,6 +50,7 @@ void child_process_init(struct child_process *);\n \n int start_command(struct child_process *);\n int finish_command(struct child_process *);\n+int finish_command_in_signal(struct child_process *);\n int run_command(struct child_process *);\n \n /*\n-- \n2.5.1\n"},{"id":"269407","messageId":"20150904092355.GA524@sigill.intra.peff.net","threadId":"40261","inReplyTo":"s5hy4gmvii2.wl-tiwai@suse.de","subject":"Re: glibc mutex deadlock in signal handler","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-09-04T09:23:55Z","receivedAt":"2015-09-04T09:23:55Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 04, 2015 at 07:52:21AM +0200, Takashi Iwai wrote:\n\n> -- 8< --\n> From: Takashi Iwai <tiwai@suse.de>\n> Subject: [PATCH] pager: don't use unsafe functions in signal handlers\n> \n> Since the commit [a3da8821208d: pager: do wait_for_pager on signal\n> death], we call wait_for_pager() in the pager's signal handler.  The\n> recent bug report revealed that this causes a deadlock in glibc at\n> aborting \"git log\" [*1].  When this happens, git process is left\n> unterminated, and it can't be killed by SIGTERM but only by SIGKILL.\n> \n> The problem is that wait_for_pager() function does more than waiting\n> for pager process's termination, but it does cleanups and printing\n> errors.  Unfortunately, the functions that may be used in a signal\n> handler are very limited [*2].  Particularly, malloc(), free() and the\n> variants can't be used in a signal handler because they take a mutex\n> internally in glibc.  This was the cause of the deadlock above.  Other\n> than the direct calls of malloc/free, many functions calling\n> malloc/free can't be used.  strerror() is such one, either.\n\nI think this approach is the only real solution here (and I agree it is\na real-world problem). Unfortunately, it is the tip of the iceberg.\nLooking at other signal handlers, there are lots of other potential\nproblems. For example, here are the first few I found by grepping:\n\n  - clone.c:remove_junk uses strbufs; these are doing useful work, and\n    can't just be skipped if we are in a signal handler\n\n  - fetch calls transport_unlock_pack, which has a free (which can be\n    skipped)\n\n  - repack uses remove_temporary_files, which uses a strbuf\n\nand so on.\n\n> Also the usage of fflush() and printf() in a signal handler is bad,\n> although it seems working so far.  In a safer side, we should avoid\n> them, too.\n\nI'd be surprised if they are safe; stdio definitely involves locking.\n\nPerhaps we should reconsider whether f4c3edc (vreportf: avoid\nintermediate buffer, 2015-08-11) is a good idea.  Note that snprintf is\nnot on the list of safe functions, but I imagine that in practice it is\nfine. Though just avoiding error()/warning() in signal handlers might be\na more practical solution anyway.\n\n> diff --git a/pager.c b/pager.c\n> index 27d4c8a17aa1..12d17af73745 100644\n> --- a/pager.c\n> +++ b/pager.c\n> @@ -26,7 +26,10 @@ static void wait_for_pager(void)\n>  \n>  static void wait_for_pager_signal(int signo)\n>  {\n> -\twait_for_pager();\n> +\t/* signal EOF to pager */\n> +\tclose(1);\n> +\tclose(2);\n> +\tfinish_command_in_signal(&pager_process);\n>  \tsigchain_pop(signo);\n>  \traise(signo);\n>  }\n\nHmm, is there is any reason to just pass an \"in_signal\" flag to\nwait_for_pager(), to avoid duplicating the logic?\n\nThe rest of the patch looks pretty straightforward.\n\n-Peff\n"},{"id":"269408","messageId":"s5hzj12jzlu.wl-tiwai@suse.de","threadId":"40261","inReplyTo":"20150904092355.GA524@sigill.intra.peff.net","subject":"Re: glibc mutex deadlock in signal handler","fromName":"Takashi Iwai","fromEmail":"tiwai@suse.de","sentAt":"2015-09-04T09:35:57Z","receivedAt":"2015-09-04T09:35:57Z","isPatch":false,"sender":{"key":"tiwai@suse.de","avatar":"https://avatars.githubusercontent.com/u/306482?v=4"},"body":"On Fri, 04 Sep 2015 11:23:55 +0200,\nJeff King wrote:\n> \n> On Fri, Sep 04, 2015 at 07:52:21AM +0200, Takashi Iwai wrote:\n> \n> > -- 8< --\n> > From: Takashi Iwai <tiwai@suse.de>\n> > Subject: [PATCH] pager: don't use unsafe functions in signal handlers\n> > \n> > Since the commit [a3da8821208d: pager: do wait_for_pager on signal\n> > death], we call wait_for_pager() in the pager's signal handler.  The\n> > recent bug report revealed that this causes a deadlock in glibc at\n> > aborting \"git log\" [*1].  When this happens, git process is left\n> > unterminated, and it can't be killed by SIGTERM but only by SIGKILL.\n> > \n> > The problem is that wait_for_pager() function does more than waiting\n> > for pager process's termination, but it does cleanups and printing\n> > errors.  Unfortunately, the functions that may be used in a signal\n> > handler are very limited [*2].  Particularly, malloc(), free() and the\n> > variants can't be used in a signal handler because they take a mutex\n> > internally in glibc.  This was the cause of the deadlock above.  Other\n> > than the direct calls of malloc/free, many functions calling\n> > malloc/free can't be used.  strerror() is such one, either.\n> \n> I think this approach is the only real solution here (and I agree it is\n> a real-world problem). Unfortunately, it is the tip of the iceberg.\n> Looking at other signal handlers, there are lots of other potential\n> problems. For example, here are the first few I found by grepping:\n> \n>   - clone.c:remove_junk uses strbufs; these are doing useful work, and\n>     can't just be skipped if we are in a signal handler\n> \n>   - fetch calls transport_unlock_pack, which has a free (which can be\n>     skipped)\n> \n>   - repack uses remove_temporary_files, which uses a strbuf\n> \n> and so on.\n\nYes, we need to revise all signal handlers...\n\n> > Also the usage of fflush() and printf() in a signal handler is bad,\n> > although it seems working so far.  In a safer side, we should avoid\n> > them, too.\n> \n> I'd be surprised if they are safe; stdio definitely involves locking.\n> \n> Perhaps we should reconsider whether f4c3edc (vreportf: avoid\n> intermediate buffer, 2015-08-11) is a good idea.  Note that snprintf is\n> not on the list of safe functions, but I imagine that in practice it is\n> fine. Though just avoiding error()/warning() in signal handlers might be\n> a more practical solution anyway.\n> \n> > diff --git a/pager.c b/pager.c\n> > index 27d4c8a17aa1..12d17af73745 100644\n> > --- a/pager.c\n> > +++ b/pager.c\n> > @@ -26,7 +26,10 @@ static void wait_for_pager(void)\n> >  \n> >  static void wait_for_pager_signal(int signo)\n> >  {\n> > -\twait_for_pager();\n> > +\t/* signal EOF to pager */\n> > +\tclose(1);\n> > +\tclose(2);\n> > +\tfinish_command_in_signal(&pager_process);\n> >  \tsigchain_pop(signo);\n> >  \traise(signo);\n> >  }\n> \n> Hmm, is there is any reason to just pass an \"in_signal\" flag to\n> wait_for_pager(), to avoid duplicating the logic?\n\nJust because wait_for_pager() itself is an atexit hook that can't take\nan argument, so we'd need to split to a new function.  I don't mind\neither way.  The revised patch is below.\n\n\nthanks,\n\nTakashi\n\n-- 8< --\nFrom: Takashi Iwai <tiwai@suse.de>\nSubject: [PATCH v2] pager: don't use unsafe functions in signal handlers\n\nSince the commit [a3da8821208d: pager: do wait_for_pager on signal\ndeath], we call wait_for_pager() in the pager's signal handler.  The\nrecent bug report revealed that this causes a deadlock in glibc at\naborting \"git log\" [*1].  When this happens, git process is left\nunterminated, and it can't be killed by SIGTERM but only by SIGKILL.\n\nThe problem is that wait_for_pager() function does more than waiting\nfor pager process's termination, but it does cleanups and printing\nerrors.  Unfortunately, the functions that may be used in a signal\nhandler are very limited [*2].  Particularly, malloc(), free() and the\nvariants can't be used in a signal handler because they take a mutex\ninternally in glibc.  This was the cause of the deadlock above.  Other\nthan the direct calls of malloc/free, many functions calling\nmalloc/free can't be used.  strerror() is such one, either.\n\nAlso the usage of fflush() and printf() in a signal handler is bad,\nalthough it seems working so far.  In a safer side, we should avoid\nthem, too.\n\nThis patch tries to reduce the calls of such functions in signal\nhandlers.  wait_for_signal() takes a flag and avoids the unsafe\ncalls.   Also, finish_command_in_signal() is introduced for the\nsame reason.  There the free() calls are removed, and only waits for\nthe children without whining at errors.\n\n[*1] https://bugzilla.opensuse.org/show_bug.cgi?id=942297\n[*2] http://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_04_03\n\nSigned-off-by: Takashi Iwai <tiwai@suse.de>\n---\n pager.c       | 22 ++++++++++++++++------\n run-command.c | 25 +++++++++++++++++--------\n run-command.h |  1 +\n 3 files changed, 34 insertions(+), 14 deletions(-)\n\ndiff --git a/pager.c b/pager.c\nindex 27d4c8a17aa1..e425070528f4 100644\n--- a/pager.c\n+++ b/pager.c\n@@ -14,19 +14,29 @@\n static const char *pager_argv[] = { NULL, NULL };\n static struct child_process pager_process = CHILD_PROCESS_INIT;\n \n-static void wait_for_pager(void)\n+static void wait_for_pager(int in_signal)\n {\n-\tfflush(stdout);\n-\tfflush(stderr);\n+\tif (!in_signal) {\n+\t\tfflush(stdout);\n+\t\tfflush(stderr);\n+\t}\n \t/* signal EOF to pager */\n \tclose(1);\n \tclose(2);\n-\tfinish_command(&pager_process);\n+\tif (in_signal)\n+\t\tfinish_command_in_signal(&pager_process);\n+\telse\n+\t\tfinish_command(&pager_process);\n+}\n+\n+static void wait_for_pager_atexit(void)\n+{\n+\twait_for_pager(0);\n }\n \n static void wait_for_pager_signal(int signo)\n {\n-\twait_for_pager();\n+\twait_for_pager(1);\n \tsigchain_pop(signo);\n \traise(signo);\n }\n@@ -90,7 +100,7 @@ void setup_pager(void)\n \n \t/* this makes sure that the parent terminates after the pager */\n \tsigchain_push_common(wait_for_pager_signal);\n-\tatexit(wait_for_pager);\n+\tatexit(wait_for_pager_atexit);\n }\n \n int pager_in_use(void)\ndiff --git a/run-command.c b/run-command.c\nindex 3277cf797ed4..e09275bd9e36 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -18,26 +18,27 @@ struct child_to_clean {\n static struct child_to_clean *children_to_clean;\n static int installed_child_cleanup_handler;\n \n-static void cleanup_children(int sig)\n+static void cleanup_children(int sig, int in_signal)\n {\n \twhile (children_to_clean) {\n \t\tstruct child_to_clean *p = children_to_clean;\n \t\tchildren_to_clean = p->next;\n \t\tkill(p->pid, sig);\n-\t\tfree(p);\n+\t\tif (!in_signal)\n+\t\t\tfree(p);\n \t}\n }\n \n static void cleanup_children_on_signal(int sig)\n {\n-\tcleanup_children(sig);\n+\tcleanup_children(sig, 1);\n \tsigchain_pop(sig);\n \traise(sig);\n }\n \n static void cleanup_children_on_exit(void)\n {\n-\tcleanup_children(SIGTERM);\n+\tcleanup_children(SIGTERM, 0);\n }\n \n static void mark_child_for_cleanup(pid_t pid)\n@@ -220,7 +221,7 @@ static inline void set_cloexec(int fd)\n \t\tfcntl(fd, F_SETFD, flags | FD_CLOEXEC);\n }\n \n-static int wait_or_whine(pid_t pid, const char *argv0)\n+static int wait_or_whine(pid_t pid, const char *argv0, int in_signal)\n {\n \tint status, code = -1;\n \tpid_t waiting;\n@@ -228,6 +229,8 @@ static int wait_or_whine(pid_t pid, const char *argv0)\n \n \twhile ((waiting = waitpid(pid, &status, 0)) < 0 && errno == EINTR)\n \t\t;\t/* nothing */\n+\tif (in_signal)\n+\t\treturn 0;\n \n \tif (waiting < 0) {\n \t\tfailed_errno = errno;\n@@ -437,7 +440,7 @@ fail_pipe:\n \t\t * At this point we know that fork() succeeded, but execvp()\n \t\t * failed. Errors have been reported to our stderr.\n \t\t */\n-\t\twait_or_whine(cmd->pid, cmd->argv[0]);\n+\t\twait_or_whine(cmd->pid, cmd->argv[0], 0);\n \t\tfailed_errno = errno;\n \t\tcmd->pid = -1;\n \t}\n@@ -536,12 +539,18 @@ fail_pipe:\n \n int finish_command(struct child_process *cmd)\n {\n-\tint ret = wait_or_whine(cmd->pid, cmd->argv[0]);\n+\tint ret = wait_or_whine(cmd->pid, cmd->argv[0], 0);\n \targv_array_clear(&cmd->args);\n \targv_array_clear(&cmd->env_array);\n \treturn ret;\n }\n \n+int finish_command_in_signal(struct child_process *cmd)\n+{\n+\treturn wait_or_whine(cmd->pid, cmd->argv[0], 1);\n+}\n+\n+\n int run_command(struct child_process *cmd)\n {\n \tint code;\n@@ -772,7 +781,7 @@ error:\n int finish_async(struct async *async)\n {\n #ifdef NO_PTHREADS\n-\treturn wait_or_whine(async->pid, \"child process\");\n+\treturn wait_or_whine(async->pid, \"child process\", 0);\n #else\n \tvoid *ret = (void *)(intptr_t)(-1);\n \ndiff --git a/run-command.h b/run-command.h\nindex 5b4425a3cbe1..275d35c442ac 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -50,6 +50,7 @@ void child_process_init(struct child_process *);\n \n int start_command(struct child_process *);\n int finish_command(struct child_process *);\n+int finish_command_in_signal(struct child_process *);\n int run_command(struct child_process *);\n \n /*\n-- \n2.5.1\n"},{"id":"269415","messageId":"20150904130448.GB25501@sigill.intra.peff.net","threadId":"40261","inReplyTo":"s5hzj12jzlu.wl-tiwai@suse.de","subject":"Re: glibc mutex deadlock in signal handler","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-09-04T13:04:48Z","receivedAt":"2015-09-04T13:04:48Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 04, 2015 at 11:35:57AM +0200, Takashi Iwai wrote:\n\n> > Hmm, is there is any reason to just pass an \"in_signal\" flag to\n> > wait_for_pager(), to avoid duplicating the logic?\n> \n> Just because wait_for_pager() itself is an atexit hook that can't take\n> an argument, so we'd need to split to a new function.  I don't mind\n> either way.  The revised patch is below.\n\nAh, right. That's unfortunate, but I think I prefer adding the extra\nwrapper to duplicating the contents of the function.\n\n> -- 8< --\n> From: Takashi Iwai <tiwai@suse.de>\n> Subject: [PATCH v2] pager: don't use unsafe functions in signal handlers\n> [...]\n\nThis looks good to me. Do you plan on fixing any of the other handlers\n(you don't have to; I just want to know if somebody is planning to work\non it).\n\nThe pattern of atexit and signal handlers is repeated in several places,\nand it seems like we will have to add the same in_signal boilerplate in\neach instance. I wonder if we should provide a global \"register_cleanup\"\nthat takes a \"void (*func)(int in_signal))\" function pointer, and:\n\n  1. Adds it to a list (ideally in a way that is atomic if we get\n     interrupted while adding to the list).\n\n  2. If not already run, registers an atexit() handler and\n     sigchain_push_common for a meta-handler which runs through the list\n     and runs each handler.\n\nIt's not a _huge_ amount of boilerplate code we'd be saving, but at\nleast conforming to the \"in_signal\" function template would make people\nthink twice about what they're doing inside the cleanup function. :)\n\n-Peff\n"},{"id":"269419","messageId":"s5htwrai9p1.wl-tiwai@suse.de","threadId":"40261","inReplyTo":"20150904130448.GB25501@sigill.intra.peff.net","subject":"Re: glibc mutex deadlock in signal handler","fromName":"Takashi Iwai","fromEmail":"tiwai@suse.de","sentAt":"2015-09-04T13:40:58Z","receivedAt":"2015-09-04T13:40:58Z","isPatch":false,"sender":{"key":"tiwai@suse.de","avatar":"https://avatars.githubusercontent.com/u/306482?v=4"},"body":"On Fri, 04 Sep 2015 15:04:48 +0200,\nJeff King wrote:\n> \n> On Fri, Sep 04, 2015 at 11:35:57AM +0200, Takashi Iwai wrote:\n> \n> > > Hmm, is there is any reason to just pass an \"in_signal\" flag to\n> > > wait_for_pager(), to avoid duplicating the logic?\n> > \n> > Just because wait_for_pager() itself is an atexit hook that can't take\n> > an argument, so we'd need to split to a new function.  I don't mind\n> > either way.  The revised patch is below.\n> \n> Ah, right. That's unfortunate, but I think I prefer adding the extra\n> wrapper to duplicating the contents of the function.\n> \n> > -- 8< --\n> > From: Takashi Iwai <tiwai@suse.de>\n> > Subject: [PATCH v2] pager: don't use unsafe functions in signal handlers\n> > [...]\n> \n> This looks good to me. Do you plan on fixing any of the other handlers\n> (you don't have to; I just want to know if somebody is planning to work\n> on it).\n\nHeh, I'd like to leave the rest for professionals :)\n\n> The pattern of atexit and signal handlers is repeated in several places,\n> and it seems like we will have to add the same in_signal boilerplate in\n> each instance. I wonder if we should provide a global \"register_cleanup\"\n> that takes a \"void (*func)(int in_signal))\" function pointer, and:\n> \n>   1. Adds it to a list (ideally in a way that is atomic if we get\n>      interrupted while adding to the list).\n> \n>   2. If not already run, registers an atexit() handler and\n>      sigchain_push_common for a meta-handler which runs through the list\n>      and runs each handler.\n> \n> It's not a _huge_ amount of boilerplate code we'd be saving, but at\n> least conforming to the \"in_signal\" function template would make people\n> think twice about what they're doing inside the cleanup function. :)\n\nOr, we may have a global in_signal flag.  Of course, this is bad if\ngit itself is multi-threaded, though.\n\n\nthanks,\n\nTakashi\n"},{"id":"269450","messageId":"xmqq7fo5g85x.fsf@gitster.mtv.corp.google.com","threadId":"40261","inReplyTo":"20150904092355.GA524@sigill.intra.peff.net","subject":"Re: glibc mutex deadlock in signal handler","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-04T21:56:58Z","receivedAt":"2015-09-04T21:56:58Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Perhaps we should reconsider whether f4c3edc (vreportf: avoid\n> intermediate buffer, 2015-08-11) is a good idea.  Note that snprintf is\n> not on the list of safe functions, but I imagine that in practice it is\n> fine. Though just avoiding error()/warning() in signal handlers might be\n> a more practical solution anyway.\n\nI had exactly the same thought when I read the initial report here.\n\nI wish we can just do \"if (in_signal) return;\" at the beginning of\nvreportf(), but we would not want a global variable there, so... ;-)\n\nFurther, I briefly hoped that avoiding error/warning in the signal\nhandler codepath would allow us to be more lax around allocations,\nbut I suspect that it unfortunately would not help us that much, as\nwe may be calling these functions in low memory situations.\n\nSo let's queue Takashi's patch as-is for now and look at other\nsignal codepaths.\n\nThanks.\n\n\t\n\t\n"},{"id":"269475","messageId":"20150905085934.GA31358@sigill.intra.peff.net","threadId":"40261","inReplyTo":"xmqq7fo5g85x.fsf@gitster.mtv.corp.google.com","subject":"Re: glibc mutex deadlock in signal handler","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-09-05T08:59:34Z","receivedAt":"2015-09-05T08:59:34Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 04, 2015 at 02:56:58PM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > Perhaps we should reconsider whether f4c3edc (vreportf: avoid\n> > intermediate buffer, 2015-08-11) is a good idea.  Note that snprintf is\n> > not on the list of safe functions, but I imagine that in practice it is\n> > fine. Though just avoiding error()/warning() in signal handlers might be\n> > a more practical solution anyway.\n> \n> I had exactly the same thought when I read the initial report here.\n> \n> I wish we can just do \"if (in_signal) return;\" at the beginning of\n> vreportf(), but we would not want a global variable there, so... ;-)\n\nWhy not? I mean, sure it's gross. But it actually seems like a pretty\nsimple fix that doesn't have to hurt other callers (or involve passing\nan \"in_signal\" through the stack). We could even fallback to snprintf()\ninto a fixed-sized buffer, or some other degraded mode.\n\n> Further, I briefly hoped that avoiding error/warning in the signal\n> handler codepath would allow us to be more lax around allocations,\n> but I suspect that it unfortunately would not help us that much, as\n> we may be calling these functions in low memory situations.\n\nI'm not sure the low-memory thing isn't a red herring. Sure, we call\ndie() when malloc fails. But only with a tiny string. Something like the\nrobust_buf patch I posted would handle that just fine.\n\nThe real danger of signal handlers is that you don't get to say \"oh,\nmalloc failed, so let's fallback to some degraded mode\". You just get\ndeadlocked in a futex and never return. :)\n\n> So let's queue Takashi's patch as-is for now and look at other\n> signal codepaths.\n\nSounds like a good first step, unless we are going to do refactoring\nthat Takashi's patch could take advantage of (either a global in_signal,\nor some register_cleanup() infrastructure).\n\n-Peff\n"}]}