On Wed, Jul 16, 2025 at 6:54 PM Jeff King <peff@xxxxxxxx> wrote:
>
> 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)
