git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* [PATCH 0/3] Fix uninitialised reads found with MSAN
@ 2021-06-10 16:48 Andrzej Hunt via GitGitGadget
  2021-06-10 16:48 ` [PATCH 1/3] bulk-checkin: make buffer reuse more obvious and safer Andrzej Hunt via GitGitGadget
                   ` (5 more replies)
  0 siblings, 6 replies; 15+ messages in thread
From: Andrzej Hunt via GitGitGadget @ 2021-06-10 16:48 UTC (permalink / raw)
  To: git; +Cc: Andrzej Hunt

This series fixes a small number of issues found when running git's
test-suite with MSAN (MemorySanitizer: a clang sanitizer that tries to
detect reads from uninitialised memory [2]). To summarise: I think there's
one real bug, one theoretical bug where compiler nevertheless produce
working code, and one false-positive that we can easily suppress.

Getting the test suite to run under MSAN is a bit trickier than simply
adding SANITIZERS=memory, I've detailed the reasons and the process I'm
using below. Unfortunately this series is also not sufficient to make the
whole test suite pass when building with MSAN:

 * t0005-sigchain and t7006-pager fail with an infinite loop inside MSAN's
   signal handling interceptors. I think this is a bad interaction between
   git's signal handling and MSAN's interceptors, and I suspect it's not
   indicative of a bug in git itself - but I haven't investigated in detail
   yet.
 * t3206-range-diff, t4013-diff-various, t4018-diff-funcname all fail due to
   a change in diff output. I can reproduce this issue when running with
   TSAN (but not ASAN or UBSAN), which suggests a bug or difference in
   behaviour in code shared between MSAN and TSAN - similarly, I haven't
   investigated in all that much detail yet.

(These issues were seen when running with clang-11 - the next step is to
test with clang built from main)

As to the tricky part: MSAN tries to detect reads from uninitialised memory
at runtime. However you need to ensure that all code performing
initialisation is built with the right instrumentation (i.e.
-fsanitize=memory). So you'll immediately run into issues if you link
against libraries provided by your system (with the exception of libc, as
MSAN provides some default interceptors for most of libc). In theory you
should rebuild all dependencies with -fsanitize=memory, although I
discovered that it's sufficient to recompile only zlib + link git against
that copy of zlib (which not a very tricky thing to do). Doing this will
uncover one intentional read from uninitialised memory inside zlib itself.
This can be worked around with an annotation in zlib (which I'm trying to
submit upstream at [1]) - but it's also possible to define an override list
at compile time - I've detailed this in my recipe below).

My recipe for running git tests against MSAN:

 1. Grab zlib sources from zlib.net or github.com/madler/zlib , I used zlib
    1.2.11 (which is also what most systems seem to ship).

 2. Create a sanitizers special cast list (named e.g. ignorelist.txt)
    containing "fun:slide_hash" (this is only needed as long as zlib doesn't
    contain [1]).

 3. Build zlib, installing it into SOME_PREFIX (I happened to use clang, but
    that might not be necessary): CC=clang-11 CFLAGS="-fsanitize=memory
    -fno-sanitize-recover=memory
    -fsanitize-ignorelist=YOUR_IGNORELIST_FROM_STEP_2" ./configure && make
    install prefix=$SOME_PREFIX

 4. Build git and run the tests (again, I'm using clang, but gcc might be OK
    too): make ZLIB_PATH=$SOME_PREFIX CC=clang-11 SANITIZERS=memory test

If you're actively trying to understand and fix issues, I also recommend
adding -fsanitize-memory-track-origins (which points you directly to where
the uninitialised memory comes from), see also further docs at [2].

ATB,

Andrzej

[1] https://github.com/madler/zlib/pull/561

[2] https://clang.llvm.org/docs/MemorySanitizer.html

Andrzej Hunt (3):
  bulk-checkin: make buffer reuse more obvious and safer
  split-index: use oideq instead of memcmp to compare object_id's
  builtin/checkout--worker: memset struct to avoid MSAN complaints

 builtin/checkout--worker.c | 11 +++++++++++
 bulk-checkin.c             |  3 +--
 split-index.c              |  3 ++-
 3 files changed, 14 insertions(+), 3 deletions(-)


base-commit: 62a8d224e6203d9d3d2d1d63a01cf5647ec312c9
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1033%2Fahunt%2Fmsan-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1033/ahunt/msan-v1
Pull-Request: https://github.com/git/git/pull/1033
-- 
gitgitgadget

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 1/3] bulk-checkin: make buffer reuse more obvious and safer
  2021-06-10 16:48 [PATCH 0/3] Fix uninitialised reads found with MSAN Andrzej Hunt via GitGitGadget
@ 2021-06-10 16:48 ` Andrzej Hunt via GitGitGadget
  2021-06-10 16:48 ` [PATCH 2/3] split-index: use oideq instead of memcmp to compare object_id's Andrzej Hunt via GitGitGadget
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 15+ messages in thread
From: Andrzej Hunt via GitGitGadget @ 2021-06-10 16:48 UTC (permalink / raw)
  To: git; +Cc: Andrzej Hunt, Andrzej Hunt

From: Andrzej Hunt <ajrhunt@google.com>

ibuf can be reused for multiple iterations of the loop. Specifically:
deflate() overwrites s.avail_in to show how much of the input buffer
has not been processed yet - and sometimes leaves 'avail_in > 0', in
which case ibuf will be processed again during the loop's subsequent
iteration.

But if we declare ibuf within the loop, then (in theory) we get a new
(and uninitialised) buffer for every iteration. In practice, my compiler
seems to resue the same buffer - meaning that this code does work - but
it doesn't seem safe to rely on this behaviour. MSAN correctly catches
this issue - as soon as we hit the 's.avail_in > 0' condition, we end up
reading from what seems to be uninitialised memory.

Therefore, we move ibuf out of the loop, making this reuse safe.

See MSAN output from t1050-large below - the interesting part is the
ibuf creation at the end, although there's a lot of indirection before
we reach the read from unitialised memory:

