# Breakage (?) in configure and git_vsnprintf() 10 messages from 2011-12-11 to 2011-12-12. Participants: Michael Haggerty, Jeff King, Johannes Sixt, Junio C Hamano, Andreas Schwab, Jonathan Nieder, Brandon Casey. Thread: https://gitlist.dev/t/29133 ## Michael Haggerty, 2011-12-11 18:42 Subject: Breakage (?) in configure and git_vsnprintf() Message-ID: <4EE4F97B.9000202@alum.mit.edu> URL: https://gitlist.dev/e/4EE4F97B.9000202%40alum.mit.edu ``` 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/ ``` ## Jeff King, 2011-12-12 06:43 Subject: Re: Breakage (?) in configure and git_vsnprintf() Message-ID: <20111212064305.GA16511@sigill.intra.peff.net> URL: https://gitlist.dev/e/20111212064305.GA16511%40sigill.intra.peff.net In-Reply-To: <4EE4F97B.9000202@alum.mit.edu> ``` 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 --- 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 ``` ## Johannes Sixt, 2011-12-12 07:45 Subject: Re: Breakage (?) in configure and git_vsnprintf() Message-ID: <4EE5B123.2030708@viscovery.net> URL: https://gitlist.dev/e/4EE5B123.2030708%40viscovery.net In-Reply-To: <20111212064305.GA16511@sigill.intra.peff.net> ``` 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 ``` ## Jeff King, 2011-12-12 08:10 Subject: Re: Breakage (?) in configure and git_vsnprintf() Message-ID: <20111212081019.GA17725@sigill.intra.peff.net> URL: https://gitlist.dev/e/20111212081019.GA17725%40sigill.intra.peff.net In-Reply-To: <4EE5B123.2030708@viscovery.net> ``` 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? ``` ## Junio C Hamano, 2011-12-12 08:23 Subject: Re: Breakage (?) in configure and git_vsnprintf() Message-ID: <7vr50a6vwf.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vr50a6vwf.fsf%40alter.siamese.dyndns.org In-Reply-To: <20111212064305.GA16511@sigill.intra.peff.net> ``` 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. ``` ## Andreas Schwab, 2011-12-12 09:30 Subject: Re: Breakage (?) in configure and git_vsnprintf() Message-ID: URL: https://gitlist.dev/e/m262hmgmrw.fsf%40igel.home In-Reply-To: <20111212064305.GA16511@sigill.intra.peff.net> ``` Jeff King 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." ``` ## Michael Haggerty, 2011-12-12 10:25 Subject: Re: Breakage (?) in configure and git_vsnprintf() Message-ID: <4EE5D694.7040407@alum.mit.edu> URL: https://gitlist.dev/e/4EE5D694.7040407%40alum.mit.edu In-Reply-To: <20111212064305.GA16511@sigill.intra.peff.net> ``` 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/ ``` ## Jeff King, 2011-12-12 14:25 Subject: Re: Breakage (?) in configure and git_vsnprintf() Message-ID: <20111212142550.GA23875@sigill.intra.peff.net> URL: https://gitlist.dev/e/20111212142550.GA23875%40sigill.intra.peff.net In-Reply-To: ``` On Mon, Dec 12, 2011 at 10:30:27AM +0100, Andreas Schwab wrote: > Jeff King 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 --- 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 ``` ## Jonathan Nieder, 2011-12-12 21:56 Subject: Re: Breakage (?) in configure and git_vsnprintf() Message-ID: <20111212215645.GA3024@elie.hsd1.il.comcast.net> URL: https://gitlist.dev/e/20111212215645.GA3024%40elie.hsd1.il.comcast.net In-Reply-To: <20111212064305.GA16511@sigill.intra.peff.net> ``` 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. ``` ## Brandon Casey, 2011-12-12 23:25 Subject: Re: Breakage (?) in configure and git_vsnprintf() Message-ID: URL: https://gitlist.dev/e/KDO2EaM6TnHQ6KGYqmkErS6Nq_qZc7b4J1aClSFkQwb60rk3bMZqU3iiY0O2LRaJukqzzBU6djUrNHGC505_oYXFc1BUezUo8EtdKcjEOSE%40cipher.nrlssc.navy.mil In-Reply-To: <20111212142550.GA23875@sigill.intra.peff.net> ``` 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 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 > --- > 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; > } ```