Re: What's cooking in git.git (Oct 2013, #03; Wed, 16)
Am 18.10.2013 21:09, schrieb Junio C Hamano:
Show 19 quoted lines
> 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.
Show 6 quoted lines
>
> 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
Karsten Blees <karsten.blees@gmail.com>