==11294==WARNING: MemorySanitizer: use-of-uninitialized-value
    #0 0x7f75db58fb1c in crc32_little crc32.c:283:9
    #1 0x7f75db58d5b3 in crc32_z crc32.c:220:20
    #2 0x7f75db59668c in crc32 crc32.c:242:12
    #3 0x8c94f8 in hashwrite csum-file.c:101:15
    #4 0x825faf in stream_to_pack bulk-checkin.c:154:5
    #5 0x82467b in deflate_to_pack bulk-checkin.c:225:8
    #6 0x823ff1 in index_bulk_checkin bulk-checkin.c:264:15
    #7 0xa7cff2 in index_stream object-file.c:2234:9
    #8 0xa7bff7 in index_fd object-file.c:2256:9
    #9 0xa7d22d in index_path object-file.c:2274:7
    #10 0xb3c8c9 in add_to_index read-cache.c:802:7
    #11 0xb3e039 in add_file_to_index read-cache.c:835:9
    #12 0x4a99c3 in add_files add.c:458:7
    #13 0x4a7276 in cmd_add add.c:670:18
    #14 0x4a1e76 in run_builtin git.c:461:11
    #15 0x49e1e7 in handle_builtin git.c:714:3
    #16 0x4a0c08 in run_argv git.c:781:4
    #17 0x49d5a8 in cmd_main git.c:912:19
    #18 0x7974da in main common-main.c:52:11
    #19 0x7f75da66f349 in __libc_start_main (/lib64/libc.so.6+0x24349)
    #20 0x421bd9 in _start start.S:120

  Uninitialized value was stored to memory at
    #0 0x7f75db58fa6b in crc32_little crc32.c:283:9
    #1 0x7f75db58d5b3 in crc32_z crc32.c:220:20
    #2 0x7f75db59668c in crc32 crc32.c:242:12
    #3 0x8c94f8 in hashwrite csum-file.c:101:15
    #4 0x825faf in stream_to_pack bulk-checkin.c:154:5
    #5 0x82467b in deflate_to_pack bulk-checkin.c:225:8
    #6 0x823ff1 in index_bulk_checkin bulk-checkin.c:264:15
    #7 0xa7cff2 in index_stream object-file.c:2234:9
    #8 0xa7bff7 in index_fd object-file.c:2256:9
    #9 0xa7d22d in index_path object-file.c:2274:7
    #10 0xb3c8c9 in add_to_index read-cache.c:802:7
    #11 0xb3e039 in add_file_to_index read-cache.c:835:9
    #12 0x4a99c3 in add_files add.c:458:7
    #13 0x4a7276 in cmd_add add.c:670:18
    #14 0x4a1e76 in run_builtin git.c:461:11
    #15 0x49e1e7 in handle_builtin git.c:714:3
    #16 0x4a0c08 in run_argv git.c:781:4
    #17 0x49d5a8 in cmd_main git.c:912:19
    #18 0x7974da in main common-main.c:52:11
    #19 0x7f75da66f349 in __libc_start_main (/lib64/libc.so.6+0x24349)

  Uninitialized value was stored to memory at
    #0 0x447eb9 in __msan_memcpy msan_interceptors.cpp:1558:3
    #1 0x7f75db5c2011 in flush_pending deflate.c:746:5
    #2 0x7f75db5cafa0 in deflate_stored deflate.c:1815:9
    #3 0x7f75db5bb7d2 in deflate deflate.c:1005:34
    #4 0xd80b7f in git_deflate zlib.c:244:12
    #5 0x825dff in stream_to_pack bulk-checkin.c:140:12
    #6 0x82467b in deflate_to_pack bulk-checkin.c:225:8
    #7 0x823ff1 in index_bulk_checkin bulk-checkin.c:264:15
    #8 0xa7cff2 in index_stream object-file.c:2234:9
    #9 0xa7bff7 in index_fd object-file.c:2256:9
    #10 0xa7d22d in index_path object-file.c:2274:7
    #11 0xb3c8c9 in add_to_index read-cache.c:802:7
    #12 0xb3e039 in add_file_to_index read-cache.c:835:9
    #13 0x4a99c3 in add_files add.c:458:7
    #14 0x4a7276 in cmd_add add.c:670:18
    #15 0x4a1e76 in run_builtin git.c:461:11
    #16 0x49e1e7 in handle_builtin git.c:714:3
    #17 0x4a0c08 in run_argv git.c:781:4
    #18 0x49d5a8 in cmd_main git.c:912:19
    #19 0x7974da in main common-main.c:52:11

  Uninitialized value was stored to memory at
    #0 0x447eb9 in __msan_memcpy msan_interceptors.cpp:1558:3
    #1 0x7f75db644241 in _tr_stored_block trees.c:873:5
    #2 0x7f75db5cad7c in deflate_stored deflate.c:1813:9
    #3 0x7f75db5bb7d2 in deflate deflate.c:1005:34
    #4 0xd80b7f in git_deflate zlib.c:244:12
    #5 0x825dff in stream_to_pack bulk-checkin.c:140:12
    #6 0x82467b in deflate_to_pack bulk-checkin.c:225:8
    #7 0x823ff1 in index_bulk_checkin bulk-checkin.c:264:15
    #8 0xa7cff2 in index_stream object-file.c:2234:9
    #9 0xa7bff7 in index_fd object-file.c:2256:9
    #10 0xa7d22d in index_path object-file.c:2274:7
    #11 0xb3c8c9 in add_to_index read-cache.c:802:7
    #12 0xb3e039 in add_file_to_index read-cache.c:835:9
    #13 0x4a99c3 in add_files add.c:458:7
    #14 0x4a7276 in cmd_add add.c:670:18
    #15 0x4a1e76 in run_builtin git.c:461:11
    #16 0x49e1e7 in handle_builtin git.c:714:3
    #17 0x4a0c08 in run_argv git.c:781:4
    #18 0x49d5a8 in cmd_main git.c:912:19
    #19 0x7974da in main common-main.c:52:11

  Uninitialized value was stored to memory at
    #0 0x447eb9 in __msan_memcpy msan_interceptors.cpp:1558:3
    #1 0x7f75db5c8fcf in deflate_stored deflate.c:1783:9
    #2 0x7f75db5bb7d2 in deflate deflate.c:1005:34
    #3 0xd80b7f in git_deflate zlib.c:244:12
    #4 0x825dff in stream_to_pack bulk-checkin.c:140:12
    #5 0x82467b in deflate_to_pack bulk-checkin.c:225:8
    #6 0x823ff1 in index_bulk_checkin bulk-checkin.c:264:15
    #7 0xa7cff2 in index_stream object-file.c:2234:9
    #8 0xa7bff7 in index_fd object-file.c:2256:9
    #9 0xa7d22d in index_path object-file.c:2274:7
    #10 0xb3c8c9 in add_to_index read-cache.c:802:7
    #11 0xb3e039 in add_file_to_index read-cache.c:835:9
    #12 0x4a99c3 in add_files add.c:458:7
    #13 0x4a7276 in cmd_add add.c:670:18
    #14 0x4a1e76 in run_builtin git.c:461:11
    #15 0x49e1e7 in handle_builtin git.c:714:3
    #16 0x4a0c08 in run_argv git.c:781:4
    #17 0x49d5a8 in cmd_main git.c:912:19
    #18 0x7974da in main common-main.c:52:11
    #19 0x7f75da66f349 in __libc_start_main (/lib64/libc.so.6+0x24349)

  Uninitialized value was stored to memory at
    #0 0x447eb9 in __msan_memcpy msan_interceptors.cpp:1558:3
    #1 0x7f75db5ea545 in read_buf deflate.c:1181:5
    #2 0x7f75db5c97f7 in deflate_stored deflate.c:1791:9
    #3 0x7f75db5bb7d2 in deflate deflate.c:1005:34
    #4 0xd80b7f in git_deflate zlib.c:244:12
    #5 0x825dff in stream_to_pack bulk-checkin.c:140:12
    #6 0x82467b in deflate_to_pack bulk-checkin.c:225:8
    #7 0x823ff1 in index_bulk_checkin bulk-checkin.c:264:15
    #8 0xa7cff2 in index_stream object-file.c:2234:9
    #9 0xa7bff7 in index_fd object-file.c:2256:9
    #10 0xa7d22d in index_path object-file.c:2274:7
    #11 0xb3c8c9 in add_to_index read-cache.c:802:7
    #12 0xb3e039 in add_file_to_index read-cache.c:835:9
    #13 0x4a99c3 in add_files add.c:458:7
    #14 0x4a7276 in cmd_add add.c:670:18
    #15 0x4a1e76 in run_builtin git.c:461:11
    #16 0x49e1e7 in handle_builtin git.c:714:3
    #17 0x4a0c08 in run_argv git.c:781:4
    #18 0x49d5a8 in cmd_main git.c:912:19
    #19 0x7974da in main common-main.c:52:11

  Uninitialized value was created by an allocation of 'ibuf' in the stack frame of function 'stream_to_pack'
    #0 0x825710 in stream_to_pack bulk-checkin.c:101

SUMMARY: MemorySanitizer: use-of-uninitialized-value crc32.c:283:9 in crc32_little
Exiting

Signed-off-by: Andrzej Hunt <andrzej@ahunt.org>
---
 bulk-checkin.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/bulk-checkin.c b/bulk-checkin.c
