Thomas Rast <trast@xxxxxxxxxxx> writes: > 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. > > -- >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" ;-). > 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. > 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; > } -- 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