Re: [PATCH 09/15] http: release the memory of a http pack request as well

From: Stefan Beller <sbeller@google.com>
Date: Mar 24, 2015, 16:54 UTC
Message-ID: <CAGZ79kZbe9d7wG+CJ0o93hDs3sFjkSp68pBHXpjOysbzgvjo3w@mail.gmail.com>
In-Reply-To: <xmqq1tkgddto.fsf@gitster.dls.corp.google.com>

On Sun, Mar 22, 2015 at 12:36 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 29 quoted lines

> Stefan Beller <sbeller@google.com> writes:
>
>> The cleanup function is used in 4 places now and it's always safe to
>> free up the memory as well.
>>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>>  http.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/http.c b/http.c
>> index 9c825af..4b179f6 100644
>> --- a/http.c
>> +++ b/http.c
>> @@ -1462,6 +1462,7 @@ void release_http_pack_request(struct http_pack_request *preq)
>>       }
>>       preq->slot = NULL;
>>       free(preq->url);
>> +     free(preq);
>>  }
>>
>>  int finish_http_pack_request(struct http_pack_request *preq)
>
> Freeing of preq in all the callers of this one looks sensible,
> except for the one in finish_request() of http-push.c that pulls an
> preq instance out of request->userData.
>
> Can somebody help me follow the dataflow to convince me that this is
> not leading to double-free in start_fetch_packed()?

I am not sure where you suspect the double free problem.

In start_fetch_packed we have a call to release_http_pack_request 2 times but just in an error-out-and-cleanup case, so either of one cases is called.

In the latter place (http-push.c lines 335-347), we have code like
    request->userData = preq;
    if (!start_active_slot(preq->slot)) {
        release_http_pack_request(preq);
        repo->can_update_info_refs = 0;
        release_request(request);
    }

Do you mean that the release_http_pack_request and release_request may collide as the `release_request(request);` has a pointer to preq via its userData field?

Well there is hope, as `release_request` only touches
    free(request->url);
    free(request);

and not the userData pointer.

I am a bit puzzled what you're trying to hint at.

>
> Thanks.
>