[test failure] t4114 binary file becomes symlink

Hi,

Running 'sh t4114-apply-typechange.sh --verbose --debug' fails since its introduction by b67b9612e1a90ae093445abeaeff930e9f4cf936 with this output:

   * expecting success: 
   	git checkout -f foo-becomes-binary &&
   	git diff-tree -p --binary HEAD foo-symlinked-to-bar > patch &&
   	git apply --index < patch
   	
   ./test-lib.sh: line 234: 26816 Segmentation fault      git checkout -f
   foo-becomes-binary
   * FAIL 8: binary file becomes symlink
   	
   		git checkout -f foo-becomes-binary &&
   		git diff-tree -p --binary HEAD foo-symlinked-to-bar > patch &&
   		git apply --index < patch
   		
   
   diff --git a/foo b/foo
   deleted file mode 120000
   index ba0e162..0000000
   --- a/foo
   +++ /dev/null
   @@ -1 +0,0 @@
   -bar
   \ No newline at end of file
   diff --git a/foo b/foo
   new file mode 100644
   index 0000000..ab26de5
   --- /dev/null
   +++ b/foo
   @@ -0,0 +1 @@
   +how far is the sun?

The filesystem used is ext3 (2.6.26-gentoo-r4-v7).

I guess it's not really expected. So, I'll start my own research but I don't know this code.

I can provide more tests if needed.

On Sat, Jul 18, 2009 at 03:45:51PM +0200, Nicolas Sebrecht wrote:

Sorry, I can't reproduce here (I tried v1.6.3 and the current 'next'). The tests pass just fine with --debug (which, IIRC, doesn't actually do much). What is the exact commit you're seeing it fail on?

Can you try running it under gdb to get a stack trace? If you have valgrind installed, can you run the test script with --valgrind?

-Peff

The 18/07/09, Jeff King wrote:

It fails on:
  - next
  - v1.6.3
  - b67b9612e1a90ae093445abeaeff930e9f4cf936
  - (other I don't remember, but does it really matter?)

$ sh t4114-apply-typechange.sh --valgrind

<snip>

* expecting success: 
	git checkout -f foo-becomes-binary &&
	git diff-tree -p --binary HEAD foo-symlinked-to-bar > patch &&
	git apply --index < patch
	
==10807== Invalid read of size 1
==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)
==10807==    by 0x412AA0: cmd_checkout (builtin-checkout.c:364)
==10807==    by 0x404222: handle_internal_command (git.c:243)
==10807==    by 0x404466: main (git.c:483)
==10807==  Address 0x1 is not stack'd, malloc'd or (recently) free'd
{
   <insert a suppression name here>
   Memcheck:Addr1
   fun:strlen
   fun:vfprintf
   fun:vsnprintf
   fun:git_vsnprintf
   fun:strbuf_addf
   fun:cmd_checkout
   fun:handle_internal_command
   fun:main
}
==10807== 
==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)
==10807==    by 0x412AA0: cmd_checkout (builtin-checkout.c:364)
==10807==    by 0x404222: handle_internal_command (git.c:243)
==10807==    by 0x404466: main (git.c:483)
==10807==  If you believe this happened as a result of a stack overflow in your
==10807==  program's main thread (unlikely but possible), you can try to increase
==10807==  the size of the main thread stack using the --main-stacksize= flag.
==10807==  The main thread stack size used in this run was 8388608.
* FAIL 8: binary file becomes symlink

<snip>

$

On Sat, Jul 18, 2009 at 04:16:58PM +0200, Nicolas Sebrecht wrote:

Hmm. So it is clearly reproducible on your system, but not on mine. I wonder what the difference could be.

Are you compiling with any special options? I usually compile with just "-g -Wall -Werror", but I also tried with "-O2" and couldn't reproduce. Maybe compiler version? I'm using gcc 4.3.3.

Looking at that strbuf_addf call, we presumably have a bogus pointer either in old->name or new->name. Which is odd, since reading the code, both get memset() to zero, and then assigned from something which should be sane.

At this point, I would try either running it under gdb or putting in some debugging printfs into update_refs_for_switch to try to isolate where the bogus value is coming from (valgrind sees it as cmd_checkout, but presumably that is because it inlines the static update_refs_for_switch).

