Am 18.10.2013 21:09, schrieb Junio C Hamano:
> Karsten Blees <karsten.bl...@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
--
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