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