Merge branch 'ti/glibc-stdio-mutex-from-signal-handler' into maint
authorJunio C Hamano <gitster@pobox.com>
Fri, 16 Oct 2015 21:32:41 +0000 (14:32 -0700)
committerJunio C Hamano <gitster@pobox.com>
Fri, 16 Oct 2015 21:32:41 +0000 (14:32 -0700)
Allocation related functions and stdio are unsafe things to call
inside a signal handler, and indeed killing the pager can cause
glibc to deadlock waiting on allocation mutex as our signal handler
tries to free() some data structures in wait_for_pager(). Reduce
these unsafe calls.

* ti/glibc-stdio-mutex-from-signal-handler:
pager: don't use unsafe functions in signal handlers

1  2 
pager.c
run-command.c
run-command.h
diff --combined pager.c
index 27d4c8a17aa17bb2cf31484227073e1a3204c17f,0f789c3ed4f513f9baa85bbf4ffcf580f2ee9bc8..e425070528f4f9fbf0953c9056d1ea43686e176f
+++ b/pager.c
  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 +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)
@@@ -150,8 -160,7 +160,8 @@@ int check_pager_config(const char *cmd
        struct strbuf key = STRBUF_INIT;
        const char *value = NULL;
        strbuf_addf(&key, "pager.%s", cmd);
 -      if (!git_config_get_value(key.buf, &value)) {
 +      if (git_config_key_is_valid(key.buf) &&
 +          !git_config_get_value(key.buf, &value)) {
                int b = git_config_maybe_bool(key.buf, value);
                if (b >= 0)
                        want = b;
diff --combined run-command.c
index 3277cf797ed41e5834b3b94fa8d2e9e9d5b4a317,fe116bc2b144cfa59204745a9667fd1bbd239508..e09275bd9e360ee7683ef5d3753ccfe753ee8dcc
@@@ -18,26 -18,27 +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)
@@@ -200,6 -201,7 +201,6 @@@ static int execv_shell_cmd(const char *
  #endif
  
  #ifndef GIT_WINDOWS_NATIVE
 -static int child_err = 2;
  static int child_notifier = -1;
  
  static void notify_parent(void)
         */
        xwrite(child_notifier, "", 1);
  }
 -
 -static NORETURN void die_child(const char *err, va_list params)
 -{
 -      vwritef(child_err, "fatal: ", err, params);
 -      exit(128);
 -}
 -
 -static void error_child(const char *err, va_list params)
 -{
 -      vwritef(child_err, "error: ", err, params);
 -}
  #endif
  
  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;
  
        while ((waiting = waitpid(pid, &status, 0)) < 0 && errno == EINTR)
                ;       /* nothing */
+       if (in_signal)
+               return 0;
  
        if (waiting < 0) {
                failed_errno = errno;
@@@ -350,10 -365,11 +353,10 @@@ fail_pipe
                 * in subsequent call paths use the parent's stderr.
                 */
                if (cmd->no_stderr || need_err) {
 -                      child_err = dup(2);
 +                      int child_err = dup(2);
                        set_cloexec(child_err);
 +                      set_error_handle(fdopen(child_err, "w"));
                }
 -              set_die_routine(die_child);
 -              set_error_routine(error_child);
  
                close(notify_pipe[0]);
                set_cloexec(notify_pipe[1]);
                 * 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;
        }
  
  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;
@@@ -772,7 -794,7 +781,7 @@@ error
  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);
  
  
  const char *find_hook(const char *name)
  {
 -      const char *path = git_path("hooks/%s", name);
 -      if (access(path, X_OK) < 0)
 -              path = NULL;
 +      static struct strbuf path = STRBUF_INIT;
  
 -      return path;
 +      strbuf_reset(&path);
 +      strbuf_git_path(&path, "hooks/%s", name);
 +      if (access(path.buf, X_OK) < 0)
 +              return NULL;
 +      return path.buf;
  }
  
  int run_hook_ve(const char *const *env, const char *name, va_list args)
diff --combined run-command.h
index 5b4425a3cbe1aea2bae40c4e060e45ee2d7a29a5,518663eef5d7c23ed947161fb1162d867d2af7b8..275d35c442ac575f8f3f40aeb915cd0d341c776f
@@@ -50,13 -50,9 +50,14 @@@ void child_process_init(struct child_pr
  
  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 *);
  
 +/*
 + * Returns the path to the hook file, or NULL if the hook is missing
 + * or disabled. Note that this points to static storage that will be
 + * overwritten by further calls to find_hook and run_hook_*.
 + */
  extern const char *find_hook(const char *name);
  LAST_ARG_MUST_BE_NULL
  extern int run_hook_le(const char *const *env, const char *name, ...);