Re: [PATCH v12 13/13] fsmonitor: fix split-index bitmap bounds in tweak_fsmonitor()
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Mar 31, 2026, 16:37 UTC
- Message-ID
- <xmqqse9g0xbx.fsf@gitster.g>
- In-Reply-To
- <84ddbb30bb3862d4230ba1775d4c061832f2623f.1774937958.git.gitgitgadget@gmail.com>
"Paul Tarjan via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 16 quoted lines
> 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.
Let's ask for input from the author of that scalar commit 05f28e4b3c, which hints that the fix should come with the focus on split-index.
I also have to wonder if we should bury the issue like this patch does, which is not limited to the split case (i.e., the assert should trigger if the requirement is not met while split-index is not active, but the patch castrates it), instead of mimicking what Derrick did to work it around on the test side, leaving the production code still broken, which will give us better feel on the urgency of the split-index case in the real world. I dunno.
Show 40 quoted lines
> 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. > > 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 > @@ -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; > @@ -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);