Re: [PATCH 1/2] diff: check range before dereferencing an array element

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
> 
>> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>>
>> Before accessing an array element at a given index, we should make sure
>> that the index is within the desired bounds, not afterwards, otherwise
>> it may not make sense to even access the array element in the first
>> place.
>>
>> Pointed out by CodeQL's `cpp/offset-use-before-range-check` rule.
>>
>> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>> ---
>>   diff.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/diff.c b/diff.c
>> index c89c15d98e0..18ba3060460 100644
>> --- a/diff.c
>> +++ b/diff.c
>> @@ -892,7 +892,7 @@ static void fill_es_indent_data(struct emitted_diff_symbol *es)
>>   
>>   	/* skip any \v \f \r at start of indentation */
>>   	while (s[off] == '\f' || s[off] == '\v' ||
>> -	       (s[off] == '\r' && off < len - 1))
>> +	       (off < len - 1 && s[off] == '\r'))
>>   		off++;
> 
> I suspect that this is another false positive, like Peff pointed out
> for [2/2] of these two patches.
> 
> Especially if this change squelches the warning.
> 
> If the check against CR for s[off] could be oob without checking how
> large 'off' is, then the earlier checks for FF and VT should also be
> equally iffy.  After all they are accessing the byte at the same
> location.
> 
> I think what is going on is that the correctness of the code depends
> on s[] having a sentinel (which is not FF/VT/CR; I do not offhand
> know if it is NUL terminated or LF at the end of line) so any byte
> other than FF/VT/CR that are in the leading part of the line would
> cause us to exit the loop safely before going beyond the end of the
> array s[].  CR alone is special cased because we want to treat it
> like FF/VT only if it is not a part of the EOL CR/LF (hence "is our
> CR at one before the end of the line?" check).