From: Phillip Wood Date: Thu, 27 Mar 2025 14:59:33 GMT Subject: Re: [PATCH 1/2] diff: check range before dereferencing an array element Message-ID: <49721689-f002-4ef0-8d42-6d9ee829c7c9@gmail.com> In-Reply-To: On 27/03/2025 11:01, Junio C Hamano wrote: > "Johannes Schindelin via GitGitGadget" > writes: > >> From: Johannes Schindelin >> >> 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 >> --- >> 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). Exactly - we do not want to count CR as being part of the indentation if it is followed by LF. It has been a while since I wrote this code but my recollection is that each string ends with "\n\0". From what I remember to detect moved lines we have to buffer the output from xdl_diff() and so copy each line with xmemdupz() and somewhere the xdiff machinery adds '\n' to incomplete lines when it generates the diff. Best Wishes Phillip