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, 17:55 UTC
Message-ID: <xmqqfr5f289u.fsf@gitster.g>
In-Reply-To: <xmqqse9g0xbx.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

Show 23 quoted lines

> 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.
>
>> 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(-)

Let me ask the question differently.

Show 11 quoted lines

>> 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;

While this may avoid this "assert" stopping the process, what is the implication of returning from here? Who are the callers, what did the callers want us to do to the cache entry at the "pos", and what happens to their expectations if we simply returned to them without doing anything they asked us to do?

Show 7 quoted lines

>> @@ -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);

This part does make sense; the minimum may be relevant only when split-index is in effect, so we limit the check to non split case.

I do have to wonder if there are some invariants we can/want to enforce here in the split case, but I do not mind punting it to a fix that is more focused on the split-index theme, as Derrick hinted in the earlier commit you found.

>>  			ewah_each_bit(istate->fsmonitor_dirty, fsmonitor_ewah_callback, istate);
>>  
>>  			refresh_fsmonitor(istate);

Thanks.