Re: [PATCH 1/3] usage: refactor die-recursion checks
From: Brandon Casey <hidden>
Date: 2016-06-15 22:56:51
On Mon, Apr 15, 2013 at 4:08 PM, Jeff King [off-list ref] wrote:
quoted hunk ↗ jump to hunk
When any git code calls die(), we chain to a custom die_routine, which we expect to print a message and exit the program. To avoid infinite loops, we detect a recursive call to die() with a simple counter, and break out of the loop by printing a message and exiting ourselves, without chaining to the die_routine. But the user does not get to see the message that would have been fed to the die_routine, which makes debugging harder. The user does not know if it was a true infinite loop, or simply a single re-entrant call, since they cannot compare the messages. Furthermore, if we are wrong about detecting the recursion, we have blocked the user from seeing the original message, which is probably the more useful one. This patch teaches die() to print the original die message to stderr before reporting the recursion. The custom die_routine may or may not have put it the message to stderr, but this is the best we can do (it is what most handlers will do anyway, and it is where our recursion error will go). While we're at it, let's mark the "recursion detected" message as a "BUG:", since it should never happen in practice. And let's factor out the repeated code in die and die_errno. This loses the information of which function was called to cause the recursion, but it's important; knowing the actual message fed to the function (which we now do) is much more useful, as it can generally pin-point the actual call-site that caused the recursion. Signed-off-by: Jeff King <redacted> --- This helped me debug the current problem. And factoring it out helps with patch 3. :) usage.c | 29 ++++++++++++++--------------- 1 file changed, 14 insertions(+), 15 deletions(-)diff --git a/usage.c b/usage.c index 40b3de5..c6b7ac5 100644 --- a/usage.c +++ b/usage.c
quoted hunk ↗ jump to hunk
@@ -80,17 +78,24 @@ void NORETURN die(const char *err, ...) usagef("%s", err); } +static void check_die_recursion(const char *fmt, va_list ap) +{ + static int dying; + + if (!dying++) + return; + + vreportf("fatal: ", fmt, ap);
How do you know it's safe to call vreportf() ? If the bug is in the vreportf code path, we will recurse infinitely (at least until the stack is used up). An implementation of vsnprintf exists in compat/snprintf.c for example. It's nice to print out the error message here, but I think doing so defeats the purpose of this "dying" check. Better to get the stack trace from a core dump.
+ fputs("BUG: recursion detected in die handler\n", stderr);
+ exit(128);
+}
+-Brandon