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

> Am 08.06.2013 00:29, schrieb Felipe Contreras:
>
>> We are not freeing 'istate->cache' properly.
>>
>> We can't rely on 'initialized' to keep track of the 'istate->cache',
>> because it doesn't really mean it's initialized. So assume it always has
>> data, and free it before overwriting it.
>
>
> That sounds a bit backwards to me.  If ->initialized doesn't mean that the
> index state is initialized then something is fishy.  Would it make sense and
> be sufficient to set ->initialized in add_index_entry?

>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>> ---
>>   read-cache.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> 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'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.