index 127312acd1ed..b023d9959aae 100644
--- a/bulk-checkin.c
+++ b/bulk-checkin.c
@@ -100,6 +100,7 @@ static int stream_to_pack(struct bulk_checkin_state *state,
 			  const char *path, unsigned flags)
 {
 	git_zstream s;
+	unsigned char ibuf[16384];
 	unsigned char obuf[16384];
 	unsigned hdrlen;
 	int status = Z_OK;
@@ -113,8 +114,6 @@ static int stream_to_pack(struct bulk_checkin_state *state,
 	s.avail_out = sizeof(obuf) - hdrlen;
 
 	while (status != Z_STREAM_END) {
-		unsigned char ibuf[16384];
-
 		if (size && !s.avail_in) {
 			ssize_t rsize = size < sizeof(ibuf) ? size : sizeof(ibuf);
 			ssize_t read_result = read_in_full(fd, ibuf, rsize);
-- 
gitgitgadget


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 2/3] split-index: use oideq instead of memcmp to compare object_id's
  2021-06-10 16:48 [PATCH 0/3] Fix uninitialised reads found with MSAN Andrzej Hunt via GitGitGadget
  2021-06-10 16:48 ` [PATCH 1/3] bulk-checkin: make buffer reuse more obvious and safer Andrzej Hunt via GitGitGadget
@ 2021-06-10 16:48 ` Andrzej Hunt via GitGitGadget
  2021-06-10 16:48 ` [PATCH 3/3] builtin/checkout--worker: memset struct to avoid MSAN complaints Andrzej Hunt via GitGitGadget
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 15+ messages in thread
From: Andrzej Hunt via GitGitGadget @ 2021-06-10 16:48 UTC (permalink / raw)
  To: git; +Cc: Andrzej Hunt, Andrzej Hunt

From: Andrzej Hunt <ajrhunt@google.com>

cache_entry contains an object_id, and compare_ce_content() would
include that field when calling memcmp on a subset of the cache_entry.
Depending on which hashing algorithm is being used, only part of
object_id.hash is actually being used, therefore including it in a
memcmp() is technically incorrect. Instead we choose to exclude the
object_id when calling memcmp(), and call oideq() separately.

This issue was found when running t1700-split-index with MSAN, see MSAN
output below (on my machine, offset 76 corresponds to 4 bytes after the
start of object_id.hash).

Uninitialized bytes in MemcmpInterceptorCommon at offset 76 inside [0x7f60e7c00118, 92)
==27914==WARNING: MemorySanitizer: use-of-uninitialized-value
    #0 0x4524ee in memcmp /home/abuild/rpmbuild/BUILD/llvm-11.0.0.src/build/../projects/compiler-rt/lib/msan/../sanitizer_common/sanitizer_common_interceptors.inc:873:10
    #1 0xc867ae in compare_ce_content /home/ahunt/git/git/split-index.c:208:8
    #2 0xc859fb in prepare_to_write_split_index /home/ahunt/git/git/split-index.c:336:9
    #3 0xb4bbca in write_split_index /home/ahunt/git/git/read-cache.c:3107:2
    #4 0xb42b4d in write_locked_index /home/ahunt/git/git/read-cache.c:3295:8
    #5 0x638058 in try_merge_strategy /home/ahunt/git/git/builtin/merge.c:758:7
    #6 0x63057f in cmd_merge /home/ahunt/git/git/builtin/merge.c:1663:9
    #7 0x4a1e76 in run_builtin /home/ahunt/git/git/git.c:461:11
    #8 0x49e1e7 in handle_builtin /home/ahunt/git/git/git.c:714:3
    #9 0x4a0c08 in run_argv /home/ahunt/git/git/git.c:781:4
    #10 0x49d5a8 in cmd_main /home/ahunt/git/git/git.c:912:19
    #11 0x7974da in main /home/ahunt/git/git/common-main.c:52:11
    #12 0x7f60e928e349 in __libc_start_main (/lib64/libc.so.6+0x24349)
    #13 0x421bd9 in _start /home/abuild/rpmbuild/BUILD/glibc-2.26/csu/../sysdeps/x86_64/start.S:120

  Uninitialized value was stored to memory at
    #0 0x447eb9 in __msan_memcpy /home/abuild/rpmbuild/BUILD/llvm-11.0.0.src/build/../projects/compiler-rt/lib/msan/msan_interceptors.cpp:1558:3
    #1 0xb4d1e6 in dup_cache_entry /home/ahunt/git/git/read-cache.c:3457:2
    #2 0xd214fa in add_entry /home/ahunt/git/git/unpack-trees.c:215:18
    #3 0xd1fae0 in keep_entry /home/ahunt/git/git/unpack-trees.c:2276:2
    #4 0xd1ff9e in twoway_merge /home/ahunt/git/git/unpack-trees.c:2504:11
    #5 0xd27028 in call_unpack_fn /home/ahunt/git/git/unpack-trees.c:593:12
    #6 0xd2443d in unpack_nondirectories /home/ahunt/git/git/unpack-trees.c:1106:12
    #7 0xd19435 in unpack_callback /home/ahunt/git/git/unpack-trees.c:1306:6
    #8 0xd0d7ff in traverse_trees /home/ahunt/git/git/tree-walk.c:532:17
    #9 0xd1773a in unpack_trees /home/ahunt/git/git/unpack-trees.c:1683:9
    #10 0xdc6370 in checkout /home/ahunt/git/git/merge-ort.c:3590:8
    #11 0xdc51c3 in merge_switch_to_result /home/ahunt/git/git/merge-ort.c:3728:7
    #12 0xa195a9 in merge_ort_recursive /home/ahunt/git/git/merge-ort-wrappers.c:58:2
    #13 0x637fff in try_merge_strategy /home/ahunt/git/git/builtin/merge.c:751:12
    #14 0x63057f in cmd_merge /home/ahunt/git/git/builtin/merge.c:1663:9
    #15 0x4a1e76 in run_builtin /home/ahunt/git/git/git.c:461:11
    #16 0x49e1e7 in handle_builtin /home/ahunt/git/git/git.c:714:3
    #17 0x4a0c08 in run_argv /home/ahunt/git/git/git.c:781:4
    #18 0x49d5a8 in cmd_main /home/ahunt/git/git/git.c:912:19
    #19 0x7974da in main /home/ahunt/git/git/common-main.c:52:11

  Uninitialized value was created by a heap allocation
    #0 0x44e73d in malloc /home/abuild/rpmbuild/BUILD/llvm-11.0.0.src/build/../projects/compiler-rt/lib/msan/msan_interceptors.cpp:901:3
    #1 0xd592f6 in do_xmalloc /home/ahunt/git/git/wrapper.c:41:8
    #2 0xd59248 in xmalloc /home/ahunt/git/git/wrapper.c:62:9
    #3 0xa17088 in mem_pool_alloc_block /home/ahunt/git/git/mem-pool.c:22:6
    #4 0xa16f78 in mem_pool_init /home/ahunt/git/git/mem-pool.c:44:3
    #5 0xb481b8 in load_all_cache_entries /home/ahunt/git/git/read-cache.c
    #6 0xb44d40 in do_read_index /home/ahunt/git/git/read-cache.c:2298:17
    #7 0xb48a1b in read_index_from /home/ahunt/git/git/read-cache.c:2389:8
    #8 0xbd5a0b in repo_read_index /home/ahunt/git/git/repository.c:276:8
    #9 0xb4bcaf in repo_read_index_unmerged /home/ahunt/git/git/read-cache.c:3326:2
    #10 0x62ed26 in cmd_merge /home/ahunt/git/git/builtin/merge.c:1362:6
    #11 0x4a1e76 in run_builtin /home/ahunt/git/git/git.c:461:11
    #12 0x49e1e7 in handle_builtin /home/ahunt/git/git/git.c:714:3
    #13 0x4a0c08 in run_argv /home/ahunt/git/git/git.c:781:4
    #14 0x49d5a8 in cmd_main /home/ahunt/git/git/git.c:912:19
    #15 0x7974da in main /home/ahunt/git/git/common-main.c:52:11
    #16 0x7f60e928e349 in __libc_start_main (/lib64/libc.so.6+0x24349)

SUMMARY: MemorySanitizer: use-of-uninitialized-value /home/abuild/rpmbuild/BUILD/llvm-11.0.0.src/build/../projects/compiler-rt/lib/msan/../sanitizer_common/sanitizer_common_interceptors.inc:873:10 in memcmp
Exiting

Signed-off-by: Andrzej Hunt <andrzej@ahunt.org>
---
 split-index.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/split-index.c b/split-index.c
index 4d6e52d46f75..8e52e891c3bc 100644
--- a/split-index.c
+++ b/split-index.c
@@ -207,7 +207,8 @@ static int compare_ce_content(struct cache_entry *a, struct cache_entry *b)
 	b->ce_flags &= ondisk_flags;
 	ret = memcmp(&a->ce_stat_data, &b->ce_stat_data,
 		     offsetof(struct cache_entry, name) -
-		     offsetof(struct cache_entry, ce_stat_data));
+		     offsetof(struct cache_entry, oid)) ||
+		!oideq(&a->oid, &b->oid);
 	a->ce_flags = ce_flags;
 	b->ce_flags = base_flags;
 
-- 
gitgitgadget


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 3/3] builtin/checkout--worker: memset struct to avoid MSAN complaints
  2021-06-10 16:48 [PATCH 0/3] Fix uninitialised reads found with MSAN Andrzej Hunt via GitGitGadget
  2021-06-10 16:48 ` [PATCH 1/3] bulk-checkin: make buffer reuse more obvious and safer Andrzej Hunt via GitGitGadget
  2021-06-10 16:48 ` [PATCH 2/3] split-index: use oideq instead of memcmp to compare object_id's Andrzej Hunt via GitGitGadget
@ 2021-06-10 16:48 ` Andrzej Hunt via GitGitGadget
  2021-06-11  4:43   ` Chris Torek
  2021-06-11 17:11 ` [PATCH 0/3] Fix uninitialised reads found with MSAN Jeff King
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 15+ messages in thread
From: Andrzej Hunt via GitGitGadget @ 2021-06-10 16:48 UTC (permalink / raw)
  To: git; +Cc: Andrzej Hunt, Andrzej Hunt

From: Andrzej Hunt <ajrhunt@google.com>

report_result() sends a struct to the parent process, but that struct
contains unintialised padding bytes. Running this code under MSAN
rightly triggers a warning - but we also don't care about this warning
because we control the receiving code, and we therefore know that those
padding bytes won't be read on the receiving end. Therefore we add a
memset to convince MSAN that this memory is safe to read - but only
when building with MSAN to avoid this cost in normal usage.

Interestingly, in the error-case branch, we only try to copy the first
two members of pc_item_result, by copying only PC_ITEM_RESULT_BASE_SIZE
bytes. However PC_ITEM_RESULT_BASE_SIZE is defined as
'offsetof(the_last_member)', which means that we're copying padding bytes
after the end of the second last member. We could avoid doing this by
redefining PC_ITEM_RESULT_BASE_SIZE as
'offsetof(second_last_member) + sizeof(second_last_member)', but there's
no huge benefit to doing so (and our memset hack silences the MSAN
warning in this scenario either way).

MSAN output from t2080 (partially interleaved due to the
parallel work :) ):

Uninitialized bytes in __interceptor_write at offset 12 inside [0x7fff37d83408, 160)
==23279==WARNING: MemorySanitizer: use-of-uninitialized-value
Uninitialized bytes in __interceptor_write at offset 12 inside [0x7ffdb8a07ec8, 160)
==23280==WARNING: MemorySanitizer: use-of-uninitialized-value
    #0 0xd5ac28 in xwrite /home/ahunt/git/git/wrapper.c:256:8
    #1 0xd5b327 in write_in_full /home/ahunt/git/git/wrapper.c:311:21
    #2 0xb0a8c4 in do_packet_write /home/ahunt/git/git/pkt-line.c:221:6
    #3 0xb0a5fd in packet_write /home/ahunt/git/git/pkt-line.c:242:6
    #4 0x4f7441 in report_result /home/ahunt/git/git/builtin/checkout--worker.c:69:2
    #5 0x4f6be6 in worker_loop /home/ahunt/git/git/builtin/checkout--worker.c:100:3
    #6 0x4f68d3 in cmd_checkout__worker /home/ahunt/git/git/builtin/checkout--worker.c:143:2
    #7 0x4a1e76 in run_builtin /home/ahunt/git/git/git.c:461:11
    #8 0x49e1e7 in handle_builtin /home/ahunt/git/git/git.c:714:3
    #9 0x4a0c08 in run_argv /home/ahunt/git/git/git.c:781:4
    #10 0x49d5a8 in cmd_main /home/ahunt/git/git/git.c:912:19
    #11 0x7974da in main /home/ahunt/git/git/common-main.c:52:11
    #12 0x7f8778114349 in __libc_start_main (/lib64/libc.so.6+0x24349)
    #13 0x421bd9 in _start /home/abuild/rpmbuild/BUILD/glibc-2.26/csu/../sysdeps/x86_64/start.S:120

  Uninitialized value was created by an allocation of 'res' in the stack frame of function 'report_result'
    #0 0x4f72c0 in report_result /home/ahunt/git/git/builtin/checkout--worker.c:55

SUMMARY: MemorySanitizer: use-of-uninitialized-value /home/ahunt/git/git/wrapper.c:256:8 in xwrite
Exiting
    #0 0xd5ac28 in xwrite /home/ahunt/git/git/wrapper.c:256:8
    #1 0xd5b327 in write_in_full /home/ahunt/git/git/wrapper.c:311:21
    #2 0xb0a8c4 in do_packet_write /home/ahunt/git/git/pkt-line.c:221:6
    #3 0xb0a5fd in packet_write /home/ahunt/git/git/pkt-line.c:242:6
    #4 0x4f7441 in report_result /home/ahunt/git/git/builtin/checkout--worker.c:69:2
    #5 0x4f6be6 in worker_loop /home/ahunt/git/git/builtin/checkout--worker.c:100:3
    #6 0x4f68d3 in cmd_checkout__worker /home/ahunt/git/git/builtin/checkout--worker.c:143:2
    #7 0x4a1e76 in run_builtin /home/ahunt/git/git/git.c:461:11
    #8 0x49e1e7 in handle_builtin /home/ahunt/git/git/git.c:714:3
    #9 0x4a0c08 in run_argv /home/ahunt/git/git/git.c:781:4
    #10 0x49d5a8 in cmd_main /home/ahunt/git/git/git.c:912:19
    #11 0x7974da in main /home/ahunt/git/git/common-main.c:52:11
    #12 0x7f2749a0e349 in __libc_start_main (/lib64/libc.so.6+0x24349)
    #13 0x421bd9 in _start /home/abuild/rpmbuild/BUILD/glibc-2.26/csu/../sysdeps/x86_64/start.S:120

  Uninitialized value was created by an allocation of 'res' in the stack frame of function 'report_result'
    #0 0x4f72c0 in report_result /home/ahunt/git/git/builtin/checkout--worker.c:55

SUMMARY: MemorySanitizer: use-of-uninitialized-value /home/ahunt/git/git/wrapper.c:256:8 in xwrite

Signed-off-by: Andrzej Hunt <andrzej@ahunt.org>
---
 builtin/checkout--worker.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/builtin/checkout--worker.c b/builtin/checkout--worker.c
index 289a9b8f89d0..02fa5285988f 100644
--- a/builtin/checkout--worker.c
+++ b/builtin/checkout--worker.c
@@ -56,6 +56,17 @@ static void report_result(struct parallel_checkout_item *pc_item)
 	struct pc_item_result res;
 	size_t size;
 
+#if defined(__has_feature)
+#  if __has_feature(memory_sanitizer)
+	// MSAN workaround: res contains padding bytes, which will remain
+	// permanently unintialised. Later, we read all of res in order to send
+	// it to the parent process - and MSAN (rightly) complains that we're
+	// reading those unintialised padding bytes. By memset'ing res we
+	// guarantee that there are no uninitialised bytes.
+	memset(&res, 0, sizeof(res));
+#endif
+#endif
+
 	res.id = pc_item->id;
 	res.status = pc_item->status;
 
-- 
gitgitgadget

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 3/3] builtin/checkout--worker: memset struct to avoid MSAN complaints
  2021-06-10 16:48 ` [PATCH 3/3] builtin/checkout--worker: memset struct to avoid MSAN complaints Andrzej Hunt via GitGitGadget
@ 2021-06-11  4:43   ` Chris Torek
  2021-06-11  6:28     ` Junio C Hamano
  0 siblings, 1 reply; 15+ messages in thread
From: Chris Torek @ 2021-06-11  4:43 UTC (permalink / raw)
  To: Andrzej Hunt via GitGitGadget; +Cc: Git List, Andrzej Hunt, Andrzej Hunt

On Thu, Jun 10, 2021 at 9:49 AM Andrzej Hunt via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> [snip] Therefore we add a
> memset to convince MSAN that this memory is safe to read - but only
> when building with MSAN to avoid this cost in normal usage.

It does not seem likely to be that expensive, and would definitely
be shorter without all the `#if` testing:

> diff --git a/builtin/checkout--worker.c b/builtin/checkout--worker.c
> index 289a9b8f89d0..02fa5285988f 100644
> --- a/builtin/checkout--worker.c
> +++ b/builtin/checkout--worker.c
> @@ -56,6 +56,17 @@ static void report_result(struct parallel_checkout_item *pc_item)
>         struct pc_item_result res;

This could just have `= { 0 }` added.

In any case, this and all the others in this series look good to me.

Chris

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 3/3] builtin/checkout--worker: memset struct to avoid MSAN complaints
  2021-06-11  4:43   ` Chris Torek
@ 2021-06-11  6:28     ` Junio C Hamano
  2021-06-11 15:37       ` Andrzej Hunt
  0 siblings, 1 reply; 15+ messages in thread
From: Junio C Hamano @ 2021-06-11  6:28 UTC (permalink / raw)
  To: Chris Torek
  Cc: Andrzej Hunt via GitGitGadget, Git List, Andrzej Hunt, Andrzej Hunt

Chris Torek <chris.torek@gmail.com> writes:

> On Thu, Jun 10, 2021 at 9:49 AM Andrzej Hunt via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>> [snip] Therefore we add a
>> memset to convince MSAN that this memory is safe to read - but only
>> when building with MSAN to avoid this cost in normal usage.
>
> It does not seem likely to be that expensive, and would definitely
> be shorter without all the `#if` testing:
>
>> diff --git a/builtin/checkout--worker.c b/builtin/checkout--worker.c
>> index 289a9b8f89d0..02fa5285988f 100644
>> --- a/builtin/checkout--worker.c
>> +++ b/builtin/checkout--worker.c
>> @@ -56,6 +56,17 @@ static void report_result(struct parallel_checkout_item *pc_item)
>>         struct pc_item_result res;
>
> This could just have `= { 0 }` added.

