Re: [PATCH v2] merge: fix cache_entry use-after-free

[David Turner <dturner@twopensource.com>]

On Mon, 2015-10-12 at 15:28 -0700, Junio C Hamano wrote:
> David Turner <dturner@twopensource.com> writes:
> 
> > From: Keith McGuigan <kmcguigan@twitter.com>
> >
> > 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 <kmcguigan@twitter.com>
> > ---
> 
> I'll forge your "messenger's sign-off" here ;-)

Thanks.

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

I like to set a pointer to NULL after I free the thing pointed to by it,
because it helps make use-after-free bugs easier to detect.