Re: [PATCH 2/2] read-cache: check range before dereferencing an array element
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. :)
Johannes Schindelin via GitGitGadget
