Git crashes on pull

Git crashes on pull

[Guido Ostkamp]

Hi,

I have a clone of http://git.postgresql.org/git/postgresql.git where head 
is at commit 167501570c74390dfb7a5dd71e260ab3d4fd9904.

I'm using Git version 1.6.5.rc1.10.g20f34 (should be at commit 
20f34902d154f390ebaa7eed7f42ad14140b8acb from Mon Sep 14 10:49:01 2009 
+0200)

Now when I 'git pull' then Git crashes with

git pull 2>&1 > /tmp/git-error
*** glibc detected *** git-remote-curl: free(): invalid pointer: 
0xb7d19140 ***
======= Backtrace: =========
/lib/libc.so.6[0xb7c4f4b6]
/lib/libc.so.6(cfree+0x89)[0xb7c51179]
git-remote-curl[0x804d290]
git-remote-curl[0x804df04]
git-remote-curl[0x8065ea5]
git-remote-curl[0x804aac6]
/lib/libc.so.6(__libc_start_main+0xe0)[0xb7bfefe0]
git-remote-curl[0x804a991]
======= Memory map: ========
08048000-080a1000 r-xp 00000000 08:15 1658246 
/usr/local/libexec/git-core/git-remote-curl
080a1000-080a2000 r--p 00058000 08:15 1658246 
/usr/local/libexec/git-core/git-remote-curl
080a2000-080a3000 rw-p 00059000 08:15 1658246 
/usr/local/libexec/git-core/git-remote-curl
080a3000-08143000 rw-p 080a3000 00:00 0          [heap]
b4400000-b4421000 rw-p b4400000 00:00 0
b4421000-b4500000 ---p b4421000 00:00 0
b45ea000-b45f4000 r-xp 00000000 08:13 1097821    /lib/libgcc_s.so.1
b45f4000-b45f6000 rw-p 00009000 08:13 1097821    /lib/libgcc_s.so.1
...

Any idea what's causing this?

Please keep me on CC, as I'm not subscribed on list.

Regards

Guido

Re: Git crashes on pull

[Junio C Hamano]

Guido Ostkamp <git@ostkamp.fastmail.fm> writes:

> I have a clone of http://git.postgresql.org/git/postgresql.git where
> head is at commit 167501570c74390dfb7a5dd71e260ab3d4fd9904.
>
> I'm using Git version 1.6.5.rc1.10.g20f34 (should be at commit
> 20f34902d154f390ebaa7eed7f42ad14140b8acb from Mon Sep 14 10:49:01 2009
> +0200)
>
> Now when I 'git pull' then Git crashes with
>
> git pull 2>&1 > /tmp/git-error
> *** glibc detected *** git-remote-curl: free(): invalid pointer:

Please try this patch, which I have been preparing for later pushout.

From: Junio C Hamano <gitster@pobox.com>
Date: Mon, 14 Sep 2009 14:48:15 -0700
Subject: [PATCH] http.c: avoid freeing an uninitialized pointer

An earlier 59b8d38 (http.c: remove verification of remote packs) left
the variable "url" uninitialized; "goto cleanup" codepath can free it
which is not very nice.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 http.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/http.c b/http.c
index d0cc1b3..15926d8 100644
--- a/http.c
+++ b/http.c
@@ -866,7 +866,7 @@ static int fetch_pack_index(unsigned char *sha1, const char *base_url)
 	int ret = 0;
 	char *hex = xstrdup(sha1_to_hex(sha1));
 	char *filename;
