On Tue, Apr 30, 2013 at 3:27 PM, Thomas Rast <trast@xxxxxxxxxxx> wrote: > Nguyễn Thái Ngọc Duy <pclouds@xxxxxxxxx> 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 <pclouds@xxxxxxxxx> >> --- >> 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 http://article.gmane.org/gmane.comp.version-control.git/222895 >> @@ -2129,6 +2132,8 @@ void *unpack_entry(struct packed_git *p, off_t obj_offset, >> "at offset %"PRIuMAX" from %s", >> (uintmax_t)curpos, p->pack_name); >> free(base); >> + if (ent) >> + ent->data = NULL; >> data = NULL; >> continue; >> } > > 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. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html