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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 28, 2025, 02:54 UTC
Message-ID: <xmqqy0wphnlj.fsf@gitster.g>
In-Reply-To: <ddfb44ed924615bdb61a30ae7627326942575567.1743073557.git.gitgitgadget@gmail.com>

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 8 quoted lines

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

Show 18 quoted lines

> 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 */