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

Commit Message

Johannes Schindelin March 26, 2025, 5:26 p.m. UTC

From: Johannes Schindelin <[e-mail]>

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

Comments

Jeff King March 26, 2025, 6:02 p.m. UTC | #1

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

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