Can you try that? Otherwise, I'm not sure how to proceed because I can't reproduce it on my box.

-Peff

Jeff King <peff@peff.net> writes:

Me neither, but I found an unrelated anomaly in the output from the test nearby.

    Switched to branch 'foo-baz-renamed-from-foo'
    *   ok 3: file renamed from foo/baz to foo

    * expecting success:
            git checkout -f foo-becomes-a-directory &&
            git diff-tree -p HEAD initial > patch &&
            git apply --index < patch

    error: Invalid path ''
    Switched to branch 'foo-becomes-a-directory'
    *   ok 4: directory becomes file

Notice the "error"?

This is coming from the oneway_merge() codepath in unpack-trees.c.

When we switch branches with "checkout -f", unpack_trees() feeds two cache_entries to oneway_merge() function in its src[] array argument. The zeroth entry comes from the current index, and the first entry represents what the merge result should be, taken from the tree recorded in the commit we are switching to.

When we have a blob (either regular file or a symlink) in the index and in the work tree at path "foo", and the switched-to tree has "foo/bar", i.e. "foo" becomes a directory, src[0] is obviously that blob currently registered at "foo". Even though we do not have anything at "foo" in the switched-to tree, src[1] is _not_ NULL.

The unpack_trees() machinery places a special marker df_conflict_entry to signal that no blob exists at "foo", but it will become a directory that may have somthing underneath it, namely "foo/bar".

Passing that df_conflict_entry marker to merged_entry() happens to remove the "foo" in the end because the df_conflict_entry does not have any name (hence the "error" message) and its addition in add_index_entry() is rejected, but it is wrong.

 unpack-trees.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

The 18/07/09, Junio C Hamano wrote:

Normally, no (cf. my mail to Johannes Sixt).

The Makefile have

  215:CFLAGS = -g -O2 -Wall
  217:ALL_CFLAGS = $(CFLAGS)

and it should be OK. I know that -Werror will fail on

  libgit.a(mkdtemp.o): In function `gitmkdtemp':
    /home/nicolas/dev/official_packages/git/compat/mkdtemp.c:5: warning: the
    use of `mktemp' is dangerous, better use `mkstemp'

but I think this is an unrelated problem.

$ gcc --version gcc (GCC) 4.1.2 (Gentoo 4.1.2 p1.3)

I couldn't reproduce this error here with

  sh t4114-apply-typechange.sh --verbose
and
  sh t4114-apply-typechange.sh --verbose --debug

on the current next.

With this patch, I still fail on the test.

On Sat, 18 Jul 2009, Junio C Hamano wrote:

Ack. This looks like a really old bug.

That whole 'df_conflict_entry' always confused me.

		Linus

On Samstag, 18. Juli 2009, Nicolas Sebrecht wrote:

amd64-linux, and you build with SNPRINTF_RETURNS_BOGUS? Why do you have this option set?

-- Hannes

The 18/07/09, Johannes Sixt wrote:

I don't tune any flags myself and

  $ printf "$CC\n $CFLAGS\n $LDFLAGS\n $LIBS\n $CPPFLAGS\n $CPP\n"

is empty. Is there another way to accidentally set a flag other than environment variables or options to 'make'?

I've tried

  make
  make install

and

  ./configure --prefix=/home/nicolas/bin
  make
  make install

I don't know if it changes anything but both failed with the same error.

The 18/07/09, Nicolas Sebrecht wrote:

Hum, 'rm configure ; make configure ; ./configure' give

 checking whether snprintf() and/or vsnprintf() return bogus value... yes

WTF?

Nicolas Sebrecht <nicolas.s.dev@gmx.fr> writes:
 

It looks like some recent bug in configure.ac, as I have run ./configure without "make configure" and it had

  NO_LIBGEN_H=@NO_LIBGEN_H@
  NEEDS_RESOLV=@NEEDS_RESOLV@

  SNPRINTF_RETURNS_BOGUS=

and when I did "rm configure ; make configure ; ./configure" it gave me

  NO_LIBGEN_H=
  NEEDS_RESOLV=

  SNPRINTF_RETURNS_BOGUS=UnfortunatelyYes

