git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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
Previous: Jeff KingNext: Jeff King
Message 5 of 23 in “avoid bogus "recursion detected in die handler" message”
  1. 0/3 avoid bogus "recursion detected in die handler" messageJeff King, Apr 15, 2013
  2. 1/3 usage: refactor die-recursion checksJeff King, Apr 15, 2013
  3. Eric SunshineApr 15, 2013
  4. Jeff KingApr 15, 2013
  5. Brandon CaseyApr 16, 2013
  6. Jeff KingApr 16, 2013
  7. Jonathan NiederApr 16, 2013
  8. Brandon CaseyApr 16, 2013
  9. Jeff KingApr 16, 2013
  10. Johannes SixtApr 16, 2013
  11. Jeff KingApr 16, 2013
  12. Johannes SixtApr 16, 2013
  13. 0/2 avoid bogus "recursion detected in die handler" messageJeff King, Apr 16, 2013
  14. 1/2 usage: allow pluggable die-recursion checksJeff King, Apr 16, 2013
  15. 2/2 run-command: use thread-aware die_is_recursing routineJeff King, Apr 16, 2013
  16. Junio C HamanoApr 16, 2013
  17. Jonathan NiederApr 17, 2013
  18. Jeff KingApr 17, 2013
  19. Erik Faye-LundApr 23, 2013
  20. 2/3 run-command: factor out running_main_thread functionJeff King, Apr 15, 2013
  21. Jonathan NiederApr 16, 2013
  22. Jeff KingApr 16, 2013
  23. 3/3 usage: do not check die recursion outside main threadJeff King, Apr 15, 2013

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.