Re: [PATCH] unpack_entry: invalidate newly added cache entry in case of error

From: Duy Nguyen <pclouds@gmail.com>
Date: Apr 30, 2013, 10:25 UTC
Message-ID: <CACsJy8Bi6UpfA-0BCFY6H=BAKMmWYgwbf-94yJXEn5Zi4gwPCA@mail.gmail.com>
In-Reply-To: <87ppxcxw1i.fsf@linux-k42r.v.cablecom.net>

On Tue, Apr 30, 2013 at 3:27 PM, Thomas Rast <trast@inf.ethz.ch> wrote:

Show 16 quoted lines

> Nguyễn Thái Ngọc Duy <pclouds@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 <pclouds@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

http://article.gmane.org/gmane.comp.version-control.git/222895

Show 12 quoted lines

>> @@ -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