From c19a490e37181de8fa94ae1074af4b9f9a518f95 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 16 Apr 2013 15:46:22 -0400 Subject: usage: allow pluggable die-recursion checks When any git code calls die or die_errno, we use a counter to detect recursion into the die functions from any of the helper functions. However, such a simple counter is not good enough for threaded programs, which may call die from a sub-thread, killing only the sub-thread (but incrementing the counter for everyone). Rather than try to deal with threads ourselves here, let's just allow callers to plug in their own recursion-detection function. This is similar to how we handle the die routine (the caller plugs in a die routine which may kill only the sub-thread). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano diff --git a/git-compat-util.h b/git-compat-util.h index 9c01e9b..6aee9df 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -301,6 +301,7 @@ extern void warning(const char *err, ...) __attribute__((format (printf, 1, 2))) extern void set_die_routine(NORETURN_PTR void (*routine)(const char *err, va_list params)); extern void set_error_routine(void (*routine)(const char *err, va_list params)); +extern void set_die_is_recursing_routine(int (*routine)(void)); extern int prefixcmp(const char *str, const char *prefix); extern int suffixcmp(const char *str, const char *suffix); diff --git a/usage.c b/usage.c index 8eab281..9d2961e 100644 --- a/usage.c +++ b/usage.c @@ -6,8 +6,6 @@ #include "git-compat-util.h" #include "cache.h" -static int dying; - void vreportf(const char *prefix, const char *err, va_list params) { char msg[4096]; @@ -49,12 +47,19 @@ static void warn_builtin(const char *warn, va_list params) vreportf("warning: ", warn, params); } +static int die_is_recursing_builtin(void) +{ + static int dying; + return dying++; +} + /* If we are in a dlopen()ed .so write to a global variable would segfault * (ugh), so keep things static. */ static NORETURN_PTR void (*usage_routine)(const char *err, va_list params) = usage_builtin; static NORETURN_PTR void (*die_routine)(const char *err, va_list params) = die_builtin; static void (*error_routine)(const char *err, va_list params) = error_builtin; static void (*warn_routine)(const char *err, va_list params) = warn_builtin; +static int (*die_is_recursing)(void) = die_is_recursing_builtin; void set_die_routine(NORETURN_PTR void (*routine)(const char *err, va_list params)) { @@ -66,6 +71,11 @@ void set_error_routine(void (*routine)(const char *err, va_list params)) error_routine = routine; } +void set_die_is_recursing_routine(int (*routine)(void)) +{ + die_is_recursing = routine; +} + void NORETURN usagef(const char *err, ...) { va_list params; @@ -84,11 +94,10 @@ void NORETURN die(const char *err, ...) { va_list params; - if (dying) { + if (die_is_recursing()) { fputs("fatal: recursion detected in die handler\n", stderr); exit(128); } - dying = 1; va_start(params, err); die_routine(err, params); @@ -102,12 +111,11 @@ void NORETURN die_errno(const char *fmt, ...) char str_error[256], *err; int i, j; - if (dying) { + if (die_is_recursing()) { fputs("fatal: recursion detected in die_errno handler\n", stderr); exit(128); } - dying = 1; err = strerror(errno); for (i = j = 0; err[i] && j < sizeof(str_error) - 1; ) { -- cgit v0.10.2-6-g49f6 From 1ece66bc9e5433dff008786daba9918f3e2a6525 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 16 Apr 2013 15:50:07 -0400 Subject: run-command: use thread-aware die_is_recursing routine If we die from an async thread, we do not actually exit the program, but just kill the thread. This confuses the static counter in usage.c's default die_is_recursing function; it updates the counter once for the thread death, and then when the main program calls die() itself, it erroneously thinks we are recursing. The end result is that we print "recursion detected in die handler" instead of the real error in such a case (the easiest way to trigger this is having a remote connection hang up while running a sideband demultiplexer). This patch solves it by using a per-thread counter when the async_die function is installed; we detect recursion in each thread (including the main one), but they do not step on each other's toes. Other threaded code does not need to worry about this, as they do not install specialized die handlers; they just let a die() from a sub-thread take down the whole program. Since we are overriding the default recursion-check function, there is an interesting corner case that is not a problem, but bears some explanation. Imagine the main thread calls die(), and then in the die_routine starts an async call. We will switch to using thread-local storage, which starts at 0, for the main thread's counter, even though the original counter was actually at 1. That's OK, though, for two reasons: 1. It would miss only the first level of recursion, and would still find recursive failures inside the async helper. 2. We do not currently and are not likely to start doing anything as heavyweight as starting an async routine from within a die routine or helper function. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano diff --git a/run-command.c b/run-command.c index 0471219..87ddce0 100644 --- a/run-command.c +++ b/run-command.c @@ -583,6 +583,7 @@ int run_command_v_opt_cd_env(const char **argv, int opt, const char *dir, const static pthread_t main_thread; static int main_thread_set; static pthread_key_t async_key; +static pthread_key_t async_die_counter; static void *run_thread(void *data) { @@ -609,6 +610,14 @@ static NORETURN void die_async(const char *err, va_list params) exit(128); } + +static int async_die_is_recursing(void) +{ + void *ret = pthread_getspecific(async_die_counter); + pthread_setspecific(async_die_counter, (void *)1); + return ret != NULL; +} + #endif int start_async(struct async *async) @@ -690,7 +699,9 @@ int start_async(struct async *async) main_thread_set = 1; main_thread = pthread_self(); pthread_key_create(&async_key, NULL); + pthread_key_create(&async_die_counter, NULL); set_die_routine(die_async); + set_die_is_recursing_routine(async_die_is_recursing); } if (proc_in >= 0) -- cgit v0.10.2-6-g49f6