Re: [PATCH v3] diff: check range before dereferencing an array element

From: Jeff King <peff@peff.net>
Date: Apr 29, 2025, 21:58 UTC
Message-ID: <20250429215847.GB36727@coredump.intra.peff.net>
In-Reply-To: <pull.1887.v3.git.1745926679028.gitgitgadget@gmail.com>

On Tue, Apr 29, 2025 at 11:37:58AM +0000, Johannes Schindelin via GitGitGadget wrote:

Show 8 quoted lines

> This CodeQL rule unfortunately is also triggered by the
> `fill_es_indent_data()` code, even though the condition `off < len - 1`
> does not even need to guarantee that the offset is in bounds (`s` points
> to a NUL-terminated string, for which `s[off] == '\r'` would fail before
> running out of bounds).
> 
> Let's work around this rare false positive to help us use an otherwise
> mostly useful tool is a worthy thing to do.

Since this is marked as fixing a false positive, and since it presumably _does_ fix the false positive in practice, this is OK with me.

But...

Show 9 quoted lines

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

...since the same pattern exists for the other s[off] checks, is it worth future-proofing this like:

  while (off < len - 1 &&
         (s[off] == '\f' || s[off] == '\v' || s[off] == '\r')

I say "future proofing" because we don't know whether future versions of CodeQL might complain about them. Presumably it does not yet because it isn't smart enough to look outside the parenthesized &&-condition. But if reading s[len] would be a problem for the '\r' check, it would be for the others as well.

-Peff