Thomas Rast <tr...@inf.ethz.ch> writes:
> Duy Nguyen <pclo...@gmail.com> writes:
>> Apply this patch on top of master (no need to apply full series) and run
>> 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
> 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
> 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
> 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.
> Reported-by: Nguyễn Thái Ngọc Duy <pclo...@gmail.com>
> Signed-off-by: Thomas Rast <tr...@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
> error("failed to unpack compressed delta "
> "at offset %"PRIuMAX" from %s",
> (uintmax_t)curpos, p->pack_name);
> - free(base);
> data = NULL;
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