On Thu, May 30, 2013 at 10:13 AM, René Scharfe
<rene.scha...@lsrfire.ath.cx> wrote:
> 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.contre...@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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to