On Tue, Apr 30, 2013 at 3:27 PM, Thomas Rast <tr...@inf.ethz.ch> wrote:
> Nguyễn Thái Ngọc Duy <pclo...@gmail.com> writes:
>> In this particular code path, we add "base" to the delta base
>> cache. Then decide to free it, but we forgot about a dangling pointer
>> in the cache. Invalidate that entry when we free "base".
>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclo...@gmail.com>
>> Some of my changes triggered a double free fault at "free(base);" in
>> t5303. This looks like a correct thing to do, but I may be missing
>> something (I'm not even sure how it happened). Please check.
> Can you describe how you triggered it?
> I ran all of origin/pu through valgrind tests just yesterday, and it
> found nothing (yay!), so it doesn't seem to reproduce here?
Apply this patch on top of master (no need to apply full series) and run t5303
>> @@ -2129,6 +2132,8 @@ void *unpack_entry(struct packed_git *p, off_t
>> "at offset %"PRIuMAX" from %s",
>> (uintmax_t)curpos, p->pack_name);
>> + if (ent)
>> + ent->data = NULL;
>> data = NULL;
> Why not clear_delta_base_cache_entry(), which also handles updating the
> lru pointers?
Simple. I didn't know about clear_de..entry().
> Also I wonder if removing free(base) is the right fix: since the failure
> is in decompressing the delta, the base might again be useful and we
> should keep it cached.
OK since you know this code much better than me, I withdraw my patch
(consider it a bug report) and let you work on a proper fix. I see you
already have the commit message ready :) Happy to test it for you if
the above instruction is still not reproducible for you.
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