Re: [test failure] Re: t4114 binary file becomes symlink

From: Jeff King <peff@peff.net>
Date: Jul 20, 2009, 09:09 UTC
Message-ID: <20090720090937.GA20412@sigill.intra.peff.net>
In-Reply-To: <200907191301.15533.j6t@kdbg.org>

On Sun, Jul 19, 2009 at 01:01:15PM +0200, Johannes Sixt wrote:

Show 9 quoted lines

> Problem is, snprintf was made for very old systems, which typically do
> not have va_copy. (E.g. Windows, but there the situation *might* have
> changed with the switch to gcc 4.)
> 
> The rationale not to use va_copy is that this function is to be used
> *only* if necessary, i.e. portability is already lacking, and if it
> can be verified that this function works as is. Portability and
> correct-by-the-law C code are *not* a goal here. If the function does
> not work as is, don't use it.

OK, I guess I can buy the "don't use this unless you need it" rationale. But two questions:

  1. _Are_ we sure it works under Windows?  That is, do we know for a
     fact that using a va_list twice is OK there, or are we just going
     on the fact that nobody has reported the bug?

     If we're not sure, then you might want to try running the recipe
     below which consistently produces a segfault for me on Linux amd64
     (but not i386, which seems to use a different representation for
     va_lists).

  2. In this case, using SNPRINTF_RETURNS_BOGUS was a mistake. But
     unfortunately using it erroneously doesn't simply cause the
     compilation to barf, or to use a slower implementation; instead it
     introduces a very subtle and hard to diagnose bug on some
     platforms. Is there anything simple we can do to protect people
     from that?

     I can't really think of anything simple, because such a mechanism
     would basically involve compiling a test program and seeing if it
     segfaults.

Anyway, bug-reproducing recipe is below.

-- >8 --
cat <<'EOF' >test-vsnprintf.c
#define SNPRINTF_RETURNS_BOGUS
#include "git-compat-util.h"
int main() {
        char buf[16];
        /* this 8 may need to be tweaked depending on
         * the system's vsnprintf return value; the goal
         * is to get git_vsnprintf to have to look at
         * it's va_list twice */
        git_snprintf(buf, 8, "%s %s", "foo", "bar");
        return 0;
}
EOF
make test-vsnprintf.o compat/snprintf.o
gcc -o test-vsnprintf test-vsnprintf.o compat/snprintf.o
./test-vsnprintf ;# or valgrind test-vsnprintf