I have tried to find which commit introduced this regression.

 $ git bisect start origin v1.6.3 v1.6.3.2 -- configure.ac config.mak.in
 $ git bisect run ~/git/test.sh

finds ecc395c (Makefile: add NEEDS_LIBGEN to optionally add -lgen to compile arguments, 2009-07-10) as a first bad commit. But I don't see how it could have changed it... Strange...

CC-ed Brandon Casey, author of blamed changeset, and David Syzdek who offered at some time help with maintaining autoconf.

P.S. Perhaps it is time for creating MAINTAINERS file for git?

-- >8 -- #!/bin/bash

rm -f configure && make configure && ./configure -q && grep -q "^SNPRINTF_RETURNS_BOGUS=$" config.mak.autogen

# end of test.sh

Thank you, I did the same investigation here. :-)

AC_CHECK_LIB may fail to check the good libraries. Use AC_SEARCH_LIBS instead as pointed out by the autotool documentation.

http://www.gnu.org/software/autoconf/manual/html_node/Libraries.html#Libraries

Signed-off-by: Nicolas Sebrecht <nicolas.s.dev@gmx.fr>
---

 configure.ac |   16 ++++++++--------
 1 files changed, 8 insertions(+), 8 deletions(-)

The 19/07/09, Nicolas Sebrecht wrote:

The wrong check of lib gen added a flag ' -lgen' to $LIBS (in configure). This wrong flag then gave:

 [...]/bin/ld: cannot find -lgen collect2: ld returned 1 exit status

which made wrongly fail the next compilation test (using $LIBS).

I wonder if we could have some feedbacks on this patch. The change on the whole file may make more bad than good. Works as expected here on Linux (glibc 2.9).

Otherwise, the following lines are sufficient to correct the original error:

Nicolas Sebrecht <nicolas.s.dev@gmx.fr> writes:

Thanks.

The autoconf documentation does not give much confidence either way:

	AC_CHECK_LIB requires some care in usage, and should be avoided in
	some common cases.

It is unclear what these "some" are, and is sufficiently unclear for us to decide if our situation falls into these "some common cases." The only problem it cites is that alternative libraries may contain a variant implementation of the same function found in a more common library, in which case you may not want to use such an alternative implementation, and I do not think "basename() in -lgen" falls into that category.

It does say

	These days it is normally better to use AC_SEARCH_LIBS(...)
	instead of AC_CHECK_LIB(...).

but without any further justification, which does not help building confidence in the suggestion.

It is very tempting to revert the configure.ac part of commit ecc395c especially in an -rc freeze, but if some autoconf experts can help us decide by clarifying the situation, and explain why AC_SEARCH_LIBS() is indeed the better way these days (which I suspect it is), I'd say we can apply your patch to favor AC_SEARCH_LIBS() over AC_CHECK_LIB() with more confidence.

If autoconf people say AC_SEARCH_LIBS() should be preferred over AC_CHECK_LIB(), it probably is the right thing to do. But I want to have a warmer-and-fuzzier feeling that we understand why it went wrong, rather than blindly doing what the documentation says because the documentation tells us to.

Junio C Hamano <gitster <at> pobox.com> writes:

Please report that as a bug to the autoconf lists, then.

As autoconf maintainer (and hoping to release autoconf 2.64 soon), yes, I can definitively say that AC_SEARCH_LIBS is nicer than AC_CHECK_LIB.

After actually reading the autoconf documentation, don't some of the tests have the [action-if-found] and [action-if-not-found] actions backwards?

AC_CHECK_LIB has the form:

   AC_CHECK_LIB (library, function, [action-if-found], [action-if-not-found], [other-libraries])

The test I added (which I blindly copied from the NEEDS_RESOLV test) looks like:

   AC_CHECK_LIB([gen], [basename], [NEEDS_LIBGEN=], [NEEDS_LIBGEN=YesPlease])
   AC_SUBST(NEEDS_LIBGEN)
   test -n "$NEEDS_LIBGEN" && LIBS="$LIBS -lgen"

Won't this check whether a program which calls basename() successfully links with -lgen? If it successfully links, then it will perform NEED_LIBGEN= and if not it will set NEEDS_LIBGEN=YesPlease, right? Isn't that the opposite of what should be done?

