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

From: Jeff King <peff@peff.net>
Date: Mar 26, 2025, 18:02 UTC
Message-ID: <20250326180225.GC2508080@coredump.intra.peff.net>
In-Reply-To: <d4e94a243b0633ab18daa6ce0ae766d5bc33364e.1743010011.git.gitgitgadget@gmail.com>

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

Show 14 quoted lines

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