Junio C Hamano <gitster@xxxxxxxxx> writes:
> When cache_ref_iterator_seek() "jumps" to a middle of the sorted ref
> list, it forgets to set the .prefix_state member of the new
> (i.e. deeper) level it just initialized. This later causes
> cache_ref_iterator_advance() to look at this uninitialized member
> to base its decision on what to do next.
>
I think the explanation is correct. For reference I had some more
details in my local patch, but this is totally okay.
ref-cache: set prefix_state when seeking
In 090eb5336c (refs: selectively set prefix in the seek functions,
2025-07-15) we separated the seeking functionality of reference
iterators from the functionality to set prefix to an iterator. This
allows users of ref iterators to seek to a particular reference to
provide pagination support.
The files-backend, uses the ref-cache iterator to iterate over loose
refs. The iterator tracks directories and entries already processed via
a stack of levels. Each level corresponds to a directory under the files
backend. New levels are added to the stack, and when all entries from a
level is yielded, the corresponding level is popped from the stack.
To accommodate seeking, we need to populate and traverse the levels to
stop the requested seek marker at the appropriate level and its entry
index. Each level also contains a 'prefix_state' which is used for
prefix matching, this allows the iterator to skip levels/entries which
don't match a prefix. The default value of 'prefix_state' is
PREFIX_CONTAINS_DIR, which yields all entries within a level. When
purely seeking without prefix matching, we want to yield all entries.
The commit however, skips setting the value explicitly. This causes the
MemorySanitizer to issue a 'use-of-uninitialized-value' error when
running 't/t6302-for-each-ref-filter'.
Set the value explicitly to avoid to fix the issue.
> Kyle Lippincott [*] and Jeff King noticed this with MSAN and
> Valgrind, and Karthik Nayak as the original author located exactly
> where the missing initialization is.
>
> [*] <CAO_smVg9TDakUnubepjPGmLyOzW6n8Z=MDbnZKvkwN2=kN2RRw@xxxxxxxxxxxxxx>
>
> Signed-off-by: Junio C Hamano <gitster@xxxxxxxxx>
> ---
> refs/ref-cache.c | 1 +
> 1 file changed, 1 insertion(+)
>
> * I had this as "fixup!" on top of your topic for quite a while and
> forgot to ask you to send in an official fix. As Kyle's
> discovery was after the topic hit 'next' (understandable, as
> their internal edition of Git is based on 'next'), we need a
> separate fix on top.
>
> To prepare for merging down the whole thing to 'master', I wrote
> the proposed log message to help expedite the process. Comments?
>
I had a set of patches locally, I just didn't get around to sending it.
Will send the others, omitting this. Thanks for doing it!
> diff --git a/refs/ref-cache.c b/refs/ref-cache.c
> index 1d95b56d40..ceef3a2008 100644
> --- a/refs/ref-cache.c
> +++ b/refs/ref-cache.c
> @@ -527,6 +527,7 @@ static int cache_ref_iterator_seek(struct ref_iterator *ref_iterator,
> level = &iter->levels[iter->levels_nr++];
> level->dir = dir;
> level->index = -1;
> + level->prefix_state = PREFIX_CONTAINS_DIR;
> } else {
> /* reduce the index so the leaf node is iterated over */
> if (cmp <= 0 && !slash)
> --
> 2.50.1-521-gf11ee0bd80
The patch looks good.
Attachment:
signature.asc
Description: PGP signature
