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

[Jeff King <peff@peff.net>]

On Mon, Aug 01, 2011 at 05:42:54PM +0200, Ævar Arnfjörð Bjarmason wrote:

> 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