[bug-gettext] Fwd: Breakage in master?

[Erik Faye-Lund]

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 <address@hidden>
Date: Sat, Feb 4, 2012 at 10:55 PM
Subject: Re: Breakage in master?
To: Jeff King <address@hidden>
Cc: Git Mailing List <address@hidden>, msysGit
<address@hidden>, Ævar Arnfjörð <address@hidden>


On Fri, Feb 3, 2012 at 1:28 PM, Erik Faye-Lund <address@hidden> wrote:
> On Thu, Feb 2, 2012 at 6:46 PM, Jeff King <address@hidden> 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