threads / discuss / 27982

Possibly solved invalid free() in git-remote-http from Git 1.7.2.1

Subject: Possibly solved invalid free() in git-remote-http from Git 1.7.2.1

## tl;dr

7 messages between Aug 1, 2011 and Aug 3, 2011.

replies: 6people: 3as markdown or json

Ævar Arnfjörð Bjarmason· Aug 1, 2011, 15:22 UTC · lore

In case anyone would like to look into this I had this undiagnosed free() error from git-remote-http in git version 1.7.2.1:

    error: Couldn't create temporary file
/tmp/clone/main.git/.git/objects/e3/2a06e85053d958f78a20f37c9604e40b8282ad.temp:
No such file or directory
    *** glibc detected *** git-remote-http: free(): invalid pointer:
0x00000000006848c0 ***
    ======= Backtrace: =========
    /lib64/libc.so.6[0x3c9b6722ef]
    /lib64/libc.so.6(cfree+0x4b)[0x3c9b67273b]
    git-remote-http[0x406e2d]
    git-remote-http[0x407671]
    git-remote-http[0x407abe]
    git-remote-http[0x429044]
    git-remote-http[0x404a9a]
    /lib64/libc.so.6(__libc_start_main+0xf4)[0x3c9b61d994]
    git-remote-http(fwrite+0x129)[0x4034e9]
    ======= Memory map: ========
    00400000-00474000 r-xp 00000000 fd:00 12260242
      /usr/libexec/git-core/git-remote-http
    00674000-00676000 rw-p 00074000 fd:00 12260242
      /usr/libexec/git-core/git-remote-http
    00676000-00696000 rw-p 00676000 00:00 0
    00875000-00877000 rw-p 00075000 fd:00 12260242
      /usr/libexec/git-core/git-remote-http
    021e9000-027a2000 rw-p 021e9000 00:00 0
      [heap]
    369c200000-369c215000 r-xp 00000000 fd:00 6193169
      /lib64/libselinux.so.1
    369c215000-369c415000 ---p 00015000 fd:00 6193169
      /lib64/libselinux.so.1
    369c415000-369c417000 rw-p 00015000 fd:00 6193169
      /lib64/libselinux.so.1
    369c417000-369c418000 rw-p 369c417000 00:00 0
    369d200000-369d20d000 r-xp 00000000 fd:00 6193154
      /lib64/libgcc_s-4.1.2-20080825.so.1
    369d20d000-369d40d000 ---p 0000d000 fd:00 6193154
      /lib64/libgcc_s-4.1.2-20080825.so.1
    369d40d000-369d40e000 rw-p 0000d000 fd:00 6193154
      /lib64/libgcc_s-4.1.2-20080825.so.1
    369de00000-369de3b000 r-xp 00000000 fd:00 6193167
      /lib64/libsepol.so.1
    369de3b000-369e03b000 ---p 0003b000 fd:00 6193167
      /lib64/libsepol.so.1
    369e03b000-369e03c000 rw-p 0003b000 fd:00 6193167
      /lib64/libsepol.so.1
    369e03c000-369e046000 rw-p 369e03c000 00:00 0
    369e200000-369e211000 r-xp 00000000 fd:00 6193165
      /lib64/libresolv-2.5.so
    369e211000-369e411000 ---p 00011000 fd:00 6193165
      /lib64/libresolv-2.5.so
    369e411000-369e412000 r--p 00011000 fd:00 6193165
      /lib64/libresolv-2.5.so
    369e412000-369e413000 rw-p 00012000 fd:00 6193165
      /lib64/libresolv-2.5.so
    369e413000-369e415000 rw-p 369e413000 00:00 0
    3a17800000-3a1783b000 r-xp 00000000 fd:00 11842620
      /usr/lib64/libcurl.so.3.0.0
    3a1783b000-3a17a3b000 ---p 0003b000 fd:00 11842620
      /usr/lib64/libcurl.so.3.0.0
    3a17a3b000-3a17a3d000 rw-p 0003b000 fd:00 11842620
      /usr/lib64/libcurl.so.3.0.0
    3c46e00000-3c46e31000 r-xp 00000000 fd:00 11840244
      /usr/lib64/libidn.so.11.5.19
    3c46e31000-3c47030000 ---p 00031000 fd:00 11840244
      /usr/lib64/libidn.so.11.5.19
    3c47030000-3c47031000 rw-p 00030000 fd:00 11840244
      /usr/lib64/libidn.so.11.5.19
    3c9b200000-3c9b21c000 r-xp 00000000 fd:00 6193446
      /lib64/ld-2.5.so
    3c9b41b000-3c9b41c000 r--p 0001b000 fd:00 6193446
      /lib64/ld-2.5.so
    3c9b41c000-3c9b41d000 rw-p 0001c000 fd:00 6193446
      /lib64/ld-2.5.so
    3c9b600000-3c9b74d000 r-xp 00000000 fd:00 6193447
      /lib64/libc-2.5.so
    3c9b74d000-3c9b94d000 ---p 0014d000 fd:00 6193447
      /lib64/libc-2.5.so
    3c9b94d000-3c9b951000 r--p 0014d000 fd:00 6193447
      /lib64/libc-2.5.so
    3c9b951000-3c9b952000 rw-p 00151000 fd:00 6193447
      /lib64/libc-2.5.so
    3c9b952000-3c9b957000 rw-p 3c9b952000 00:00 0
    3c9ba00000-3c9ba02000 r-xp 00000000 fd:00 6193448
      /lib64/libdl-2.5.so
    3c9ba02000-3c9bc02000 ---p 00002000 fd:00 6193448
      /lib64/libdl-2.5.so
    3c9bc02000-3c9bc03000 r--p 00002000 fd:00 6193448
      /lib64/libdl-2.5.so
    3c9bc03000-3c9bc04000 rw-p 00003000 fd:00 6193448
      /lib64/libdl-2.5.so
    3c9be00000-3c9be16000 r-xp 00000000 fd:00 6193452
      /lib64/libpthread-2.5.so
    3c9be16000-3c9c015000 ---p 00016000 fd:00 6193452
      /lib64/libpthread-2.5.so
    3c9c015000-3c9c016000 r--p 00015000 fd:00 6193452
      /lib64/libpthread-2.5.so
    3c9c016000-3c9c017000 rw-p 00016000 fd:00 6193452
      /lib64/libpthread-2.5.so
    3c9c017000-3c9c01b000 rw-p 3c9c017000 00:00 0
    3c9ce00000-3c9ce14000 r-xp 00000000 fd:00 11835868
      /usr/lib64/libz.so.1.2.3
    3c9ce14000-3c9d013000 ---p 00014000 fd:00 11835868
      /usr/lib64/libz.so.1.2.3
    3c9d013000-3c9d014000 rw-p 00013000 fd:00 11835868
      /usr/lib64/libz.so.1.2.3
    3c9ea00000-3c9ea02000 r-xp 00000000 fd:00 6193461
      /lib64/libcom_err.so.2.1
    3c9ea02000-3c9ec01000 ---p 00002000 fd:00 6193461
      /lib64/libcom_err.so.2.1
    3c9ec01000-3c9ec02000 rw-p 00001000 fd:00 6193461
      /lib64/libcom_err.so.2.1
    3c9f600000-3c9f620000 r-xp 00000000 fd:00 6193466
      /lib64/libexpat.so.0.5.0
    3c9f620000-3c9f81f000 ---p 00020000 fd:00 6193466
      /lib64/libexpat.so.0.5.0
    3c9f81f000-3c9f822000 rw-p 0001f000 fd:00 6193466
      /lib64/libexpat.so.0.5.0
    3ec6e00000-3ec6e45000 r-xp 00000000 fd:00 6193218
      /lib64/libssl.so.0.9.8e
    3ec6e45000-3ec7044000 ---p 00045000 fd:00 6193218
      /lib64/libssl.so.0.9.8e
    3ec7044000-3ec704a000 rw-p 00044000 fd:00 6193218
      /lib64/libssl.so.0.9.8e
    3ec7200000-3ec7208000 r-xp 00000000 fd:00 11841734
      /usr/lib64/libkrb5support.so.0.1
    3ec7208000-3ec7407000 ---p 00008000 fd:00 11841734
      /usr/lib64/libkrb5support.so.0.1
    3ec7407000-3ec7408000 rw-p 00007000 fd:00 11841734
      /usr/lib64/libkrb5support.so.0.1
    3ec7600000-3ec7602000 r-xp 00000000 fd:00 6193214
      /lib64/libkeyutils-1.2.so
    3ec7602000-3ec7801000 ---p 00002000 fd:00 6193214
      /lib64/libkeyutils-1.2.so
    3ec7801000-3ec7802000 rw-p 00001000 fd:00 6193214
      /lib64/libkeyutils-1.2.so
    3ec8a00000-3ec8a91000 r-xp 00000000 fd:00 11842706
      /usr/lib64/libkrb5.so.3.3
    3ec8a91000-3ec8c91000 ---p 00091000 fd:00 11842706
      /usr/lib64/libkrb5.so.3.3
    3ec8c91000-3ec8c95000 rw-p 00091000 fd:00 11842706
      /usr/lib64/libkrb5.so.3.3
    3ec8e00000-3ec8e2c000 r-xp 00000000 fd:00 11843236
      /usr/lib64/libgssapi_krb5.so.2.2
    3ec8e2c000-3ec902c000 ---p 0002c000 fd:00 11843236
      /usr/lib64/libgssapi_krb5.so.2.2
    3ec902c000-3ec902e000 rw-p 0002c000 fd:00 11843236
      /usr/lib64/libgssapi_krb5.so.2.2
    3ec9200000-3ec9224000 r-xp 00000000 fd:00 11841735
      /usr/lib64/libk5crypto.so.3.1
    3ec9224000-3ec9423000 ---p 00024000 fd:00 11841735
      /usr/lib64/libk5crypto.so.3.1
    3ec9423000-3ec9425000 rw-p 00023000 fd:00 11841735
      /usr/lib64/libk5crypto.so.3.1
    3ec9a00000-3ec9b2d000 r-xp 00000000 fd:00 6193209
      /lib64/libcrypto.so.0.9.8e
    3ec9b2d000-3ec9d2c000 ---p 0012d000 fd:00 6193209
      /lib64/libcrypto.so.0.9.8e
    3ec9d2c000-3ec9d4d000 rw-p 0012c000 fd:00 6193209
      /lib64/libcrypto.so.0.9.8e
    3ec9d4d000-3ec9d51000 rw-p 3ec9d4d000 00:00 0
    2afc5e15e000-2afc5e161000 rw-p 2afc5e15e000 00:00 0
    2afc5e16a000-2afc5e172000 rw-p 2afc5e16a000 00:00 0
    2afc5e17d000-2afc5e187000 r-xp 00000000 fd:00 6193177
      /lib64/libnss_files-2.5.so
    2afc5e187000-2afc5e386000 ---p 0000a000 fd:00 6193177
      /lib64/libnss_files-2.5.so
    2afc5e386000-2afc5e387000 r--p 00009000 fd:00 6193177
      /lib64/libnss_files-2.5.so
    2afc5e387000-2afc5e388000 rw-p 0000a000 fd:00 6193177
      /lib64/libnss_files-2.5.so
    2afc5e388000-2afc5e38c000 r-xp 00000000 fd:00 6193175
      /lib64/libnss_dns-2.5.so
    2afc5e38c000-2afc5e58b000 ---p 00004000 fd:00 6193175
      /lib64/libnss_dns-2.5.so
    2afc5e58b000-2afc5e58c000 r--p 00003000 fd:00 6193175
      /lib64/libnss_dns-2.5.so
    2afc5e58c000-2afc5e58d000 rw-p 00004000 fd:00 6193175
      /lib64/libnss_dns-2.5.so
    2afc5e58d000-2afc5e86b000 rw-p 2afc5e58d000 00:00 0
    7fff9f4e1000-7fff9f4f6000 rw-p 7ffffffe9000 00:00 0
      [stack]
    ffffffffff600000-ffffffffffe00000 ---p 00000000 00:00 0
      [vdso]

