Jeff King <peff@xxxxxxxx> writes:
> 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.
> 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.
> 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
Attachment:
signature.asc
Description: PGP signature
