Re: [PATCH 1/3] usage: refactor die-recursion checks
- From
Brandon Casey <drafnel@gmail.com>
- Date
- Apr 16, 2013, 00:11 UTC
- Message-ID
- <CA+sFfMes99EepY4FCW32s1L3ywv_gyFb76=Y=35rvPbc2K1BWA@mail.gmail.com>
- In-Reply-To
- <20130415230802.GA11267@sigill.intra.peff.net>
On Mon, Apr 15, 2013 at 4:08 PM, Jeff King <peff@peff.net> wrote:
Show 43 quoted lines
> 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 <peff@peff.net> > --- > 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
Show 12 quoted lines
> @@ -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