Re: [PATCH v5 0/5] for-each-ref: introduce seeking functionality via '--start-after'

From: Kyle Lippincott <spectral@google.com>
Date: Jul 17, 2025, 17:08 UTC
Message-ID: <CAO_smVgdaOxiD_494qA+VxkmxNd6c=XqJDcCE2weCTknWfmkkA@mail.gmail.com>
In-Reply-To: <20250717015402.GA2127425@coredump.intra.peff.net>

On Wed, Jul 16, 2025 at 6:54 PM Jeff King <peff@peff.net> wrote:

Show 55 quoted lines

>
> On Wed, Jul 16, 2025 at 06:19:32PM -0700, Kyle Lippincott wrote:
>
> > Unfortunately I can't provide great instructions for reproducing this
> > locally, because it relies on our internal build stack (which uses
> > blaze). Getting MemorySanitizer running can be quite annoying, though
> > you might not have any issues if this test doesn't invoke any third
> > party libraries (like zlib).
> >
> > I need to sign off for the night soon, but if this isn't sufficient
> > enough information to identify what's happening here, I can try to dig
> > deeper tomorrow. This run was executed on an import of upstream commit
> > 4ea3c74afd42a503b3e0d60e1fec33bc0431e7bc (Junio's merge of this
> > series)
>
> valgrind can often find the same issues as MSan without as much headache
> to get it running (the downside is that it is _way_ slower). And indeed:
>
>   git checkout 4ea3c74afd42a503b3e0d60e1fec33bc0431e7bc &&
>   make &&
>   (cd t && ./t6302-for-each-ref-filter.sh --valgrind-only=48)
>
> yields:
>
>   ==2177572== Conditional jump or move depends on uninitialised value(s)
>   ==2177572==    at 0x3BC380: cache_ref_iterator_advance (ref-cache.c:409)
>   ==2177572==    by 0x3B69D7: ref_iterator_advance (iterator.c:15)
>   ==2177572==    by 0x3B6CC3: merge_ref_iterator_advance (iterator.c:179)
>   ==2177572==    by 0x3B69D7: ref_iterator_advance (iterator.c:15)
>   ==2177572==    by 0x3A9770: files_ref_iterator_advance (files-backend.c:902)
>   ==2177572==    by 0x3B69D7: ref_iterator_advance (iterator.c:15)
>   ==2177572==    by 0x3B7457: do_for_each_ref_iterator (iterator.c:478)
>   ==2177572==    by 0x399B43: for_each_fullref_with_seek (ref-filter.c:2718)
>   ==2177572==    by 0x399C09: for_each_fullref_in_pattern (ref-filter.c:2756)
>   ==2177572==    by 0x39B031: do_filter_refs (ref-filter.c:3263)
>   ==2177572==    by 0x39B2B7: filter_and_format_refs (ref-filter.c:3364)
>   ==2177572==    by 0x18C1D2: cmd_for_each_ref (for-each-ref.c:115)
>   ==2177572==  Uninitialised value was created by a heap allocation
>   ==2177572==    at 0x484BDD0: realloc (vg_replace_malloc.c:1801)
>   ==2177572==    by 0x44E941: xrealloc (wrapper.c:140)
>   ==2177572==    by 0x3BCAD9: cache_ref_iterator_begin (ref-cache.c:580)
>   ==2177572==    by 0x3A988A: files_ref_iterator_begin (files-backend.c:995)
>   ==2177572==    by 0x3A295E: refs_ref_iterator_begin (refs.c:1776)
>   ==2177572==    by 0x399AF6: for_each_fullref_with_seek (ref-filter.c:2710)
>   ==2177572==    by 0x399C09: for_each_fullref_in_pattern (ref-filter.c:2756)
>   ==2177572==    by 0x39B031: do_filter_refs (ref-filter.c:3263)
>   ==2177572==    by 0x39B2B7: filter_and_format_refs (ref-filter.c:3364)
>   ==2177572==    by 0x18C1D2: cmd_for_each_ref (for-each-ref.c:115)
>   ==2177572==    by 0x128C90: run_builtin (git.c:480)
>   ==2177572==    by 0x1290EB: handle_builtin (git.c:746)
>
> Bisecting doesn't tell us much, though (the first commit that introduces
> the test shows the problem). I didn't dig further than that.
>
> -Peff

