Skip to content

Range-check array index before access #1887

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
+3 −3
Open

Range-check array index before access #1887

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ddfb44e
diff: check range before dereferencing an array element
dscho Oct 27, 2022
73cae30
read-cache: check range before dereferencing an array element
dscho Oct 27, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion diff.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -892,7 +892,7 @@ static void fill_es_indent_data(struct emitted_diff_symbol *es)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

"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).

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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" <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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

"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.

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 <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++;
>  
>  	/* calculate the visual width of indentation */

/* 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 */
Expand Down
4 changes: 2 additions & 2 deletions read-cache.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2686,8 +2686,8 @@ static int ce_write_entry(struct hashfile *f, struct cache_entry *ce,
int common, to_remove, prefix_size;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. :)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 <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, 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).

-Peff

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

Jeff King <peff@peff.net> writes:

> On Thu, Mar 27, 2025 at 11:05:57AM +0000, Johannes Schindelin via GitGitGadget wrote:
>
>> 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, 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.

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++)
; /* still matching */
Expand Down
Loading