From: Johannes Schindelin Date: Sat, 04 Apr 2026 22:10:36 GMT Subject: Re: [PATCH v12 13/13] fsmonitor: fix split-index bitmap bounds in tweak_fsmonitor() Message-ID: In-Reply-To: <84ddbb30bb3862d4230ba1775d4c061832f2623f.1774937958.git.gitgitgadget@gmail.com> Hi Paul, On Tue, 31 Mar 2026, Paul Tarjan via GitGitGadget wrote: > From: Paul Tarjan > > When GIT_TEST_SPLIT_INDEX=yes is set and the fsmonitor daemon is > active, tweak_fsmonitor() can hit a BUG() assertion: > > BUG: fsmonitor.c:27: fsmonitor_dirty has more entries than the index (2 > 0) > > The fsmonitor_dirty EWAH bitmap may reference positions from a > previous index state. With split-index, cache_nr can be smaller > than the bitmap expects because entries have not been merged yet. > > This is related to the issue that 05f28e4b3c (scalar: use > index.skipHash=true for performance, 2025-06-04) worked around by > disabling GIT_TEST_SPLIT_INDEX in t9210, noting "the issue should > be resolved in a series focused on the split index." This fixes > the fsmonitor bitmap side; the index.skipHash interaction remains. > > Two places hit this: > > - tweak_fsmonitor() calls assert_index_minimum() without the > !istate->split_index guard that the read path (line 98) and > write path (line 128) already have. Add the same guard. > > - fsmonitor_ewah_callback() unconditionally asserts and then > accesses istate->cache[pos], which is out of bounds with > split-index. Replace the assertion with a bounds check that > silently skips positions beyond cache_nr. This issue (and in particular the BUG assertion reported by the CI failures Junio pointed at in https://lore.kernel.org/git/xmqqjyus4qp2.fsf@gitster.g/) sounded quite familiar to me, so I consulted my notes. I had fixed a similar symptom after a seriously tedious debugging session via 3b7a4475b091 (split-index; stop abusing the `base_oid` to strip the "link" extension, 2023-03-26). But alas, this reference did not help, I found no similar bug that could be the culprit here. So I bisected the CI failure and it turns out that, as I suspected, this patch addresses a symptom, not the root cause. The bug has nothing to do with Linux FSMonitor support and this patch should be dropped from the series. See below for a suggestion what to do instead. The failing CI job is `linux-TEST-vars`, which is the _only_ job that sets `GIT_TEST_SPLIT_INDEX=yes`. Before this series, `fsmonitor_ipc__is_supported()` returned 0 on Linux, so Scalar never set `core.fsmonitor=true`, and therefore there was never a FSMonitor bitmap in the index, and the assertion in `tweak_fsmonitor()` could never fire. Now that Linux has FSMonitor support, Scalar enables it, and the bitmap sanity check exposes a _pre-existing_ corruption in the `index.skipHash` + split-index interaction. The interacting topic is `hn/git-checkout-m-with-stash` (specifically ced477a20fa4, "checkout: -m (--merge) uses autostash when switching branches"). That commit adds an unconditional `discard_index()` + `repo_read_index()` cycle at the end of `switch_branches()`, even when no autostash was created. (That is probably undesirable behavior, it should be guarded.) Here is the chain of events: 1. Scalar sets `index.skipHash=true` (this is Scalar's default since 05f28e4b3cc1 (scalar: use index.skipHash=true for performance, 2025-12-12)). 2. With `GIT_TEST_SPLIT_INDEX=yes`, `write_locked_index()` writes a split index. It calls `write_shared_index()`, which calls `do_write_index()` for the shared index. Because `skipHash=true`, the trailing hash is left as all-zeros, so `si->base->oid` is the null OID. 3. `write_shared_index()` then does (https://gitlab.com/git-scm/git/-/blob/v2.53.0/read-cache.c#L3283): oidcpy(&si->base_oid, &si->base->oid); ...copying the null OID into `base_oid`. The shared index file is renamed to `sharedindex.0000000000000000000000000000000000000000`. 4. The `hn/git-checkout-m-with-stash` topic's unconditional `discard_index()` + `repo_read_index()` re-reads the index from disk. 5. In `read_index_from()` (read-cache.c:2375), the condition if (!split_index || is_null_oid(&split_index->base_oid)) is true, so the shared index is never loaded, `merge_base_index()` is never called, and we jump straight to `post_read_index_from()`. 6. This leaves `cache_nr=0` (all entries live in the never-loaded shared index), but the FSMonitor extension's `fsmonitor_dirty` bitmap was written against the full merged index (2 entries). 7. `tweak_fsmonitor()` (called from `post_read_index_from()`) hits: assert_index_minimum(istate, istate->fsmonitor_dirty->bit_size); and fires the BUG: `bit_size=2 > cache_nr=0`. The same bug would reproduce on macOS or Windows if those platforms had a TEST-vars CI job with `GIT_TEST_SPLIT_INDEX=yes`. Now, about the patch itself (reordering the hunks for clarity): > Signed-off-by: Paul Tarjan > --- > fsmonitor.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/fsmonitor.c b/fsmonitor.c > index d07dc18967..5e5a4fadea 100644 > --- a/fsmonitor.c > +++ b/fsmonitor.c > @@ -805,7 +806,8 @@ void tweak_fsmonitor(struct index_state *istate) > } > > /* Mark all previously saved entries as dirty */ > - assert_index_minimum(istate, istate->fsmonitor_dirty->bit_size); > + if (!istate->split_index) > + assert_index_minimum(istate, istate->fsmonitor_dirty->bit_size); > ewah_each_bit(istate->fsmonitor_dirty, fsmonitor_ewah_callback, istate); > > refresh_fsmonitor(istate); This guard in `tweak_fsmonitor()` is not analogous to the guards in `read_fsmonitor_extension()` and `write_fsmonitor_extension()`. Those guards exist because those functions run before `merge_base_index()` has been called. But `tweak_fsmonitor()` runs after `merge_base_index()` in the normal read path (via `post_read_index_from()`). The fact that `cache_nr=0` when `tweak_fsmonitor()` runs means the shared index was never loaded at all, which is the real bug. For historical context: ba1b9caca699 ("fsmonitor: delay updating state until after split index is merged", 2017-10-27) intentionally arranged for `tweak_fsmonitor()` to run post-merge, and 392d797e2e60 (fsmonitor: do not check fsmonitor_dirty bits against cache entries on split index, 2019-10-31) and 7621a6f43428 (fsmonitor: do not compare bitmap size with size of split index, 2019-11-21) added the `!istate->split_index` guard to the read/write paths only, deliberately leaving `tweak_fsmonitor()` unguarded because at that point the merge is expected to have happened. cae70acf2431 (fsmonitor: de-duplicate BUG()s around dirty bits, 2021-01-23) refactored the BUG()s into `assert_index_minimum()`, again preserving this pattern. Suppressing the assertion here would hide the fact that the index is in a broken state. > @@ -33,7 +33,8 @@ static void fsmonitor_ewah_callback(size_t pos, void *is) > struct index_state *istate = (struct index_state *)is; > struct cache_entry *ce; > > - assert_index_minimum(istate, pos + 1); > + if (pos >= istate->cache_nr) > + return; > > ce = istate->cache[pos]; > ce->ce_flags &= ~CE_FSMONITOR_VALID; This change to `fsmonitor_ewah_callback()` is particularly dangerous: silently skipping dirty entries means files that the FSMonitor daemon reported as changed could be treated as unchanged. That is a correctness bug. The proper fix belongs in one of two places: 1. `write_shared_index()` should not allow `base_oid` to become the null OID. When `index.skipHash=true`, the shared index needs a real identifier. This is the root cause. 2. Alternatively, the `hn/git-checkout-m-with-stash` topic should guard its `discard_index()` + `repo_read_index()` with something like `if (created_autostash)`, since the index reload is only needed when an autostash was actually applied. But that would probably only buy some time until another code path introduces a similar pattern, and maybe shouldn't even be considered a real fix. Alternatively, I'd suggest: 3. The same work-around as was introduced in 05f28e4b3cc1 (scalar: use index.skipHash=true for performance, 2025-12-12), namely to simply force-disable split index in the offending test case (or move the existing `sane_unset` to the top of the file, since `index.skipHash=true` is a Scalar thing): -- snip -- diff --git a/t/t9210-scalar.sh b/t/t9210-scalar.sh index 009437a5f316..a4e38ad644e4 100755 --- a/t/t9210-scalar.sh +++ b/t/t9210-scalar.sh @@ -152,6 +152,11 @@ test_expect_success 'set up repository to clone' ' ' test_expect_success 'scalar clone' ' + # The split index refers to the base index via OID; Scalar sets + # index.skipHash, though, and therefore that OID is always bogus; + # Scalar/index.skipHash are simply incompatible with split-index + sane_unset GIT_TEST_SPLIT_INDEX && + second=$(git rev-parse --verify second:second.t) && scalar clone "file://$(pwd)" cloned --single-branch && ( -- snap -- Either way, this 13/13 patch should be dropped from the fsmonitor-linux series, I think. Ciao, Johannes