It happened when I rm -rf'd the /tmp/clone directory that git-remote-http was working on. It's possibly fixed in later versions of git, I didn't check.

Just posting it here in case someone has time to write a test for this / track it down. I don't at the moment.q

Ævar Arnfjörð Bjarmason· Aug 1, 2011, 15:39 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: Possibly solved invalid free() in git-remote-http from Git 1.7.2.1

On Mon, Aug 1, 2011 at 17:22, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> In case anyone would like to look into this I had this undiagnosed
> free() error from git-remote-http in git version 1.7.2.1:

Actually this is still an issue in master, CC-ing Tay who introduced this error.

This evil hack would get around it most of the time, but would introduce another race condition:

    diff --git a/http.c b/http.c
    index a1ea3db..ba81158 100644
    --- a/http.c
    +++ b/http.c
    @@ -1212,6 +1212,7 @@ struct http_object_request
*new_http_object_request(const char *base_url,
            ssize_t prev_read = 0;
            long prev_posn = 0;
            char range[RANGE_HEADER_SIZE];
    +       int allocated_url = 0;
            struct curl_slist *range_header = NULL;
            struct http_object_request *freq;
    @@ -1260,6 +1261,7 @@ struct http_object_request
*new_http_object_request(const char *base_url,
            git_SHA1_Init(&freq->c);
            freq->url = get_remote_object_url(base_url, hex, 0);
    +       allocated_url = 1;
            /*
             * If a previous temp file is present, process what was already
    @@ -1330,7 +1332,8 @@ struct http_object_request
*new_http_object_request(const char *base_url,
     abort:
            free(filename);
    -       free(freq->url);
    +       if (allocated_url)
    +               free(freq->url);
            free(freq);
            return NULL;
     }

One option would be to memzero freq, but that'll trip up xmalloc which tries to set things to 0xA5 under XMALLOC_POISON. I don't know what the proper way to solve this would be.

Ævar Arnfjörð Bjarmason· Aug 1, 2011, 15:42 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: Possibly solved invalid free() in git-remote-http from Git 1.7.2.1

On Mon, Aug 1, 2011 at 17:39, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
Show 9 quoted lines
> On Mon, Aug 1, 2011 at 17:22, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>> In case anyone would like to look into this I had this undiagnosed
>> free() error from git-remote-http in git version 1.7.2.1:
>
> Actually this is still an issue in master, CC-ing Tay who introduced
> this error.
>
> This evil hack would get around it most of the time, but would
> introduce another race condition:
*brainfart*. This would be better:
    diff --git a/http.c b/http.c
    index a1ea3db..c5da23a 100644
    --- a/http.c
    +++ b/http.c
    @@ -1298,7 +1298,7 @@ struct http_object_request
*new_http_object_request(const char *base_url,
                            if (ftruncate(freq->localfile, 0) < 0) {
                                    error("Couldn't truncate temporary
file %s: %s",
                                              freq->tmpfile, strerror(errno));
    -                               goto abort;
    +                               goto abort_url;
                            }
                    }
            }
    @@ -1328,9 +1328,10 @@ struct http_object_request
*new_http_object_request(const char *base_url,
            return freq;
    +abort_url:
    +       free(freq->url);
     abort:
            free(filename);
    -       free(freq->url);
            free(freq);
            return NULL;
     }
Jeff King· Aug 1, 2011, 18:00 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: Possibly solved invalid free() in git-remote-http from Git 1.7.2.1

On Mon, Aug 01, 2011 at 05:42:54PM +0200, Ævar Arnfjörð Bjarmason wrote:
Show 40 quoted lines
> On Mon, Aug 1, 2011 at 17:39, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> > On Mon, Aug 1, 2011 at 17:22, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> >> In case anyone would like to look into this I had this undiagnosed
> >> free() error from git-remote-http in git version 1.7.2.1:
> >
> > Actually this is still an issue in master, CC-ing Tay who introduced
> > this error.
> >
> > This evil hack would get around it most of the time, but would
> > introduce another race condition:
> 
> *brainfart*. This would be better:
> 
>     diff --git a/http.c b/http.c
>     index a1ea3db..c5da23a 100644
>     --- a/http.c
>     +++ b/http.c
>     @@ -1298,7 +1298,7 @@ struct http_object_request
> *new_http_object_request(const char *base_url,
>                             if (ftruncate(freq->localfile, 0) < 0) {
>                                     error("Couldn't truncate temporary
> file %s: %s",
>                                               freq->tmpfile, strerror(errno));
>     -                               goto abort;
>     +                               goto abort_url;
>                             }
>                     }
>             }
>     @@ -1328,9 +1328,10 @@ struct http_object_request
> *new_http_object_request(const char *base_url,
> 
>             return freq;
> 
>     +abort_url:
>     +       free(freq->url);
>      abort:
>             free(filename);
>     -       free(freq->url);
>             free(freq);
>             return NULL;
Hmm. The memory management in the function seems weird to me.

Why do we free filename only on abort? Are we leaking it otherwise? Even weirder, AFAICT, filename can only ever point to the static buffer returned by sha1_file_name. So it should _never_ be freed. And I'm surprised glibc's free() checker isn't complaining about that. Am I reading it wrong?

With respect to freq->url, from your patch I expected that some code paths would allocate it on the heap, and some would not. But it looks like there is only one spot, and the problem is that freq is filled with uninitialized cruft. Wouldn't it be simpler to just s/xmalloc/xcalloc/, and then you know all of the pointers are initialized properly to NULL? You can also get rid of the memset-to-zero later in the function.

-Peff
Tay Ray Chuan· Aug 2, 2011, 02:51 UTC · re: Jeff King · lore

Re: Possibly solved invalid free() in git-remote-http from Git 1.7.2.1

On Mon, 1 Aug 2011 12:00:19 -0600 Jeff King <peff@peff.net> wrote:

> Hmm. The memory management in the function seems weird to me.
How about this?
-->8--
diff --git a/http.c b/http.c
index b2ae8de..4da6293 100644
--- a/http.c
+++ b/http.c
@@ -1116,9 +1116,8 @@ struct http_pack_request *new_http_pack_request(
 	struct strbuf buf = STRBUF_INIT;
 	struct http_pack_request *preq;
 
-	preq = xmalloc(sizeof(*preq));
+	preq = xcalloc(1, sizeof(*preq));
 	preq->target = target;
-	preq->range_header = NULL;
 
 	end_url_with_slash(&buf, base_url);
 	strbuf_addf(&buf, "objects/pack/pack-%s.pack",
@@ -1210,7 +1209,7 @@ struct http_object_request *new_http_object_request(const char *base_url,
 	struct curl_slist *range_header = NULL;
 	struct http_object_request *freq;
 
-	freq = xmalloc(sizeof(*freq));
+	freq = xcalloc(1, sizeof(*freq));
 	hashcpy(freq->sha1, sha1);
 	freq->localfile = -1;
 
@@ -1248,8 +1247,6 @@ struct http_object_request *new_http_object_request(const char *base_url,
 		goto abort;
 	}
 
-	memset(&freq->stream, 0, sizeof(freq->stream));
-
 	git_inflate_init(&freq->stream);
 
 	git_SHA1_Init(&freq->c);
@@ -1324,7 +1321,6 @@ struct http_object_request *new_http_object_request(const char *base_url,
 	return freq;
 
 abort:
-	free(filename);
 	free(freq->url);
 	free(freq);
 	return NULL;


--
Cheers,
Ray Chuan
Jeff King· Aug 2, 2011, 03:33 UTC · re: Tay Ray Chuan · lore

Re: Possibly solved invalid free() in git-remote-http from Git 1.7.2.1

On Tue, Aug 02, 2011 at 10:51:24AM +0800, Tay Ray Chuan wrote:
Show 5 quoted lines
> On Mon, 1 Aug 2011 12:00:19 -0600
> Jeff King <peff@peff.net> wrote:
> > Hmm. The memory management in the function seems weird to me.
> 
> How about this?

Yes, that fixes all of the oddities I found (I assume you looked and agreed with me that they were wrong).

-Peff
Tay Ray Chuan· Aug 3, 2011, 11:54 UTC · re: Jeff King · lore

[PATCH] http.c: fix an invalid free()

Remove a free() on the static buffer returned by sha1_file_name().

While we're at it, replace xmalloc() calls on the structs http_(object|pack)_request with xcalloc() so that pointers in the structs get initialized to NULL. That way, free()'s are safe - for example, a free() on the url string member when aborting.

This fixes an invalid free().
Reported-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Helped-by: Jeff King peff@peff.net
Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
---
 http.c |    8 ++------
 1 files changed, 2 insertions(+), 6 deletions(-)
diff --git a/http.c b/http.c
index a1ea3db..a59cac4 100644
--- a/http.c
+++ b/http.c
@@ -1121,9 +1121,8 @@ struct http_pack_request *new_http_pack_request(
 	struct strbuf buf = STRBUF_INIT;
 	struct http_pack_request *preq;

-	preq = xmalloc(sizeof(*preq));
+	preq = xcalloc(1, sizeof(*preq));
 	preq->target = target;
-	preq->range_header = NULL;

 	end_url_with_slash(&buf, base_url);
 	strbuf_addf(&buf, "objects/pack/pack-%s.pack",
@@ -1215,7 +1214,7 @@ struct http_object_request *new_http_object_request(const char *base_url,
 	struct curl_slist *range_header = NULL;
 	struct http_object_request *freq;

-	freq = xmalloc(sizeof(*freq));
+	freq = xcalloc(1, sizeof(*freq));
 	hashcpy(freq->sha1, sha1);
 	freq->localfile = -1;

@@ -1253,8 +1252,6 @@ struct http_object_request *new_http_object_request(const char *base_url,
 		goto abort;
 	}

-	memset(&freq->stream, 0, sizeof(freq->stream));
-
 	git_inflate_init(&freq->stream);

 	git_SHA1_Init(&freq->c);
@@ -1329,7 +1326,6 @@ struct http_object_request *new_http_object_request(const char *base_url,
 	return freq;

 abort:
-	free(filename);
 	free(freq->url);
 	free(freq);
 	return NULL;
--
1.7.6.11.g49037.dirty

← back to recent threads