Breakage (?) in configure and git_vsnprintf()

Breakage (?) in configure and git_vsnprintf()

[Michael Haggerty]

I found a mysterious bunch of test suite failures when I compiled git on
a 64-bit Linux 3.0.0 using gcc 4.6.1:

    git clean -f -x -d
    make clean
    make configure
    ./configure --prefix=$HOME CFLAGS='-g -O0 -std=c89 -Wall -Werror'
    make test

The failure unexpectedly depended on the presence of all three compiler
options "-std=c89 -Wall -Werror".  There is no difference between -O0
and -O2.  The breakage is in v1.7.7, v1.7.8, and master (I didn't try
older versions).

I have been using "-std=c89" when compiling to avoid accidentally using
newer C features.  Perhaps that is unwise :-)

The same test succeeds on 32-bit Linux 2.6.32 using gcc 4.4.3.

There seem to be two levels to the problem:


1. With this choice of compiler options, configure incorrectly convinces
itself that the system's snprintf() is broken and sets
SNPRINTF_RETURNS_BOGUS.  From config.log:

configure:5368: checking whether snprintf() and/or vsnprintf() return
bogus value
configure:5406: cc -o conftest -g -O0 --std=c89 -Wall -Werror
conftest.c  >&5
conftest.c: In function 'test_vsnprintf':
conftest.c:62:5: error: implicit declaration of function 'vsnprintf'
[-Werror=implicit-function-declaration]
conftest.c: In function 'main':
conftest.c:72:5: error: implicit declaration of function 'snprintf'
[-Werror=implicit-function-declaration]
cc1: all warnings being treated as errors

configure:5406: $? = 1
configure: program exited with status 1

According to the manpage, snprintf() and vsnprintf() are truly not
supported for "-std=c89" and indeed they are not declared by any of the
files included by the test program that configure is using.  (Oddly,
although they are nominally not supported, vsnprintf() is used anyway in
the definition of git_vsnprintf().)

If I leave off any of the compilation options "-std=c89 -Wall -Werror"
or if I toggle the "#ifdef SNPRINTF_RETURNS_BOGUS" line in
git-compat-util.h off, the problem goes away.


2. The configure problem causes git_vsnprintf() to be wrapped around the
C library version.  This leads to many failures in the test suite.  I
suppose that git_vsnprintf() is broken in some way.


I'm kindof busy with my ref-api patch series so I won't have time to
look further into this problem in the near future.  But perhaps somebody
with experience with the configuration system and/or git_vsnprintf() is
interested.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: Breakage (?) in configure and git_vsnprintf()

[Jeff King]

On Sun, Dec 11, 2011 at 07:42:03PM +0100, Michael Haggerty wrote:

> 2. The configure problem causes git_vsnprintf() to be wrapped around the
> C library version.  This leads to many failures in the test suite.  I
> suppose that git_vsnprintf() is broken in some way.

I enabled SNPRINTF_RETURNS_BOGUS manually and was able to see the test
suite failures. Very oddly, I could get them while running the full
suite in parallel, but when I ran individual scripts, the problem went
away. Which makes no sense to me at all.

However, I peeked at the git_vsnprintf function, and one obvious error
is that it calls vsnprintf multiple times on the same va_list.

Fixing that (patch below) makes the test failures go away. I think it's
an Obviously Correct thing to do, anyway, but I'm slightly unnerved by
not understanding why it sometimes caused failures and sometimes not.
Clearly the existing code invokes nasal daemons and anything is allowed
to happen, but it would be nice to understand what triggers the
difference.

I'll leave the issue of "-std=c89" triggering SNPRINTF_RETURNS_BOGUS to
people who know and care about autoconf. My gut is to say "don't do
that". Git is not actually pure c89. We typically target systems that
are _at least_ c89, but it's more important to match and run well on
real-world systems than what was defined in the standard. So we don't
depend on c99, but we do depend on quirks and features that were
prominent in mid-90's Unix variants.

-- >8 --
Subject: [PATCH] compat/snprintf: don't look at va_list twice

If you define SNPRINTF_RETURNS_BOGUS, we use a special
git_vsnprintf wrapper assumes that vsnprintf returns "-1"
instead of the number of characters that you would need to
store the result.

To do this, it invokes vsnprintf multiple times, growing a
heap buffer until we have enough space to hold the result.
However, this means we evaluate the va_list parameter
multiple times, which is generally a bad thing (it may be
modified by calls to vsnprintf, yielding undefined
behavior).

