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

From: Patrick Steinhardt <ps@pks.im>
Date: Sep 24, 2025, 12:28 UTC
Message-ID: <aNPkA9UZtEqHsa3f@pks.im>
In-Reply-To: <20250924-583-git-for-each-ref-start-after-v1-1-c73be2b5db5a@gmail.com>

On Wed, Sep 24, 2025 at 09:55:51AM +0200, Karthik Nayak wrote:

Show 10 quoted lines

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

Show 5 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.

Show 16 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.

Show 19 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