Re: [PATCH v2] refs/ref-cache: fix SEGFAULT when seeking in empty directories
- From
Karthik Nayak <karthik.188@gmail.com>
- Date
- Sep 26, 2025, 07:41 UTC
- Message-ID
- <CAOLa=ZSfL1CVCNjD0Vi3kpQqLM6t7YVa=G2ChGpaza6WOjZTCw@mail.gmail.com>
- In-Reply-To
- <xmqqbjmyh353.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
Show 31 quoted lines
> 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.
Show 14 quoted lines
> 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.
Show 16 quoted lines
> > [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.