Instead, we must va_copy it and hand the copy to vsnprintf,
so we always have a pristine va_list.

Signed-off-by: Jeff King <peff@peff.net>
---
 compat/snprintf.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/compat/snprintf.c b/compat/snprintf.c
index e1e0e75..38fc08d 100644
--- a/compat/snprintf.c
+++ b/compat/snprintf.c
@@ -19,11 +19,13 @@
 #undef vsnprintf
 int git_vsnprintf(char *str, size_t maxsize, const char *format, va_list ap)
 {
+	va_list cp;
 	char *s;
 	int ret = -1;
 
 	if (maxsize > 0) {
-		ret = vsnprintf(str, maxsize-SNPRINTF_SIZE_CORR, format, ap);
+		va_copy(cp, ap);
+		ret = vsnprintf(str, maxsize-SNPRINTF_SIZE_CORR, format, cp);
 		if (ret == maxsize-1)
 			ret = -1;
 		/* Windows does not NUL-terminate if result fills buffer */
@@ -42,7 +44,8 @@ int git_vsnprintf(char *str, size_t maxsize, const char *format, va_list ap)
 		if (! str)
 			break;
 		s = str;
-		ret = vsnprintf(str, maxsize-SNPRINTF_SIZE_CORR, format, ap);
+		va_copy(cp, ap);
+		ret = vsnprintf(str, maxsize-SNPRINTF_SIZE_CORR, format, cp);
 		if (ret == maxsize-1)
 			ret = -1;
 	}
-- 
1.7.8.13.g74677

Re: Breakage (?) in configure and git_vsnprintf()

[Johannes Sixt]

Am 12/12/2011 7:43, schrieb Jeff King:
> I'll leave the issue of "-std=c89" triggering SNPRINTF_RETURNS_BOGUS to
> people who know and care about autoconf. My gut is to say "don't do
> that".

Right. But Michael's problem was actually that SNPRINTF_RETURNS_BOGUS was
set incorrectly; his system has a working snprintf (or so I assume). The
reason for the failure is that ./configure's test program produced a
warning, and that warning was turned into an error due to -Werror. Without
-Werror, the test program would have compiled successfully, and the
working snprintf would have been detected.

-- Hannes

Re: Breakage (?) in configure and git_vsnprintf()

[Jeff King]

On Mon, Dec 12, 2011 at 08:45:39AM +0100, Johannes Sixt wrote:

> Am 12/12/2011 7:43, schrieb Jeff King:
> > I'll leave the issue of "-std=c89" triggering SNPRINTF_RETURNS_BOGUS to
> > people who know and care about autoconf. My gut is to say "don't do
> > that".
> 
> Right. But Michael's problem was actually that SNPRINTF_RETURNS_BOGUS was
> set incorrectly; his system has a working snprintf (or so I assume). The
> reason for the failure is that ./configure's test program produced a
> warning, and that warning was turned into an error due to -Werror. Without
> -Werror, the test program would have compiled successfully, and the
> working snprintf would have been detected.

Right, I understand that. But he has given a set of options that
shouldn't compile git at all (he tells the compiler not to use snprintf
via -std=c89, but we require that it exists, because even our
git_vsnprintf wrapper uses the underlying system vsnprintf).

So yes, the configure script is broken to detect the situation as
SNPRINTF_RETURNS_BOGUS and not "this platform doesn't have snprintf at
all"[1]. But I'm saying that the "we do not have snprintf at all" case
is not all that interesting: git needs it. So I'm not sure compiling
with -std=c89 really makes sense[2].

If somebody wants to make the configure script more accurate, I
certainly don't want to stop them. I'm just not sure it is worth
anybody's time in this case.

-Peff

[1] Yes, obviously we do actually have it, but it is somewhat a fluke
    that it works. We tell the compiler during the compile phase that we
    don't have it, but then during the link phase it is magically
    available in libc.

[2] I can convince git to compile on recent Linux with gcc using
    CFLAGS='-std=c89 -Dinline='.  Turning on "-Wall -Werror" doesn't
    work because all of the inline functions appear to be unused
    statics.  But if I understand Michael's problem correctly, wouldn't
    we be missing the prototype for snprintf, which could cause subtle
    errors?

Re: Breakage (?) in configure and git_vsnprintf()

[Junio C Hamano]

