Re: Breakage in master?

From

Erik Faye-Lund <kusmabite@gmail.com>

Date

Feb 3, 2012, 12:28 UTC

Message-ID

<CABPQNSZfKCTsuusPpHa2djEOeGVN9z5s_Fr+S3EaHiv7Q4Re9w@mail.gmail.com>

In-Reply-To

<20120202174601.GB30857@sigill.intra.peff.net>

On Thu, Feb 2, 2012 at 6:46 PM, Jeff King <peff@peff.net> wrote:

> On Thu, Feb 02, 2012 at 01:14:19PM +0100, Erik Faye-Lund wrote:
>
>> 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?

diff --git a/strbuf.c b/strbuf.c
index ff0b96b..52dfdd6 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -218,7 +218,7 @@ void strbuf_vaddf(struct strbuf *sb, const char
*fmt, va_list ap)
 	len = vsnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, cp);
 	va_end(cp);
 	if (len < 0)
-		die("BUG: your vsnprintf is broken (returned %d)", len);
+		die_errno("BUG: your vsnprintf is broken (returned %d)", len);
 	if (len > strbuf_avail(sb)) {
 		strbuf_grow(sb, len);
 		len = vsnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, ap);
---8<---

Then I get "fatal: BUG: your vsnprintf is broken (returned -1): Result
too large". This goes both for both failure cases I described. I
assume this means errno=ERANGE.

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

No. SNPRINTF_RETURNS_BOGUS is only set for the MSVC target, not for
the MinGW target. I'm assuming that means MinGW-runtime has a sane
vsnprintf implementation. But even if I enable SNPRINTF_RETURNS_BOGUS,
the problem occurs. And it's still "Result too large".

So I decided to do a bit of stepping, and it seems libintl takes over
vsnprintf, directing us to libintl_vsnprintf instead. I guess this is
so it can ensure we support reordering the parameters with $1 etc...
And aparently this vsnprintf implementation calls the system vnsprintf
if the format string does not contain '$', and it's using _vsnprintf
rather than vsnprintf on Windows. _vsnprintf is the MSVCRT-version,
and not the MinGW-runtime, which needs SNPRINTF_RETURNS_BOGUS.

So I guess I can patch libintl to call vsnprintf from MinGW-runtime instead.

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

Well, now at least I have some better leads, but I'm still not able to
explain the "if I add a random function, the problem is more
reproducible" bit. :(

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

Compiling with NO_GETTEXT does make the symptoms go away, but that's
not curing the problem ;)

But, I have a lead now. I'll see if I can find out *why* libintl calls
_vsnprintf on MinGW. I expect it's so the MSVC and the MinGW versions
behave similarly, MSVC doesn't have a sane vsnprintf. Perhaps I should
back-port SNPRINTF_RETURNS_BOGUS-workaround to libintl, so our MSVC
builds doesn't break also?