Re: [PATCH] pack-bitmap: add loading corrupt bitmap_index test

[Jeff King <peff@peff.net>]

On Mon, May 19, 2025 at 02:44:22PM +0800, lidongyan wrote:

> 2025年5月19日 14:02，Patrick Steinhardt <ps@pks.im> 写道：
> > Okay. We _can_ do that now, but the patch doesn't explain why we
> > _should_.
> 
> The main purpose of this patch is to provide a test case to check whether
>  a memory leak occurs when loading a corrupt index file as requested here
> https://lore.kernel.org/git/20250514180325.GB2196784@coredump.intra.peff.net.
> A potential memory leak is mentioned in this patch here https://lore.kernel.org/git/pull.1962.git.git.1747052530271.gitgitgadget@gmail.com/.

I think we'd need to mark it with the !SANITIZE_LEAK prereq until the
leaks are actually fixed. Or simpler, just apply this on top of the leak
fixes once they are ready. That ordering needs to be communicated to the
maintainer, and the simplest way to do that is probably to just post a
3-patch series: your initial leak fix, a polished version of the one
from Taylor, and then the test on top.

> > My proposal would be to either move the logic into "test-bitmap.c", or
> > to even better to write a unit test in "t/unit-tests/". After all, we
> > expect that the code should fail gracefully, so a unit test might be a
> > good fit after all.
> 
> I found that the header size of an index file depends only on the type of hash algorithm.
> To trigger the condition for the memory leak, I need to corrupt a few bytes right after the
> index file header size. It's more convenient to implement this functionality in pack-bitmap.c.
> However, I think I can place the test itself under t/unit-tests/.

I don't think you can do a prereq for a unit-test file (though I might
be wrong, as I have not really paid attention to that area).

If the corruption offsets are easy-ish to compute statically (and it
sounds like they mostly just depend on the hash algorithm size), then
I'd actually prefer to just do it with "dd". That avoids extra C code,
and simulates a real on-disk corruption more exactly (using real
commands).

Something like:

diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh
index 042f62f16e..16bd607654 100755
--- a/t/t5310-pack-bitmaps.sh
+++ b/t/t5310-pack-bitmaps.sh
@@ -498,7 +498,17 @@ test_bitmap_cases () {
 			git commit -am "add hello_world.txt" &&
 
 			git repack -adb &&
-			test-tool bitmap load-corrupt
+			bitmap=$(ls .git/objects/pack/pack-*.bitmap) &&
+			chmod +w "$bitmap" &&
+
+			# this matches the xor offset
+			offset=$((120 + $(test_oid rawsz))) &&
+			printf "\241" |
+				dd of=$bitmap count=1 bs=1 conv=notrunc seek=$offset &&
+
+			git rev-list --count HEAD >expect &&
+			git rev-list --use-bitmap-index --count HEAD >actual &&
+			test_cmp expect actual
 		)
 	'
 }

-Peff