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

Duy Nguyen [off-list ref] 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

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().

Reported-by: Nguyễn Thái Ngọc Duy <redacted>
Signed-off-by: Thomas Rast <redacted>
---
 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;
 		}

-- 
1.8.3.rc0.333.gdb39496

On Tue, Apr 30, 2013 at 7:53 PM, Thomas Rast [off-list ref] wrote:

Duy Nguyen [off-list ref] writes:

quoted

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

[...]

quoted

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.

Confirmed the double free is gone. I also run it under valgrind and
found nothing special.

--
Duy