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

From: René Scharfe <rene.scharfe@lsrfire.ath.cx>
Date: Jun 8, 2013, 13:22 UTC
Message-ID: <51B32FFD.5070302@lsrfire.ath.cx>
In-Reply-To: <CAMP44s2Bp5p1211e6Utdch4B+v3J83GCY0_ucG7duakswkb+pg@mail.gmail.com>

Am 08.06.2013 14:15, schrieb Felipe Contreras:

Show 33 quoted lines

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

Show 8 quoted lines

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