Re: What's cooking in git.git (Oct 2013, #03; Wed, 16)

[Karsten Blees <karsten.blees@gmail.com>]

Am 18.10.2013 21:09, schrieb Junio C Hamano:
> Karsten Blees <karsten.blees@gmail.com> writes:
> 
>> The coredumps are caused by my patch #10, which free()s
>> cache_entries when they are removed, in combination with ...
> 
> Looking at that patch, it makes me wonder if remove_index_entry_at()
> and replace_index_entry() should be the ones that frees the old
> entry in the first place.  A caller may already have a ce pointing
> at an old entry and use the information from old_ce to update a new
> one after it installed it, e.g.
> 
> 	old_ce = ...
>         new_ce = make_cache_entry(... old_ce->name, ...);
>         replace_index_entry(... new_ce);
> 	new_ce->ce_mode = old_ce->cd_mode;
> 	free(old_ce);
> 
> The same goes for the functions that remove the entry.
> 

Moving free() to the callers or caller's callers would make it much more complicated (more places to change). Besides, most callers don't even have a reference to old_ce and simply remove by position. Of course, this doesn't prevent caller's caller's callers to keep a reference to a removed / replaced entry, as found by Thomas.

> 
> Going forward, I do agree with your patch #10 that removal or
> replacing that may make an existing entry unreferenced should free
> entries that are no longer used, and "use after free" should be
> forbidden.
> 

OK, I'll spend some more time analyzing the call hierarchies to see if there are more uses of removed cache_entries. I'll try to post an updated v4 by the end of the week.

Karsten