Re: [PATCH 1/3] usage: refactor die-recursion checks

[Jeff King <peff@peff.net>]

On Mon, Apr 15, 2013 at 05:11:36PM -0700, Brandon Casey wrote:

> > +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() ?

Because I hand-audited it. But I think the more important question you
are getting at is: how do I know that it will remain 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.

Right. My assumption was that we are primarily interested in protecting
against the die_routine. Compat functions should never be calling die.
Of course that is assuming nobody violates that rule, which is part of
the point of the check.

> 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.

Easier said than done, sometimes, if you are debugging somebody else's
problem from a dump of a terminal session. :)

But I can live with dropping this patch; my primary goal is the bug-fix
in patch three.

I was also tempted to suggest just dropping the recursion check
altogether. While it is neat to detect such things, it's a "should never
happen" bug situation, and an infinite loop of printing out the same
message is pretty easy to notice. Do you remember what spurred the
original check? The message in cd163d4 doesn't say.

-Peff