On Mon, 2015-10-12 at 15:28 -0700, Junio C Hamano wrote: > David Turner <dturner@xxxxxxxxxxxxxxxx> writes: > > > From: Keith McGuigan <kmcguigan@xxxxxxxxxxx> > > > > 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@xxxxxxxxxxx> > > --- > > 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. -- 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