{"thread":{"id":"40521","subject":"[PATCH 0/1] merge: fix cache_entry use-after-free","startedAt":"2015-10-08T18:47:50Z","lastAt":"2015-10-09T22:51:23Z","messageCount":5,"participants":["David Turner","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"271334","messageId":"1444330071-8909-1-git-send-email-dturner@twitter.com","threadId":"40521","inReplyTo":null,"subject":"[PATCH 0/1] merge: fix cache_entry use-after-free","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2015-10-08T18:47:50Z","receivedAt":"2015-10-08T18:47:50Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"Keith diagnosed the problem and wrote the patch.  I wrote the commit\nmessage and am sending it upstream with his OK.\n\nKeith McGuigan (1):\n  merge: fix cache_entry use-after-free\n\n cache.h        | 27 +++++++++++++++++++++++++++\n name-hash.c    |  7 ++++++-\n read-cache.c   |  5 ++++-\n split-index.c  | 12 +++++++-----\n unpack-trees.c |  6 ++++--\n 5 files changed, 48 insertions(+), 9 deletions(-)\n\n-- \n2.4.2.644.g97b850b-twtrsrc\n"},{"id":"271333","messageId":"1444330071-8909-2-git-send-email-dturner@twitter.com","threadId":"40521","inReplyTo":"1444330071-8909-1-git-send-email-dturner@twitter.com","subject":"[PATCH 0/1] merge: fix cache_entry use-after-free","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2015-10-08T18:47:51Z","receivedAt":"2015-10-08T18:47:51Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"From: Keith McGuigan <kmcguigan@twitter.com>\n\nDuring merges, we would previously free entries that we no longer need\nin the destination index.  But those entries might also be stored in\nthe dir_entry cache, and when a later call to add_to_index found them,\nthey would be used after being freed.\n\nTo prevent this, add a ref count for struct cache_entry.  Whenever\na cache entry is added to a data structure, the ref count is incremented;\nwhen it is removed from the data structure, it is decremented.  When\nit hits zero, the cache_entry is freed.\n\nSigned-off-by: David Turner <dturner@twopensource.com>\nSigned-off-by: Keith McGuigan <kmcguigan@twitter.com>\n---\n cache.h        | 27 +++++++++++++++++++++++++++\n name-hash.c    |  7 ++++++-\n read-cache.c   |  5 ++++-\n split-index.c  | 12 +++++++-----\n unpack-trees.c |  6 ++++--\n 5 files changed, 48 insertions(+), 9 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 752031e..7563658 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -149,6 +149,7 @@ struct stat_data {\n \n struct cache_entry {\n \tstruct hashmap_entry ent;\n+\tunsigned int ref_count; /* count the number of refs to this in dir_hash */\n \tstruct stat_data ce_stat_data;\n \tunsigned int ce_mode;\n \tunsigned int ce_flags;\n@@ -213,6 +214,32 @@ struct cache_entry {\n struct pathspec;\n \n /*\n+ * Increment the cache_entry reference count.  Should be called\n+ * whenever a pointer to a cache_entry is retained in a data structure,\n+ * thus marking it as alive.\n+ */\n+static inline void add_ce_ref(struct cache_entry *ce)\n+{\n+\tassert(ce != NULL && ce->ref_count >= 0);\n+\t++ce->ref_count;\n+}\n+\n+/*\n+ * Decrement the cache_entry reference count.  Should be called whenever\n+ * a pointer to a cache_entry is dropped.  Once the counter drops to 0\n+ * the cache_entry memory will be safely freed.\n+ */\n+static inline void drop_ce_ref(struct cache_entry *ce)\n+{\n+\tif (ce != NULL) {\n+\t\tassert(ce->ref_count >= 0);\n+\t\tif (--ce->ref_count < 1) {\n+\t\t\tfree(ce);\n+\t\t}\n+\t}\n+}\n+\n+/*\n  * Copy the sha1 and stat state of a cache entry from one to\n  * another. But we never change the name, or the hash state!\n  */\ndiff --git a/name-hash.c b/name-hash.c\nindex 702cd05..f12c919 100644\n--- a/name-hash.c\n+++ b/name-hash.c\n@@ -66,6 +66,7 @@ static struct dir_entry *hash_dir_entry(struct index_state *istate,\n \t\tdir = xcalloc(1, sizeof(struct dir_entry));\n \t\thashmap_entry_init(dir, memihash(ce->name, namelen));\n \t\tdir->namelen = namelen;\n+\t\tadd_ce_ref(ce);\n \t\tdir->ce = ce;\n \t\thashmap_add(&istate->dir_hash, dir);\n \n@@ -92,7 +93,9 @@ static void remove_dir_entry(struct index_state *istate, struct cache_entry *ce)\n \tstruct dir_entry *dir = hash_dir_entry(istate, ce, ce_namelen(ce));\n \twhile (dir && !(--dir->nr)) {\n \t\tstruct dir_entry *parent = dir->parent;\n-\t\thashmap_remove(&istate->dir_hash, dir, NULL);\n+\t\tstruct dir_entry *removed = hashmap_remove(&istate->dir_hash, dir, NULL);\n+\t\tassert(removed == dir);\n+\t\tdrop_ce_ref(dir->ce);\n \t\tfree(dir);\n \t\tdir = parent;\n \t}\n@@ -105,6 +108,7 @@ static void hash_index_entry(struct index_state *istate, struct cache_entry *ce)\n \tce->ce_flags |= CE_HASHED;\n \thashmap_entry_init(ce, memihash(ce->name, ce_namelen(ce)));\n \thashmap_add(&istate->name_hash, ce);\n+\tadd_ce_ref(ce);\n \n \tif (ignore_case)\n \t\tadd_dir_entry(istate, ce);\n@@ -147,6 +151,7 @@ void remove_name_hash(struct index_state *istate, struct cache_entry *ce)\n \t\treturn;\n \tce->ce_flags &= ~CE_HASHED;\n \thashmap_remove(&istate->name_hash, ce, ce);\n+\tdrop_ce_ref(ce);\n \n \tif (ignore_case)\n \t\tremove_dir_entry(istate, ce);\ndiff --git a/read-cache.c b/read-cache.c\nindex 87204a5..442de34 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -53,6 +53,7 @@ static const char *alternate_index_output;\n static void set_index_entry(struct index_state *istate, int nr, struct cache_entry *ce)\n {\n \tistate->cache[nr] = ce;\n+\tadd_ce_ref(ce);\n \tadd_name_hash(istate, ce);\n }\n \n@@ -62,7 +63,7 @@ static void replace_index_entry(struct index_state *istate, int nr, struct cache\n \n \treplace_index_entry_in_base(istate, old, ce);\n \tremove_name_hash(istate, old);\n-\tfree(old);\n+\tdrop_ce_ref(old);\n \tset_index_entry(istate, nr, ce);\n \tce->ce_flags |= CE_UPDATE_IN_BASE;\n \tistate->cache_changed |= CE_ENTRY_CHANGED;\n@@ -75,6 +76,7 @@ void rename_index_entry_at(struct index_state *istate, int nr, const char *new_n\n \n \tnew = xmalloc(cache_entry_size(namelen));\n \tcopy_cache_entry(new, old);\n+\tnew->ref_count = 0;\n \tnew->ce_flags &= ~CE_HASHED;\n \tnew->ce_namelen = namelen;\n \tnew->index = 0;\n@@ -1426,6 +1428,7 @@ static struct cache_entry *cache_entry_from_ondisk(struct ondisk_cache_entry *on\n {\n \tstruct cache_entry *ce = xmalloc(cache_entry_size(len));\n \n+\tce->ref_count = 0;\n \tce->ce_stat_data.sd_ctime.sec = get_be32(&ondisk->ctime.sec);\n \tce->ce_stat_data.sd_mtime.sec = get_be32(&ondisk->mtime.sec);\n \tce->ce_stat_data.sd_ctime.nsec = get_be32(&ondisk->ctime.nsec);\ndiff --git a/split-index.c b/split-index.c\nindex 968b780..ad28f7b 100644\n--- a/split-index.c\n+++ b/split-index.c\n@@ -124,7 +124,7 @@ static void replace_entry(size_t pos, void *data)\n \tsrc->ce_flags |= CE_UPDATE_IN_BASE;\n \tsrc->ce_namelen = dst->ce_namelen;\n \tcopy_cache_entry(dst, src);\n-\tfree(src);\n+\tdrop_ce_ref(src);\n \tsi->nr_replacements++;\n }\n \n@@ -227,7 +227,7 @@ void prepare_to_write_split_index(struct index_state *istate)\n \t\t\tbase->ce_flags = base_flags;\n \t\t\tif (ret)\n \t\t\t\tce->ce_flags |= CE_UPDATE_IN_BASE;\n-\t\t\tfree(base);\n+\t\t\tdrop_ce_ref(base);\n \t\t\tsi->base->cache[ce->index - 1] = ce;\n \t\t}\n \t\tfor (i = 0; i < si->base->cache_nr; i++) {\n@@ -302,7 +302,7 @@ void save_or_free_index_entry(struct index_state *istate, struct cache_entry *ce\n \t    ce == istate->split_index->base->cache[ce->index - 1])\n \t\tce->ce_flags |= CE_REMOVE;\n \telse\n-\t\tfree(ce);\n+\t\tdrop_ce_ref(ce);\n }\n \n void replace_index_entry_in_base(struct index_state *istate,\n@@ -314,8 +314,10 @@ void replace_index_entry_in_base(struct index_state *istate,\n \t    istate->split_index->base &&\n \t    old->index <= istate->split_index->base->cache_nr) {\n \t\tnew->index = old->index;\n-\t\tif (old != istate->split_index->base->cache[new->index - 1])\n-\t\t\tfree(istate->split_index->base->cache[new->index - 1]);\n+\t\tif (old != istate->split_index->base->cache[new->index - 1]) {\n+\t\t\tstruct cache_entry *ce = istate->split_index->base->cache[new->index - 1];\n+\t\t\tdrop_ce_ref(ce);\n+\t\t}\n \t\tistate->split_index->base->cache[new->index - 1] = new;\n \t}\n }\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex f932e80..1a0a637 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -606,8 +606,10 @@ static int unpack_nondirectories(int n, unsigned long mask,\n \t\t\t\t\to);\n \t\tfor (i = 0; i < n; i++) {\n \t\t\tstruct cache_entry *ce = src[i + o->merge];\n-\t\t\tif (ce != o->df_conflict_entry)\n-\t\t\t\tfree(ce);\n+\t\t\tif (ce != o->df_conflict_entry) {\n+\t\t\t\tdrop_ce_ref(ce);\n+\t\t\t\tsrc[i + o->merge] = NULL;\n+\t\t\t}\n \t\t}\n \t\treturn rc;\n \t}\n-- \n2.4.2.644.g97b850b-twtrsrc\n"},{"id":"271349","messageId":"xmqqmvvtqgei.fsf@gitster.mtv.corp.google.com","threadId":"40521","inReplyTo":"1444330071-8909-2-git-send-email-dturner@twitter.com","subject":"Re: [PATCH 0/1] merge: fix cache_entry use-after-free","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-08T20:00:53Z","receivedAt":"2015-10-08T20:00:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Turner <dturner@twopensource.com> writes:\n\n> From: Keith McGuigan <kmcguigan@twitter.com>\n>\n> During merges, we would previously free entries that we no longer need\n> in the destination index.  But those entries might also be stored in\n> the dir_entry cache, and when a later call to add_to_index found them,\n> they would be used after being freed.\n>\n> To prevent this, add a ref count for struct cache_entry.  Whenever\n> a cache entry is added to a data structure, the ref count is incremented;\n> when it is removed from the data structure, it is decremented.  When\n> it hits zero, the cache_entry is freed.\n>\n> Signed-off-by: David Turner <dturner@twopensource.com>\n> Signed-off-by: Keith McGuigan <kmcguigan@twitter.com>\n> ---\n\nThanks.\n\n> @@ -213,6 +214,32 @@ struct cache_entry {\n>  struct pathspec;\n>  \n>  /*\n> + * Increment the cache_entry reference count.  Should be called\n> + * whenever a pointer to a cache_entry is retained in a data structure,\n> + * thus marking it as alive.\n> + */\n> +static inline void add_ce_ref(struct cache_entry *ce)\n> +{\n> +\tassert(ce != NULL && ce->ref_count >= 0);\n> +\t++ce->ref_count;\n> +}\n\nWe tend to prefer post-increment when the distinction between pre-\nor post- does not make any difference, which is the case here.\n\n> diff --git a/name-hash.c b/name-hash.c\n> index 702cd05..f12c919 100644\n> --- a/name-hash.c\n> +++ b/name-hash.c\n> @@ -92,7 +93,9 @@ static void remove_dir_entry(struct index_state *istate, struct cache_entry *ce)\n>  \tstruct dir_entry *dir = hash_dir_entry(istate, ce, ce_namelen(ce));\n>  \twhile (dir && !(--dir->nr)) {\n>  \t\tstruct dir_entry *parent = dir->parent;\n> -\t\thashmap_remove(&istate->dir_hash, dir, NULL);\n> +\t\tstruct dir_entry *removed = hashmap_remove(&istate->dir_hash, dir, NULL);\n> +\t\tassert(removed == dir);\n> +\t\tdrop_ce_ref(dir->ce);\n\nThis is curious.  In remove_name_hash() you do not have the\ncorresponding assert.  Why is it necessary here (or is it\nunnecessary over there)?\n\n> @@ -147,6 +151,7 @@ void remove_name_hash(struct index_state *istate, struct cache_entry *ce)\n>  \t\treturn;\n>  \tce->ce_flags &= ~CE_HASHED;\n>  \thashmap_remove(&istate->name_hash, ce, ce);\n> +\tdrop_ce_ref(ce);\n>  \n>  \tif (ignore_case)\n>  \t\tremove_dir_entry(istate, ce);\n> diff --git a/read-cache.c b/read-cache.c\n> index 87204a5..442de34 100644\n> --- a/read-cache.c\n> +++ b/read-cache.c\n> @@ -53,6 +53,7 @@ static const char *alternate_index_output;\n>  static void set_index_entry(struct index_state *istate, int nr, struct cache_entry *ce)\n>  {\n>  \tistate->cache[nr] = ce;\n> +\tadd_ce_ref(ce);\n>  \tadd_name_hash(istate, ce);\n>  }\n\nWhat happens to the existing entry that used to be istate->cache[nr],\nwhich may or may not be 'ce' that is replacing it?\n\nIt turns out that all three calling sites are safe, but it is not\nimmediately obvious why.  Perhaps some comment in front of the\nfunction is in order, to warn those who may have to add a new caller\nor restructure the existing calling chain, that istate->cache[nr] is\nexpected not to hold a live reference when the function is called,\nor something?\n\n> @@ -314,8 +314,10 @@ void replace_index_entry_in_base(struct index_state *istate,\n>  \t    istate->split_index->base &&\n>  \t    old->index <= istate->split_index->base->cache_nr) {\n>  \t\tnew->index = old->index;\n> -\t\tif (old != istate->split_index->base->cache[new->index - 1])\n> -\t\t\tfree(istate->split_index->base->cache[new->index - 1]);\n> +\t\tif (old != istate->split_index->base->cache[new->index - 1]) {\n> +\t\t\tstruct cache_entry *ce = istate->split_index->base->cache[new->index - 1];\n> +\t\t\tdrop_ce_ref(ce);\n> +\t\t}\n>  \t\tistate->split_index->base->cache[new->index - 1] = new;\n\nDoes 'new' already have the right refcount at this point?\n"},{"id":"271393","messageId":"1444428963.8836.36.camel@twopensource.com","threadId":"40521","inReplyTo":"xmqqmvvtqgei.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 0/1] merge: fix cache_entry use-after-free","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2015-10-09T22:16:03Z","receivedAt":"2015-10-09T22:16:03Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"+Duy Nguyen, who knows the split index better.\n\nOn Thu, 2015-10-08 at 13:00 -0700, Junio C Hamano wrote:\n> David Turner <dturner@twopensource.com> writes:\n> \n> > From: Keith McGuigan <kmcguigan@twitter.com>\n> >\n> > During merges, we would previously free entries that we no longer need\n> > in the destination index.  But those entries might also be stored in\n> > the dir_entry cache, and when a later call to add_to_index found them,\n> > they would be used after being freed.\n> >\n> > To prevent this, add a ref count for struct cache_entry.  Whenever\n> > a cache entry is added to a data structure, the ref count is incremented;\n> > when it is removed from the data structure, it is decremented.  When\n> > it hits zero, the cache_entry is freed.\n> >\n> > Signed-off-by: David Turner <dturner@twopensource.com>\n> > Signed-off-by: Keith McGuigan <kmcguigan@twitter.com>\n> > ---\n> \n> Thanks.\n> \n> > @@ -213,6 +214,32 @@ struct cache_entry {\n> >  struct pathspec;\n> >  \n> >  /*\n> > + * Increment the cache_entry reference count.  Should be called\n> > + * whenever a pointer to a cache_entry is retained in a data structure,\n> > + * thus marking it as alive.\n> > + */\n> > +static inline void add_ce_ref(struct cache_entry *ce)\n> > +{\n> > +\tassert(ce != NULL && ce->ref_count >= 0);\n> > +\t++ce->ref_count;\n> > +}\n> \n> We tend to prefer post-increment when the distinction between pre-\n> or post- does not make any difference, which is the case here.\n\nWill fix.\n\n> > diff --git a/name-hash.c b/name-hash.c\n> > index 702cd05..f12c919 100644\n> > --- a/name-hash.c\n> > +++ b/name-hash.c\n> > @@ -92,7 +93,9 @@ static void remove_dir_entry(struct index_state *istate, struct cache_entry *ce)\n> >  \tstruct dir_entry *dir = hash_dir_entry(istate, ce, ce_namelen(ce));\n> >  \twhile (dir && !(--dir->nr)) {\n> >  \t\tstruct dir_entry *parent = dir->parent;\n> > -\t\thashmap_remove(&istate->dir_hash, dir, NULL);\n> > +\t\tstruct dir_entry *removed = hashmap_remove(&istate->dir_hash, dir, NULL);\n> > +\t\tassert(removed == dir);\n> > +\t\tdrop_ce_ref(dir->ce);\n> \n> This is curious.  In remove_name_hash() you do not have the\n> corresponding assert.  Why is it necessary here (or is it\n> unnecessary over there)?\n\nIt is unnecessary in any case: it's an assert rather than a check for an\nexpected (or even unexpected) case.  That just happens to be where Keith\nfirst managed to track down the use-after free, so that's where he\nhappened to put the assert while trying to debug it.  I'm in general in\nfavor of more asserts rather than fewer, so I would prefer to keep it\nin.  But I can remove it if you like.\n\n> > @@ -147,6 +151,7 @@ void remove_name_hash(struct index_state *istate, struct cache_entry *ce)\n> >  \t\treturn;\n> >  \tce->ce_flags &= ~CE_HASHED;\n> >  \thashmap_remove(&istate->name_hash, ce, ce);\n> > +\tdrop_ce_ref(ce);\n> >  \n> >  \tif (ignore_case)\n> >  \t\tremove_dir_entry(istate, ce);\n> > diff --git a/read-cache.c b/read-cache.c\n> > index 87204a5..442de34 100644\n> > --- a/read-cache.c\n> > +++ b/read-cache.c\n> > @@ -53,6 +53,7 @@ static const char *alternate_index_output;\n> >  static void set_index_entry(struct index_state *istate, int nr, struct cache_entry *ce)\n> >  {\n> >  \tistate->cache[nr] = ce;\n> > +\tadd_ce_ref(ce);\n> >  \tadd_name_hash(istate, ce);\n> >  }\n> \n> What happens to the existing entry that used to be istate->cache[nr],\n> which may or may not be 'ce' that is replacing it?\n> \n> It turns out that all three calling sites are safe, but it is not\n> immediately obvious why.  Perhaps some comment in front of the\n> function is in order, to warn those who may have to add a new caller\n> or restructure the existing calling chain, that istate->cache[nr] is\n> expected not to hold a live reference when the function is called,\n> or something?\n\nHappy to add it if you want, but to me it was clear without because if\nthere were a value in istate->cache[nr], that old value would be leaked.\nLet me know.\n\n> > @@ -314,8 +314,10 @@ void replace_index_entry_in_base(struct index_state *istate,\n> >  \t    istate->split_index->base &&\n> >  \t    old->index <= istate->split_index->base->cache_nr) {\n> >  \t\tnew->index = old->index;\n> > -\t\tif (old != istate->split_index->base->cache[new->index - 1])\n> > -\t\t\tfree(istate->split_index->base->cache[new->index - 1]);\n> > +\t\tif (old != istate->split_index->base->cache[new->index - 1]) {\n> > +\t\t\tstruct cache_entry *ce = istate->split_index->base->cache[new->index - 1];\n> > +\t\t\tdrop_ce_ref(ce);\n> > +\t\t}\n> >  \t\tistate->split_index->base->cache[new->index - 1] = new;\n> \n> Does 'new' already have the right refcount at this point?\n\nI spoke to Keith, and he thinks we do need an add_ce_ref there. I'll fix\nthat on the reroll.  Duy, do you agree?\n"},{"id":"271394","messageId":"xmqqfv1joduc.fsf@gitster.mtv.corp.google.com","threadId":"40521","inReplyTo":"1444428963.8836.36.camel@twopensource.com","subject":"Re: [PATCH 0/1] merge: fix cache_entry use-after-free","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-09T22:51:23Z","receivedAt":"2015-10-09T22:51:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Turner <dturner@twopensource.com> writes:\n\n>> > +\t\tassert(removed == dir);\n>> > +\t\tdrop_ce_ref(dir->ce);\n>> \n>> This is curious.  In remove_name_hash() you do not have the\n>> corresponding assert.  Why is it necessary here (or is it\n>> unnecessary over there)?\n>\n> It is unnecessary in any case: it's an assert rather than a check for an\n> expected (or even unexpected) case.  That just happens to be where Keith\n> first managed to track down the use-after free, so that's where he\n> happened to put the assert while trying to debug it.  I'm in general in\n> favor of more asserts rather than fewer, so I would prefer to keep it\n> in.  But I can remove it if you like.\n\nOK, it was just the inconsistency between the two made them look\ncurious, as if one of them is more likely to get broken, or the\npatch author was not so sure about the assumption, or something.\n\n>> > +\tadd_ce_ref(ce);\n>> >  \tadd_name_hash(istate, ce);\n>> >  }\n>> \n>> What happens to the existing entry that used to be istate->cache[nr],\n>> which may or may not be 'ce' that is replacing it?\n>> \n>> It turns out that all three calling sites are safe, but it is not\n>> immediately obvious why.  Perhaps some comment in front of the\n>> function is in order, to warn those who may have to add a new caller\n>> or restructure the existing calling chain, that istate->cache[nr] is\n>> expected not to hold a live reference when the function is called,\n>> or something?\n>\n> Happy to add it if you want, but to me it was clear without because if\n> there were a value in istate->cache[nr], that old value would be leaked.\n\nOK, that's sort-of-cheating, but is a sound short-cut ;-).\n\n>> > +\t\tif (old != istate->split_index->base->cache[new->index - 1]) {\n>> > +\t\t\tstruct cache_entry *ce = istate->split_index->base->cache[new->index - 1];\n>> > +\t\t\tdrop_ce_ref(ce);\n>> > +\t\t}\n>> >  \t\tistate->split_index->base->cache[new->index - 1] = new;\n>> \n>> Does 'new' already have the right refcount at this point?\n>\n> I spoke to Keith, and he thinks we do need an add_ce_ref there. I'll fix\n> that on the reroll.  Duy, do you agree?\n"}]}