Re: [PATCH v5 3/5] refs: selectively set prefix in the seek functions

From: Karthik Nayak <karthik.188@gmail.com>
Date: Jul 17, 2025, 19:49 UTC
Message-ID: <CAOLa=ZTdUsv-6jVhPSwEW5dYr3vEh2bLT5ECRZZ1ot3MGAYhBg@mail.gmail.com>
In-Reply-To: <20250717020905.GA2193264@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 18 quoted lines

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

That's an oversight from my side.

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

Indeed, I didn't think of that.

Show 15 quoted lines

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

Agreed.

Show 23 quoted lines

> 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

Thanks, I think we have to typecast `slash - refname` to size_t, but this is the right way to do it. Thanks for the review!

Karthik