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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 30, 2013, 22:39 UTC
Message-ID: <7vfvy7abip.fsf@alter.siamese.dyndns.org>
In-Reply-To: <87d2tcw571.fsf@linux-k42r.v.cablecom.net>

Thomas Rast <trast@inf.ethz.ch> writes:

Show 21 quoted lines

> Duy Nguyen <pclouds@gmail.com> writes:
>
>> 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
> [...]
>> 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.
>
> Ok.  So I really think just dropping the free() is the way to go.  Can
> you test this?  Your series didn't apply cleanly on anything I had
> locally, and 'am -3' doesn't work.  A simpler reproducer, and using
> valgrind to detect the use-after-free, didn't get me anywhere either.
>
> -- >8 --
> Subject: [PATCH] unpack_entry: avoid freeing objects in base cache
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit

I see you used "git-pu format-patch --inline-single" here, and the above is the reason why it is marked as "not ready for public consumption" ;-).

Show 23 quoted lines

> In the !delta_data error path of unpack_entry(), we run free(base).
> This became a window for use-after-free() in abe601b (sha1_file:
> remove recursion in unpack_entry, 2013-03-27), as follows:
>
> Before abe601b, we got the 'base' from cache_or_unpack_entry(..., 0);
> keep_cache=0 tells it to also remove that entry.  So the 'base' is at
> this point not cached, and freeing it in the error path is the right
> thing.
>
> After abe601b, the structure changed: we use a three-phase approach
> where phase 1 finds the innermost base or a base that is already in
> the cache.  In phase 3 we therefore know that all bases we unpack are
> not part of the delta cache yet.  (Observe that we pop from the cache
> in phase 1, so this is also true for the very first base.)  So we make
> no further attempts to look up the bases in the cache, and just call
> add_delta_base_cache() on every base object we have assembled.
>
> But the !delta_data error path remained unchanged, and now calls
> free() on a base that has already been entered in the cache.  This
> means that there is a use-after-free if we later use the same base
> again.
>
> So remove that free().

I wish I saw "We are still going to use it, and it will be freed after we are done" or something like that after this sentence.

But other than that, I think the logic is correct.

Show 18 quoted lines

> Reported-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> Signed-off-by: Thomas Rast <trast@inf.ethz.ch>
> ---
>  sha1_file.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/sha1_file.c b/sha1_file.c
> index 64228a2..67e815b 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -2128,7 +2128,6 @@ void *unpack_entry(struct packed_git *p, off_t obj_offset,
>  			error("failed to unpack compressed delta "
>  			      "at offset %"PRIuMAX" from %s",
>  			      (uintmax_t)curpos, p->pack_name);
> -			free(base);
>  			data = NULL;
>  			continue;
>  		}