Re: [PATCH v12 13/13] fsmonitor: fix split-index bitmap bounds in tweak_fsmonitor()

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Paul,

On Tue, 31 Mar 2026, Paul Tarjan via GitGitGadget wrote:

> From: Paul Tarjan <github@paulisageek.com>
> 
> 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 <github@paulisageek.com>
> ---
>  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