From: Karthik Nayak Date: Thu, 25 Sep 2025 08:20:11 GMT Subject: Re: [PATCH] refs/ref-cache: stop seeking into empty directories Message-ID: In-Reply-To: Patrick Steinhardt writes: > 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`. >> 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. >> Signed-off-by: Karthik Nayak >> --- >> 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. >> 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.