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

> 
> 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
>