From: Karthik Nayak <karthik.188@gmail.com>
Date: Fri, 26 Sep 2025 07:41:58 GMT
Subject: Re: [PATCH v2] refs/ref-cache: fix SEGFAULT when seeking in empty directories
Message-ID: <CAOLa=ZSfL1CVCNjD0Vi3kpQqLM6t7YVa=G2ChGpaza6WOjZTCw@mail.gmail.com>
In-Reply-To: <xmqqbjmyh353.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> diff --git a/refs/ref-cache.c b/refs/ref-cache.c
>> index c180e0aad7..e5e5df16d8 100644
>> --- a/refs/ref-cache.c
>> +++ b/refs/ref-cache.c
>> @@ -539,7 +539,7 @@ static int cache_ref_iterator_seek(struct ref_iterator *ref_iterator,
>>  				 */
>>  				break;
>>  			}
>> -		} while (slash);
>> +		} while (slash && dir->nr);
>>  	}
>
> This is at the tail of a "do { ... } while (...);" loop, but inside
> the loop I see this construct:
>
> 			for (idx = 0; idx < dir->nr; idx++) {
> 				cmp = strncmp(refname, dir->entries[idx]->name, len);
> 				if (cmp <= 0)
> 					break;
> 			}
> 			/* don't overflow the index */
> 			idx = idx >= dir->nr ? dir->nr - 1 : idx;
>
> i.e., if we scan all the dir->entries[] elements in the innter loop
> and did not find any hit, idx would become dir->nr and this inner
> loop runs to the end.  If (dir->nr == 0), then ?: operator [*] would
> become the idx = (dir->nr - 1);  And that idx is used for a while
> before we get to this "while (slash && dir->nr)".
>

This wouldn't happen because before the loop starts, we set:

   dir = get_ref_dir(iter->cache->root);

So, `dir` will always have an entry (i.e. 'refs') in the first
iteration, after that, the checks in the while section  of the 'do {}
while (...)' loop will kick in.

> And then tha tis used like this.
>
> 			if (slash)
> 				slash = slash + 1;
>
> 			level->index = idx;
> 			if (dir->entries[idx]->flag & REF_DIR) {
> 				...
>
> IOW, isn't this check a bit too late?  I wonder if we can leave at
> the beginning of the outer loop, even before sort_ref_dir(dir), when
> dir->nr is zero, or something?
>
>

We could add it there too. I can't see the merit of one over the other.
But if you see it being more readable. I'll happily make that change.

As the author of this code, I do find it complex already.

>
> [Side note]
>
>  * I found the problematic ?: extremely hard to read, given its
>    contrast with the terminating condnition of for loop.  If you
>    apply the discipline to keep the textual order match the actual
>    order, i.e.
>
> 	for (idx = 0; idx < dir->nr; idx++)
> 		...;
>
> 	idx = dir->nr <= idx ? dir->nr - 1 : idx;
>
>    people would have spotted this more easily.
>
>    But perhaps that may be just me.  Anyway...

I'm not really sold on this being easier to read. But that could be me,
So I'll make a second commit to make this change, we can drop it if
unwanted.