Re: [PATCH] refs/ref-cache: stop seeking into empty directories

From: Karthik Nayak <karthik.188@gmail.com>
Date: Sep 25, 2025, 08:20 UTC
Message-ID: <CAOLa=ZS=y5Q-_BH+q2xhzh6mpg2oQJptv1rp_T0jdqH-jBf0+g@mail.gmail.com>
In-Reply-To: <aNPkA9UZtEqHsa3f@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

Show 20 quoted lines

> On Wed, Sep 24, 2025 at 09:55:51AM +0200, Karthik Nayak wrote:
>> The 'cache_ref_iterator_seek()' function is used to seek the
>> `ref_iterator` to the desired reference in the ref-cache mechanism. We
>> use the seeking functionality to implement the '--start-after' flag in
>> 'git-for-each-ref(1)'.
>>
>> When using the files-backend with packed-refs, it is possible that some
>> of the refs directories are empty. For e.g. just after repacking, the
>> 'refs/heads' directory would be empty. The ref-cache seek mechanism
>> doesn't take this into consideration, causing SEGFAULT as we try to
>> access entries within the directory.
>
> Why do we even try to access any entry in an empty directory? We have
> `dir->nr`, so shouldn't we check that `idx < dir->nr` every time we try
> to deref an entry?
>
> In other words, I wonder whether we fix a symptom of a missing bounds
> check instead of addressing that missing bounds check as the root cause
> directly.
>

We do a bounds check to ensure that we don't overflow the entries in a directory with `idx = idx >= dir->nr ? dir->nr - 1 : idx;`.

The issue is that when `dir->nr = 0`, this will set the `idx = -1`. So this is where it fails.

Taking a step back, the while loop shouldn't even operate when we hit a directory with no entries. So I'll modify the loop condition to include a check for `dir->nr`.

Show 10 quoted lines

>> Fix this by breaking out of the
>> loop when we enter an empty directory.
>>
>> Add tests which simulate this behavior and also provide coverage over
>> using the feature over packed-refs.
>
> Nit: the commit subject might be adjusted to mention the symptom we're
> fixing rather than what we're doing in the commit. At least, it feels
> like the segfault is the more important thing to talk about here.
>

That's fair, let me modify that.

Show 19 quoted lines

>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>> ---
>>  refs/ref-cache.c               |  3 ++
>>  t/t6302-for-each-ref-filter.sh | 65 ++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 68 insertions(+)
>>
>> diff --git a/refs/ref-cache.c b/refs/ref-cache.c
>> index c180e0aad7..8a260028ec 100644
>> --- a/refs/ref-cache.c
>> +++ b/refs/ref-cache.c
>> @@ -507,6 +507,9 @@ static int cache_ref_iterator_seek(struct ref_iterator *ref_iterator,
>>  			slash = strchr(slash, '/');
>>  			len = slash ? (size_t)(slash - refname) : strlen(refname);
>>
>> +			if (dir->nr == 0)
>> +				break;
>
> Can't we break before sorting already? Avoids a couple of no-op changes.
>

Yup, in the next iteration, I'll move it to the loop condition, so we don't even enter the loop.

Show 25 quoted lines

>>  			for (idx = 0; idx < dir->nr; idx++) {
>>  				cmp = strncmp(refname, dir->entries[idx]->name, len);
>>  				if (cmp <= 0)
>> diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
>> index 9b80ea1e3b..d14567cb62 100755
>> --- a/t/t6302-for-each-ref-filter.sh
>> +++ b/t/t6302-for-each-ref-filter.sh
>> @@ -754,4 +754,69 @@ test_expect_success 'start after used with custom sort order' '
>>  	test_cmp expect actual
>>  '
>>
>> +test_expect_success 'start after with packed refs' '
>> +	test_when_finished "rm -rf repo" &&
>> +	git init repo &&
>> +	(
>> +		cd repo &&
>> +		test_commit default &&
>> +
>> +		git update-ref --stdin <<-EOF &&
>
> s/EOF/\EOF/
>
> The same is true for the other test.
>
> Patrick

This always trips me up. Will fix.

Thanks for the review.