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

[Felipe Contreras <felipe.contreras@gmail.com>]

On Sat, Jun 8, 2013 at 6:32 AM, René Scharfe
<rene.scharfe@lsrfire.ath.cx> wrote:
> 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?

I don't know.

> Or to get rid of it and check for ->cache_alloc instead?

That might make sense. I was thinking on renaming 'initialized' to
'loaded', but I really don't care.

>> 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 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.

> 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?

-- 
Felipe Contreras