Range-check array index before access #1887
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
/submit |
|
Submitted as [email protected] To fetch this version into To fetch this version to local tag |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
read-cache.c
Outdated
| @@ -2686,8 +2686,8 @@ static int ce_write_entry(struct hashfile *f, struct cache_entry *ce, | |||
| int common, to_remove, prefix_size; | |||
There was a problem hiding this comment.
On the Git mailing list, Jeff King wrote (reply to this):
On Wed, Mar 26, 2025 at 05:26:51PM +0000, Johannes Schindelin via GitGitGadget wrote:
> 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.
Certainly we should, but...
> diff --git a/read-cache.c b/read-cache.c
> index e678c13e8f1..08ae66ad609 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -2686,8 +2686,8 @@ static int ce_write_entry(struct hashfile *f, struct cache_entry *ce,
> int common, to_remove, prefix_size;
> unsigned char to_remove_vi[16];
> for (common = 0;
> - (ce->name[common] &&
> - common < previous_name->len &&
> + (common < previous_name->len &&
> + ce->name[common] &&
> ce->name[common] == previous_name->buf[common]);
> common++)
Is previous_name->len an actual bound for ce->name?
I think we are iterating through ce->name looking for either the
terminating NUL, or matching the prefix from previous_name. So the
length check is for the third condition:
ce->name[common] == previous_name->buf[common]
and correctly comes before it.
So unless I'm mistaken, this is a false positive in CodeQL. I don't mind
re-ordering the condition to fix it, but the commit message should
probably say so.
Since previous_name is a strbuf, it is also NUL-terminated (and interior
NUL bytes cannot be important, because we are comparing against a
NUL-terminated ce->name). So I suspect that a simpler way to write it is
to remove the length check and rely on the NUL/not-NUL mismatch to
break, like:
for (common = 0;
ce->name[common] &&
ce->name[common] == previous_name->buf[common];
common++)
Which would also presumably remove the warning.
-Peff
PS I notice that "common" is an int, which always makes me wonder what
would happen with a 2GB+1 filename. But I think that is nothing
specific here, as there are ints all over the place for index name
lengths. And anyway, one thing at a time, I suppose. :)
|
User |
dscho
force-pushed
the
range-check-array-index-before-access
branch
from
d4e94a2 to
73cae30
Compare
|
/submit |
| @@ -892,7 +892,7 @@ static void fill_es_indent_data(struct emitted_diff_symbol *es) | |||
|
|
|||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"Johannes Schindelin via GitGitGadget" <[email protected]>
writes:
> From: Johannes Schindelin <[email protected]>
>
> 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 <[email protected]>
> ---
> 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).There was a problem hiding this comment.
On the Git mailing list, Phillip Wood wrote (reply to this):
On 27/03/2025 11:01, Junio C Hamano wrote:
> "Johannes Schindelin via GitGitGadget" <[email protected]>
> writes:
> >> From: Johannes Schindelin <[email protected]>
>>
>> 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 <[email protected]>
>> ---
>> 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
|
Submitted as [email protected] To fetch this version into To fetch this version to local tag |
|
User |
| @@ -892,7 +892,7 @@ static void fill_es_indent_data(struct emitted_diff_symbol *es) | |||
|
|
|||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"Johannes Schindelin via GitGitGadget" <[email protected]>
writes:
> From: Johannes Schindelin <[email protected]>
>
> 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.
> Signed-off-by: Johannes Schindelin <[email protected]>
> ---
> 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 */
read-cache.c
Outdated
| @@ -2686,8 +2686,8 @@ static int ce_write_entry(struct hashfile *f, struct cache_entry *ce, | |||
| int common, to_remove, prefix_size; | |||
There was a problem hiding this comment.
On the Git mailing list, Jeff King wrote (reply to this):
On Thu, Mar 27, 2025 at 11:05:57AM +0000, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <[email protected]>
>
> Before accessing an array element at a given index, we should make sure
> that the index is within the desired bounds, otherwise it makes little
> sense to access the array element in the first place.
>
> In this instance, testing whether `ce->name[common]` is the trailing NUL
> byte is technically different from testing whether `common` is within
> the bounds of `previous_name`. It is also redundant, as the range-check
> guarantees that `previous_name->buf[common]` cannot be NUL and therefore
> the condition `ce->name[common] == previous_name->buf[common]` would not
> be met if `ce->name[common]` evaluated to NUL.
>
> However, in the interest of reducing the cognitive load to reason about
> the correctness of this loop (so that I can focus on interesting
> projects again), I'll simply move the range-check to the beginning of
> the loop condition and keep the redundant NUL check.
Thanks, I think this explanation works, and the patch looks fine. (I
didn't dig deeply into patch 1, but I agree with Junio's analysis that
it is also a false positive).
-PeffThere was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
Jeff King <[email protected]> writes:
> On Thu, Mar 27, 2025 at 11:05:57AM +0000, Johannes Schindelin via GitGitGadget wrote:
>
>> From: Johannes Schindelin <[email protected]>
>>
>> Before accessing an array element at a given index, we should make sure
>> that the index is within the desired bounds, otherwise it makes little
>> sense to access the array element in the first place.
>>
>> In this instance, testing whether `ce->name[common]` is the trailing NUL
>> byte is technically different from testing whether `common` is within
>> the bounds of `previous_name`. It is also redundant, as the range-check
>> guarantees that `previous_name->buf[common]` cannot be NUL and therefore
>> the condition `ce->name[common] == previous_name->buf[common]` would not
>> be met if `ce->name[common]` evaluated to NUL.
>>
>> However, in the interest of reducing the cognitive load to reason about
>> the correctness of this loop (so that I can focus on interesting
>> projects again), I'll simply move the range-check to the beginning of
>> the loop condition and keep the redundant NUL check.
>
> Thanks, I think this explanation works, and the patch looks fine. (I
> didn't dig deeply into patch 1, but I agree with Junio's analysis that
> it is also a false positive).
I think (1) working around a rare false positive to help us use an
otherwise mostly useful tool is a worthy thing to do, and (2) when
we need to make a workaround for a false positive, we should mark a
change to do so as such. And I agree with you that this step in the
updated form, both the change in the patch and the proposed log
message do their job.
Thanks, both. Will mark this one for 'next'.
Note that I think [1/2] needs similar updates relative to the
initial iteration, but since these two patches do not depend on each
other, I'll fast track this step without waiting updates to the
other one.
|
This patch series was integrated into seen via git@1cd87f3. |
|
This branch is now known as |
|
This patch series was integrated into seen via git@9ce3506. |
|
There was a status update in the "New Topics" section about the branch Work around false positive from CodeQL checker. Will merge to 'next'? source: <73cae30129338cf219a810c3a2a78ef48d5637d0.1743073557.git.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@552e7e3. |
|
This patch series was integrated into seen via git@db6cd1d. |
|
This patch series was integrated into seen via git@d570f63. |
|
This patch series was integrated into seen via git@639858b. |
|
There was a status update in the "Cooking" section about the branch Work around false positive from CodeQL checker. Will merge to 'next'? source: <73cae30129338cf219a810c3a2a78ef48d5637d0.1743073557.git.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@df44a40. |
|
This patch series was integrated into seen via git@d4366b4. |
|
There was a status update in the "Cooking" section about the branch Work around false positive from CodeQL checker. Will merge to 'next'? source: <73cae30129338cf219a810c3a2a78ef48d5637d0.1743073557.git.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@ca97576. |
|
This patch series was integrated into seen via git@1536bd1. |
|
This patch series was integrated into seen via git@2e23423. |
|
This patch series was integrated into next via git@809b3c3. |
|
There was a status update in the "Cooking" section about the branch Work around false positive from CodeQL checker. Will merge to 'master'. source: <73cae30129338cf219a810c3a2a78ef48d5637d0.1743073557.git.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@3a39653. |
|
This patch series was integrated into seen via git@e931a66. |
|
There was a status update in the "Cooking" section about the branch Work around false positive from CodeQL checker. Will merge to 'master'. source: <73cae30129338cf219a810c3a2a78ef48d5637d0.1743073557.git.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@480ddc5. |
|
This patch series was integrated into master via git@480ddc5. |
|
This patch series was integrated into next via git@480ddc5. |
|
Closed via 480ddc5. |
|
Closed via 480ddc5. |
Before accessing an array element at a given index, it should be verified 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. This is the point of CodeQL's `cpp/offset-use-before-range-check` rule. 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. Signed-off-by: Johannes Schindelin <[email protected]>
dscho
force-pushed
the
range-check-array-index-before-access
branch
from
73cae30 to
3c6e264
Compare
|
/submit |
|
Submitted as [email protected] To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): "Johannes Schindelin via GitGitGadget" <[email protected]>
writes:
> From: Johannes Schindelin <[email protected]>
>
> Before accessing an array element at a given index, it should be
> verified 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. This is the point of CodeQL's
> `cpp/offset-use-before-range-check` rule.
>
> 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.
Thanks. I have almost forgotten about this one. The above
explanation works very well.
|
|
On the Git mailing list, Jeff King wrote (reply to this): 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 |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): Jeff King <[email protected]> writes:
>> @@ -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')
>
> ?
But doesn't it change the semantics?
s[off] == '\f', even if off is at the end of the string, i.e. (off
== len - 1), must trigger the off++ increment.
On the other hand, CR that is the part of CRLF at the end of line is
*not* treated like other funny whitespace control characters. This
"is off not at the end of line, if so check CR" comparison is about
that. |
|
On the Git mailing list, Jeff King wrote (reply to this): On Tue, Apr 29, 2025 at 03:37:35PM -0700, Junio C Hamano wrote:
> Jeff King <[email protected]> writes:
>
> >> @@ -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')
> >
> > ?
>
> But doesn't it change the semantics?
>
> s[off] == '\f', even if off is at the end of the string, i.e. (off
> == len - 1), must trigger the off++ increment.
>
> On the other hand, CR that is the part of CRLF at the end of line is
> *not* treated like other funny whitespace control characters. This
> "is off not at the end of line, if so check CR" comparison is about
> that.
Ah, you're right. I was reading the offset check as "are we past the end
of string" (guided by CodeQL's complaint), and if that were the case the
logic would apply equally to all values we are checking.
But that is not what is going on at all. The offset check is for "len -
1", and so is "do not do this one CR match for the final character of
the string". And thus applying it elsewhere is wrong.
And CodeQL's false positive is doubly wrong. We do not even need to say
"the string is NUL-terminated, so it is OK in this case to look past the
end-of-string". The check is not even a string bounds check at all.
-Peff |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): Jeff King <[email protected]> writes:
> Ah, you're right. I was reading the offset check as "are we past the end
> of string" (guided by CodeQL's complaint), and if that were the case the
> logic would apply equally to all values we are checking.
>
> But that is not what is going on at all. The offset check is for "len -
> 1", and so is "do not do this one CR match for the final character of
> the string". And thus applying it elsewhere is wrong.
>
> And CodeQL's false positive is doubly wrong. We do not even need to say
> "the string is NUL-terminated, so it is OK in this case to look past the
> end-of-string". The check is not even a string bounds check at all.
Exactly.
That is what makes it hard to give a reasonable explanation in the
log message, which I thought that Dscho did a much better job in
this iteration.
Thanks. |
|
This patch series was integrated into seen via git@72fbe86. |
|
This branch is now known as |
|
This patch series was integrated into seen via git@dcfa7cc. |
|
This patch series was integrated into seen via git@b94ee9e. |
|
This patch series was integrated into next via git@a840276. |
|
This patch series was integrated into seen via git@d96520c. |
|
There was a status update in the "New Topics" section about the branch Work around false positive given by CodeQL. Will merge to 'master'. source: <[email protected]> |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
If we want to check the range of an array index, it makes much more sense to do it before accessing the corresponding array element, not afterwards.
There are two more instances of this in the
clarcode, fixes for which I offer in clar-test/clar#115.Changes since v2:
js/range-check-codeql-workaround.Changes since v1:
cc: Jeff King [email protected]
cc: Phillip Wood [email protected]