[PATCH 0/2] Range-check array index before access

[PATCH 0/2] Range-check array index before access

[Johannes Schindelin via GitGitGadget]

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 clar code, fixes for which I
offer in https://github.com/clar-test/clar/pull/115.

Johannes Schindelin (2):
  diff: check range before dereferencing an array element
  read-cache: check range before dereferencing an array element

 diff.c       | 2 +-
 read-cache.c | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)


base-commit: 683c54c999c301c2cd6f715c411407c413b1d84e
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1887%2Fdscho%2Frange-check-array-index-before-access-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1887/dscho/range-check-array-index-before-access-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1887
-- 
gitgitgadget

[PATCH 1/2] diff: check range before dereferencing an array element

[Johannes Schindelin via GitGitGadget]

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

Re: [PATCH 1/2] diff: check range before dereferencing an array element

[Junio C Hamano]

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

Re: [PATCH 1/2] diff: check range before dereferencing an array element

[Phillip Wood]

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

[PATCH 2/2] read-cache: check range before dereferencing an array element

[Johannes Schindelin via GitGitGadget]

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>
---
 read-cache.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

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++)
 			; /* still matching */
-- 
gitgitgadget

Re: [PATCH 2/2] read-cache: check range before dereferencing an array element

[Jeff King]

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

[PATCH v2 0/2] Range-check array index before access

[Johannes Schindelin via GitGitGadget]

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 clar code, fixes for which I
offer in https://github.com/clar-test/clar/pull/115.

Changes since v1:

 * Clarified in the commit message of the second patch that this range-check
   technically was already right before the array access it wants to guard,
   but that it still makes sense to move that range-check to the beginning
   of the loop condition.

Johannes Schindelin (2):
  diff: check range before dereferencing an array element
  read-cache: check range before dereferencing an array element

 diff.c       | 2 +-
 read-cache.c | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)


base-commit: 683c54c999c301c2cd6f715c411407c413b1d84e
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1887%2Fdscho%2Frange-check-array-index-before-access-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1887/dscho/range-check-array-index-before-access-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1887

Range-diff vs v1:

 1:  ddfb44ed924 = 1:  ddfb44ed924 diff: check range before dereferencing an array element
 2:  d4e94a243b0 ! 2:  73cae301293 read-cache: check range before dereferencing an array element
     @@ Commit message
          read-cache: check range before dereferencing an array element
      
          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.
     +    that the index is within the desired bounds, otherwise it makes little
     +    sense to access the array element in the first place.
      
     -    Pointed out by CodeQL's `cpp/offset-use-before-range-check` rule.
     +    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.
     +
     +    This acquiesces CodeQL's `cpp/offset-use-before-range-check` rule.
      
          Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
      

-- 
gitgitgadget

[PATCH v2 1/2] diff: check range before dereferencing an array element

[Johannes Schindelin via GitGitGadget]

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

Re: [PATCH v2 1/2] diff: check range before dereferencing an array element

[Junio C Hamano]

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

[PATCH v2 2/2] read-cache: check range before dereferencing an array element

[Johannes Schindelin via GitGitGadget]

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.

This acquiesces CodeQL's `cpp/offset-use-before-range-check` rule.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 read-cache.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

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++)
 			; /* still matching */
-- 
gitgitgadget

Re: [PATCH v2 2/2] read-cache: check range before dereferencing an array element

[Jeff King]

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

Re: [PATCH v2 2/2] read-cache: check range before dereferencing an array element

[Junio C Hamano]

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.

[PATCH v3] diff: check range before dereferencing an array element

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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 <johannes.schindelin@gmx.de>
---
    Range-check array index before access
    
    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 clar code, fixes for which I
    offer in https://github.com/clar-test/clar/pull/115.
    
    Changes since v2:
    
     * Rebased on top of js/range-check-codeql-workaround.
     * Rephrased the commit message.
    
    Changes since v1:
    
     * Clarified in the commit message of the second patch that this
       range-check technically was already right before the array access it
       wants to guard, but that it still makes sense to move that
       range-check to the beginning of the loop condition.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1887%2Fdscho%2Frange-check-array-index-before-access-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1887/dscho/range-check-array-index-before-access-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1887

Range-diff vs v2:

 1:  ddfb44ed924 ! 1:  3c6e2647863 diff: check range before dereferencing an array element
     @@ Metadata
       ## Commit message ##
          diff: check range before dereferencing an array element
      
     -    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.
     +    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.
      
     -    Pointed out by 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 <johannes.schindelin@gmx.de>
      
 2:  73cae301293 < -:  ----------- read-cache: check range before dereferencing an array element


 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 */

base-commit: 0f558141ed3b93b393151367b9569446cd24caab
-- 
gitgitgadget

Re: [PATCH v3] diff: check range before dereferencing an array element

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> 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.

Re: [PATCH v3] diff: check range before dereferencing an array element

[Jeff King]

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

Re: [PATCH v3] diff: check range before dereferencing an array element

[Junio C Hamano]

Jeff King <peff@peff.net> 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.

Re: [PATCH v3] diff: check range before dereferencing an array element

[Jeff King]

On Tue, Apr 29, 2025 at 03:37:35PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> 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

Re: [PATCH v3] diff: check range before dereferencing an array element

[Junio C Hamano]

Jeff King <peff@peff.net> 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.