Re: [PATCH v3 2/2] read-cache: plug a few leaks

["René Scharfe" <rene.scharfe@lsrfire.ath.cx>]

Am 08.06.2013 14:15, schrieb Felipe Contreras:
> On Sat, Jun 8, 2013 at 6:32 AM, René Scharfe
> <rene.scharfe@lsrfire.ath.cx> wrote:
>>> diff --git a/read-cache.c b/read-cache.c
>>> index 5e30746..a1dd04d 100644
>>> --- a/read-cache.c
>>> +++ b/read-cache.c
>>> @@ -1451,6 +1451,7 @@ int read_index_from(struct index_state *istate,
>>> const char *path)
>>>          istate->version = ntohl(hdr->hdr_version);
>>>          istate->cache_nr = ntohl(hdr->hdr_entries);
>>>          istate->cache_alloc = alloc_nr(istate->cache_nr);
>>> +       free(istate->cache);
>>>          istate->cache = xcalloc(istate->cache_alloc,
>>> sizeof(*istate->cache));
>>>          istate->initialized = 1;
>>
>>
>> You wrote earlier that this change is safe with current callers and that it
>> prevents leaks with the following sequence:
>>
>> discard_cache();
>> # add entries
>> read_cache();
>>
>> Do we currently have such a call sequence somewhere?
>
> I don't know.
>
>> Wouldn't that be a
>> bug, namely forgetting to call discard_cache before read_cache?
>
> Why would it be a bug? There's nothing in the API that hints there's a
> problem with that.

A comment before read_index_from says "remember to discard_cache() 
before reading a different cache!".  That is probably a reminder that 
read_index_from does nothing if ->initialized is set.  Entries added 
before calling read_index_from make up a different cache, however, so I 
think this comment applies for the call sequence above as well.

Only read_index_from and add_index_entry allocate ->cache, and only 
discard_index frees it, so the two are a triple like malloc, realloc and 
free.

Granted, these hints are not part of the API -- that looks like a 
documentation bug, however.

Side note: I wonder why we need to guard against multiple 
read_index_from calls in a row with ->initialized.  Wouldn't it be 
easier to avoid the duplicate calls in the first place?  Finding them 
now might be not so easy, though.

>> I've added a "assert(istate->cache_nr == 0);" a few lines above and the test
>> suite still passed.  With the hunk below, ->cache is also always NULL and
>> cache_alloc is always 0 at that point.  So we don't need that free call
>> there in the cases covered by the test suite at least -- better leave it
>> out.
>
> Why leave it out? If somebody makes the mistake of doing the above
> sequence, would you prefer that we leak?

Leaking is better than silently cleaning up after a buggy caller because 
it still allows the underlying bug to be found.  Even better would be an 
assert, but it's important to make sure it doesn't trigger for existing 
use cases.

René