-	char *url;
+	char *url = NULL;
 	struct strbuf buf = STRBUF_INIT;
 
 	if (has_pack_index(sha1)) {
-- 
1.6.5.rc1

Re: Git crashes on pull

[Guido Ostkamp]

On Tue, 15 Sep 2009, Junio C Hamano wrote:

> Please try this patch, which I have been preparing for later pushout.
>
> From: Junio C Hamano <gitster@pobox.com>
> Date: Mon, 14 Sep 2009 14:48:15 -0700
> Subject: [PATCH] http.c: avoid freeing an uninitialized pointer
>
> An earlier 59b8d38 (http.c: remove verification of remote packs) left
> the variable "url" uninitialized; "goto cleanup" codepath can free it
> which is not very nice.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Appears to be working ok now, thanks.

BTW: Is there any way to easily invoke GDB in case of such a problem to 
get a real symbolic stack backtrace?

I tried it on the 'git' binary, but of course this didn't work because it 
invokes a git-pull script which again runs another git-remote-curl binary.

Regards

Guido

Re: Git crashes on pull

[Junio C Hamano]

Guido Ostkamp <git@ostkamp.fastmail.fm> writes:

> On Tue, 15 Sep 2009, Junio C Hamano wrote:
>
>> Please try this patch, which I have been preparing for later pushout.
>>
>> From: Junio C Hamano <gitster@pobox.com>
>> Date: Mon, 14 Sep 2009 14:48:15 -0700
>> Subject: [PATCH] http.c: avoid freeing an uninitialized pointer
>>
>> An earlier 59b8d38 (http.c: remove verification of remote packs) left
>> the variable "url" uninitialized; "goto cleanup" codepath can free it
>> which is not very nice.
>>
>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>
> Appears to be working ok now, thanks.

Thanks.

The sad part of the story was that this regression was introduced by a
change to work around recent breakage observed when fetching from the http
server github runs, and it was the primary purpose of pushing 1.6.4.3 out.

Now we need to cut a 1.6.4.4 with this fix-on-fix soon, like tomorrow.

> BTW: Is there any way to easily invoke GDB in case of such a problem
> to get a real symbolic stack backtrace?
>
> I tried it on the 'git' binary, but of course this didn't work because
> it invokes a git-pull script which again runs another git-remote-curl
> binary.

Not very easily.  The best you can do is to run with GIT_TRACE to see what
command actually dies and run that binary directly.  gdb can choose to
follow either parent or child across forks, but I do not know how to tell
it to follow across execs into a different binary.

Re: Git crashes on pull

[Michael Wookey]

2009/9/16 Junio C Hamano <gitster@pobox.com>:
> Guido Ostkamp <git@ostkamp.fastmail.fm> writes:
>
>> On Tue, 15 Sep 2009, Junio C Hamano wrote:
>>
>>> Please try this patch, which I have been preparing for later pushout.
>>>
>>> From: Junio C Hamano <gitster@pobox.com>
>>> Date: Mon, 14 Sep 2009 14:48:15 -0700
>>> Subject: [PATCH] http.c: avoid freeing an uninitialized pointer
>>>
>>> An earlier 59b8d38 (http.c: remove verification of remote packs) left
>>> the variable "url" uninitialized; "goto cleanup" codepath can free it
>>> which is not very nice.
>>>
>>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>>
>> Appears to be working ok now, thanks.
>
> Thanks.
>
> The sad part of the story was that this regression was introduced by a
> change to work around recent breakage observed when fetching from the http
> server github runs, and it was the primary purpose of pushing 1.6.4.3 out.

If only I had given it a run with the clang static analyzer earlier :(

Here is what Xcode would have shown -

    http://dl.getdropbox.com/u/1006983/git-clang.png

I can make the Xcode project available if anyone is interested.

Re: Git crashes on pull

[Tay Ray Chuan]

Hi,

On Wed, Sep 16, 2009 at 6:54 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Thanks.
>
> The sad part of the story was that this regression was introduced by a
> change to work around recent breakage observed when fetching from the http
> server github runs, and it was the primary purpose of pushing 1.6.4.3 out.
>
> Now we need to cut a 1.6.4.4 with this fix-on-fix soon, like tomorrow.

sorry for all the trouble caused.

Junio, do you think moving out the free() would be a better option? Setting it to NULL just so we can free() is rather contrived, I feel.

-- >8 --

Subject: [PATCH] http.c: move free() out of cleanup block

Instead of initializing a variable (url) just so we can do a free() on
it, as in b202514 (http.c: avoid freeing an uninitialized pointer), we
move the free() out of cleanup block.

Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
---
 http.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/http.c b/http.c
index 23b2a19..a67f62e 100644
--- a/http.c
+++ b/http.c
@@ -866,7 +866,7 @@ static int fetch_pack_index(unsigned char *sha1, const char *base_url)
 	int ret = 0;
 	char *hex = xstrdup(sha1_to_hex(sha1));
 	char *filename;
-	char *url = NULL;
+	char *url;
 	struct strbuf buf = STRBUF_INIT;

 	if (has_pack_index(sha1)) {
@@ -885,9 +885,9 @@ static int fetch_pack_index(unsigned char *sha1, const char *base_url)
 	if (http_get_file(url, filename, 0) != HTTP_OK)
 		ret = error("Unable to get pack index %s\n", url);

+	free(url);
 cleanup:
 	free(hex);
-	free(url);
 	return ret;
 }

--
1.6.4.2