From: Junio C Hamano <gitster@pobox.com>
Date: Thu, 25 Sep 2025 17:07:36 GMT
Subject: Re: [PATCH v2] refs/ref-cache: fix SEGFAULT when seeking in empty directories
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:

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