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

From: Jeff King <peff@peff.net>
Date: Jul 17, 2025, 22:09 UTC
Message-ID: <20250717220929.GB2244266@coredump.intra.peff.net>
In-Reply-To: <CAO_smVj4e0XOQyQr5sDyMZ8WPvqcBe2Y33DFHrED7C=VJnm4eg@mail.gmail.com>

On Thu, Jul 17, 2025 at 12:35:58PM -0700, Kyle Lippincott wrote:

Show 39 quoted lines

> > ==3275333==WARNING: MemorySanitizer: use-of-uninitialized-value
> >     #0 0x557bd886f4bb in git_mkstemps_mode ../wrapper.c:487:27
> >     #1 0x557bd886fb55 in git_mkstemp_mode ../wrapper.c:509:9
> >     #2 0x557bd8100d1a in create_tmpfile ../object-file.c:736:7
> >     #3 0x557bd80f1630 in start_loose_object_common ../object-file.c:781:7
> >     #4 0x557bd80f5203 in write_loose_object ../object-file.c:881:7
> >     #5 0x557bd80f4875 in write_object_file_flags ../object-file.c:1086:6
> >     #6 0x557bd80f9f65 in write_object_file ../object-file.h:181:9
> >     #7 0x557bd8101eb8 in index_mem ../object-file.c:1177:9
> >     #8 0x557bd80f8bd5 in index_core ../object-file.c:1247:10
> >     #9 0x557bd80f731d in index_fd ../object-file.c:1274:9
> >     #10 0x557bd80f95e4 in index_path ../object-file.c:1295:7
> >     #11 0x557bd831132d in add_to_index ../read-cache.c:771:7
> >     #12 0x557bd8313cb1 in add_file_to_index ../read-cache.c:804:9
> >     #13 0x557bd73f892c in add_files ../builtin/add.c:355:7
> >     #14 0x557bd73f4752 in cmd_add ../builtin/add.c:578:18
> >     #15 0x557bd7a38b6f in run_builtin ../git.c:480:11
> >     #16 0x557bd7a31d54 in handle_builtin ../git.c:746:9
> >     #17 0x557bd7a36644 in run_argv ../git.c:813:4
> >     #18 0x557bd7a30e09 in cmd_main ../git.c:953:19
> >     #19 0x557bd7a3ca01 in main ../common-main.c:9:11
> >     #20 0x7f7e3f02a4d7 in __libc_start_call_main
> > (/nix/store/g2jzxk3s7cnkhh8yq55l4fbvf639zy37-glibc-2.40-66/lib/libc.so.6+0x2a4d7)
> > (BuildId: f117ee0f586dfa828cbdd08e37393c8f04f6480a)
> >     #21 0x7f7e3f02a59a in __libc_start_main@GLIBC_2.2.5
> > (/nix/store/g2jzxk3s7cnkhh8yq55l4fbvf639zy37-glibc-2.40-66/lib/libc.so.6+0x2a59a)
> > (BuildId: f117ee0f586dfa828cbdd08e37393c8f04f6480a)
> >     #22 0x557bd7352b34 in _start (git+0x5db34)
> >
> > Possibly something we need to look into cleaning up.
> 
> I also saw those msan issues when trying `make
> CFLAGS=-fsanitize=memory CC=clang`, but not with Google's internal
> msan build. I don't know which variable in wrapper.c:487 it's
> complaining about - you'd think it'd be `letters`, but if it's `v`,
> then that potentially comes from OpenSSL or some other library, and
> that library would also need to be built with msan (which is why it's
> such a pain to get msan builds working - EVERY library needs to be
> built with memory sanitizer).

Yeah, presumably it is "v" from csprng_bytes(). If there are only a few such spots, we can manually "unpoison" memory coming from libraries. On my system, I didn't hit the case shown above but do have trouble with bytes coming back from zlib.

Applying this ancient patch:

  https://lore.kernel.org/git/20171004101932.pai6wzcv2eohsicr@sigill.intra.peff.net/

and building with "make SANITIZE=memory CC=clang" let me run t6302 to completion, modulo the bug that started this thread (and which I confirmed goes away both with MSan and valgrind with the fix Karthik posted).

Probably:

diff --git a/wrapper.c b/wrapper.c
index 2f00d2ac87..6a4c1c1c29 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -482,6 +482,8 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode)
 		if (csprng_bytes(&v, sizeof(v), 0) < 0)
 			return error_errno("unable to get random bytes for temporary file");
 
+		msan_unpoison(&v, sizeof(v));
+
 		/* Fill in the random bits. */
 		for (i = 0; i < num_x; i++) {
 			filename_template[i] = letters[v % num_letters];


on top of that would fix the problem you guys are seeing. I don't know
if that path leads to insanity, though. Using MSan-enabled libraries is
probably a better direction (should increase accuracy, and we don't have
to carry these manual annotations around).

-Peff