On Tue, Apr 30, 2013 at 7:53 PM, Thomas Rast <trast@xxxxxxxxxxx> wrote: > Duy Nguyen <pclouds@xxxxxxxxx> 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. Confirmed the double free is gone. I also run it under valgrind and found nothing special. > -- >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 <pclouds@xxxxxxxxx> > Signed-off-by: Thomas Rast <trast@xxxxxxxxxxx> > --- > 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 -- 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