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

[Jeff King <peff@peff.net>]

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