{"thread":{"id":"63475","subject":"[PATCH] pack-bitmap: add loading corrupt bitmap_index test","startedAt":"2025-05-17T14:26:26Z","lastAt":"2025-05-19T07:58:52Z","messageCount":7,"participants":["Lidong Yan via GitGitGadget","Patrick Steinhardt","lidongyan","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"518332","messageId":"pull.1967.git.git.1747491983066.gitgitgadget@gmail.com","threadId":"63475","inReplyTo":null,"subject":"[PATCH] pack-bitmap: add loading corrupt bitmap_index test","fromName":"Lidong Yan via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-05-17T14:26:22Z","receivedAt":"2025-05-17T14:26:26Z","isPatch":true,"sender":{"key":"yldhome2d2@gmail.com","avatar":"https://avatars.githubusercontent.com/u/77328395?v=4"},"body":"From: Lidong Yan <502024330056@smail.nju.edu.cn>\n\nThis patch add a test function `test_bitmap_load_corrupt` in patch-bitmap.c\n, a `load corrupt bitmap` test case in t5310-pack-bitmaps.sh and\na new command `load-corrupt` for `test-tool` in t/helper/test-bitmap.c.\n\nTo make sure we are loading a corrupt bitmap, we need enable bitmap table\nlookup so that `prepare_bitmap()` won't call `load_bitmap_entries_v1()`.\nSo to test corrupt bitmap_index, we first call `prepare_bitmap()` to set\neverything up but `bitmap_index->bitmaps` for us. Then we do any\ncorruption we want to the bitmap_index. Finally we can test loading\ncorrupt bitmap by calling `load_bitmap_entries_v1()`.\n\nSigned-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>\n---\n    pack-bitmap: add loading corrupt bitmap_index test\n    \n    This patch add a test function test_bitmap_load_corrupt in\n    patch-bitmap.c , a load corrupt bitmap test case in\n    t5310-pack-bitmaps.sh and a new command load-corrupt for test-tool in\n    t/helper/test-bitmap.c.\n    \n    To make sure we are loading a corrupt bitmap, we need enable bitmap\n    table lookup so that prepare_bitmap() won't call\n    load_bitmap_entries_v1(). So to test corrupt bitmap_index, we first\n    prepare_bitmap() to set everything up but bitmap_index->bitmaps for us.\n    Then we do any corruption we want to the bitmap_index. Finally we call\n    load_bitmap_entries_v1() to test loading corrupt bitmap.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1967%2Fbrandb97%2Fcorrupt-bitmap-test-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1967/brandb97/corrupt-bitmap-test-v1\nPull-Request: https://github.com/git/git/pull/1967\n\n pack-bitmap.c           | 65 +++++++++++++++++++++++++++++++++++++++++\n pack-bitmap.h           |  1 +\n t/helper/test-bitmap.c  |  8 +++++\n t/t5310-pack-bitmaps.sh | 15 ++++++++++\n 4 files changed, 89 insertions(+)\n\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex b9f1d866046..9642a06b3fe 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -3022,6 +3022,71 @@ cleanup:\n \treturn ret;\n }\n \n+typedef void(corrupt_fn)(struct bitmap_index *);\n+\n+static int bitmap_corrupt_then_load(struct repository *r, corrupt_fn *do_corrupt)\n+{\n+\tstruct bitmap_index *bitmap_git;\n+\tunsigned char *map;\n+\n+\tif (!(bitmap_git = prepare_bitmap_git(r)))\n+\t\tdie(_(\"failed to prepare bitmap indexes\"));\n+\t/*\n+\t * If the table lookup extension is not used,\n+\t * prepare_bitmap_git has already called load_bitmap_entries_v1(),\n+\t * making it impossible to corrupt the bitmap.\n+\t */\n+\tif (!bitmap_git->table_lookup)\n+\t\treturn 0;\n+\n+\t/*\n+\t * bitmap_git->map is read-only;\n+\t * to corrupt it, we need a writable memory block.\n+\t */\n+\tmap = bitmap_git->map;\n+\tbitmap_git->map = xmalloc(bitmap_git->map_size);\n+\tif (!bitmap_git->map)\n+\t\treturn 0;\n+\tmemcpy(bitmap_git->map, map, bitmap_git->map_size);\n+\n+\tdo_corrupt(bitmap_git);\n+\tif (!load_bitmap_entries_v1(bitmap_git))\n+\t\tdie(_(\"load corrupt bitmap successfully\"));\n+\n+\tfree(bitmap_git->map);\n+\tbitmap_git->map = map;\n+\tfree_bitmap_index(bitmap_git);\n+\n+\treturn 0;\n+}\n+\n+static void do_corrupt_commit_pos(struct bitmap_index *bitmap_git)\n+{\n+\tuint32_t *commit_pos_ptr;\n+\n+\tcommit_pos_ptr = (uint32_t *)(bitmap_git->map + bitmap_git->map_pos);\n+\t*commit_pos_ptr = (uint32_t)-1;\n+}\n+\n+static void do_corrupt_xor_offset(struct bitmap_index *bitmap_git)\n+{\n+\tuint8_t *xor_offset_ptr;\n+\n+\txor_offset_ptr = (uint8_t *)(bitmap_git->map + bitmap_git->map_pos +\n+\t\t\t\t     sizeof(uint32_t));\n+\t*xor_offset_ptr = MAX_XOR_OFFSET + 1;\n+}\n+\n+int test_bitmap_load_corrupt(struct repository *r)\n+{\n+\tint res = 0;\n+\tif ((res = bitmap_corrupt_then_load(r, do_corrupt_commit_pos)))\n+\t\treturn res;\n+\tif ((res = bitmap_corrupt_then_load(r, do_corrupt_xor_offset)))\n+\t\treturn res;\n+\treturn res;\n+}\n+\n int rebuild_bitmap(const uint32_t *reposition,\n \t\t   struct ewah_bitmap *source,\n \t\t   struct bitmap *dest)\ndiff --git a/pack-bitmap.h b/pack-bitmap.h\nindex 382d39499af..7770abe6bff 100644\n--- a/pack-bitmap.h\n+++ b/pack-bitmap.h\n@@ -85,6 +85,7 @@ int test_bitmap_hashes(struct repository *r);\n int test_bitmap_pseudo_merges(struct repository *r);\n int test_bitmap_pseudo_merge_commits(struct repository *r, uint32_t n);\n int test_bitmap_pseudo_merge_objects(struct repository *r, uint32_t n);\n+int test_bitmap_load_corrupt(struct repository *r);\n \n struct list_objects_filter_options;\n \ndiff --git a/t/helper/test-bitmap.c b/t/helper/test-bitmap.c\nindex 3f23f210726..7aabcfed2f9 100644\n--- a/t/helper/test-bitmap.c\n+++ b/t/helper/test-bitmap.c\n@@ -20,6 +20,11 @@ static int bitmap_dump_pseudo_merges(void)\n \treturn test_bitmap_pseudo_merges(the_repository);\n }\n \n+static int bitmap_load_corrupt(void)\n+{\n+\treturn test_bitmap_load_corrupt(the_repository);\n+}\n+\n static int bitmap_dump_pseudo_merge_commits(uint32_t n)\n {\n \treturn test_bitmap_pseudo_merge_commits(the_repository, n);\n@@ -40,6 +45,8 @@ int cmd__bitmap(int argc, const char **argv)\n \t\treturn bitmap_dump_hashes();\n \tif (argc == 2 && !strcmp(argv[1], \"dump-pseudo-merges\"))\n \t\treturn bitmap_dump_pseudo_merges();\n+\tif (argc == 2 && !strcmp(argv[1], \"load-corrupt\"))\n+\t\treturn bitmap_load_corrupt();\n \tif (argc == 3 && !strcmp(argv[1], \"dump-pseudo-merge-commits\"))\n \t\treturn bitmap_dump_pseudo_merge_commits(atoi(argv[2]));\n \tif (argc == 3 && !strcmp(argv[1], \"dump-pseudo-merge-objects\"))\n@@ -48,6 +55,7 @@ int cmd__bitmap(int argc, const char **argv)\n \tusage(\"\\ttest-tool bitmap list-commits\\n\"\n \t      \"\\ttest-tool bitmap dump-hashes\\n\"\n \t      \"\\ttest-tool bitmap dump-pseudo-merges\\n\"\n+\t      \"\\ttest-tool bitmap load-corrupt\\n\"\n \t      \"\\ttest-tool bitmap dump-pseudo-merge-commits <n>\\n\"\n \t      \"\\ttest-tool bitmap dump-pseudo-merge-objects <n>\");\n \ndiff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh\nindex a62b463eaf0..042f62f16ea 100755\n--- a/t/t5310-pack-bitmaps.sh\n+++ b/t/t5310-pack-bitmaps.sh\n@@ -486,6 +486,21 @@ test_bitmap_cases () {\n \t\t\tgrep \"ignoring extra bitmap\" trace2.txt\n \t\t)\n \t'\n+\n+\ttest_expect_success 'load corrupt bitmap' '\n+\t\tgit init repo &&\n+\t\ttest_when_finished \"rm -fr repo\" && (\n+\t\t\tcd repo &&\n+\t\t\tgit config pack.writeBitmapLookupTable '\"$writeLookupTable\"' &&\n+\n+\t\t\techo \"Hello world\" > hello_world.txt &&\n+\t\t\tgit add hello_world.txt &&\n+\t\t\tgit commit -am \"add hello_world.txt\" &&\n+\n+\t\t\tgit repack -adb &&\n+\t\t\ttest-tool bitmap load-corrupt\n+\t\t)\n+\t'\n }\n \n test_bitmap_cases\n\nbase-commit: 6f84262c44a89851c3ae5a6e4c1a9d06b2068d75\n-- \ngitgitgadget\n"},{"id":"518377","messageId":"aCrJcK6ml4r4S-mF@pks.im","threadId":"63475","inReplyTo":"pull.1967.git.git.1747491983066.gitgitgadget@gmail.com","subject":"Re: [PATCH] pack-bitmap: add loading corrupt bitmap_index test","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-05-19T06:02:24Z","receivedAt":"2025-05-19T06:02:34Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sat, May 17, 2025 at 02:26:22PM +0000, Lidong Yan via GitGitGadget wrote:\n> From: Lidong Yan <502024330056@smail.nju.edu.cn>\n> \n> This patch add a test function `test_bitmap_load_corrupt` in patch-bitmap.c\n> , a `load corrupt bitmap` test case in t5310-pack-bitmaps.sh and\n> a new command `load-corrupt` for `test-tool` in t/helper/test-bitmap.c.\n> \n> To make sure we are loading a corrupt bitmap, we need enable bitmap table\n> lookup so that `prepare_bitmap()` won't call `load_bitmap_entries_v1()`.\n> So to test corrupt bitmap_index, we first call `prepare_bitmap()` to set\n> everything up but `bitmap_index->bitmaps` for us. Then we do any\n> corruption we want to the bitmap_index. Finally we can test loading\n> corrupt bitmap by calling `load_bitmap_entries_v1()`.\n\nOkay. We _can_ do that now, but the patch doesn't explain why we\n_should_.\n\n> diff --git a/pack-bitmap.c b/pack-bitmap.c\n> index b9f1d866046..9642a06b3fe 100644\n> --- a/pack-bitmap.c\n> +++ b/pack-bitmap.c\n> @@ -3022,6 +3022,71 @@ cleanup:\n>  \treturn ret;\n>  }\n>  \n> +typedef void(corrupt_fn)(struct bitmap_index *);\n> +\n> +static int bitmap_corrupt_then_load(struct repository *r, corrupt_fn *do_corrupt)\n> +{\n> +\tstruct bitmap_index *bitmap_git;\n> +\tunsigned char *map;\n> +\n> +\tif (!(bitmap_git = prepare_bitmap_git(r)))\n> +\t\tdie(_(\"failed to prepare bitmap indexes\"));\n> +\t/*\n> +\t * If the table lookup extension is not used,\n> +\t * prepare_bitmap_git has already called load_bitmap_entries_v1(),\n> +\t * making it impossible to corrupt the bitmap.\n> +\t */\n> +\tif (!bitmap_git->table_lookup)\n> +\t\treturn 0;\n> +\n> +\t/*\n> +\t * bitmap_git->map is read-only;\n> +\t * to corrupt it, we need a writable memory block.\n> +\t */\n> +\tmap = bitmap_git->map;\n> +\tbitmap_git->map = xmalloc(bitmap_git->map_size);\n> +\tif (!bitmap_git->map)\n> +\t\treturn 0;\n> +\tmemcpy(bitmap_git->map, map, bitmap_git->map_size);\n> +\n> +\tdo_corrupt(bitmap_git);\n> +\tif (!load_bitmap_entries_v1(bitmap_git))\n> +\t\tdie(_(\"load corrupt bitmap successfully\"));\n> +\n> +\tfree(bitmap_git->map);\n> +\tbitmap_git->map = map;\n> +\tfree_bitmap_index(bitmap_git);\n> +\n> +\treturn 0;\n> +}\n> +\n> +static void do_corrupt_commit_pos(struct bitmap_index *bitmap_git)\n> +{\n> +\tuint32_t *commit_pos_ptr;\n> +\n> +\tcommit_pos_ptr = (uint32_t *)(bitmap_git->map + bitmap_git->map_pos);\n> +\t*commit_pos_ptr = (uint32_t)-1;\n> +}\n> +\n> +static void do_corrupt_xor_offset(struct bitmap_index *bitmap_git)\n> +{\n> +\tuint8_t *xor_offset_ptr;\n> +\n> +\txor_offset_ptr = (uint8_t *)(bitmap_git->map + bitmap_git->map_pos +\n> +\t\t\t\t     sizeof(uint32_t));\n> +\t*xor_offset_ptr = MAX_XOR_OFFSET + 1;\n> +}\n> +\n> +int test_bitmap_load_corrupt(struct repository *r)\n> +{\n> +\tint res = 0;\n> +\tif ((res = bitmap_corrupt_then_load(r, do_corrupt_commit_pos)))\n> +\t\treturn res;\n> +\tif ((res = bitmap_corrupt_then_load(r, do_corrupt_xor_offset)))\n> +\t\treturn res;\n> +\treturn res;\n> +}\n> +\n\nDoes all of this logic really have to be part of \"pack-bitmap.c\"? It\nwould generally preferable to not have our production logic be cluttered\nwith test logic. Sometimes we don't have a better way to do this, but\nyou should explain why we cannot host the logic elsewhere in that case.\n\nMy proposal would be to either move the logic into \"test-bitmap.c\", or\nto even better to write a unit test in \"t/unit-tests/\". After all, we\nexpect that the code should fail gracefully, so a unit test might be a\ngood fit after all.\n\nPatrick\n"},{"id":"518380","messageId":"80BCF957-002B-4532-8E3D-8CAB45AC0349@smail.nju.edu.cn","threadId":"63475","inReplyTo":"aCrJcK6ml4r4S-mF@pks.im","subject":"Re: [PATCH] pack-bitmap: add loading corrupt bitmap_index test","fromName":"lidongyan","fromEmail":"502024330056@smail.nju.edu.cn","sentAt":"2025-05-19T06:44:22Z","receivedAt":"2025-05-19T06:44:59Z","isPatch":true,"sender":{"key":"502024330056@smail.nju.edu.cn","avatar":"https://avatars.githubusercontent.com/u/77328395?v=4"},"body":"2025年5月19日 14:02，Patrick Steinhardt <ps@pks.im> 写道：\n> Okay. We _can_ do that now, but the patch doesn't explain why we\n> _should_.\n\nThe main purpose of this patch is to provide a test case to check whether\n a memory leak occurs when loading a corrupt index file as requested here\nhttps://lore.kernel.org/git/20250514180325.GB2196784@coredump.intra.peff.net.\nA potential memory leak is mentioned in this patch here https://lore.kernel.org/git/pull.1962.git.git.1747052530271.gitgitgadget@gmail.com/.\n\n> Does all of this logic really have to be part of \"pack-bitmap.c\"? It\n> would generally preferable to not have our production logic be cluttered\n> with test logic. Sometimes we don't have a better way to do this, but\n> you should explain why we cannot host the logic elsewhere in that case.\n> \n> My proposal would be to either move the logic into \"test-bitmap.c\", or\n> to even better to write a unit test in \"t/unit-tests/\". After all, we\n> expect that the code should fail gracefully, so a unit test might be a\n> good fit after all.\n> \n> Patrick\n\nI found that the header size of an index file depends only on the type of hash algorithm.\nTo trigger the condition for the memory leak, I need to corrupt a few bytes right after the\nindex file header size. It's more convenient to implement this functionality in pack-bitmap.c.\nHowever, I think I can place the test itself under t/unit-tests/.\n\n"},{"id":"518390","messageId":"aCrdWbcOtbYsw8t6@pks.im","threadId":"63475","inReplyTo":"80BCF957-002B-4532-8E3D-8CAB45AC0349@smail.nju.edu.cn","subject":"Re: [PATCH] pack-bitmap: add loading corrupt bitmap_index test","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-05-19T07:27:21Z","receivedAt":"2025-05-19T07:27:27Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, May 19, 2025 at 02:44:22PM +0800, lidongyan wrote:\n> 2025年5月19日 14:02，Patrick Steinhardt <ps@pks.im> 写道：\n> > Okay. We _can_ do that now, but the patch doesn't explain why we\n> > _should_.\n> \n> The main purpose of this patch is to provide a test case to check whether\n>  a memory leak occurs when loading a corrupt index file as requested here\n> https://lore.kernel.org/git/20250514180325.GB2196784@coredump.intra.peff.net.\n> A potential memory leak is mentioned in this patch here https://lore.kernel.org/git/pull.1962.git.git.1747052530271.gitgitgadget@gmail.com/.\n\nWouldn't it make sense to include this in a v2 of that patch series\nthen? That makes it way easier for the reviewer to draw the connection\nand allows you to draft commit messages in a way that they refer to one\nanother.\n\nPatrick\n"},{"id":"518392","messageId":"AC0F256C-4EEC-41A6-9AAF-78B3FEF26251@smail.nju.edu.cn","threadId":"63475","inReplyTo":"aCrdWbcOtbYsw8t6@pks.im","subject":"Re: [PATCH] pack-bitmap: add loading corrupt bitmap_index test","fromName":"lidongyan","fromEmail":"502024330056@smail.nju.edu.cn","sentAt":"2025-05-19T07:37:55Z","receivedAt":"2025-05-19T07:38:26Z","isPatch":true,"sender":{"key":"502024330056@smail.nju.edu.cn","avatar":"https://avatars.githubusercontent.com/u/77328395?v=4"},"body":"\n> 2025年5月19日 15:27，Patrick Steinhardt <ps@pks.im> 写道：\n> \n> On Mon, May 19, 2025 at 02:44:22PM +0800, lidongyan wrote:\n>> 2025年5月19日 14:02，Patrick Steinhardt <ps@pks.im> 写道：\n>>> Okay. We _can_ do that now, but the patch doesn't explain why we\n>>> _should_.\n>> \n>> The main purpose of this patch is to provide a test case to check whether\n>> a memory leak occurs when loading a corrupt index file as requested here\n>> https://lore.kernel.org/git/20250514180325.GB2196784@coredump.intra.peff.net.\n>> A potential memory leak is mentioned in this patch here https://lore.kernel.org/git/pull.1962.git.git.1747052530271.gitgitgadget@gmail.com/.\n> \n> Wouldn't it make sense to include this in a v2 of that patch series\n> then? That makes it way easier for the reviewer to draw the connection\n> and allows you to draft commit messages in a way that they refer to one\n> another.\n> \n> Patrick\n> \n\nThanks for the suggestion, including it in v2 sounds like the right approach. \nI’ll update the series and revise the commit messages accordingly."},{"id":"518393","messageId":"20250519073949.GD102701@coredump.intra.peff.net","threadId":"63475","inReplyTo":"80BCF957-002B-4532-8E3D-8CAB45AC0349@smail.nju.edu.cn","subject":"Re: [PATCH] pack-bitmap: add loading corrupt bitmap_index test","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-05-19T07:39:49Z","receivedAt":"2025-05-19T07:39:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 19, 2025 at 02:44:22PM +0800, lidongyan wrote:\n\n> 2025年5月19日 14:02，Patrick Steinhardt <ps@pks.im> 写道：\n> > Okay. We _can_ do that now, but the patch doesn't explain why we\n> > _should_.\n> \n> The main purpose of this patch is to provide a test case to check whether\n>  a memory leak occurs when loading a corrupt index file as requested here\n> https://lore.kernel.org/git/20250514180325.GB2196784@coredump.intra.peff.net.\n> A potential memory leak is mentioned in this patch here https://lore.kernel.org/git/pull.1962.git.git.1747052530271.gitgitgadget@gmail.com/.\n\nI think we'd need to mark it with the !SANITIZE_LEAK prereq until the\nleaks are actually fixed. Or simpler, just apply this on top of the leak\nfixes once they are ready. That ordering needs to be communicated to the\nmaintainer, and the simplest way to do that is probably to just post a\n3-patch series: your initial leak fix, a polished version of the one\nfrom Taylor, and then the test on top.\n\n> > My proposal would be to either move the logic into \"test-bitmap.c\", or\n> > to even better to write a unit test in \"t/unit-tests/\". After all, we\n> > expect that the code should fail gracefully, so a unit test might be a\n> > good fit after all.\n> \n> I found that the header size of an index file depends only on the type of hash algorithm.\n> To trigger the condition for the memory leak, I need to corrupt a few bytes right after the\n> index file header size. It's more convenient to implement this functionality in pack-bitmap.c.\n> However, I think I can place the test itself under t/unit-tests/.\n\nI don't think you can do a prereq for a unit-test file (though I might\nbe wrong, as I have not really paid attention to that area).\n\nIf the corruption offsets are easy-ish to compute statically (and it\nsounds like they mostly just depend on the hash algorithm size), then\nI'd actually prefer to just do it with \"dd\". That avoids extra C code,\nand simulates a real on-disk corruption more exactly (using real\ncommands).\n\nSomething like:\n\ndiff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh\nindex 042f62f16e..16bd607654 100755\n--- a/t/t5310-pack-bitmaps.sh\n+++ b/t/t5310-pack-bitmaps.sh\n@@ -498,7 +498,17 @@ test_bitmap_cases () {\n \t\t\tgit commit -am \"add hello_world.txt\" &&\n \n \t\t\tgit repack -adb &&\n-\t\t\ttest-tool bitmap load-corrupt\n+\t\t\tbitmap=$(ls .git/objects/pack/pack-*.bitmap) &&\n+\t\t\tchmod +w \"$bitmap\" &&\n+\n+\t\t\t# this matches the xor offset\n+\t\t\toffset=$((120 + $(test_oid rawsz))) &&\n+\t\t\tprintf \"\\241\" |\n+\t\t\t\tdd of=$bitmap count=1 bs=1 conv=notrunc seek=$offset &&\n+\n+\t\t\tgit rev-list --count HEAD >expect &&\n+\t\t\tgit rev-list --use-bitmap-index --count HEAD >actual &&\n+\t\t\ttest_cmp expect actual\n \t\t)\n \t'\n }\n\n-Peff\n"},{"id":"518396","messageId":"4D908BBD-A930-49C4-B69D-E150ECE8597C@smail.nju.edu.cn","threadId":"63475","inReplyTo":"20250519073949.GD102701@coredump.intra.peff.net","subject":"Re: [PATCH] pack-bitmap: add loading corrupt bitmap_index test","fromName":"lidongyan","fromEmail":"502024330056@smail.nju.edu.cn","sentAt":"2025-05-19T07:58:14Z","receivedAt":"2025-05-19T07:58:52Z","isPatch":true,"sender":{"key":"502024330056@smail.nju.edu.cn","avatar":"https://avatars.githubusercontent.com/u/77328395?v=4"},"body":"2025年5月19日 15:39，Jeff King <peff@peff.net> 写道：\n> \n> On Mon, May 19, 2025 at 02:44:22PM +0800, lidongyan wrote:\n> \n>> 2025年5月19日 14:02，Patrick Steinhardt <ps@pks.im> 写道：\n>>> Okay. We _can_ do that now, but the patch doesn't explain why we\n>>> _should_.\n>> \n>> The main purpose of this patch is to provide a test case to check whether\n>> a memory leak occurs when loading a corrupt index file as requested here\n>> https://lore.kernel.org/git/20250514180325.GB2196784@coredump.intra.peff.net.\n>> A potential memory leak is mentioned in this patch here https://lore.kernel.org/git/pull.1962.git.git.1747052530271.gitgitgadget@gmail.com/.\n> \n> I think we'd need to mark it with the !SANITIZE_LEAK prereq until the\n> leaks are actually fixed. Or simpler, just apply this on top of the leak\n> fixes once they are ready. That ordering needs to be communicated to the\n> maintainer, and the simplest way to do that is probably to just post a\n> 3-patch series: your initial leak fix, a polished version of the one\n> from Taylor, and then the test on top.\n> \n>>> My proposal would be to either move the logic into \"test-bitmap.c\", or\n>>> to even better to write a unit test in \"t/unit-tests/\". After all, we\n>>> expect that the code should fail gracefully, so a unit test might be a\n>>> good fit after all.\n>> \n>> I found that the header size of an index file depends only on the type of hash algorithm.\n>> To trigger the condition for the memory leak, I need to corrupt a few bytes right after the\n>> index file header size. It's more convenient to implement this functionality in pack-bitmap.c.\n>> However, I think I can place the test itself under t/unit-tests/.\n> \n> I don't think you can do a prereq for a unit-test file (though I might\n> be wrong, as I have not really paid attention to that area).\n> \n> If the corruption offsets are easy-ish to compute statically (and it\n> sounds like they mostly just depend on the hash algorithm size), then\n> I'd actually prefer to just do it with \"dd\". That avoids extra C code,\n> and simulates a real on-disk corruption more exactly (using real\n> commands).\n> \n> Something like:\n> \n> diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh\n> index 042f62f16e..16bd607654 100755\n> --- a/t/t5310-pack-bitmaps.sh\n> +++ b/t/t5310-pack-bitmaps.sh\n> @@ -498,7 +498,17 @@ test_bitmap_cases () {\n> git commit -am \"add hello_world.txt\" &&\n> \n> git repack -adb &&\n> - test-tool bitmap load-corrupt\n> + bitmap=$(ls .git/objects/pack/pack-*.bitmap) &&\n> + chmod +w \"$bitmap\" &&\n> +\n> + # this matches the xor offset\n> + offset=$((120 + $(test_oid rawsz))) &&\n> + printf \"\\241\" |\n> + dd of=$bitmap count=1 bs=1 conv=notrunc seek=$offset &&\n> +\n> + git rev-list --count HEAD >expect &&\n> + git rev-list --use-bitmap-index --count HEAD >actual &&\n> + test_cmp expect actual\n> )\n> '\n> }\n> \n> -Peff\n> \n\nGot it, using dd is indeed simpler and better. Thanks a lot!\n\n"}]}