Re: [PATCH v2] refs/ref-cache: fix SEGFAULT when seeking in empty directories

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 25, 2025, 17:07 UTC
Message-ID: <xmqqbjmyh353.fsf@gitster.g>
In-Reply-To: <20250925-583-git-for-each-ref-start-after-v2-1-3613b5a27ff1@gmail.com>

Karthik Nayak <karthik.188@gmail.com> writes:

Show 11 quoted lines

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

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?

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