| Message ID | 20190209112328.26317-1-pclouds@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | unpack-trees.c: fix writing "link" index ext with null base oid | expand |
On Sat, 9 Feb 2019 at 11:23, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote: > > Since commit 7db118303a (unpack_trees: fix breakage when o->src_index != > o->dst_index - 2018-04-23) and changes in merge code to use separate > index_state for source and destination, when doing a merge with split > index activated, we may run into this line in unpack_trees(): > > o->result.split_index = init_split_index(&o->result); > > This is by itself not wrong. But this split index information is not > fully populated (and it's only so when move_cache_to_base_index() is > called, aka force splitting the index, or loading index_state from a > file). Both "base_oid" and "base" in this case remain null. > > So when writing the main index down, we link to this index with null > oid (default value after init_split_index()), which also means "no split > index" internally. This triggers an incorrect base index refresh: > > warning: could not freshen shared index '.../sharedindex.0{40}' > > This patch makes sure we will not refresh null base_oid (because the > file is never there). It also makes sure not to write "link" extension > with null base_oid in the first place (no point having it at > all). Read code already has protection against null base_oid. > > There is also another side fix in remove_split_index() that causes a > crash when doing "git update-index --no-split-index" when base_oid in > the index file is null. In this case we will not load > istate->split_index->base but we dereference it anyway and are rewarded > with a segfault. This should not happen anymore, but it's still wrong to > dereference a potential NULL pointer, especially when we do check for > NULL pointer in the next code. > > Reported-by: Luke Diamand <luke@diamand.org> > Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> > --- > I considered adding a test, but since the problem is a warning, not > sure how to catch that. And a test would not be able to verify all > changes in this patch anyway. > > read-cache.c | 5 +++-- > split-index.c | 34 ++++++++++++++++++---------------- > 2 files changed, 21 insertions(+), 18 deletions(-) > > diff --git a/read-cache.c b/read-cache.c > index 0e0c93edc9..d6fb09984f 100644 > --- a/read-cache.c > +++ b/read-cache.c > @@ -2894,7 +2894,8 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile, > return -1; > } > > - if (!strip_extensions && istate->split_index) { > + if (!strip_extensions && istate->split_index && > + !is_null_oid(&istate->split_index->base_oid)) { > struct strbuf sb = STRBUF_INIT; > > err = write_link_extension(&sb, istate) < 0 || > @@ -3189,7 +3190,7 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock, > ret = write_split_index(istate, lock, flags); > > /* Freshen the shared index only if the split-index was written */ > - if (!ret && !new_shared_index) { > + if (!ret && !new_shared_index && !is_null_oid(&si->base_oid)) { > const char *shared_index = git_path("sharedindex.%s", > oid_to_hex(&si->base_oid)); > freshen_shared_index(shared_index, 1); > diff --git a/split-index.c b/split-index.c > index 5820412dc5..a9d13611a4 100644 > --- a/split-index.c > +++ b/split-index.c > @@ -440,24 +440,26 @@ void add_split_index(struct index_state *istate) > void remove_split_index(struct index_state *istate) > { > if (istate->split_index) { > - /* > - * When removing the split index, we need to move > - * ownership of the mem_pool associated with the > - * base index to the main index. There may be cache entries > - * allocated from the base's memory pool that are shared with > - * the_index.cache[]. > - */ > - mem_pool_combine(istate->ce_mem_pool, istate->split_index->base->ce_mem_pool); > + if (istate->split_index->base) { > + /* > + * When removing the split index, we need to move > + * ownership of the mem_pool associated with the > + * base index to the main index. There may be cache entries > + * allocated from the base's memory pool that are shared with > + * the_index.cache[]. > + */ > + mem_pool_combine(istate->ce_mem_pool, > + istate->split_index->base->ce_mem_pool); > > - /* > - * The split index no longer owns the mem_pool backing > - * its cache array. As we are discarding this index, > - * mark the index as having no cache entries, so it > - * will not attempt to clean up the cache entries or > - * validate them. > - */ > - if (istate->split_index->base) > + /* > + * The split index no longer owns the mem_pool backing > + * its cache array. As we are discarding this index, > + * mark the index as having no cache entries, so it > + * will not attempt to clean up the cache entries or > + * validate them. > + */ > istate->split_index->base->cache_nr = 0; > + } > > /* > * We can discard the split index because its > -- > 2.20.1.682.gd5861c6d90 Ack, I no longer see those messages. >
On Sat, Feb 9, 2019 at 3:23 AM Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote: > > Since commit 7db118303a (unpack_trees: fix breakage when o->src_index != > o->dst_index - 2018-04-23) and changes in merge code to use separate > index_state for source and destination, when doing a merge with split > index activated, we may run into this line in unpack_trees(): > > o->result.split_index = init_split_index(&o->result); > > This is by itself not wrong. But this split index information is not > fully populated (and it's only so when move_cache_to_base_index() is > called, aka force splitting the index, or loading index_state from a > file). Both "base_oid" and "base" in this case remain null. > > So when writing the main index down, we link to this index with null > oid (default value after init_split_index()), which also means "no split > index" internally. This triggers an incorrect base index refresh: > > warning: could not freshen shared index '.../sharedindex.0{40}' > > This patch makes sure we will not refresh null base_oid (because the > file is never there). It also makes sure not to write "link" extension > with null base_oid in the first place (no point having it at > all). Read code already has protection against null base_oid. > > There is also another side fix in remove_split_index() that causes a > crash when doing "git update-index --no-split-index" when base_oid in > the index file is null. In this case we will not load > istate->split_index->base but we dereference it anyway and are rewarded > with a segfault. This should not happen anymore, but it's still wrong to > dereference a potential NULL pointer, especially when we do check for > NULL pointer in the next code. > > Reported-by: Luke Diamand <luke@diamand.org> > Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> Thanks for digging this down and fixing it. When I saw this split index bug bisect to me that my heart sank a little; there's so much of that code I don't understand. Nice to see you've already come along and fixed it all up. :-) > --- > I considered adding a test, but since the problem is a warning, not > sure how to catch that. And a test would not be able to verify all > changes in this patch anyway. > > read-cache.c | 5 +++-- > split-index.c | 34 ++++++++++++++++++---------------- > 2 files changed, 21 insertions(+), 18 deletions(-) > > diff --git a/read-cache.c b/read-cache.c > index 0e0c93edc9..d6fb09984f 100644 > --- a/read-cache.c > +++ b/read-cache.c > @@ -2894,7 +2894,8 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile, > return -1; > } > > - if (!strip_extensions && istate->split_index) { > + if (!strip_extensions && istate->split_index && > + !is_null_oid(&istate->split_index->base_oid)) { > struct strbuf sb = STRBUF_INIT; > > err = write_link_extension(&sb, istate) < 0 || > @@ -3189,7 +3190,7 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock, > ret = write_split_index(istate, lock, flags); > > /* Freshen the shared index only if the split-index was written */ > - if (!ret && !new_shared_index) { > + if (!ret && !new_shared_index && !is_null_oid(&si->base_oid)) { > const char *shared_index = git_path("sharedindex.%s", > oid_to_hex(&si->base_oid)); > freshen_shared_index(shared_index, 1); > diff --git a/split-index.c b/split-index.c > index 5820412dc5..a9d13611a4 100644 > --- a/split-index.c > +++ b/split-index.c > @@ -440,24 +440,26 @@ void add_split_index(struct index_state *istate) > void remove_split_index(struct index_state *istate) > { > if (istate->split_index) { > - /* > - * When removing the split index, we need to move > - * ownership of the mem_pool associated with the > - * base index to the main index. There may be cache entries > - * allocated from the base's memory pool that are shared with > - * the_index.cache[]. > - */ > - mem_pool_combine(istate->ce_mem_pool, istate->split_index->base->ce_mem_pool); > + if (istate->split_index->base) { > + /* > + * When removing the split index, we need to move > + * ownership of the mem_pool associated with the > + * base index to the main index. There may be cache entries > + * allocated from the base's memory pool that are shared with > + * the_index.cache[]. > + */ > + mem_pool_combine(istate->ce_mem_pool, > + istate->split_index->base->ce_mem_pool); > > - /* > - * The split index no longer owns the mem_pool backing > - * its cache array. As we are discarding this index, > - * mark the index as having no cache entries, so it > - * will not attempt to clean up the cache entries or > - * validate them. > - */ > - if (istate->split_index->base) > + /* > + * The split index no longer owns the mem_pool backing > + * its cache array. As we are discarding this index, > + * mark the index as having no cache entries, so it > + * will not attempt to clean up the cache entries or > + * validate them. > + */ > istate->split_index->base->cache_nr = 0; > + } > > /* > * We can discard the split index because its > -- > 2.20.1.682.gd5861c6d90 >
On Sat, Feb 09 2019, Nguyễn Thái Ngọc Duy wrote: I ran into this bug in the past but wasn't able to reproduce it long enough to track it down. Thanks for the fix. It would be nice to have a test for it if it's easily narrowed down, tricky edge cases we fix are worth testing for... > I considered adding a test, but since the problem is a warning, not > sure how to catch that. And a test would not be able to verify all > changes in this patch anyway. You'd write the test like this: git [...] 2>stderr && test_i18ngrep "could not.*000000000" stderr Or something like that. We have a lot of tests like this already, some can be found with: git grep -B1 test_i18ngrep.*stderr So something being a warning shouldn't stop us from testing for it.
Elijah Newren <newren@gmail.com> writes: >> There is also another side fix in remove_split_index() that causes a >> crash when doing "git update-index --no-split-index" when base_oid in >> the index file is null. In this case we will not load >> istate->split_index->base but we dereference it anyway and are rewarded >> with a segfault. This should not happen anymore, but it's still wrong to >> dereference a potential NULL pointer, especially when we do check for >> NULL pointer in the next code. >> >> Reported-by: Luke Diamand <luke@diamand.org> >> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> > > Thanks for digging this down and fixing it. When I saw this split > index bug bisect to me that my heart sank a little; there's so much of > that code I don't understand. Nice to see you've already come along > and fixed it all up. :-) Thanks, all.
diff --git a/read-cache.c b/read-cache.c index 0e0c93edc9..d6fb09984f 100644 --- a/read-cache.c +++ b/read-cache.c @@ -2894,7 +2894,8 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile, return -1; } - if (!strip_extensions && istate->split_index) { + if (!strip_extensions && istate->split_index && + !is_null_oid(&istate->split_index->base_oid)) { struct strbuf sb = STRBUF_INIT; err = write_link_extension(&sb, istate) < 0 || @@ -3189,7 +3190,7 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock, ret = write_split_index(istate, lock, flags); /* Freshen the shared index only if the split-index was written */ - if (!ret && !new_shared_index) { + if (!ret && !new_shared_index && !is_null_oid(&si->base_oid)) { const char *shared_index = git_path("sharedindex.%s", oid_to_hex(&si->base_oid)); freshen_shared_index(shared_index, 1); diff --git a/split-index.c b/split-index.c index 5820412dc5..a9d13611a4 100644 --- a/split-index.c +++ b/split-index.c @@ -440,24 +440,26 @@ void add_split_index(struct index_state *istate) void remove_split_index(struct index_state *istate) { if (istate->split_index) { - /* - * When removing the split index, we need to move - * ownership of the mem_pool associated with the - * base index to the main index. There may be cache entries - * allocated from the base's memory pool that are shared with - * the_index.cache[]. - */ - mem_pool_combine(istate->ce_mem_pool, istate->split_index->base->ce_mem_pool); + if (istate->split_index->base) { + /* + * When removing the split index, we need to move + * ownership of the mem_pool associated with the + * base index to the main index. There may be cache entries + * allocated from the base's memory pool that are shared with + * the_index.cache[]. + */ + mem_pool_combine(istate->ce_mem_pool, + istate->split_index->base->ce_mem_pool); - /* - * The split index no longer owns the mem_pool backing - * its cache array. As we are discarding this index, - * mark the index as having no cache entries, so it - * will not attempt to clean up the cache entries or - * validate them. - */ - if (istate->split_index->base) + /* + * The split index no longer owns the mem_pool backing + * its cache array. As we are discarding this index, + * mark the index as having no cache entries, so it + * will not attempt to clean up the cache entries or + * validate them. + */ istate->split_index->base->cache_nr = 0; + } /* * We can discard the split index because its
Since commit 7db118303a (unpack_trees: fix breakage when o->src_index != o->dst_index - 2018-04-23) and changes in merge code to use separate index_state for source and destination, when doing a merge with split index activated, we may run into this line in unpack_trees(): o->result.split_index = init_split_index(&o->result); This is by itself not wrong. But this split index information is not fully populated (and it's only so when move_cache_to_base_index() is called, aka force splitting the index, or loading index_state from a file). Both "base_oid" and "base" in this case remain null. So when writing the main index down, we link to this index with null oid (default value after init_split_index()), which also means "no split index" internally. This triggers an incorrect base index refresh: warning: could not freshen shared index '.../sharedindex.0{40}' This patch makes sure we will not refresh null base_oid (because the file is never there). It also makes sure not to write "link" extension with null base_oid in the first place (no point having it at all). Read code already has protection against null base_oid. There is also another side fix in remove_split_index() that causes a crash when doing "git update-index --no-split-index" when base_oid in the index file is null. In this case we will not load istate->split_index->base but we dereference it anyway and are rewarded with a segfault. This should not happen anymore, but it's still wrong to dereference a potential NULL pointer, especially when we do check for NULL pointer in the next code. Reported-by: Luke Diamand <luke@diamand.org> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> --- I considered adding a test, but since the problem is a warning, not sure how to catch that. And a test would not be able to verify all changes in this patch anyway. read-cache.c | 5 +++-- split-index.c | 34 ++++++++++++++++++---------------- 2 files changed, 21 insertions(+), 18 deletions(-)