From: Junio C Hamano Date: Mon, 12 Oct 2015 22:28:07 GMT Subject: Re: [PATCH v2] merge: fix cache_entry use-after-free Message-ID: In-Reply-To: <1444687413-928-1-git-send-email-dturner@twitter.com> David Turner writes: > From: Keith McGuigan > > During merges, we would previously free entries that we no longer need > in the destination index. But those entries might also be stored in > the dir_entry cache, and when a later call to add_to_index found them, > they would be used after being freed. > > To prevent this, add a ref count for struct cache_entry. Whenever > a cache entry is added to a data structure, the ref count is incremented; > when it is removed from the data structure, it is decremented. When > it hits zero, the cache_entry is freed. > > Signed-off-by: Keith McGuigan > --- I'll forge your "messenger's sign-off" here ;-) > diff --git a/unpack-trees.c b/unpack-trees.c > index f932e80..1a0a637 100644 > --- a/unpack-trees.c > +++ b/unpack-trees.c > @@ -606,8 +606,10 @@ static int unpack_nondirectories(int n, unsigned long mask, > o); > for (i = 0; i < n; i++) { > struct cache_entry *ce = src[i + o->merge]; > - if (ce != o->df_conflict_entry) > - free(ce); > + if (ce != o->df_conflict_entry) { > + drop_ce_ref(ce); > + src[i + o->merge] = NULL; > + } This one smelled iffy. I think it is safe because the caller does not look at src[] other than src[0] after this function returns, and this setting to NULL happens only when o->merge is set to 1, so I do not think this is buggy, but at the same time I do not think setting to NULL is necessary. Other than that, looks nice. Thanks.