If that is correct, then the NEEDS_RESOLV and NEEDS_LIBGEN tests are wrong and they may still be wrong even if AC_SEARCH_LIBS is used instead of AC_CHECK_LIB.

A question about AC_SEARCH_LIBS:

With AC_SEARCH_LIBS, which of [action-if-found] or [action-if-not-found] is executed if the function is found in the standard c library i.e. "calling `AC_LINK_IFELSE([AC_LANG_CALL([], [function])])' first with no libraries"? Is the answer neither? If the answer is [action-if-found], won't the NEEDS_LIBGEN=YesPlease be set when the function is found in the c library? Assuming the NEEDS_LIBGEN test is made to look like this:

   AC_SEARCH_LIBS([basename], [gen], [NEEDS_LIBGEN=YesPlease], [NEEDS_LIBGEN=])

Depending on the answer to that question, we either will want to use AC_SEARCH_LIBS, or stick with AC_CHECK_LIB but correct the [action] fields, or maybe even stick with AC_CHECK_LIB but rework the NEEDS_RESOLV and NEEDS_LIBGEN tests to look like the NEEDS_SOCKET test.

-brandon

Brandon Casey wrote:

btw, the "`AC_LINK_IFELSE(..." line is from the autoconf documentation. It is the first thing that AC_SEARCH_LIBS does. It tries to link without any of the specified libraries first, then it iterates trying to link using each specified library, stopping when it is successful.

If you didn't recognize that that line was from the documentation, it may be confusing.

-brandon

It evaluates the action-if-found and adds nothing to LIBS. Instead, if it is found in a library, it evaluates the action-if-found after adding (actually prepending) -lBLAH to LIBS.

Paolo

Paolo Bonzini wrote:

That's what I suspected. It means we can't have NEEDS_SOMETHING=true in the action-if-found parameter when using AC_SEARCH_LIBS since that may cause the Makefile to append a library requirement when none is necessary.

-brandon

From: Brandon Casey <drafnel@gmail.com>

The "action" parameters for these two tests were supplied incorrectly for the way the tests were implemented. The tests check whether a program which calls hstrerror() or basename() successfully links when -lresolv or -lgen are used, respectively. A successful linking would result in NEEDS_RESOLV or NEEDS_LIBGEN being unset, and failure would result in setting the respective variable.

Aside from that issue, the tests did not handle the case where neither library was necessary for accessing the functions in question. So solve both of these issues by re-working the two tests so that their form is like the NEEDS_SOCKET test which attempts to link with just the c library, and if it fails then assumes that the additional library is necessary and sets the appropriate variable.

Signed-off-by: Brandon Casey <drafnel@gmail.com>
---

Maybe this is the appropriate thing to do?

-brandon

 configure.ac |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

Brandon Casey <casey@nrlssc.navy.mil> writes:

Thanks.

Instead of saying that "hstrerror not in -lc means we do have -lresolv and the function will be found there" blindly, we may want to have a nested check.

	AC_CHECK_LIB([c], [hstrerror], [NEEDS_RESOLV=],
       	    AC_CHECK_LIB([resolv], [hstrerror], [NEEDS_RESOLV=YesPlease]))

But we do not have any provision for the case where -lc does not have it and -lresolv does not have it either (or -lresolv does not exist) anyway, so we might as well go with your patch.

I take it that swapping [if-found][if-not-found] parameters is what the autoconf documentation warns against? That is, both -lc and -lresolv may have it but -lresolv one may be a specialized one you would not normally want.

Junio C Hamano wrote:

Yeah, I understood how the NEEDS_SSL_WITH_CRYPTO test worked like this, but I didn't make the tests for NEEDS_RESOLV and NEEDS_LIBGEN any more complex for the same reason you state: we don't have any other provision...

That's what I assume, and that's probably why AC_SEARCH_LIBS was invented which attempts to link without using any of the specified libraries first. It just doesn't seem to fit our needs in this case.

-brandon

From: Brandon Casey <drafnel@gmail.com>

An entry in the config.mak.in file is necessary for the NEEDS_LIBGEN variable to appear in the config.mak.autogen file with the value assigned by the configure script.

Signed-off-by: Brandon Casey <drafnel@gmail.com>
---

Junio,

You probably want to apply or squash this somewhere.

