Re: [PATCH v5 3/5] refs: selectively set prefix in the seek functions
- From
Jeff King <peff@peff.net>
- Date
- Jul 17, 2025, 02:09 UTC
- Message-ID
- <20250717020905.GA2193264@coredump.intra.peff.net>
- In-Reply-To
- <20250715-306-git-for-each-ref-pagination-v5-3-852d5a2f56e1@gmail.com>
On Tue, Jul 15, 2025 at 01:28:28PM +0200, Karthik Nayak wrote:
Show 11 quoted lines
> +static int cache_ref_iterator_seek(struct ref_iterator *ref_iterator,
> + const char *refname, unsigned int flags)
> [...]
> + do {
> + int len, idx;
> + int cmp = 0;
> +
> + sort_ref_dir(dir);
> +
> + slash = strchr(slash, '/');
> + len = slash ? slash - refname : (int)strlen(refname);I was looking at this code due to a nearby thread and noticed this funny cast to int. I guess you added it to silence -Wsign-compare, but Why are we not using a size_t in the first place?
This kind of conversion can sometimes have security implications because a very large "refname" would cause "len" to become negative (i.e., if it's between 2GB and 4GB).
In this particular case it ends up cast back to a size_t via strncmp:
Show 5 quoted lines
> + for (idx = 0; idx < dir->nr; idx++) {
> + cmp = strncmp(refname, dir->entries[idx]->name, len);
> + if (cmp <= 0)
> + break;
> + }so we get the original value back. We'd still get truncation for a refname value over 4GB, which would presumably give us a slightly wrong answer. But I don't think we'd ever look outside the array.
Such sizes are probably unlikely if we are feeding filesystem paths. But we probably should not set a bad example, and just do;
diff --git a/refs/ref-cache.c b/refs/ref-cache.c index 1d95b56d40..3949d145e8 100644 --- a/refs/ref-cache.c +++ b/refs/ref-cache.c @@ -498,13 +498,14 @@ static int cache_ref_iterator_seek(struct ref_iterator *ref_iterator, * indexing to each level as needed. */ do { - int len, idx; + size_t len; + int idx; int cmp = 0; sort_ref_dir(dir); slash = strchr(slash, '/'); - len = slash ? slash - refname : (int)strlen(refname); + len = slash ? slash - refname : strlen(refname); for (idx = 0; idx < dir->nr; idx++) { cmp = strncmp(refname, dir->entries[idx]->name, len); -Peff