Re: Breakage in master?

From: Jeff King <peff@peff.net>
Date: Feb 2, 2012, 17:46 UTC
Message-ID: <20120202174601.GB30857@sigill.intra.peff.net>
In-Reply-To: <CABPQNSbWu0r_gKGvCHk567pUtQiyDOCO8vFfrzPMFW1eUaj1nw@mail.gmail.com>

On Thu, Feb 02, 2012 at 01:14:19PM +0100, Erik Faye-Lund wrote:

Show 12 quoted lines

> But here's the REALLY puzzling part: If I add a simple, unused
> function to diff-lib.c, like this:
> [...]
> "git status" starts to error out with that same vsnprintf complaint!
> 
> ---8<---
> $ git status
> # On branch master
> # Changes not staged for commit:
> #   (use "git add <file>..." to update what will be committed)
> fatal: BUG: your vsnprintf is broken (returned -1)
> ---8<---

OK, that's definitely odd.

At the moment of the die() in strbuf_vaddf, what does errno say? vsnprintf should generally never be returning -1 (it should return the number of characters that would have been written). Since you're on Windows, I assume you're using the replacement version in compat/snprintf.c.

That one will return -1 if realloc fails. So I'm curious if that is what is happening (you might also instrument the call to realloc in snprintf.c to see if it is failing, and if so, at what maxsize). And/or check errno in git_vsnprintf after calling the native vsnprintf and getting -1.

Here's one possible sequence of events that seems plausible to me (and remember that this is a wild guess):

  1. gettext somehow munges the format string in a way that Windows
     vsnprintf doesn't like, and it returns -1.

  2. Our git_vsnprintf wrapper interprets this -1 as "you didn't give me
     enough space to store the result", and we grow our test-buffer to
     try again

  3. Eventually the test buffer gets unreasonably large, and realloc
     fails. We have no choice but to return -1 from our wrapper.

  4. strbuf_vaddf sees the -1 and thinks you are using a broken
     vsnprintf.

All of that would make sense to me, _except_ for your weird "if I add a random function, the problem is more reproducible" bit. Which does seem like something is invoking undefined behavior (of course, it could be that undefined behavior or stack-smashing that is causing vsnprintf to report an error). Lacking any better leads, it might be worth pursuing.

Show 5 quoted lines

> I've bisected the issues down to 5e9637c (i18n: add infrastructure for
> translating Git with gettext). Trying to apply my unused-function
> patch on top of this commit starts giving the same "fatal: BUG: your
> vsnprintf is broken (returned -1)" error. It's ancestor, bc1bbe0(Git
> 1.7.8-rc2), does not yield any of the issues.

I've looked at 5e9637c, and it really doesn't do anything that looks bad. I wonder if your gettext library is buggy. Does compiling with NO_GETTEXT help?

-Peff