Fwd: Breakage in master?
- From
Erik Faye-Lund <kusmabite@gmail.com>
- Date
- Feb 5, 2012, 14:46 UTC
- Message-ID
- <CABPQNSbPA3jicuPiaHuSBY_qEy1Pmy1uWnLBWt=7O0dETbKZ3g@mail.gmail.com>
- In-Reply-To
- <CABPQNSbj8QKqkdY49Y7tpAOQd53t+z6Gc5U-CS0-TZWyNz1WfQ@mail.gmail.com>
Git has recently switched to using gettext for translations, and I have observed a breakage on Windows due to the way gettext handles vsnprintf.
On MinGW, vsnprintf and _vsnprintf are two different implementations; vsnprintf is from MinGW-runtime, and provides a reasonably sane implementation. _vnsprintf on the other hand is from MSVCRT.dll, and has some issues with it's return value. Before using gettext, the MinGW-built version of Git called the version from mingw-runtime, and everything worked fine. When built with MSVC, a shim was used to fixup the bogus return value. This shim was injected through a define, similar to what gettext does.
The shim in gettext lead to issues for Git, both on MinGW and on MSVC.
For MinGW, the problem is that libintl_vsnprintf calls _vsnprintf rather than vsnprintf, giving us the same, broken return value that we tried to prevent. This means that our code intended to call the MinGW-runtime version, but gettext ended up calling the MSVCRT.dll version. I don't find this very reasonable; a call to vsnprintf ends up as a call to _vsnprintf.
On MSVC the problem is a bit easier to spot; libgnuintl.h.in contains the following:
---8<--- #if !(defined vsnprintf && defined _GL_STDIO_H) /* don't override gnulib */ #undef vsnprintf #define vsnprintf libintl_vsnprintf extern int vsnprintf (char *, size_t, const char *, va_list); #endif ---8<---
Uhm, what? Unless we're using Gnulib, our definition of vsnprintf should simply be ignored?
I'm not saying figuring out what to do here is exactly trivial; but I think undefining any definitions of vsnprintf that aren't exactly "_vsnprintf" is dangerous. The forwarded mail below contains a quick-fix I did locally that seems to side-step the problem for me, by not using _vsnprintf on MinGW. But perhaps there's something better we can do?
---------- Forwarded message ---------- From: Erik Faye-Lund <kusmabite@gmail.com> Date: Sat, Feb 4, 2012 at 10:55 PM Subject: Re: Breakage in master? To: Jeff King <peff@peff.net> Cc: Git Mailing List <git@vger.kernel.org>, msysGit <msysgit@googlegroups.com>, Ævar Arnfjörð <avarab@gmail.com>
On Fri, Feb 3, 2012 at 1:28 PM, Erik Faye-Lund <kusmabite@gmail.com> wrote:
Show 62 quoted lines
> 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?
>
> If I apply this patch:
> ---8<---
> 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.
>Indeed, I just got around to testing this, and doing this on top of gettext seems to fix the problem for me. For the MSVC, a more elaborate fix is needed, as it doesn't have a sane vsnprintf.
---
diff --git a/gettext-runtime/intl/printf.c b/gettext-runtime/intl/printf.c index b7cdc5d..f55023e 100644 --- a/gettext-runtime/intl/printf.c +++ b/gettext-runtime/intl/printf.c @@ -192,7 +192,7 @@ libintl_sprintf (char *resultbuf, const char *format, ...) #if HAVE_SNPRINTF -# if HAVE_DECL__SNPRINTF +# if HAVE_DECL__SNPRINTF && !defined(__MINGW32__) /* Windows. */ # define system_vsnprintf _vsnprintf # else