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

[Junio C Hamano <gitster@pobox.com>]

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

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

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

>> @@ -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.