Re: [PATCH v2 1/3] read-cache: plug a few leaks

From: Felipe Contreras <felipe.contreras@gmail.com>
Date: May 31, 2013, 08:22 UTC
Message-ID: <CAMP44s1PYK1efXjk8WhCpXM9g_tBf-vXbXRY4eC2tiVym+_07g@mail.gmail.com>
In-Reply-To: <51A76C8E.1080009@lsrfire.ath.cx>

On Thu, May 30, 2013 at 10:13 AM, René Scharfe <rene.scharfe@lsrfire.ath.cx> wrote:

Show 30 quoted lines

> Am 30.05.2013 15:34, schrieb Felipe Contreras:
>> We don't free 'istate->cache' properly.
>>
>> Apparently 'initialized' doesn't really mean initialized, but loaded, or
>> rather 'not-empty', and the cache can be used even if it's not
>> 'initialized', so we can't rely on this variable to keep track of the
>> 'istate->cache'.
>>
>> So assume it always has data, and free it before overwriting it.
>>
>> 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 04ed561..e5dc96f 100644
>> --- a/read-cache.c
>> +++ b/read-cache.c
>> @@ -1449,6 +1449,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);
>
> With that change, callers of read_index_from need to set ->cache to
> NULL for uninitialized (on-stack) index_state variables.  They only had
> to set ->initialized to 0 before in that situation.  It this chunk safe
> for all existing callers?  Shouldn't the same free in discard_index
> (added below) be enough?

It would be enough if every discard_cache() is not followed by a read_cache() after adding entries.

I was adding a init_index() helper, but it turns out only very few places initialize the index, and all of them zero the structure (or declare it so it's zeroed on load), so I think this change is safe like that. Also, I don't see any place manually doing initialize=0.

-- 
Felipe Contreras