# could not freshen shared index ..../sharedindex.0000000000000000000000000000000000000000' 16 messages from 2019-02-06 to 2019-02-13. Participants: Luke Diamand, Christian Couder, Duy Nguyen, Junio C Hamano, Nguyễn Thái Ngọc Duy, Elijah Newren, Ævar Arnfjörð Bjarmason. Thread: https://gitlist.dev/t/50409 ## Luke Diamand, 2019-02-06 10:25 Subject: could not freshen shared index ..../sharedindex.0000000000000000000000000000000000000000' Message-ID: URL: https://gitlist.dev/e/CAE5ih79DuU5Wrk4p%2BwpQX3j6bgyA2_ZgXYJX-tGvZpYhWazWqA%40mail.gmail.com ``` I've recently started seeing a lot of this message when doing a rebase: warning: could not freshen shared index '/home/ldiamand/git/dev_full/.git/worktrees/gcc8-take-2/sharedindex.0000000000000000000000000000000000000000' (There's a repo called dev_full, and I've got a worktree where I'm working on my 3rd attempt to make it work with gcc8). That file doesn't actually exist but there are a bunch of sharedindex.XXX files in there with more convincing looking names. 2.20.1.611.gfbb209baf1 ``` ## Christian Couder, 2019-02-06 13:17 Subject: Re: could not freshen shared index ..../sharedindex.0000000000000000000000000000000000000000' Message-ID: URL: https://gitlist.dev/e/CAP8UFD3K47mfp_7gt2J19TZCXH4u4kf5D8McXd9ioguWb6umZQ%40mail.gmail.com In-Reply-To: ``` On Wed, Feb 6, 2019 at 11:25 AM Luke Diamand wrote: > > I've recently started seeing a lot of this message when doing a rebase: > > warning: could not freshen shared index > '/home/ldiamand/git/dev_full/.git/worktrees/gcc8-take-2/sharedindex.0000000000000000000000000000000000000000' Thanks for the report! Have you tried to run the test suite with GIT_TEST_SPLIT_INDEX set to "true" on your machine/environment? Also does the rebase fail or otherwise misbehave? ``` ## Duy Nguyen, 2019-02-08 10:02 Subject: Re: could not freshen shared index ..../sharedindex.0000000000000000000000000000000000000000' Message-ID: <20190208100221.GA22283@ash> URL: https://gitlist.dev/e/20190208100221.GA22283%40ash In-Reply-To: ``` On Wed, Feb 06, 2019 at 10:25:25AM +0000, Luke Diamand wrote: > I've recently started seeing a lot of this message when doing a rebase: > > warning: could not freshen shared index > '/home/ldiamand/git/dev_full/.git/worktrees/gcc8-take-2/sharedindex.0000000000000000000000000000000000000000' There are only two places in the code that could print this. The one in read_index_from() can't happen unless is_null_oid() is broken (very very unlikely). The other one is in write_locked_index() which could happen in theory but I don't understand how it got there. If you could build git, could you try this patch and see if it helps? -- 8< -- diff --git a/read-cache.c b/read-cache.c index f68b367613..5ad71478dc 100644 --- a/read-cache.c +++ b/read-cache.c @@ -3165,6 +3165,7 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock, fill_fsmonitor_bitmap(istate); if (!si || alternate_index_output || + (si && is_null_oid(&si->base_oid)) || (istate->cache_changed & ~EXTMASK)) { if (si) oidclr(&si->base_oid); -- 8< -- > (There's a repo called dev_full, and I've got a worktree where I'm > working on my 3rd attempt to make it work with gcc8). > > That file doesn't actually exist but there are a bunch of > sharedindex.XXX files in there with more convincing looking names. > > 2.20.1.611.gfbb209baf1 -- Duy ``` ## Luke Diamand, 2019-02-08 16:38 Subject: Re: could not freshen shared index ..../sharedindex.0000000000000000000000000000000000000000' Message-ID: URL: https://gitlist.dev/e/CAE5ih7_b_LXfPp5eMcrr4iSuyzaiKxvQqN73Osw%2Bck9CPrfdqw%40mail.gmail.com In-Reply-To: <20190208100221.GA22283@ash> ``` On Fri, 8 Feb 2019 at 10:02, Duy Nguyen wrote: > > On Wed, Feb 06, 2019 at 10:25:25AM +0000, Luke Diamand wrote: > > I've recently started seeing a lot of this message when doing a rebase: > > > > warning: could not freshen shared index > > '/home/ldiamand/git/dev_full/.git/worktrees/gcc8-take-2/sharedindex.0000000000000000000000000000000000000000' > > There are only two places in the code that could print this. The one > in read_index_from() can't happen unless is_null_oid() is broken (very > very unlikely). > > The other one is in write_locked_index() which could happen in theory > but I don't understand how it got there. If you could build git, could > you try this patch and see if it helps? They've gone away! > > -- 8< -- > diff --git a/read-cache.c b/read-cache.c > index f68b367613..5ad71478dc 100644 > --- a/read-cache.c > +++ b/read-cache.c > @@ -3165,6 +3165,7 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock, > fill_fsmonitor_bitmap(istate); > > if (!si || alternate_index_output || > + (si && is_null_oid(&si->base_oid)) || > (istate->cache_changed & ~EXTMASK)) { > if (si) > oidclr(&si->base_oid); > -- 8< -- > Luke ``` ## Junio C Hamano, 2019-02-08 17:23 Subject: Re: could not freshen shared index ..../sharedindex.0000000000000000000000000000000000000000' Message-ID: URL: https://gitlist.dev/e/xmqqzhr6kxcj.fsf%40gitster-ct.c.googlers.com In-Reply-To: <20190208100221.GA22283@ash> ``` Duy Nguyen writes: > On Wed, Feb 06, 2019 at 10:25:25AM +0000, Luke Diamand wrote: >> I've recently started seeing a lot of this message when doing a rebase: >> >> warning: could not freshen shared index >> '/home/ldiamand/git/dev_full/.git/worktrees/gcc8-take-2/sharedindex.0000000000000000000000000000000000000000' > > There are only two places in the code that could print this. The one > in read_index_from() can't happen unless is_null_oid() is broken (very > very unlikely). > > The other one is in write_locked_index() which could happen in theory > but I don't understand how it got there. If you could build git, could > you try this patch and see if it helps? ... meaning, if it hides the symptom, we'd know the codepath that causes a NULL si->base_oid to appear here is the culprit? Or do you mean that it is expected si->base_oid sometimes is NULL and we should have been pretending as if si were NULL (i.e. split_index is not being used)? I take it as the latter (i.e. "helps" to narrow down the bug hunting field, not "helps" by fixing the bug). Thanks. > > -- 8< -- > diff --git a/read-cache.c b/read-cache.c > index f68b367613..5ad71478dc 100644 > --- a/read-cache.c > +++ b/read-cache.c > @@ -3165,6 +3165,7 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock, > fill_fsmonitor_bitmap(istate); > > if (!si || alternate_index_output || > + (si && is_null_oid(&si->base_oid)) || > (istate->cache_changed & ~EXTMASK)) { > if (si) > oidclr(&si->base_oid); > -- 8< -- > > >> (There's a repo called dev_full, and I've got a worktree where I'm >> working on my 3rd attempt to make it work with gcc8). >> >> That file doesn't actually exist but there are a bunch of >> sharedindex.XXX files in there with more convincing looking names. >> >> 2.20.1.611.gfbb209baf1 > -- > Duy ``` ## Duy Nguyen, 2019-02-09 04:56 Subject: Re: could not freshen shared index ..../sharedindex.0000000000000000000000000000000000000000' Message-ID: URL: https://gitlist.dev/e/CACsJy8AzQLvWNq4hGGva%3DW4CKJAKHXiopF-cVw--wdJE2npdGQ%40mail.gmail.com In-Reply-To: ``` On Sat, Feb 9, 2019 at 12:24 AM Junio C Hamano wrote: > > Duy Nguyen writes: > > > On Wed, Feb 06, 2019 at 10:25:25AM +0000, Luke Diamand wrote: > >> I've recently started seeing a lot of this message when doing a rebase: > >> > >> warning: could not freshen shared index > >> '/home/ldiamand/git/dev_full/.git/worktrees/gcc8-take-2/sharedindex.0000000000000000000000000000000000000000' > > > > There are only two places in the code that could print this. The one > > in read_index_from() can't happen unless is_null_oid() is broken (very > > very unlikely). > > > > The other one is in write_locked_index() which could happen in theory > > but I don't understand how it got there. If you could build git, could > > you try this patch and see if it helps? > > ... meaning, if it hides the symptom, we'd know the codepath that > causes a NULL si->base_oid to appear here is the culprit? Or do you > mean that it is expected si->base_oid sometimes is NULL and we should > have been pretending as if si were NULL (i.e. split_index is not being > used)? I didn't go that far when I suggested my patch. Looking more at the code, I think we may have written the "link" extension with null oid. Which could trigger this case and is definitely wrong. > I take it as the latter (i.e. "helps" to narrow down the bug hunting > field, not "helps" by fixing the bug). Yeah definitely not the fix (how can I write "I don't know why" in the commit message for the fix). At least now I know I'm on the right track. Szeder recently fixed some racy bug in split-index.c, which seems fit in this rebase scenario since we'll be updating the index very often in a short period of time. Perhaps that uncovers some case that we don't handle well. I haven't been able to connect the dots though. -- Duy ``` ## Duy Nguyen, 2019-02-09 05:00 Subject: Re: could not freshen shared index ..../sharedindex.0000000000000000000000000000000000000000' Message-ID: URL: https://gitlist.dev/e/CACsJy8CeRFyhkdohwU_KtXtgmkOzNsQPb4oKFcn%2BR210MpS1KA%40mail.gmail.com In-Reply-To: ``` On Fri, Feb 8, 2019 at 11:39 PM Luke Diamand wrote: > > On Fri, 8 Feb 2019 at 10:02, Duy Nguyen wrote: > > > > On Wed, Feb 06, 2019 at 10:25:25AM +0000, Luke Diamand wrote: > > > I've recently started seeing a lot of this message when doing a rebase: > > > > > > warning: could not freshen shared index > > > '/home/ldiamand/git/dev_full/.git/worktrees/gcc8-take-2/sharedindex.0000000000000000000000000000000000000000' > > > > There are only two places in the code that could print this. The one > > in read_index_from() can't happen unless is_null_oid() is broken (very > > very unlikely). > > > > The other one is in write_locked_index() which could happen in theory > > but I don't understand how it got there. If you could build git, could > > you try this patch and see if it helps? > > They've gone away! Great! Since you seem able to reproduce this (and can build git!) could you try bisect to pin point the commit that causes this problem? That would help a lot. I think you could start maybe from v2.19.0 -- Duy ``` ## Luke Diamand, 2019-02-09 09:57 Subject: Re: could not freshen shared index ..../sharedindex.0000000000000000000000000000000000000000' Message-ID: URL: https://gitlist.dev/e/CAE5ih79JYrcV9cxMBU88Hq%3DHHQOOyzpkq%2BkT2zAgDzs%3Dao%2BPMg%40mail.gmail.com In-Reply-To: ``` On Sat, 9 Feb 2019 at 05:01, Duy Nguyen wrote: > > On Fri, Feb 8, 2019 at 11:39 PM Luke Diamand wrote: > > > > On Fri, 8 Feb 2019 at 10:02, Duy Nguyen wrote: > > > > > > On Wed, Feb 06, 2019 at 10:25:25AM +0000, Luke Diamand wrote: > > > > I've recently started seeing a lot of this message when doing a rebase: > > > > > > > > warning: could not freshen shared index > > > > '/home/ldiamand/git/dev_full/.git/worktrees/gcc8-take-2/sharedindex.0000000000000000000000000000000000000000' > > > > > > There are only two places in the code that could print this. The one > > > in read_index_from() can't happen unless is_null_oid() is broken (very > > > very unlikely). > > > > > > The other one is in write_locked_index() which could happen in theory > > > but I don't understand how it got there. If you could build git, could > > > you try this patch and see if it helps? > > > > They've gone away! > > Great! Since you seem able to reproduce this (and can build git!) > could you try bisect to pin point the commit that causes this problem? > That would help a lot. I think you could start maybe from v2.19.0 The first bad commit was d658196f3c4b2478ebdff638e2c3e4fb2f9cbba2. $ git show d658196f3c4b2478ebdff638e2c3e4fb2f9cbba2 commit d658196f3c4b2478ebdff638e2c3e4fb2f9cbba2 (refs/bisect/bad) Merge: 6b0f1d9c47 7db118303a Author: Junio C Hamano Date: Wed May 23 14:38:22 2018 +0900 Merge branch 'en/unpack-trees-split-index-fix' The split-index feature had a long-standing and dormant bug in certain use of the in-core merge machinery, which has been fixed. * en/unpack-trees-split-index-fix: unpack_trees: fix breakage when o->src_index != o->dst_index The test I'm doing is just: 1. git checkout some_tag 2. git rebase -i HEAD~5 3. Swap the top and bottom commit 4. repeat I just chose "5" as my first wild guess, other numbers are also available. With "5" I get 3 lots of: warning: could not freshen shared index '.git/sharedindex.0000000000000000000000000000000000000000' As far as I can tell the actual rebasing is fine. ``` ## Duy Nguyen, 2019-02-09 10:36 Subject: Re: could not freshen shared index ..../sharedindex.0000000000000000000000000000000000000000' Message-ID: URL: https://gitlist.dev/e/CACsJy8A7A%2BFu%3D5PWGuQsxtdJLBP_2Ueb-1MCKGHwKHGoQ7J2GA%40mail.gmail.com In-Reply-To: ``` On Sat, Feb 9, 2019 at 4:58 PM Luke Diamand wrote: > > On Sat, 9 Feb 2019 at 05:01, Duy Nguyen wrote: > > > > On Fri, Feb 8, 2019 at 11:39 PM Luke Diamand wrote: > > > > > > On Fri, 8 Feb 2019 at 10:02, Duy Nguyen wrote: > > > > > > > > On Wed, Feb 06, 2019 at 10:25:25AM +0000, Luke Diamand wrote: > > > > > I've recently started seeing a lot of this message when doing a rebase: > > > > > > > > > > warning: could not freshen shared index > > > > > '/home/ldiamand/git/dev_full/.git/worktrees/gcc8-take-2/sharedindex.0000000000000000000000000000000000000000' > > > > > > > > There are only two places in the code that could print this. The one > > > > in read_index_from() can't happen unless is_null_oid() is broken (very > > > > very unlikely). > > > > > > > > The other one is in write_locked_index() which could happen in theory > > > > but I don't understand how it got there. If you could build git, could > > > > you try this patch and see if it helps? > > > > > > They've gone away! > > > > Great! Since you seem able to reproduce this (and can build git!) > > could you try bisect to pin point the commit that causes this problem? > > That would help a lot. I think you could start maybe from v2.19.0 > > The first bad commit was d658196f3c4b2478ebdff638e2c3e4fb2f9cbba2. Thanks! I could reproduce it and I think I know what the problem is now. Patches coming soon. > > $ git show d658196f3c4b2478ebdff638e2c3e4fb2f9cbba2 > commit d658196f3c4b2478ebdff638e2c3e4fb2f9cbba2 (refs/bisect/bad) > Merge: 6b0f1d9c47 7db118303a > Author: Junio C Hamano > Date: Wed May 23 14:38:22 2018 +0900 > > Merge branch 'en/unpack-trees-split-index-fix' > > The split-index feature had a long-standing and dormant bug in > certain use of the in-core merge machinery, which has been fixed. > > * en/unpack-trees-split-index-fix: > unpack_trees: fix breakage when o->src_index != o->dst_index > > The test I'm doing is just: > > 1. git checkout some_tag > 2. git rebase -i HEAD~5 > 3. Swap the top and bottom commit > 4. repeat > > I just chose "5" as my first wild guess, other numbers are also available. > > With "5" I get 3 lots of: > warning: could not freshen shared index > '.git/sharedindex.0000000000000000000000000000000000000000' > > As far as I can tell the actual rebasing is fine. -- Duy ``` ## Nguyễn Thái Ngọc Duy, 2019-02-09 11:23 Subject: [PATCH] unpack-trees.c: fix writing "link" index ext with null base oid Message-ID: <20190209112328.26317-1-pclouds@gmail.com> URL: https://gitlist.dev/e/20190209112328.26317-1-pclouds%40gmail.com In-Reply-To: ``` 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 Signed-off-by: Nguyễn Thái Ngọc Duy --- 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 ``` ## Luke Diamand, 2019-02-09 14:14 Subject: Re: [PATCH] unpack-trees.c: fix writing "link" index ext with null base oid Message-ID: URL: https://gitlist.dev/e/CAE5ih7-0Qq6DOrE0Y7jQ%2B%2BJer0P5SGM38KA0n1_busAi08ux%3DQ%40mail.gmail.com In-Reply-To: <20190209112328.26317-1-pclouds@gmail.com> ``` On Sat, 9 Feb 2019 at 11:23, Nguyễn Thái Ngọc Duy 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 > Signed-off-by: Nguyễn Thái Ngọc Duy > --- > 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. > ``` ## Elijah Newren, 2019-02-12 05:43 Subject: Re: [PATCH] unpack-trees.c: fix writing "link" index ext with null base oid Message-ID: URL: https://gitlist.dev/e/CABPp-BF%2Bq0QY1YAKOk4iSrCRuEc7xsG7iqCzeDD9v8%2BjmvQvzg%40mail.gmail.com In-Reply-To: <20190209112328.26317-1-pclouds@gmail.com> ``` On Sat, Feb 9, 2019 at 3:23 AM Nguyễn Thái Ngọc Duy 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 > Signed-off-by: Nguyễn Thái Ngọc Duy 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 > ``` ## Ævar Arnfjörð Bjarmason, 2019-02-12 09:36 Subject: Re: [PATCH] unpack-trees.c: fix writing "link" index ext with null base oid Message-ID: <8736otl56m.fsf@evledraar.gmail.com> URL: https://gitlist.dev/e/8736otl56m.fsf%40evledraar.gmail.com In-Reply-To: <20190209112328.26317-1-pclouds@gmail.com> ``` 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. ``` ## Junio C Hamano, 2019-02-12 16:49 Subject: Re: [PATCH] unpack-trees.c: fix writing "link" index ext with null base oid Message-ID: URL: https://gitlist.dev/e/xmqq1s4deyuw.fsf%40gitster-ct.c.googlers.com In-Reply-To: ``` Elijah Newren 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 >> Signed-off-by: Nguyễn Thái Ngọc Duy > > 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. ``` ## Nguyễn Thái Ngọc Duy, 2019-02-13 09:51 Subject: [PATCH v2] read-cache.c: fix writing "link" index ext with null base oid Message-ID: <20190213095129.31272-1-pclouds@gmail.com> URL: https://gitlist.dev/e/20190213095129.31272-1-pclouds%40gmail.com In-Reply-To: <20190209112328.26317-1-pclouds@gmail.com> ``` 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 Signed-off-by: Nguyễn Thái Ngọc Duy --- v2 added a new test read-cache.c | 5 +++-- split-index.c | 34 ++++++++++++++++++---------------- t/t1700-split-index.sh | 18 ++++++++++++++++++ 3 files changed, 39 insertions(+), 18 deletions(-) diff --git a/read-cache.c b/read-cache.c index 8f644f68b4..d140b44f8f 100644 --- a/read-cache.c +++ b/read-cache.c @@ -2520,7 +2520,8 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile, return err; /* Write extension data here */ - 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 || @@ -2794,7 +2795,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 diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh index f053bf83eb..ea5181aff9 100755 --- a/t/t1700-split-index.sh +++ b/t/t1700-split-index.sh @@ -447,4 +447,22 @@ test_expect_success 'writing split index with null sha1 does not write cache tre test_line_count = 0 cache-tree.out ' +test_expect_success 'do not refresh null base index' ' + test_create_repo merge && + ( + cd merge && + test_commit initial && + git checkout -b side-branch && + test_commit extra && + git checkout master && + git update-index --split-index && + test_commit more && + # must not write a new shareindex, or we wont catch the problem + git -c splitIndex.maxPercentChange=100 merge --no-edit side-branch 2>err && + # i.e. do not expect warnings like + # could not freshen shared index .../shareindex.00000... + test_must_be_empty err + ) +' + test_done -- 2.21.0.rc0.328.g0e39304f8d ``` ## Ævar Arnfjörð Bjarmason, 2019-02-13 12:14 Subject: Re: [PATCH v2] read-cache.c: fix writing "link" index ext with null base oid Message-ID: <87imxnkhqb.fsf@evledraar.gmail.com> URL: https://gitlist.dev/e/87imxnkhqb.fsf%40evledraar.gmail.com In-Reply-To: <20190213095129.31272-1-pclouds@gmail.com> ``` On Wed, Feb 13 2019, Nguyễn Thái Ngọc Duy 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 > Signed-off-by: Nguyễn Thái Ngọc Duy > --- > v2 added a new test This round looks good to me. Passes all tests with/without GIT_TEST_SPLIT_INDEX=true > read-cache.c | 5 +++-- > split-index.c | 34 ++++++++++++++++++---------------- > t/t1700-split-index.sh | 18 ++++++++++++++++++ > 3 files changed, 39 insertions(+), 18 deletions(-) > > diff --git a/read-cache.c b/read-cache.c > index 8f644f68b4..d140b44f8f 100644 > --- a/read-cache.c > +++ b/read-cache.c > @@ -2520,7 +2520,8 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile, > return err; > > /* Write extension data here */ > - 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 || However, it looks like you based this on a pre-2.20.0 version of git. This conflicts with read-cache.c earlier than 3b1d9e045e ("eoie: add End of Index Entry (EOIE) extension", 2018-10-10). I fixed that manually, and pushed it out to github.com/avar/git.git duy-read-cache-null-split-index-fix, that's what I've tested on top of current "master". ```