Re: [PATCH 2/2] read-cache: check range before dereferencing an array element
From: Jeff King <hidden>
Date: 2025-03-26 18:02:27
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...
quoted hunk ↗ jump to hunk
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. :)