Thanks for that, that helped me a bit too as it provides more information than I was getting out of MemorySanitizer (I suspect MemorySanitizer was producing the information it just wasn't going to stderr or something, or maybe I was missing a flag to get it to report more). I'm not sure what the right fix would be; my guess is that the fix would be to modify the places where we set levels_nr and initialize the other fields in level to also set it to prefix_state (around lines 488 and 527 in ref-cache.c); and indeed setting the prefix_state to PREFIX_CONTAINS_DIR (the 0 value of the enum) makes the test pass even under valgrind. Unfortunately without a much more in-depth knowledge of the code and the enum values I can't definitively state that those are the correct values. I can say that setting it to PREFIX_WITHIN_DIR causes both additional valgrind failures and test failures even without valgrind, but setting it to PREFIX_EXCLUDES_DIR doesn't seem to be a problem. I also moved the `if` around like 409 into the following if, because that was the only time entry_prefix_state was used, I'd been thinking that maybe it needed the check for entry->flag & REF_DIR prior to referencing level->prefix_state, but that didn't resolve it on its own.

I don't mind if anyone else picks up this fix and runs with it, but I'm not comfortable sending this patch myself because I don't have enough knowledge of this are of the code to know if it's right, just that it fixes the issue we encountered, and I'm extremely overloaded right now and can't get that knowledge nor see the patch through to the end.

diff --git a/refs/ref-cache.c b/refs/ref-cache.c
index 1d95b56d40..24feb33fcb 100644
--- a/refs/ref-cache.c
+++ b/refs/ref-cache.c
@@ -391,7 +391,6 @@ static int cache_ref_iterator_advance(struct
ref_iterator *ref_iterator)
                        &iter->levels[iter->levels_nr - 1];
                struct ref_dir *dir = level->dir;
                struct ref_entry *entry;
-               enum prefix_state entry_prefix_state;

                if (level->index == -1)
                        sort_ref_dir(dir);
@@ -406,16 +405,17 @@ static int cache_ref_iterator_advance(struct
ref_iterator *ref_iterator)

                entry = dir->entries[level->index];

-               if (level->prefix_state == PREFIX_WITHIN_DIR) {
-                       entry_prefix_state =
overlaps_prefix(entry->name, iter->prefix);
-                       if (entry_prefix_state == PREFIX_EXCLUDES_DIR ||
-                           (entry_prefix_state == PREFIX_WITHIN_DIR
&& !(entry->flag & REF_DIR)))
-                               continue;
-               } else {
-                       entry_prefix_state = level->prefix_state;
-               }
-
                if (entry->flag & REF_DIR) {
+                       enum prefix_state entry_prefix_state;
+                       if (level->prefix_state == PREFIX_WITHIN_DIR) {
+                               entry_prefix_state =
overlaps_prefix(entry->name, iter->prefix);
+                               if (entry_prefix_state == PREFIX_EXCLUDES_DIR ||
+                                   (entry_prefix_state ==
PREFIX_WITHIN_DIR && !(entry->flag & REF_DIR)))
+                                       continue;
+                       } else {
+                               entry_prefix_state = level->prefix_state;
+                       }
+
                        /* push down a level */
                        ALLOC_GROW(iter->levels, iter->levels_nr + 1,
                                   iter->levels_alloc);
@@ -489,6 +489,7 @@ static int cache_ref_iterator_seek(struct
ref_iterator *ref_iterator,
                level = &iter->levels[0];
                level->index = -1;
                level->dir = dir;
+               level->prefix_state = PREFIX_EXCLUDES_DIR;      //
FIXME: PROBABLY NOT CORRECT

                /* Unset any previously set prefix */
                FREE_AND_NULL(iter->prefix);
@@ -527,6 +528,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_EXCLUDES_DIR;      // FIXME: PROBABLY NOT CORRECT
                        } else {
                                /* reduce the index so the leaf node
is iterated over */
                                if (cmp <= 0 && !slash)