On Sat, Jul 18, 2009 at 09:06:06PM +0200, Johannes Sixt wrote:
> On Samstag, 18. Juli 2009, Nicolas Sebrecht wrote:
> > ==10807== Process terminating with default action of signal 11 (SIGSEGV)
> > ==10807== Access not within mapped region at address 0x1
> > ==10807== at 0x4C22349: strlen (in
> > /usr/lib64/valgrind/amd64-linux/vgpreload_memcheck.so) ==10807== by
> > 0x5616ED6: vfprintf (in /lib64/libc-2.8.so)
> > ==10807== by 0x563C159: vsnprintf (in /lib64/libc-2.8.so)
> > ==10807== by 0x495E90: git_vsnprintf (snprintf.c:38)
> > ==10807== by 0x48917B: strbuf_addf (strbuf.c:203)
>
> amd64-linux, and you build with SNPRINTF_RETURNS_BOGUS? Why do you have this
> option set?
Ah, that's what I was missing. I can reproduce it by setting
SNPRINTF_RETURNS_BOGUS. I think the problem is in the git_vsnprintf
code, and it just by coincidence triggers in this test because of the
exact string we are trying to format.
Look at compat/snprintf.c. In git_vsnprintf, we are passed a "va_list
ap", which we then repeatedly call vsnprintf on, checking the return to
make sure we have enough space. But using a va_list repeatedly without a
va_end and va_start in the middle invokes undefined behavior. So we need
to va_copy it and use the copy.
A patch is below, which fixes the problem for me. However, va_copy is
C99, so we would generally try to avoid it. But I don't think there is a
portable way of writing this function without it. And most systems
shouldn't need to use our snprintf at all, so maybe it is portable
enough. I dunno.
---
diff --git a/compat/snprintf.c b/compat/snprintf.c
index 6c0fb05..e862db6 100644
--- a/compat/snprintf.c
+++ b/compat/snprintf.c
@@ -18,9 +18,12 @@ int git_vsnprintf(char *str, size_t maxsize, const char *format, va_list ap)
{
char *s;
int ret = -1;
+ va_list cp;
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 */
@@ -39,7 +42,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;
}
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html