From: Jeff King <peff@peff.net>
Date: Thu, 17 Jul 2025 02:09:05 GMT
Subject: Re: [PATCH v5 3/5] refs: selectively set prefix in the seek functions
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:

> +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:

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