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

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

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

At least this part should say this is a false positive, forcing us
to make an unnecessary change to help future developers who are
running "git blame" and "git log -p" to find out why only s[off]
checked against CR needs this check _before_ it, while checking
against other values needs _no_ check.

In other words, the first paragraph of the proposed log message is a
total red herring.  We are accessing an array element at a given
index 'off' in the original, we are still accessing the same element
in the updated code, and we know the index is within the array
bounds.  If the condition were "We want to skip CR only at odd
places", we would have written 

	|| (s[off] == '\r' && (off & 01))

or

	|| ((off & 01) || s[off] == '\r')

and both are equally valid.  (off < len -1) should be no different.

> 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++;
>  
>  	/* calculate the visual width of indentation */