Good analysis; thanks.

Back when c4582f9 (Add compat/snprintf.c for systems that return bogus,
2008-03-05) was done we didn't have va_copy emulation available in
git-compat-util.h but now we do, so I agree that this is an Obviously
Correct thing to do.

Re: Breakage (?) in configure and git_vsnprintf()

[Andreas Schwab]

Jeff King <peff@peff.net> writes:

>  	if (maxsize > 0) {
> -		ret = vsnprintf(str, maxsize-SNPRINTF_SIZE_CORR, format, ap);
> +		va_copy(cp, ap);
> +		ret = vsnprintf(str, maxsize-SNPRINTF_SIZE_CORR, format, cp);

You also need to call va_end on the copy.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: Breakage (?) in configure and git_vsnprintf()

[Michael Haggerty]

On 12/12/2011 07:43 AM, Jeff King wrote:
> On Sun, Dec 11, 2011 at 07:42:03PM +0100, Michael Haggerty wrote:
>> 2. The configure problem causes git_vsnprintf() to be wrapped around the
>> C library version.  This leads to many failures in the test suite.  I
>> suppose that git_vsnprintf() is broken in some way.
> 
> I enabled SNPRINTF_RETURNS_BOGUS manually and was able to see the test
> suite failures. Very oddly, I could get them while running the full
> suite in parallel, but when I ran individual scripts, the problem went
> away. Which makes no sense to me at all.
> 
> However, I peeked at the git_vsnprintf function, and one obvious error
> is that it calls vsnprintf multiple times on the same va_list.

Thanks for the quick response!  Yes, I think you've hit the nail on the
head.  (Though I think Andreas is correct that va_end() needs to be
called on the copies.)  Either with or without va_end(), your patch
fixes the test suite failures for me.

> I'll leave the issue of "-std=c89" triggering SNPRINTF_RETURNS_BOGUS to
> people who know and care about autoconf. My gut is to say "don't do
> that". Git is not actually pure c89. [...]

OK, I can live with that.  Poor Junio will probably be stuck correcting
my non-c89isms again, though :-(

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: Breakage (?) in configure and git_vsnprintf()

[Jeff King]

On Mon, Dec 12, 2011 at 10:30:27AM +0100, Andreas Schwab wrote:

> Jeff King <peff@peff.net> writes:
> 
> >  	if (maxsize > 0) {
> > -		ret = vsnprintf(str, maxsize-SNPRINTF_SIZE_CORR, format, ap);
> > +		va_copy(cp, ap);
> > +		ret = vsnprintf(str, maxsize-SNPRINTF_SIZE_CORR, format, cp);
> 
> You also need to call va_end on the copy.

Silly me. Thanks for catching.

Junio, here's a corrected version.

-- >8 --
Subject: [PATCH] compat/snprintf: don't look at va_list twice

If you define SNPRINTF_RETURNS_BOGUS, we use a special
git_vsnprintf wrapper assumes that vsnprintf returns "-1"
instead of the number of characters that you would need to
store the result.

To do this, it invokes vsnprintf multiple times, growing a
heap buffer until we have enough space to hold the result.
However, this means we evaluate the va_list parameter
multiple times, which is generally a bad thing (it may be
modified by calls to vsnprintf, yielding undefined
behavior).

Instead, we must va_copy it and hand the copy to vsnprintf,
so we always have a pristine va_list.

Signed-off-by: Jeff King <peff@peff.net>
---
 compat/snprintf.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/compat/snprintf.c b/compat/snprintf.c
index e1e0e75..42ea1ac 100644
--- a/compat/snprintf.c
+++ b/compat/snprintf.c
@@ -19,11 +19,14 @@
 #undef vsnprintf
 int git_vsnprintf(char *str, size_t maxsize, const char *format, va_list ap)
 {
+	va_list cp;
 	char *s;
 	int ret = -1;
 
 	if (maxsize > 0) {
-		ret = vsnprintf(str, maxsize-SNPRINTF_SIZE_CORR, format, ap);
+		va_copy(cp, ap);
+		ret = vsnprintf(str, maxsize-SNPRINTF_SIZE_CORR, format, cp);
+		va_end(cp);
 		if (ret == maxsize-1)
 			ret = -1;
 		/* Windows does not NUL-terminate if result fills buffer */
@@ -42,7 +45,9 @@ int git_vsnprintf(char *str, size_t maxsize, const char *format, va_list ap)
 		if (! str)
 			break;
 		s = str;
-		ret = vsnprintf(str, maxsize-SNPRINTF_SIZE_CORR, format, ap);
+		va_copy(cp, ap);
+		ret = vsnprintf(str, maxsize-SNPRINTF_SIZE_CORR, format, cp);
+		va_end(cp);
 		if (ret == maxsize-1)
 			ret = -1;
 	}
-- 
1.7.8.13.g74677

Re: Breakage (?) in configure and git_vsnprintf()

[Jonathan Nieder]

Jeff King wrote:

> I'll leave the issue of "-std=c89" triggering SNPRINTF_RETURNS_BOGUS to
> people who know and care about autoconf. My gut is to say "don't do
> that". Git is not actually pure c89. We typically target systems that
> are _at least_ c89, but it's more important to match and run well on
> real-world systems than what was defined in the standard. So we don't
> depend on c99, but we do depend on quirks and features that were
> prominent in mid-90's Unix variants.

Using vsnprintf isn't even wrong from the standards-pedant point of
view --- vsnprintf has been in POSIX for a long time.  The problem is
that the configure script is not setting appropriate feature test
macros such as _XOPEN_SOURCE (like git-compat-util.h does) during its
feature tests.

Maybe it would be possible to hook the appropriate magic into
AC_LANG_PROGRAM.

Re: Breakage (?) in configure and git_vsnprintf()

[Brandon Casey]

FYI: all tests passed on IRIX (not that I suspected otherwise).

-Brandon

On 12/12/2011 08:25 AM, Jeff King wrote:
> On Mon, Dec 12, 2011 at 10:30:27AM +0100, Andreas Schwab wrote:
> 
>> Jeff King <peff@peff.net> writes:
>>
>>>  	if (maxsize > 0) {
>>> -		ret = vsnprintf(str, maxsize-SNPRINTF_SIZE_CORR, format, ap);
>>> +		va_copy(cp, ap);
>>> +		ret = vsnprintf(str, maxsize-SNPRINTF_SIZE_CORR, format, cp);
>>
>> You also need to call va_end on the copy.
> 
> Silly me. Thanks for catching.
> 
> Junio, here's a corrected version.
> 
> -- >8 --
> Subject: [PATCH] compat/snprintf: don't look at va_list twice
> 
> If you define SNPRINTF_RETURNS_BOGUS, we use a special
> git_vsnprintf wrapper assumes that vsnprintf returns "-1"
> instead of the number of characters that you would need to
> store the result.
> 
> To do this, it invokes vsnprintf multiple times, growing a
> heap buffer until we have enough space to hold the result.
> However, this means we evaluate the va_list parameter
> multiple times, which is generally a bad thing (it may be
> modified by calls to vsnprintf, yielding undefined
> behavior).
> 
> Instead, we must va_copy it and hand the copy to vsnprintf,
> so we always have a pristine va_list.
> 
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  compat/snprintf.c |    9 +++++++--
>  1 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/compat/snprintf.c b/compat/snprintf.c
> index e1e0e75..42ea1ac 100644
> --- a/compat/snprintf.c
> +++ b/compat/snprintf.c
> @@ -19,11 +19,14 @@
>  #undef vsnprintf
>  int git_vsnprintf(char *str, size_t maxsize, const char *format, va_list ap)
>  {
> +	va_list cp;
>  	char *s;
>  	int ret = -1;
>  
>  	if (maxsize > 0) {
> -		ret = vsnprintf(str, maxsize-SNPRINTF_SIZE_CORR, format, ap);
> +		va_copy(cp, ap);
> +		ret = vsnprintf(str, maxsize-SNPRINTF_SIZE_CORR, format, cp);
> +		va_end(cp);
>  		if (ret == maxsize-1)
>  			ret = -1;
>  		/* Windows does not NUL-terminate if result fills buffer */
> @@ -42,7 +45,9 @@ int git_vsnprintf(char *str, size_t maxsize, const char *format, va_list ap)
>  		if (! str)
>  			break;
>  		s = str;
> -		ret = vsnprintf(str, maxsize-SNPRINTF_SIZE_CORR, format, ap);
> +		va_copy(cp, ap);
> +		ret = vsnprintf(str, maxsize-SNPRINTF_SIZE_CORR, format, cp);
> +		va_end(cp);
>  		if (ret == maxsize-1)
>  			ret = -1;
>  	}