On Tue, Apr 29, 2025 at 11:37:58AM +0000, Johannes Schindelin via GitGitGadget wrote:
> 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...
> --- 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
