{"thread":{"id":"61606","subject":"MSan failures in pack-bitmap","startedAt":"2024-06-08T02:44:14Z","lastAt":"2024-06-11T09:12:49Z","messageCount":10,"participants":["Kyle Lippincott","Jeff King","Taylor Blau","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"496689","messageId":"CAO_smVjSy6j7jpSxHNsxzJfxnHwjLphsqu5jwd8TBhMLp72XPw@mail.gmail.com","threadId":"61606","inReplyTo":null,"subject":"MSan failures in pack-bitmap","fromName":"Kyle Lippincott","fromEmail":"spectral@google.com","sentAt":"2024-06-08T02:43:56Z","receivedAt":"2024-06-08T02:44:14Z","isPatch":false,"sender":{"key":"spectral@google.com","avatar":"https://avatars.githubusercontent.com/u/6371650?v=4"},"body":"While running t/t5326-multi-pack-bitmaps:\n\ntest 24:\nexpecting success of 5326.24 'clone from bitmapped repository':\nrm -fr clone.git &&\ngit clone --no-local --bare . clone.git &&\ngit rev-parse HEAD >expect &&\ngit --git-dir=clone.git rev-parse HEAD >actual &&\ntest_cmp expect actual\n\nCloning into bare repository 'clone.git'...\nremote: ==7474==WARNING: MemorySanitizer: use-of-uninitialized-value\nremote: #0 0x55c5cd191dde in try_partial_reuse pack-bitmap.c:1887:8\nremote: #1 0x55c5cd191dde in reuse_partial_packfile_from_bitmap_1\npack-bitmap.c:2001:8\nremote: #2 0x55c5cd191dde in reuse_partial_packfile_from_bitmap\npack-bitmap.c:2105:3\nremote: #3 0x55c5cce0bd0e in get_object_list_from_bitmap\nbuiltin/pack-objects.c:4043:3\nremote: #4 0x55c5cce0bd0e in get_object_list builtin/pack-objects.c:4156:27\nremote: #5 0x55c5cce0bd0e in cmd_pack_objects builtin/pack-objects.c:4596:3\nremote: #6 0x55c5ccc8fac8 in run_builtin git.c:474:11\nremote: #7 0x55c5ccc8d660 in handle_builtin git.c:729:3\nremote: #8 0x55c5ccc8b634 in run_argv git.c:793:4\nremote: #9 0x55c5ccc8b634 in cmd_main git.c:928:19\nremote: #10 0x55c5ccf10a5b in main common-main.c:62:11\nremote: #11 0x7f9ef142e3d3 in __libc_start_main\nremote: #12 0x55c5ccbf28e9 in _start\nremote:\nremote: Uninitialized value was stored to memory at\nremote: #0 0x55c5cd191dd7 in try_partial_reuse pack-bitmap.c:1888:15\nremote: #1 0x55c5cd191dd7 in reuse_partial_packfile_from_bitmap_1\npack-bitmap.c:2001:8\nremote: #2 0x55c5cd191dd7 in reuse_partial_packfile_from_bitmap\npack-bitmap.c:2105:3\nremote: #3 0x55c5cce0bd0e in get_object_list_from_bitmap\nbuiltin/pack-objects.c:4043:3\nremote: #4 0x55c5cce0bd0e in get_object_list builtin/pack-objects.c:4156:27\nremote: #5 0x55c5cce0bd0e in cmd_pack_objects builtin/pack-objects.c:4596:3\nremote: #6 0x55c5ccc8fac8 in run_builtin git.c:474:11\nremote: #7 0x55c5ccc8d660 in handle_builtin git.c:729:3\nremote: #8 0x55c5ccc8b634 in run_argv git.c:793:4\nremote: #9 0x55c5ccc8b634 in cmd_main git.c:928:19\nremote: #10 0x55c5ccf10a5b in main common-main.c:62:11\nremote: #11 0x7f9ef142e3d3 in __libc_start_main\nremote: #12 0x55c5ccbf28e9 in _start\nremote:\nremote: Uninitialized value was created by a heap allocation\nremote: #0 0x55c5ccc052ca in realloc msan/msan_interceptors.cpp:1009:3\nremote: #1 0x55c5cd3d04d6 in xrealloc wrapper.c:137:8\nremote: #2 0x55c5cd190d95 in reuse_partial_packfile_from_bitmap\npack-bitmap.c:2091:3\nremote: #3 0x55c5cce0bd0e in get_object_list_from_bitmap\nbuiltin/pack-objects.c:4043:3\nremote: #4 0x55c5cce0bd0e in get_object_list builtin/pack-objects.c:4156:27\nremote: #5 0x55c5cce0bd0e in cmd_pack_objects builtin/pack-objects.c:4596:3\nremote: #6 0x55c5ccc8fac8 in run_builtin git.c:474:11\nremote: #7 0x55c5ccc8d660 in handle_builtin git.c:729:3\nremote: #8 0x55c5ccc8b634 in run_argv git.c:793:4\nremote: #9 0x55c5ccc8b634 in cmd_main git.c:928:19\nremote: #10 0x55c5ccf10a5b in main common-main.c:62:11\nremote: #11 0x7f9ef142e3d3 in __libc_start_main\nremote: #12 0x55c5ccbf28e9 in _start\nremote:\nremote: SUMMARY: MemorySanitizer: use-of-uninitialized-value\npack-bitmap.c:1887:8 in try_partial_reuse\nremote: Exiting\nerror: git upload-pack: git-pack-objects died with error.\nfatal: git upload-pack: aborting due to possible repository corruption\non the remote side.\nremote: aborting due to possible repository corruption on the remote side.\nfatal: early EOF\nfatal: fetch-pack: invalid index-pack output\nnot ok 24 - clone from bitmapped repository\n#\n# rm -fr clone.git &&\n# git clone --no-local --bare . clone.git &&\n# git rev-parse HEAD >expect &&\n# git --git-dir=clone.git rev-parse HEAD >actual &&\n# test_cmp expect actual\n#\n\nThere are similar failures in test 25, 198, 199, 319, and 320.\n---\n\nI believe what's happening is that pack-bitmap.c:2091 grows the packs\nlist and sets up some of the fields, but doesn't set pack_int_id. We\nthen use it at pack-bitmap.c:1888.\n\nI investigated, but couldn't prove to myself what value should be\nplaced there while growing it, or if it's incorrect to read from it in\nthis case (so we shouldn't be in pack-bitmap.c:1888 with this pack).\n\nReproducing is potentially non-trivial. This may work:\n\nmake -j CFLAGS=\"-fsanitize=memory -fsanitize-memory-track-origins\n-fno-omit-frame-pointer -g -O2\" CC=clang && \\\nmake -C t t5326-multi-pack-bitmaps.sh GIT_TEST_OPTS=\"--verbose --debug\"\n"},{"id":"496691","messageId":"20240608081855.GA2390433@coredump.intra.peff.net","threadId":"61606","inReplyTo":"CAO_smVjSy6j7jpSxHNsxzJfxnHwjLphsqu5jwd8TBhMLp72XPw@mail.gmail.com","subject":"Re: MSan failures in pack-bitmap","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-06-08T08:18:55Z","receivedAt":"2024-06-08T08:19:05Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 07, 2024 at 07:43:56PM -0700, Kyle Lippincott wrote:\n\n> I believe what's happening is that pack-bitmap.c:2091 grows the packs\n> list and sets up some of the fields, but doesn't set pack_int_id. We\n> then use it at pack-bitmap.c:1888.\n> \n> I investigated, but couldn't prove to myself what value should be\n> placed there while growing it, or if it's incorrect to read from it in\n> this case (so we shouldn't be in pack-bitmap.c:1888 with this pack).\n\nHmm, I'm not sure.\n\nIn reuse_partial_packfile_from_bitmap(), the code path that creates the\nstruct only kicks in when the \"multi_pack_reuse\" flag isn't set. Which\ngenerally would correspond to whether we have a midx. And then the code\nin try_partial_reuse() that uses the struct similarly checks\nbitmap_is_midx() before looking at the pack_int_id field.\n\nBut that changed in 795006fff4 (pack-bitmap: gracefully handle missing\nBTMP chunks, 2024-04-15), where we also disable multi_pack_reuse if we\nhave a midx but it has no BTMP chunk. So we end up in the non-multi code\npath to create the struct, but then try_partial_reuse() still realizes\nwe have a midx and uses that code path.\n\nI guess this gets into the \"we have a midx, but are only doing reuse out\nof a single pack\" case. Which I think is supported, but I'm not familiar\nenough with the code to know where the assumption is going wrong.\n\nAnyway...\n\n> Reproducing is potentially non-trivial. This may work:\n> \n> make -j CFLAGS=\"-fsanitize=memory -fsanitize-memory-track-origins\n> -fno-omit-frame-pointer -g -O2\" CC=clang && \\\n> make -C t t5326-multi-pack-bitmaps.sh GIT_TEST_OPTS=\"--verbose --debug\"\n\nYeah, I forgot what a pain MSan is:\n\n  - I needed to use a recent version of clang (the default for Debian\n    unstable is clang-16, which is missing intercepts for newer versions\n    of glibc strtoimax(), and complains that it does not set the\n    outgoing \"end\" pointer). Using clang-19 fixed that.\n\n  - Since I didn't recompile all of the dependent libraries with MSan\n    support, I hit the usual zlib problems discussed long ago in:\n\n     https://lore.kernel.org/git/20171004101932.pai6wzcv2eohsicr@sigill.intra.peff.net/\n\n    and had to resurrect the patch there.\n\n  - While bisecting (which is how I found 795006fff4), some versions\n    complain that attr.c does:\n\n      buf = read_blob_data_from_index(istate, path, &size);\n      stack = read_attr_from_buf(buf, size, path, flags);\n\n    If reading failed, then \"buf\" is NULL and size is uninitialized.\n    Inside read_attr_from_buf() we bail immediately when we see the NULL\n    buf, but MSan complains that we passed the uninitialized \"size\" at\n    all. Newer versions (starting with c793f9cb08) move the NULL check\n    into the caller.\n\nAn easier (but slower) reproduction is to just use valgrind. After doing\na normal build, running:\n\n  ./t5326-multi-pack-bitmaps.sh --run=1-24 --valgrind-only=24 -i\n\nis enough to see the problem.\n\n-Peff\n"},{"id":"496738","messageId":"ZmXKzJ9CpbYTmTwC@nand.local","threadId":"61606","inReplyTo":"20240608081855.GA2390433@coredump.intra.peff.net","subject":"Re: MSan failures in pack-bitmap","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-06-09T15:31:24Z","receivedAt":"2024-06-09T15:31:27Z","isPatch":false,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Sat, Jun 08, 2024 at 04:18:55AM -0400, Jeff King wrote:\n> On Fri, Jun 07, 2024 at 07:43:56PM -0700, Kyle Lippincott wrote:\n>\n> > I believe what's happening is that pack-bitmap.c:2091 grows the packs\n> > list and sets up some of the fields, but doesn't set pack_int_id. We\n> > then use it at pack-bitmap.c:1888.\n> >\n> > I investigated, but couldn't prove to myself what value should be\n> > placed there while growing it, or if it's incorrect to read from it in\n> > this case (so we shouldn't be in pack-bitmap.c:1888 with this pack).\n>\n> Hmm, I'm not sure.\n>\n> In reuse_partial_packfile_from_bitmap(), the code path that creates the\n> struct only kicks in when the \"multi_pack_reuse\" flag isn't set. Which\n> generally would correspond to whether we have a midx. And then the code\n> in try_partial_reuse() that uses the struct similarly checks\n> bitmap_is_midx() before looking at the pack_int_id field.\n>\n> But that changed in 795006fff4 (pack-bitmap: gracefully handle missing\n> BTMP chunks, 2024-04-15), where we also disable multi_pack_reuse if we\n> have a midx but it has no BTMP chunk. So we end up in the non-multi code\n> path to create the struct, but then try_partial_reuse() still realizes\n> we have a midx and uses that code path.\n>\n> I guess this gets into the \"we have a midx, but are only doing reuse out\n> of a single pack\" case. Which I think is supported, but I'm not familiar\n> enough with the code to know where the assumption is going wrong.\n\nThat's right. We support single-pack reuse even with a MIDX either (a)\nbecause it was configured that way with\npack.allowPackReuse=(true|single), or (b) because the MIDX has no BTMP\nchunk, which is what prompted the change in 795006fff4.\n\nWhen in that case, our reuse packfile is either the pack attached to a\nsingle-pack bitmap, or the MIDX's preferred pack. When using the MIDX's\npreferred pack, we need to make sure that we correctly assign the\npack_int_id to be the ID of the preferred pack. (We use this field to\nreject cross-pack deltas later on in try_partial_reuse(), which is where\nthe MSan failure is happening).\n\nThe fix should be to set pack_int_id to the preferred pack's ID in the\nMIDX case, and an arbitrary value in the single-pack bitmap case. I\nposted a patch which should fix that here:\n\n    https://lore.kernel.org/git/4aceb9233ed24fb1e1a324a77b665eea2cf22b39.1717946847.git.me@ttaylorr.com/T/#u\n\nUnfortunately, the regression happened in 795006fff4, so this is in a\nreleased version of Git. But this is all behind a configuration option,\nso affected users can reasonably work around this issue.\n\nThanks,\nTaylor\n"},{"id":"496740","messageId":"xmqqmsnuaqus.fsf@gitster.g","threadId":"61606","inReplyTo":"ZmXKzJ9CpbYTmTwC@nand.local","subject":"Re: MSan failures in pack-bitmap","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-09T18:55:07Z","receivedAt":"2024-06-09T18:55:18Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau <me@ttaylorr.com> writes:\n\n> Unfortunately, the regression happened in 795006fff4, so this is in a\n> released version of Git. But this is all behind a configuration option,\n> so affected users can reasonably work around this issue.\n\nCan you elaborate a bit about \"reasonably work around\"?\n\nI am imagining that you are saying that the user can say \"do not use\nbitmap, because it may be corrupted due to an earlier regression\" in\nsome configuration, so that a corrupt bitmap data is not used to\nfurther spread the damage, but how does a user find out that on-disc\nmidx is broken and a workaround needs to be employed in the first\nplace?  After working around the issue by avoiding to use the\ncorrupt on-disc data while waiting for a fixed version of Git to\ncome, what does the user need to do to start using the now-corrected\nfeature again?  Does the fixed code notice there is an existing\nbreakage caused by the regression and remove the broken file\nautomatically?\n\nThanks.\n"},{"id":"496741","messageId":"ZmYJ+d3+j1E08Ms/@nand.local","threadId":"61606","inReplyTo":"xmqqmsnuaqus.fsf@gitster.g","subject":"Re: MSan failures in pack-bitmap","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-06-09T20:00:57Z","receivedAt":"2024-06-09T20:01:04Z","isPatch":false,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Sun, Jun 09, 2024 at 11:55:07AM -0700, Junio C Hamano wrote:\n> Taylor Blau <me@ttaylorr.com> writes:\n>\n> > Unfortunately, the regression happened in 795006fff4, so this is in a\n> > released version of Git. But this is all behind a configuration option,\n> > so affected users can reasonably work around this issue.\n>\n> Can you elaborate a bit about \"reasonably work around\"?\n>\n> I am imagining that you are saying that the user can say \"do not use\n> bitmap, because it may be corrupted due to an earlier regression\" in\n> some configuration, so that a corrupt bitmap data is not used to\n> further spread the damage, but how does a user find out that on-disc\n> midx is broken and a workaround needs to be employed in the first\n> place?\n\nI don't think the issue here is a corrupt on-disk bitmap or MIDX. The\nregression that Kyle reported was a logic issue reading a\nordinary/non-corrupt MIDX file, not a result of some on-disk corruption.\n\nI think it is possible to generate a corrupt pack as a result of this\nbug which would incorrectly \"reuse\" parts of an existing MIDX's\npreferred pack, but the result would be far worse than a corrupt MIDX\n(which, again, I don't think is what's happening here).\n\nBut I do not think that repository corruption is a likely outcome here,\nsince we only do pack reuse when packing to stdout (i.e. by passing the\n`--stdout` option to `pack-objects`). So repacks (which would otherwise\nbe the thing I'm most worried about) are safe: repacks trigger\npack-objects to write a tempfile directly, and so `pack_to_stdout` is\nzero'd, and we do not allow verbatim pack reuse to take place.\n\nSo a doomsday scenario whereby we'd write a corrupt pack based on this\nbug and then delete a non-corrupt pack is not directly possible via\n'repack' alone. The lesser-used 'git multi-pack-index repack' is also\nnot affected, since it similarly does not use `pack-objects --stdout`.\n\nA more likely outcome would be if a Git client who was using MIDX\nbitmaps, but only doing single-pack reuse tried to generate a pack to\npush to some remote (or vice-versa, replacing client and server/remote)\nwhich ended up being corrupt in some fashion. I haven't had enough of a\nchance to determine if this possible, though I suspect it is.\n\nThanks,\nTaylor\n"},{"id":"496742","messageId":"xmqqh6e1c1bf.fsf@gitster.g","threadId":"61606","inReplyTo":"ZmYJ+d3+j1E08Ms/@nand.local","subject":"Re: MSan failures in pack-bitmap","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-09T20:23:48Z","receivedAt":"2024-06-09T20:23:51Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau <me@ttaylorr.com> writes:\n\n> I don't think the issue here is a corrupt on-disk bitmap or MIDX. The\n> regression that Kyle reported was a logic issue reading a\n> ordinary/non-corrupt MIDX file, not a result of some on-disk corruption.\n> ...\n> But I do not think that repository corruption is a likely outcome here,\n\nYes, exactly.  What I was asking was how on-disk corruption that\nwould result from the logic issue of reading a correct MIDX file.\n\n> A more likely outcome would be if a Git client who was using MIDX\n> bitmaps, but only doing single-pack reuse tried to generate a pack\n> to push to some remote (or vice-versa, replacing client and\n> server/remote) which ended up being corrupt in some fashion. I\n> haven't had enough of a chance to determine if this possible,\n> though I suspect it is.\n\nSo, ...\n\n>> > Unfortunately, the regression happened in 795006fff4, so this is in a\n>> > released version of Git. But this is all behind a configuration option,\n>> > so affected users can reasonably work around this issue.\n>>\n>> Can you elaborate a bit about \"reasonably work around\"?\n\n... there is no reasonable work around here, is there?  We do not\neven know exactly what kind of breakage this will cause (which is\nexpected, as we would be \"reading\" a garbage value and basing future\nactions on it)?\n"},{"id":"496743","messageId":"ZmYPjuGnh+UvT5dt@nand.local","threadId":"61606","inReplyTo":"ZmYJ+d3+j1E08Ms/@nand.local","subject":"Re: MSan failures in pack-bitmap","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-06-09T20:24:46Z","receivedAt":"2024-06-09T20:24:53Z","isPatch":false,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Sun, Jun 09, 2024 at 04:00:57PM -0400, Taylor Blau wrote:\n> A more likely outcome would be if a Git client who was using MIDX\n> bitmaps, but only doing single-pack reuse tried to generate a pack to\n> push to some remote (or vice-versa, replacing client and server/remote)\n> which ended up being corrupt in some fashion. I haven't had enough of a\n> chance to determine if this possible, though I suspect it is.\n\nOK. I looked a little more, and I think that though this is possible in\ntheory, it is exceedingly rare to hit in practice.\n\nThe thing that we care about reading pack_int_id is for rejecting\ncross-pack deltas in try_partial_reuse(). A cross-pack delta occurs, for\nsome delta/base pair when either half of the pair was selected from\ndifferent packs in the MIDX. When this is the case, we can't verbatim\nreuse the delta object, since the base object may be selected from a\npack that was reused later in the MIDX, thus we'd have to convert to a\nREF_DELTA on the fly.\n\n(This is a bit of an aside, but it is technically possible to allow\ncross-pack deltas if you are careful, I just didn't implement it thus\nfar, though I would like to do so in the future).\n\nSo when we try and do a partial reuse (i.e. on a single object instead\nof saying, \"these N words in the bitmap of objects that we want to pack\nare all ones, so we can blindly reuse a chunk of objects from the\nbeginning of this pack) we check if the object we're reusing is either\nan OFS_DELTA or a REF_DELTA. If it is, *and* we're reusing it from a\nMIDX, we need to check that its base object was selected in the same\npack.\n\nThat's what we're doing when we call:\n\n    midx_pair_to_pack_pos(bitmap_git->midx, pack->pack_int_id,\n                          base_offset, &base_bitmap_pos);\n\nIf we can't find the base object in the same pack, we toss out the\ncross-pack delta and kick it back to the regular pack generation logic.\n\nSo in order to hit this in a way that would produce a broken pack, you'd\nhave to have the uninitialized memory at pack->pack_int_id be set to the\nsame value as some MIDX'd pack's pack_int_id, *and* have that pack\nhappen to contain the same object as the base of some delta that you\nwould otherwise want to throw out.\n\nIf that were all to happen, then you would likely end up with a broken\npack. But I think in the vast majority of cases try_partial_reuse()\nwould read garbage out of pack->pack_int_id, and kick it back to the\nregular pack generation logic, and produce a correct pack (albeit doing\nso using a slightly slower path).\n\nBut this is all much too close for comfort for me. I think that this\npoints to a gap in our test coverage, and that we should ensure that\nthis case is covered.\n\n(Note, I'm not sure exactly how to do this, since such a test would\ndepend on an uninitialized read. I'll think about this a bit more and\nsee if it is possible to write such a test.)\n\nBut in summary, though I think it is possible for either a client to\nsend a broken push to a server (if the client were using MIDX bitmaps\nand only doing single-pack reuse) or for a server to send a broken pack\nto a client (if the server was also using MIDX bitmaps in the same\nfashion), I think that it is exceedingly rare to hit in practice. Which\nis not the same as saying that it is impossible, of course, but I am\nconfident with the fix I posted in:\n\n    https://lore.kernel.org/git/4aceb9233ed24fb1e1a324a77b665eea2cf22b39.1717946847.git.me@ttaylorr.com/T/#u\n\nto fix the issue. Note also that I don't think a repository would be\nable to actually corrupt itself using this bug, since we don't bother\ntaking this code path at all during repacks.\n\nSo in short, I think the fix I posted above should be tracked down to\n'maint' at least for the 2.45.x series. It will avoid the MSan failures\nand more importantly the issue I described above. I would also like to\nfind a way to further test this case so that we aren't bit by such a bug\nin the future.\n\nThanks,\nTaylor\n"},{"id":"496744","messageId":"ZmYQ4f+qmQIxf+5g@nand.local","threadId":"61606","inReplyTo":"xmqqh6e1c1bf.fsf@gitster.g","subject":"Re: MSan failures in pack-bitmap","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-06-09T20:30:25Z","receivedAt":"2024-06-09T20:30:28Z","isPatch":false,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Sun, Jun 09, 2024 at 01:23:48PM -0700, Junio C Hamano wrote:\n> Taylor Blau <me@ttaylorr.com> writes:\n>\n> > I don't think the issue here is a corrupt on-disk bitmap or MIDX. The\n> > regression that Kyle reported was a logic issue reading a\n> > ordinary/non-corrupt MIDX file, not a result of some on-disk corruption.\n> > ...\n> > But I do not think that repository corruption is a likely outcome here,\n>\n> Yes, exactly.  What I was asking was how on-disk corruption that\n> would result from the logic issue of reading a correct MIDX file.\n\nOK, I think I misunderstood your original email.\n\n> ... there is no reasonable work around here, is there?  We do not\n> even know exactly what kind of breakage this will cause (which is\n> expected, as we would be \"reading\" a garbage value and basing future\n> actions on it)?\n\nI think the end-result would be that packs sent between client and\nserver during fetches or pushes would be broken, resulting in index-pack\nfailing to index them, thus causing one side or the other to (correctly)\nthrow away the received pack.\n\nThe only thing I can think of to avoid this would be to enable\nmulti-pack reuse, which is only possible if your MIDX has a BTMP chunk.\nThe other work around would be to disable pack-reuse altogether, which\nis very unfortunate.\n\nThanks,\nTaylor\n"},{"id":"496840","messageId":"20240611080220.GG3248245@coredump.intra.peff.net","threadId":"61606","inReplyTo":"ZmYPjuGnh+UvT5dt@nand.local","subject":"Re: MSan failures in pack-bitmap","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-06-11T08:02:20Z","receivedAt":"2024-06-11T08:02:21Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jun 09, 2024 at 04:24:46PM -0400, Taylor Blau wrote:\n\n> But in summary, though I think it is possible for either a client to\n> send a broken push to a server (if the client were using MIDX bitmaps\n> and only doing single-pack reuse) or for a server to send a broken pack\n> to a client (if the server was also using MIDX bitmaps in the same\n> fashion), I think that it is exceedingly rare to hit in practice. Which\n> is not the same as saying that it is impossible, of course, but I am\n> confident with the fix I posted in:\n> \n>     https://lore.kernel.org/git/4aceb9233ed24fb1e1a324a77b665eea2cf22b39.1717946847.git.me@ttaylorr.com/T/#u\n> \n> to fix the issue. Note also that I don't think a repository would be\n> able to actually corrupt itself using this bug, since we don't bother\n> taking this code path at all during repacks.\n\nRight, I think this is mostly just a vanilla bug. It could cause bogus\npacks on fetch, but the clients would realize it. If people aren't\nscreaming about broken fetches, then it is not happening very often (and\nyour analysis nicely explains why). But I don't see how it could ever\ncause corruption.\n\nDirecting the fix to \"maint\" as you suggest is sensible, but I don't\nthink it merits any special treatment.\n\n> So in short, I think the fix I posted above should be tracked down to\n> 'maint' at least for the 2.45.x series. It will avoid the MSan failures\n> and more importantly the issue I described above. I would also like to\n> find a way to further test this case so that we aren't bit by such a bug\n> in the future.\n\nI don't think we can test the case where the bug would produce a bogus\npack, since that implies guessing the uninitialized data. I guess we\ncould come up with a case where try_partial_reuse() should say \"this is\nOK to reuse\", but the bogus pack_int_id prevents it. Which implies\nlooking at the resulting pack and checking that some delta is there that\nwe expect?\n\n-Peff\n"},{"id":"496859","messageId":"20240611091248.GA3264564@coredump.intra.peff.net","threadId":"61606","inReplyTo":"20240611080220.GG3248245@coredump.intra.peff.net","subject":"Re: MSan failures in pack-bitmap","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-06-11T09:12:48Z","receivedAt":"2024-06-11T09:12:49Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 11, 2024 at 04:02:20AM -0400, Jeff King wrote:\n\n> > So in short, I think the fix I posted above should be tracked down to\n> > 'maint' at least for the 2.45.x series. It will avoid the MSan failures\n> > and more importantly the issue I described above. I would also like to\n> > find a way to further test this case so that we aren't bit by such a bug\n> > in the future.\n> \n> I don't think we can test the case where the bug would produce a bogus\n> pack, since that implies guessing the uninitialized data. I guess we\n> could come up with a case where try_partial_reuse() should say \"this is\n> OK to reuse\", but the bogus pack_int_id prevents it. Which implies\n> looking at the resulting pack and checking that some delta is there that\n> we expect?\n\nAh, never mind. I see you worked up a test in the v2 patches you sent.\n\n-Peff\n"}]}