I'd prefer that very much more than the #if testing, within which //
comments that we do not want in our codebase are enclosed.

Thanks.



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 3/3] builtin/checkout--worker: memset struct to avoid MSAN complaints
  2021-06-11  6:28     ` Junio C Hamano
@ 2021-06-11 15:37       ` Andrzej Hunt
  2021-06-14  1:04         ` Junio C Hamano
  0 siblings, 1 reply; 15+ messages in thread
From: Andrzej Hunt @ 2021-06-11 15:37 UTC (permalink / raw)
  To: Junio C Hamano, Chris Torek
  Cc: Andrzej Hunt via GitGitGadget, Git List, Andrzej Hunt



On 11/06/2021 08:28, Junio C Hamano wrote:
> Chris Torek <chris.torek@gmail.com> writes:
> 
>> On Thu, Jun 10, 2021 at 9:49 AM Andrzej Hunt via GitGitGadget
>> <gitgitgadget@gmail.com> wrote:
>>> [snip] Therefore we add a
>>> memset to convince MSAN that this memory is safe to read - but only
>>> when building with MSAN to avoid this cost in normal usage.
>>
>> It does not seem likely to be that expensive, and would definitely
>> be shorter without all the `#if` testing:
>>
>>> diff --git a/builtin/checkout--worker.c b/builtin/checkout--worker.c
>>> index 289a9b8f89d0..02fa5285988f 100644
>>> --- a/builtin/checkout--worker.c
>>> +++ b/builtin/checkout--worker.c
>>> @@ -56,6 +56,17 @@ static void report_result(struct parallel_checkout_item *pc_item)
>>>          struct pc_item_result res;
>>
>> This could just have `= { 0 }` added.
> 
> I'd prefer that very much more than the #if testing, within which //
> comments that we do not want in our codebase are enclosed.


I'll fix this for V2 - thanks Chris and Junio!

(At the time I wasn't aware that this would include all members and 
padding, but I've learned more since reading the clang developer's 
discussion around padding and brace intialisation :) : 
https://reviews.llvm.org/D61280 . )

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 0/3] Fix uninitialised reads found with MSAN
  2021-06-10 16:48 [PATCH 0/3] Fix uninitialised reads found with MSAN Andrzej Hunt via GitGitGadget
                   ` (2 preceding siblings ...)
  2021-06-10 16:48 ` [PATCH 3/3] builtin/checkout--worker: memset struct to avoid MSAN complaints Andrzej Hunt via GitGitGadget
