2025年5月19日 15:39，Jeff King <peff@xxxxxxxx> 写道： > > On Mon, May 19, 2025 at 02:44:22PM +0800, lidongyan wrote: > >> 2025年5月19日 14:02，Patrick Steinhardt <ps@xxxxxx> 写道： >>> 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@xxxxxxxxxxxxxxxxxxxxxxx. >> A potential memory leak is mentioned in this patch here https://lore.kernel.org/git/pull.1962.git.git.1747052530271.gitgitgadget@xxxxxxxxx/. > > 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 > Got it, using dd is indeed simpler and better. Thanks a lot!
