| Message ID | ddfb44ed924615bdb61a30ae7627326942575567.1743010011.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Range-check array index before access | expand |
"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. > > 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++; 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).
On 27/03/2025 11:01, Junio C Hamano wrote: > "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. >> >> 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++; > > 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
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 */