As I noted in the original patch, the autoconf part was untested. I actually did some light testing on this one. I created the configure file on linux and ran it on Solaris and IRIX. Both produce an error which looks something like:

   configure[1234]: syntax error at line 4806 : `;' unexpected

And line 4806 looks like:

   for ac_lib in ; do
      ...

It works with bash though, and it works with /bin/sh on Solaris 10. On Solaris 10, the configure script correctly detects that hstrerror cannot be used without -lresolv, and basename can be used without -lgen. In config.mak.autogen, NEEDS_RESOLV is set to 'YesPlease' and NEEDS_LIBGEN is unset. On my Solaris 7, bash is not available, but the informational messages indicate the same results as for Solaris 10. On IRIX, hstrerror() can be used without -lresolv and basename cannot be used without -lgen. In config.mak.autogen, NEEDS_RESOLV is unset, and NEEDS_LIBGEN is set to 'YesPlease'.

So, I think this patch should be the final one.

-brandon

 config.mak.in |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

Brandon Casey <casey@nrlssc.navy.mil> writes:

Thanks.

Junio,

I see you squashed this and applied it to master (a1142892), but since my email was not clear, the commit message is not accurate (in an unimportant way).

You said

    Without [the addition of NEEDS_LIBGEN to config.mak.in], the
    generated shell script would contain a snippet like this:

       for ac_lib in ; do
         ...

    which is incorrect.

Actually, the "for ac_lib in ; do" snippet is produced with or without my patch. I didn't mean to imply that there was a change in that respect. It's just that that snippet is what prevents the configure script from completing successfully on IRIX and Solaris 7 when executing it with /bin/sh or /bin/ksh. When bash is used, the configure script can execute successfully, and then it can be verified that the patched configure.ac produces a configure script that operates correctly.

Sorry for the confusion.

-brandon

Brandon Casey wrote:

On Samstag, 18. Juli 2009, Nicolas Sebrecht wrote:

If you are on Linux and only want a special prefix, you don't need ./configure; just:

   echo prefix=/home/nicolas/bin > config.mak
   make && make install

-- Hannes

On Sat, Jul 18, 2009 at 09:06:06PM +0200, Johannes Sixt wrote:

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.

---

The 18/07/09, Jeff King wrote:

My investigations made me realize I was building a 64-bits git version in a 32-bits userland (gentoo flag multilib set) which is not the best thing to do. So, another possible fix is to export CFLAGS with '-m32'. Mixing 32 and 64-bits applications is bad. :-)

I confirm this patch does fix the failure for the 64 bits version. Thank you all.

Now, I wonder if it is safe to run a 32-bits git version on repositories built with a 64-bits version. It should be safe but do you think it actually is?

On Sonntag, 19. Juli 2009, Jeff King wrote:

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.

-- Hannes

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

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

On Montag, 20. Juli 2009, Jeff King wrote:

We are sure (well, I am sure ;) that it worked on Windows with gcc 3. It certainly is a reasonable workaround. It remains to confirm that the workaround works as expected on the other systems that use it (IRIX, HP/UX). 'git branch -v' is a test that calls the system provided vsnprintf twice (as long as the branch head commits have moderatly long summary lines).

On Windows, however, today everybody who compiles git is most likely using msysgit's gcc 4.4. This version has C99 vsnprintf, and the workaround should not be used anymore, although it does not hurt.

-- Hannes

On Mon, 20 Jul 2009, Johannes Sixt wrote:

Not only that, but it's literally how people used to historically do things.

Sure, some places do require 'va_copy()', and there's a reason why it got added in C99. But there's also a reason why it wasn't added earlier - because it didn't exist!

So I agree with Johannes: we should _not_ make our compat version of 'snprintf()' use va_copy, for the simple reason that

 - modern C library versions will have a working snprintf already

 - old setups that don't have a working snprintf quite likely don't have a 
   va_copy either.

So the set of systems that need both the va_copy _and_ our compat version of snprintf is likely much smaller than the set of systems that would actively break compilation if they don't have va_copy.

Of course, we could also add a new "NEEDS_VA_COPY" thing, and do

	#ifdef NEEDS_VA_COPY
	  #define va_copy(dst,src) (dst) = (src)
	#endif

or something like that.

		Linus