@ 2021-06-11 17:11 ` Jeff King
  2021-06-14 15:51 ` [PATCH v2 " Andrzej Hunt via GitGitGadget
  2021-06-17  9:28 ` [PATCH 0/3] Fix uninitialised reads found with MSAN Philip Oakley
  5 siblings, 0 replies; 15+ messages in thread
From: Jeff King @ 2021-06-11 17:11 UTC (permalink / raw)
  To: Andrzej Hunt via GitGitGadget; +Cc: git, Andrzej Hunt

On Thu, Jun 10, 2021 at 04:48:29PM +0000, Andrzej Hunt via GitGitGadget wrote:

> As to the tricky part: MSAN tries to detect reads from uninitialised memory
> at runtime. However you need to ensure that all code performing
> initialisation is built with the right instrumentation (i.e.
> -fsanitize=memory). So you'll immediately run into issues if you link
> against libraries provided by your system (with the exception of libc, as
> MSAN provides some default interceptors for most of libc). In theory you
> should rebuild all dependencies with -fsanitize=memory, although I
> discovered that it's sufficient to recompile only zlib + link git against
> that copy of zlib (which not a very tricky thing to do). Doing this will
> uncover one intentional read from uninitialised memory inside zlib itself.
> This can be worked around with an annotation in zlib (which I'm trying to
> submit upstream at [1]) - but it's also possible to define an override list
> at compile time - I've detailed this in my recipe below).

I played with MSAN a while ago, and yeah, the trickiest part is dealing
with libraries. I came up with this patch for handling zlib from within
Git itself:

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

It's entirely possible that it papers over actual bugs (perhaps even the
one your first patch is addressing). But I wonder if it's easier to
convince people to try the tool if there's an easy way to do it without
recompiling dependencies (I also hit issues with pcre and the libc
regex; that was a few years ago, though, so I would not be at all
surprised if they know intercept the system regex routines, at least).

-Peff

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 3/3] builtin/checkout--worker: memset struct to avoid MSAN complaints
  2021-06-11 15:37       ` Andrzej Hunt
@ 2021-06-14  1:04         ` Junio C Hamano
  0 siblings, 0 replies; 15+ messages in thread
From: Junio C Hamano @ 2021-06-14  1:04 UTC (permalink / raw)
  To: Andrzej Hunt
  Cc: Chris Torek, Andrzej Hunt via GitGitGadget, Git List, Andrzej Hunt

Andrzej Hunt <andrzej@ahunt.org> writes:

> (At the time I wasn't aware that this would include all members and
> padding, but I've learned more since reading the clang developer's 
> discussion around padding and brace intialisation :) :
> https://reviews.llvm.org/D61280 . )

Thanks for a pointer ;-)

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v2 0/3] Fix uninitialised reads found with MSAN
  2021-06-10 16:48 [PATCH 0/3] Fix uninitialised reads found with MSAN Andrzej Hunt via GitGitGadget
                   ` (3 preceding siblings ...)
  2021-06-11 17:11 ` [PATCH 0/3] Fix uninitialised reads found with MSAN Jeff King
@ 2021-06-14 15:51 ` Andrzej Hunt via GitGitGadget
  2021-06-14 15:51   ` [PATCH v2 1/3] bulk-checkin: make buffer reuse more obvious and safer Andrzej Hunt via GitGitGadget
                     ` (2 more replies)
  2021-06-17  9:28 ` [PATCH 0/3] Fix uninitialised reads found with MSAN Philip Oakley
  5 siblings, 3 replies; 15+ messages in thread
From: Andrzej Hunt via GitGitGadget @ 2021-06-14 15:51 UTC (permalink / raw)
  To: git; +Cc: Chris Torek, Jeff King, Andrzej Hunt

V2 replaces an #if'd memset with some brace initialisation (patch 3/3) as
per review comments.

I've also removed an irrelevant "technically" from commit message 2/3, and
fixed a typo in commit message 3/3.

Andrzej Hunt (3):
  bulk-checkin: make buffer reuse more obvious and safer
  split-index: use oideq instead of memcmp to compare object_id's
  builtin/checkout--worker: zero-initialise struct to avoid MSAN
    complaints

 builtin/checkout--worker.c | 2 +-
 bulk-checkin.c             | 3 +--
 split-index.c              | 3 ++-
 3 files changed, 4 insertions(+), 4 deletions(-)


base-commit: 62a8d224e6203d9d3d2d1d63a01cf5647ec312c9
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1033%2Fahunt%2Fmsan-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1033/ahunt/msan-v2
Pull-Request: https://github.com/git/git/pull/1033

Range-diff vs v1:

 1:  7659d4bf13c2 = 1:  7659d4bf13c2 bulk-checkin: make buffer reuse more obvious and safer
 2:  14b0d5dd7fce ! 2:  6943eb511bee split-index: use oideq instead of memcmp to compare object_id's
     @@ Commit message
          include that field when calling memcmp on a subset of the cache_entry.
          Depending on which hashing algorithm is being used, only part of
          object_id.hash is actually being used, therefore including it in a
     -    memcmp() is technically incorrect. Instead we choose to exclude the
     -    object_id when calling memcmp(), and call oideq() separately.
     +    memcmp() is incorrect. Instead we choose to exclude the object_id when
     +    calling memcmp(), and call oideq() separately.
      
          This issue was found when running t1700-split-index with MSAN, see MSAN
          output below (on my machine, offset 76 corresponds to 4 bytes after the
 3:  cd1e1f6985c7 ! 3:  4bdc0b77f6f2 builtin/checkout--worker: memset struct to avoid MSAN complaints
     @@ Metadata
      Author: Andrzej Hunt <ajrhunt@google.com>
      
       ## Commit message ##
     -    builtin/checkout--worker: memset struct to avoid MSAN complaints
     +    builtin/checkout--worker: zero-initialise struct to avoid MSAN complaints
      
          report_result() sends a struct to the parent process, but that struct
     -    contains unintialised padding bytes. Running this code under MSAN
     -    rightly triggers a warning - but we also don't care about this warning
     -    because we control the receiving code, and we therefore know that those
     -    padding bytes won't be read on the receiving end. Therefore we add a
     -    memset to convince MSAN that this memory is safe to read - but only
     -    when building with MSAN to avoid this cost in normal usage.
     +    would contain uninitialised padding bytes. Running this code under MSAN
     +    rightly triggers a warning - but we don't particularly care about this
     +    warning because we control the receiving code, and we therefore know
     +    that those padding bytes won't be read on the receiving end.
     +
     +    We could simply suppress this warning under MSAN with the approporiate
     +    ifdef'd attributes, but a less intrusive solution is to 0-initialise the
     +    struct, which guarantees that the padding will also be initialised.
      
          Interestingly, in the error-case branch, we only try to copy the first
          two members of pc_item_result, by copying only PC_ITEM_RESULT_BASE_SIZE
     @@ Commit message
          after the end of the second last member. We could avoid doing this by
          redefining PC_ITEM_RESULT_BASE_SIZE as
          'offsetof(second_last_member) + sizeof(second_last_member)', but there's
     -    no huge benefit to doing so (and our memset hack silences the MSAN
     -    warning in this scenario either way).
     +    no huge benefit to doing so (and this patch silences the MSAN warning in
     +    this scenario either way).
      
          MSAN output from t2080 (partially interleaved due to the
          parallel work :) ):
     @@ Commit message
          Signed-off-by: Andrzej Hunt <andrzej@ahunt.org>
      
       ## builtin/checkout--worker.c ##
     -@@ builtin/checkout--worker.c: static void report_result(struct parallel_checkout_item *pc_item)
     - 	struct pc_item_result res;
     +@@ builtin/checkout--worker.c: static void packet_to_pc_item(const char *buffer, int len,
     + 
     + static void report_result(struct parallel_checkout_item *pc_item)
     + {
     +-	struct pc_item_result res;
     ++	struct pc_item_result res = { 0 };
       	size_t size;
       
     -+#if defined(__has_feature)
     -+#  if __has_feature(memory_sanitizer)
     -+	// MSAN workaround: res contains padding bytes, which will remain
     -+	// permanently unintialised. Later, we read all of res in order to send
     -+	// it to the parent process - and MSAN (rightly) complains that we're
     -+	// reading those unintialised padding bytes. By memset'ing res we
     -+	// guarantee that there are no uninitialised bytes.
     -+	memset(&res, 0, sizeof(res));
     -+#endif
     -+#endif
     -+
       	res.id = pc_item->id;
     - 	res.status = pc_item->status;
     - 

-- 
gitgitgadget

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v2 1/3] bulk-checkin: make buffer reuse more obvious and safer
  2021-06-14 15:51 ` [PATCH v2 " Andrzej Hunt via GitGitGadget
@ 2021-06-14 15:51   ` Andrzej Hunt via GitGitGadget
  2021-06-14 15:51   ` [PATCH v2 2/3] split-index: use oideq instead of memcmp to compare object_id's Andrzej Hunt via GitGitGadget
  2021-06-14 15:51   ` [PATCH v2 3/3] builtin/checkout--worker: zero-initialise struct to avoid MSAN complaints Andrzej Hunt via GitGitGadget
  2 siblings, 0 replies; 15+ messages in thread
From: Andrzej Hunt via GitGitGadget @ 2021-06-14 15:51 UTC (permalink / raw)
  To: git; +Cc: Chris Torek, Jeff King, Andrzej Hunt, Andrzej Hunt

From: Andrzej Hunt <ajrhunt@google.com>

ibuf can be reused for multiple iterations of the loop. Specifically:
deflate() overwrites s.avail_in to show how much of the input buffer
has not been processed yet - and sometimes leaves 'avail_in > 0', in
which case ibuf will be processed again during the loop's subsequent
iteration.

But if we declare ibuf within the loop, then (in theory) we get a new
(and uninitialised) buffer for every iteration. In practice, my compiler
seems to resue the same buffer - meaning that this code does work - but
it doesn't seem safe to rely on this behaviour. MSAN correctly catches
this issue - as soon as we hit the 's.avail_in > 0' condition, we end up
reading from what seems to be uninitialised memory.

Therefore, we move ibuf out of the loop, making this reuse safe.

See MSAN output from t1050-large below - the interesting part is the
ibuf creation at the end, although there's a lot of indirection before
we reach the read from unitialised memory:

==11294==WARNING: MemorySanitizer: use-of-uninitialized-value
    #0 0x7f75db58fb1c in crc32_little crc32.c:283:9
    #1 0x7f75db58d5b3 in crc32_z crc32.c:220:20
    #2 0x7f75db59668c in crc32 crc32.c:242:12
    #3 0x8c94f8 in hashwrite csum-file.c:101:15
    #4 0x825faf in stream_to_pack bulk-checkin.c:154:5
    #5 0x82467b in deflate_to_pack bulk-checkin.c:225:8
    #6 0x823ff1 in index_bulk_checkin bulk-checkin.c:264:15
    #7 0xa7cff2 in index_stream object-file.c:2234:9
    #8 0xa7bff7 in index_fd object-file.c:2256:9
    #9 0xa7d22d in index_path object-file.c:2274:7
    #10 0xb3c8c9 in add_to_index read-cache.c:802:7
    #11 0xb3e039 in add_file_to_index read-cache.c:835:9
    #12 0x4a99c3 in add_files add.c:458:7
    #13 0x4a7276 in cmd_add add.c:670:18
    #14 0x4a1e76 in run_builtin git.c:461:11
    #15 0x49e1e7 in handle_builtin git.c:714:3
    #16 0x4a0c08 in run_argv git.c:781:4
    #17 0x49d5a8 in cmd_main git.c:912:19
    #18 0x7974da in main common-main.c:52:11
    #19 0x7f75da66f349 in __libc_start_main (/lib64/libc.so.6+0x24349)
    #20 0x421bd9 in _start start.S:120

  Uninitialized value was stored to memory at
    #0 0x7f75db58fa6b in crc32_little crc32.c:283:9
    #1 0x7f75db58d5b3 in crc32_z crc32.c:220:20
    #2 0x7f75db59668c in crc32 crc32.c:242:12
    #3 0x8c94f8 in hashwrite csum-file.c:101:15
    #4 0x825faf in stream_to_pack bulk-checkin.c:154:5
    #5 0x82467b in deflate_to_pack bulk-checkin.c:225:8
    #6 0x823ff1 in index_bulk_checkin bulk-checkin.c:264:15
    #7 0xa7cff2 in index_stream object-file.c:2234:9
    #8 0xa7bff7 in index_fd object-file.c:2256:9
    #9 0xa7d22d in index_path object-file.c:2274:7
    #10 0xb3c8c9 in add_to_index read-cache.c:802:7
    #11 0xb3e039 in add_file_to_index read-cache.c:835:9
    #12 0x4a99c3 in add_files add.c:458:7
    #13 0x4a7276 in cmd_add add.c:670:18
    #14 0x4a1e76 in run_builtin git.c:461:11
    #15 0x49e1e7 in handle_builtin git.c:714:3
    #16 0x4a0c08 in run_argv git.c:781:4
    #17 0x49d5a8 in cmd_main git.c:912:19
    #18 0x7974da in main common-main.c:52:11
    #19 0x7f75da66f349 in __libc_start_main (/lib64/libc.so.6+0x24349)

  Uninitialized value was stored to memory at
    #0 0x447eb9 in __msan_memcpy msan_interceptors.cpp:1558:3
    #1 0x7f75db5c2011 in flush_pending deflate.c:746:5
    #2 0x7f75db5cafa0 in deflate_stored deflate.c:1815:9
    #3 0x7f75db5bb7d2 in deflate deflate.c:1005:34
    #4 0xd80b7f in git_deflate zlib.c:244:12
    #5 0x825dff in stream_to_pack bulk-checkin.c:140:12
    #6 0x82467b in deflate_to_pack bulk-checkin.c:225:8
    #7 0x823ff1 in index_bulk_checkin bulk-checkin.c:264:15
    #8 0xa7cff2 in index_stream object-file.c:2234:9
    #9 0xa7bff7 in index_fd object-file.c:2256:9
    #10 0xa7d22d in index_path object-file.c:2274:7
    #11 0xb3c8c9 in add_to_index read-cache.c:802:7
    #12 0xb3e039 in add_file_to_index read-cache.c:835:9
    #13 0x4a99c3 in add_files add.c:458:7
    #14 0x4a7276 in cmd_add add.c:670:18
    #15 0x4a1e76 in run_builtin git.c:461:11
    #16 0x49e1e7 in handle_builtin git.c:714:3
    #17 0x4a0c08 in run_argv git.c:781:4
    #18 0x49d5a8 in cmd_main git.c:912:19
    #19 0x7974da in main common-main.c:52:11

  Uninitialized value was stored to memory at
    #0 0x447eb9 in __msan_memcpy msan_interceptors.cpp:1558:3
    #1 0x7f75db644241 in _tr_stored_block trees.c:873:5
    #2 0x7f75db5cad7c in deflate_stored deflate.c:1813:9
    #3 0x7f75db5bb7d2 in deflate deflate.c:1005:34
    #4 0xd80b7f in git_deflate zlib.c:244:12
    #5 0x825dff in stream_to_pack bulk-checkin.c:140:12
    #6 0x82467b in deflate_to_pack bulk-checkin.c:225:8
    #7 0x823ff1 in index_bulk_checkin bulk-checkin.c:264:15
    #8 0xa7cff2 in index_stream object-file.c:2234:9
    #9 0xa7bff7 in index_fd object-file.c:2256:9
    #10 0xa7d22d in index_path object-file.c:2274:7
    #11 0xb3c8c9 in add_to_index read-cache.c:802:7
    #12 0xb3e039 in add_file_to_index read-cache.c:835:9
    #13 0x4a99c3 in add_files add.c:458:7
    #14 0x4a7276 in cmd_add add.c:670:18
    #15 0x4a1e76 in run_builtin git.c:461:11
    #16 0x49e1e7 in handle_builtin git.c:714:3
    #17 0x4a0c08 in run_argv git.c:781:4
    #18 0x49d5a8 in cmd_main git.c:912:19
    #19 0x7974da in main common-main.c:52:11

  Uninitialized value was stored to memory at
    #0 0x447eb9 in __msan_memcpy msan_interceptors.cpp:1558:3
    #1 0x7f75db5c8fcf in deflate_stored deflate.c:1783:9
    #2 0x7f75db5bb7d2 in deflate deflate.c:1005:34
    #3 0xd80b7f in git_deflate zlib.c:244:12
    #4 0x825dff in stream_to_pack bulk-checkin.c:140:12
    #5 0x82467b in deflate_to_pack bulk-checkin.c:225:8
    #6 0x823ff1 in index_bulk_checkin bulk-checkin.c:264:15
    #7 0xa7cff2 in index_stream object-file.c:2234:9
    #8 0xa7bff7 in index_fd object-file.c:2256:9
    #9 0xa7d22d in index_path object-file.c:2274:7
    #10 0xb3c8c9 in add_to_index read-cache.c:802:7
    #11 0xb3e039 in add_file_to_index read-cache.c:835:9
    #12 0x4a99c3 in add_files add.c:458:7
    #13 0x4a7276 in cmd_add add.c:670:18
    #14 0x4a1e76 in run_builtin git.c:461:11
    #15 0x49e1e7 in handle_builtin git.c:714:3
    #16 0x4a0c08 in run_argv git.c:781:4
    #17 0x49d5a8 in cmd_main git.c:912:19
    #18 0x7974da in main common-main.c:52:11
    #19 0x7f75da66f349 in __libc_start_main (/lib64/libc.so.6+0x24349)

  Uninitialized value was stored to memory at
    #0 0x447eb9 in __msan_memcpy msan_interceptors.cpp:1558:3
    #1 0x7f75db5ea545 in read_buf deflate.c:1181:5
    #2 0x7f75db5c97f7 in deflate_stored deflate.c:1791:9
    #3 0x7f75db5bb7d2 in deflate deflate.c:1005:34
    #4 0xd80b7f in git_deflate zlib.c:244:12
    #5 0x825dff in stream_to_pack bulk-checkin.c:140:12
    #6 0x82467b in deflate_to_pack bulk-checkin.c:225:8
    #7 0x823ff1 in index_bulk_checkin bulk-checkin.c:264:15
    #8 0xa7cff2 in index_stream object-file.c:2234:9
    #9 0xa7bff7 in index_fd object-file.c:2256:9
    #10 0xa7d22d in index_path object-file.c:2274:7
    #11 0xb3c8c9 in add_to_index read-cache.c:802:7
    #12 0xb3e039 in add_file_to_index read-cache.c:835:9
    #13 0x4a99c3 in add_files add.c:458:7
    #14 0x4a7276 in cmd_add add.c:670:18
    #15 0x4a1e76 in run_builtin git.c:461:11
    #16 0x49e1e7 in handle_builtin git.c:714:3
    #17 0x4a0c08 in run_argv git.c:781:4
    #18 0x49d5a8 in cmd_main git.c:912:19
    #19 0x7974da in main common-main.c:52:11

  Uninitialized value was created by an allocation of 'ibuf' in the stack frame of function 'stream_to_pack'
    #0 0x825710 in stream_to_pack bulk-checkin.c:101

SUMMARY: MemorySanitizer: use-of-uninitialized-value crc32.c:283:9 in crc32_little
Exiting

Signed-off-by: Andrzej Hunt <andrzej@ahunt.org>
---
 bulk-checkin.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/bulk-checkin.c b/bulk-checkin.c
index 127312acd1ed..b023d9959aae 100644
--- a/bulk-checkin.c
+++ b/bulk-checkin.c
@@ -100,6 +100,7 @@ static int stream_to_pack(struct bulk_checkin_state *state,
 			  const char *path, unsigned flags)
 {
 	git_zstream s;
+	unsigned char ibuf[16384];
 	unsigned char obuf[16384];
 	unsigned hdrlen;
 	int status = Z_OK;
@@ -113,8 +114,6 @@ static int stream_to_pack(struct bulk_checkin_state *state,
 	s.avail_out = sizeof(obuf) - hdrlen;
 
 	while (status != Z_STREAM_END) {
-		unsigned char ibuf[16384];
-
 		if (size && !s.avail_in) {
 			ssize_t rsize = size < sizeof(ibuf) ? size : sizeof(ibuf);
 			ssize_t read_result = read_in_full(fd, ibuf, rsize);
-- 
gitgitgadget


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v2 2/3] split-index: use oideq instead of memcmp to compare object_id's
  2021-06-14 15:51 ` [PATCH v2 " Andrzej Hunt via GitGitGadget
  2021-06-14 15:51   ` [PATCH v2 1/3] bulk-checkin: make buffer reuse more obvious and safer Andrzej Hunt via GitGitGadget
@ 2021-06-14 15:51   ` Andrzej Hunt via GitGitGadget
  2021-06-14 15:51   ` [PATCH v2 3/3] builtin/checkout--worker: zero-initialise struct to avoid MSAN complaints Andrzej Hunt via GitGitGadget
  2 siblings, 0 replies; 15+ messages in thread
From: Andrzej Hunt via GitGitGadget @ 2021-06-14 15:51 UTC (permalink / raw)
  To: git; +Cc: Chris Torek, Jeff King, Andrzej Hunt, Andrzej Hunt

From: Andrzej Hunt <ajrhunt@google.com>

cache_entry contains an object_id, and compare_ce_content() would
include that field when calling memcmp on a subset of the cache_entry.
Depending on which hashing algorithm is being used, only part of
object_id.hash is actually being used, therefore including it in a
memcmp() is incorrect. Instead we choose to exclude the object_id when
calling memcmp(), and call oideq() separately.

This issue was found when running t1700-split-index with MSAN, see MSAN
output below (on my machine, offset 76 corresponds to 4 bytes after the
start of object_id.hash).

Uninitialized bytes in MemcmpInterceptorCommon at offset 76 inside [0x7f60e7c00118, 92)
==27914==WARNING: MemorySanitizer: use-of-uninitialized-value
    #0 0x4524ee in memcmp /home/abuild/rpmbuild/BUILD/llvm-11.0.0.src/build/../projects/compiler-rt/lib/msan/../sanitizer_common/sanitizer_common_interceptors.inc:873:10
    #1 0xc867ae in compare_ce_content /home/ahunt/git/git/split-index.c:208:8
    #2 0xc859fb in prepare_to_write_split_index /home/ahunt/git/git/split-index.c:336:9
    #3 0xb4bbca in write_split_index /home/ahunt/git/git/read-cache.c:3107:2
    #4 0xb42b4d in write_locked_index /home/ahunt/git/git/read-cache.c:3295:8
    #5 0x638058 in try_merge_strategy /home/ahunt/git/git/builtin/merge.c:758:7
    #6 0x63057f in cmd_merge /home/ahunt/git/git/builtin/merge.c:1663:9
    #7 0x4a1e76 in run_builtin /home/ahunt/git/git/git.c:461:11
    #8 0x49e1e7 in handle_builtin /home/ahunt/git/git/git.c:714:3
    #9 0x4a0c08 in run_argv /home/ahunt/git/git/git.c:781:4
    #10 0x49d5a8 in cmd_main /home/ahunt/git/git/git.c:912:19
    #11 0x7974da in main /home/ahunt/git/git/common-main.c:52:11
    #12 0x7f60e928e349 in __libc_start_main (/lib64/libc.so.6+0x24349)
    #13 0x421bd9 in _start /home/abuild/rpmbuild/BUILD/glibc-2.26/csu/../sysdeps/x86_64/start.S:120

  Uninitialized value was stored to memory at
    #0 0x447eb9 in __msan_memcpy /home/abuild/rpmbuild/BUILD/llvm-11.0.0.src/build/../projects/compiler-rt/lib/msan/msan_interceptors.cpp:1558:3
    #1 0xb4d1e6 in dup_cache_entry /home/ahunt/git/git/read-cache.c:3457:2
    #2 0xd214fa in add_entry /home/ahunt/git/git/unpack-trees.c:215:18
    #3 0xd1fae0 in keep_entry /home/ahunt/git/git/unpack-trees.c:2276:2
    #4 0xd1ff9e in twoway_merge /home/ahunt/git/git/unpack-trees.c:2504:11
    #5 0xd27028 in call_unpack_fn /home/ahunt/git/git/unpack-trees.c:593:12
    #6 0xd2443d in unpack_nondirectories /home/ahunt/git/git/unpack-trees.c:1106:12
    #7 0xd19435 in unpack_callback /home/ahunt/git/git/unpack-trees.c:1306:6
    #8 0xd0d7ff in traverse_trees /home/ahunt/git/git/tree-walk.c:532:17
    #9 0xd1773a in unpack_trees /home/ahunt/git/git/unpack-trees.c:1683:9
    #10 0xdc6370 in checkout /home/ahunt/git/git/merge-ort.c:3590:8
    #11 0xdc51c3 in merge_switch_to_result /home/ahunt/git/git/merge-ort.c:3728:7
    #12 0xa195a9 in merge_ort_recursive /home/ahunt/git/git/merge-ort-wrappers.c:58:2
    #13 0x637fff in try_merge_strategy /home/ahunt/git/git/builtin/merge.c:751:12
    #14 0x63057f in cmd_merge /home/ahunt/git/git/builtin/merge.c:1663:9
    #15 0x4a1e76 in run_builtin /home/ahunt/git/git/git.c:461:11
    #16 0x49e1e7 in handle_builtin /home/ahunt/git/git/git.c:714:3
    #17 0x4a0c08 in run_argv /home/ahunt/git/git/git.c:781:4
    #18 0x49d5a8 in cmd_main /home/ahunt/git/git/git.c:912:19
    #19 0x7974da in main /home/ahunt/git/git/common-main.c:52:11

  Uninitialized value was created by a heap allocation
    #0 0x44e73d in malloc /home/abuild/rpmbuild/BUILD/llvm-11.0.0.src/build/../projects/compiler-rt/lib/msan/msan_interceptors.cpp:901:3
    #1 0xd592f6 in do_xmalloc /home/ahunt/git/git/wrapper.c:41:8
    #2 0xd59248 in xmalloc /home/ahunt/git/git/wrapper.c:62:9
    #3 0xa17088 in mem_pool_alloc_block /home/ahunt/git/git/mem-pool.c:22:6
    #4 0xa16f78 in mem_pool_init /home/ahunt/git/git/mem-pool.c:44:3
    #5 0xb481b8 in load_all_cache_entries /home/ahunt/git/git/read-cache.c
    #6 0xb44d40 in do_read_index /home/ahunt/git/git/read-cache.c:2298:17
    #7 0xb48a1b in read_index_from /home/ahunt/git/git/read-cache.c:2389:8
    #8 0xbd5a0b in repo_read_index /home/ahunt/git/git/repository.c:276:8
    #9 0xb4bcaf in repo_read_index_unmerged /home/ahunt/git/git/read-cache.c:3326:2
    #10 0x62ed26 in cmd_merge /home/ahunt/git/git/builtin/merge.c:1362:6
    #11 0x4a1e76 in run_builtin /home/ahunt/git/git/git.c:461:11
    #12 0x49e1e7 in handle_builtin /home/ahunt/git/git/git.c:714:3
    #13 0x4a0c08 in run_argv /home/ahunt/git/git/git.c:781:4
    #14 0x49d5a8 in cmd_main /home/ahunt/git/git/git.c:912:19
    #15 0x7974da in main /home/ahunt/git/git/common-main.c:52:11
    #16 0x7f60e928e349 in __libc_start_main (/lib64/libc.so.6+0x24349)

SUMMARY: MemorySanitizer: use-of-uninitialized-value /home/abuild/rpmbuild/BUILD/llvm-11.0.0.src/build/../projects/compiler-rt/lib/msan/../sanitizer_common/sanitizer_common_interceptors.inc:873:10 in memcmp
Exiting

Signed-off-by: Andrzej Hunt <andrzej@ahunt.org>
---
 split-index.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/split-index.c b/split-index.c
index 4d6e52d46f75..8e52e891c3bc 100644
--- a/split-index.c
+++ b/split-index.c
@@ -207,7 +207,8 @@ static int compare_ce_content(struct cache_entry *a, struct cache_entry *b)
 	b->ce_flags &= ondisk_flags;
 	ret = memcmp(&a->ce_stat_data, &b->ce_stat_data,
 		     offsetof(struct cache_entry, name) -
-		     offsetof(struct cache_entry, ce_stat_data));
+		     offsetof(struct cache_entry, oid)) ||
+		!oideq(&a->oid, &b->oid);
 	a->ce_flags = ce_flags;
 	b->ce_flags = base_flags;
 
-- 
gitgitgadget


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v2 3/3] builtin/checkout--worker: zero-initialise struct to avoid MSAN complaints
  2021-06-14 15:51 ` [PATCH v2 " Andrzej Hunt via GitGitGadget
  2021-06-14 15:51   ` [PATCH v2 1/3] bulk-checkin: make buffer reuse more obvious and safer Andrzej Hunt via GitGitGadget
  2021-06-14 15:51   ` [PATCH v2 2/3] split-index: use oideq instead of memcmp to compare object_id's Andrzej Hunt via GitGitGadget
@ 2021-06-14 15:51   ` Andrzej Hunt via GitGitGadget
  2 siblings, 0 replies; 15+ messages in thread
From: Andrzej Hunt via GitGitGadget @ 2021-06-14 15:51 UTC (permalink / raw)
  To: git; +Cc: Chris Torek, Jeff King, Andrzej Hunt, Andrzej Hunt

From: Andrzej Hunt <ajrhunt@google.com>

report_result() sends a struct to the parent process, but that struct
would contain uninitialised padding bytes. Running this code under MSAN
rightly triggers a warning - but we don't particularly care about this
warning because we control the receiving code, and we therefore know
that those padding bytes won't be read on the receiving end.

We could simply suppress this warning under MSAN with the approporiate
ifdef'd attributes, but a less intrusive solution is to 0-initialise the
struct, which guarantees that the padding will also be initialised.

Interestingly, in the error-case branch, we only try to copy the first
two members of pc_item_result, by copying only PC_ITEM_RESULT_BASE_SIZE
bytes. However PC_ITEM_RESULT_BASE_SIZE is defined as
'offsetof(the_last_member)', which means that we're copying padding bytes
after the end of the second last member. We could avoid doing this by
redefining PC_ITEM_RESULT_BASE_SIZE as
'offsetof(second_last_member) + sizeof(second_last_member)', but there's
no huge benefit to doing so (and this patch silences the MSAN warning in
this scenario either way).

MSAN output from t2080 (partially interleaved due to the
parallel work :) ):

Uninitialized bytes in __interceptor_write at offset 12 inside [0x7fff37d83408, 160)
==23279==WARNING: MemorySanitizer: use-of-uninitialized-value
Uninitialized bytes in __interceptor_write at offset 12 inside [0x7ffdb8a07ec8, 160)
==23280==WARNING: MemorySanitizer: use-of-uninitialized-value
    #0 0xd5ac28 in xwrite /home/ahunt/git/git/wrapper.c:256:8
    #1 0xd5b327 in write_in_full /home/ahunt/git/git/wrapper.c:311:21
    #2 0xb0a8c4 in do_packet_write /home/ahunt/git/git/pkt-line.c:221:6
    #3 0xb0a5fd in packet_write /home/ahunt/git/git/pkt-line.c:242:6
    #4 0x4f7441 in report_result /home/ahunt/git/git/builtin/checkout--worker.c:69:2
    #5 0x4f6be6 in worker_loop /home/ahunt/git/git/builtin/checkout--worker.c:100:3
    #6 0x4f68d3 in cmd_checkout__worker /home/ahunt/git/git/builtin/checkout--worker.c:143:2
    #7 0x4a1e76 in run_builtin /home/ahunt/git/git/git.c:461:11
    #8 0x49e1e7 in handle_builtin /home/ahunt/git/git/git.c:714:3
    #9 0x4a0c08 in run_argv /home/ahunt/git/git/git.c:781:4
    #10 0x49d5a8 in cmd_main /home/ahunt/git/git/git.c:912:19
    #11 0x7974da in main /home/ahunt/git/git/common-main.c:52:11
    #12 0x7f8778114349 in __libc_start_main (/lib64/libc.so.6+0x24349)
    #13 0x421bd9 in _start /home/abuild/rpmbuild/BUILD/glibc-2.26/csu/../sysdeps/x86_64/start.S:120

  Uninitialized value was created by an allocation of 'res' in the stack frame of function 'report_result'
    #0 0x4f72c0 in report_result /home/ahunt/git/git/builtin/checkout--worker.c:55

SUMMARY: MemorySanitizer: use-of-uninitialized-value /home/ahunt/git/git/wrapper.c:256:8 in xwrite
Exiting
    #0 0xd5ac28 in xwrite /home/ahunt/git/git/wrapper.c:256:8
    #1 0xd5b327 in write_in_full /home/ahunt/git/git/wrapper.c:311:21
    #2 0xb0a8c4 in do_packet_write /home/ahunt/git/git/pkt-line.c:221:6
    #3 0xb0a5fd in packet_write /home/ahunt/git/git/pkt-line.c:242:6
    #4 0x4f7441 in report_result /home/ahunt/git/git/builtin/checkout--worker.c:69:2
    #5 0x4f6be6 in worker_loop /home/ahunt/git/git/builtin/checkout--worker.c:100:3
    #6 0x4f68d3 in cmd_checkout__worker /home/ahunt/git/git/builtin/checkout--worker.c:143:2
    #7 0x4a1e76 in run_builtin /home/ahunt/git/git/git.c:461:11
    #8 0x49e1e7 in handle_builtin /home/ahunt/git/git/git.c:714:3
    #9 0x4a0c08 in run_argv /home/ahunt/git/git/git.c:781:4
    #10 0x49d5a8 in cmd_main /home/ahunt/git/git/git.c:912:19
    #11 0x7974da in main /home/ahunt/git/git/common-main.c:52:11
    #12 0x7f2749a0e349 in __libc_start_main (/lib64/libc.so.6+0x24349)
    #13 0x421bd9 in _start /home/abuild/rpmbuild/BUILD/glibc-2.26/csu/../sysdeps/x86_64/start.S:120

  Uninitialized value was created by an allocation of 'res' in the stack frame of function 'report_result'
    #0 0x4f72c0 in report_result /home/ahunt/git/git/builtin/checkout--worker.c:55

SUMMARY: MemorySanitizer: use-of-uninitialized-value /home/ahunt/git/git/wrapper.c:256:8 in xwrite

Signed-off-by: Andrzej Hunt <andrzej@ahunt.org>
---
 builtin/checkout--worker.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/checkout--worker.c b/builtin/checkout--worker.c
index 289a9b8f89d0..fb9fd13b73c4 100644
--- a/builtin/checkout--worker.c
+++ b/builtin/checkout--worker.c
@@ -53,7 +53,7 @@ static void packet_to_pc_item(const char *buffer, int len,
 
 static void report_result(struct parallel_checkout_item *pc_item)
 {
-	struct pc_item_result res;
+	struct pc_item_result res = { 0 };
 	size_t size;
 
 	res.id = pc_item->id;
-- 
gitgitgadget

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 0/3] Fix uninitialised reads found with MSAN
  2021-06-10 16:48 [PATCH 0/3] Fix uninitialised reads found with MSAN Andrzej Hunt via GitGitGadget
                   ` (4 preceding siblings ...)
  2021-06-14 15:51 ` [PATCH v2 " Andrzej Hunt via GitGitGadget
@ 2021-06-17  9:28 ` Philip Oakley
  2021-06-20 15:19   ` Andrzej Hunt
  5 siblings, 1 reply; 15+ messages in thread
From: Philip Oakley @ 2021-06-17  9:28 UTC (permalink / raw)
  To: Andrzej Hunt via GitGitGadget, git; +Cc: Andrzej Hunt

On 10/06/2021 17:48, Andrzej Hunt via GitGitGadget wrote:
> This can be worked around with an annotation in zlib (which I'm trying to
> submit upstream at [1]) 
> Andrzej
>
> [1] https://github.com/madler/zlib/pull/561
Andrzey,

Just had a look at the zlib PR and it has CI check failure asking for
extra info.

Philip

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 0/3] Fix uninitialised reads found with MSAN
  2021-06-17  9:28 ` [PATCH 0/3] Fix uninitialised reads found with MSAN Philip Oakley
@ 2021-06-20 15:19   ` Andrzej Hunt
  0 siblings, 0 replies; 15+ messages in thread
From: Andrzej Hunt @ 2021-06-20 15:19 UTC (permalink / raw)
  To: Philip Oakley, Andrzej Hunt via GitGitGadget, git



On 17/06/2021 11:28, Philip Oakley wrote:
> On 10/06/2021 17:48, Andrzej Hunt via GitGitGadget wrote:
>> This can be worked around with an annotation in zlib (which I'm trying to
>> submit upstream at [1])
>> Andrzej
>>
>> [1] https://github.com/madler/zlib/pull/561
> Andrzey,
> 
> Just had a look at the zlib PR and it has CI check failure asking for
> extra info.

Thanks for spotting this. This seems to be a configuration issue with 
zlib's CI itself, which I noticed is also affecting other PR's on the 
same repo: /. I'll wait for feedback from the zlib maintainer for now!

ATB,


Andrzej

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2021-06-20 15:20 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-10 16:48 [PATCH 0/3] Fix uninitialised reads found with MSAN Andrzej Hunt via GitGitGadget
2021-06-10 16:48 ` [PATCH 1/3] bulk-checkin: make buffer reuse more obvious and safer Andrzej Hunt via GitGitGadget
2021-06-10 16:48 ` [PATCH 2/3] split-index: use oideq instead of memcmp to compare object_id's Andrzej Hunt via GitGitGadget
2021-06-10 16:48 ` [PATCH 3/3] builtin/checkout--worker: memset struct to avoid MSAN complaints Andrzej Hunt via GitGitGadget
2021-06-11  4:43   ` Chris Torek
2021-06-11  6:28     ` Junio C Hamano
2021-06-11 15:37       ` Andrzej Hunt
2021-06-14  1:04         ` Junio C Hamano
2021-06-11 17:11 ` [PATCH 0/3] Fix uninitialised reads found with MSAN Jeff King
2021-06-14 15:51 ` [PATCH v2 " Andrzej Hunt via GitGitGadget
2021-06-14 15:51   ` [PATCH v2 1/3] bulk-checkin: make buffer reuse more obvious and safer Andrzej Hunt via GitGitGadget
2021-06-14 15:51   ` [PATCH v2 2/3] split-index: use oideq instead of memcmp to compare object_id's Andrzej Hunt via GitGitGadget
2021-06-14 15:51   ` [PATCH v2 3/3] builtin/checkout--worker: zero-initialise struct to avoid MSAN complaints Andrzej Hunt via GitGitGadget
2021-06-17  9:28 ` [PATCH 0/3] Fix uninitialised reads found with MSAN Philip Oakley
2021-06-20 15:19   ` Andrzej Hunt

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://7fh6tueqddpjyxjmgtdiueylzoqt6pt7hec3pukyptlmohoowvhde4yd.onion/inbox.comp.version-control.git
	nntp://ie5yzdi7fg72h7s4sdcztq5evakq23rdt33mfyfcddc5u3ndnw24ogqd.onion/inbox.comp.version-control.git
	nntp://4uok3hntl7oi7b4uf4rtfwefqeexfzil2w6kgk2jn5z2f764irre7byd.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git