{"thread":{"id":"63447","subject":"[PATCH] pack-bitmap: fix memory leak if `load_bitmap_entries_v1` failed","startedAt":"2025-05-12T12:22:13Z","lastAt":"2025-07-08T22:35:55Z","messageCount":45,"participants":["Lidong Yan via GitGitGadget","Jeff King","Taylor Blau","Junio C Hamano","lidongyan","Taylor Blau via GitGitGadget"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"517829","messageId":"pull.1962.git.git.1747052530271.gitgitgadget@gmail.com","threadId":"63447","inReplyTo":null,"subject":"[PATCH] pack-bitmap: fix memory leak if `load_bitmap_entries_v1` failed","fromName":"Lidong Yan via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-05-12T12:22:10Z","receivedAt":"2025-05-12T12:22:13Z","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\nIn pack-bitmap.c:load_bitmap_entries_v1, the function `read_bitmap_1`\nallocates a bitmap and reads index data into it. However, if any of\nthe validation checks following the allocation fail, the allocated bitmap\nis not freed, resulting in a memory leak. To avoid this, the validation\nchecks should be performed before the bitmap is allocated.\n\nSigned-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>\n---\n pack-bitmap: fix memory leak if load_bitmap_entries_v1 failed\n \n In pack-bitmap.c:load_bitmap_entries_v1, the function read_bitmap_1\n allocates a bitmap and reads index data into it. However, if any of the\n validation checks following the allocation fail, the allocated bitmap is\n not freed, resulting in a memory leak. To avoid this, the validation\n checks should be performed before the bitmap is allocated.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1962%2Fbrandb97%2Ffix-pack-bitmap-leak-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1962/brandb97/fix-pack-bitmap-leak-v1\nPull-Request: https://github.com/git/git/pull/1962\n\n pack-bitmap.c | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex b9f1d866046..ac6d62b980c 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -388,10 +388,6 @@ static int load_bitmap_entries_v1(struct bitmap_index *index)\n \t\t\treturn error(_(\"corrupt ewah bitmap: commit index %u out of range\"),\n \t\t\t\t (unsigned)commit_idx_pos);\n \n-\t\tbitmap = read_bitmap_1(index);\n-\t\tif (!bitmap)\n-\t\t\treturn -1;\n-\n \t\tif (xor_offset > MAX_XOR_OFFSET || xor_offset > i)\n \t\t\treturn error(_(\"corrupted bitmap pack index\"));\n \n@@ -402,6 +398,10 @@ static int load_bitmap_entries_v1(struct bitmap_index *index)\n \t\t\t\treturn error(_(\"invalid XOR offset in bitmap pack index\"));\n \t\t}\n \n+\t\tbitmap = read_bitmap_1(index);\n+\t\tif (!bitmap)\n+\t\t\treturn -1;\n+\n \t\trecent_bitmaps[i % MAX_XOR_OFFSET] = store_bitmap(\n \t\t\tindex, bitmap, &oid, xor_bitmap, flags);\n \t}\n\nbase-commit: 6f84262c44a89851c3ae5a6e4c1a9d06b2068d75\n-- \ngitgitgadget\n"},{"id":"517842","messageId":"20250512131315.GD1191360@coredump.intra.peff.net","threadId":"63447","inReplyTo":"pull.1962.git.git.1747052530271.gitgitgadget@gmail.com","subject":"Re: [PATCH] pack-bitmap: fix memory leak if `load_bitmap_entries_v1` failed","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-05-12T13:13:15Z","receivedAt":"2025-05-12T13:13:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 12, 2025 at 12:22:10PM +0000, Lidong Yan via GitGitGadget wrote:\n\n> From: Lidong Yan <502024330056@smail.nju.edu.cn>\n> \n> In pack-bitmap.c:load_bitmap_entries_v1, the function `read_bitmap_1`\n> allocates a bitmap and reads index data into it. However, if any of\n> the validation checks following the allocation fail, the allocated bitmap\n> is not freed, resulting in a memory leak. To avoid this, the validation\n> checks should be performed before the bitmap is allocated.\n\nThanks, this looks correct to me.\n\n> @@ -388,10 +388,6 @@ static int load_bitmap_entries_v1(struct bitmap_index *index)\n> \t\t\treturn error(_(\"corrupt ewah bitmap: commit index %u out of range\"),\n> \t\t\t\t (unsigned)commit_idx_pos);\n> \n> -\t\tbitmap = read_bitmap_1(index);\n> -\t\tif (!bitmap)\n> -\t\t\treturn -1;\n> -\n> \t\tif (xor_offset > MAX_XOR_OFFSET || xor_offset > i)\n> \t\t\treturn error(_(\"corrupted bitmap pack index\"));\n\nI noticed that this code is also within a loop, so we could still return\nearly on the next loop iteration. But by that point we will have called\nstore_bitmap() on the result, so we only have to worry about leaking the\nbitmap from the current loop iteration.\n\n-Peff\n"},{"id":"517960","messageId":"aCOFqYdnPp1Lne4Y@nand.local","threadId":"63447","inReplyTo":"20250512131315.GD1191360@coredump.intra.peff.net","subject":"Re: [PATCH] pack-bitmap: fix memory leak if `load_bitmap_entries_v1` failed","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-05-13T17:47:21Z","receivedAt":"2025-05-13T17:47:28Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, May 12, 2025 at 09:13:15AM -0400, Jeff King wrote:\n> On Mon, May 12, 2025 at 12:22:10PM +0000, Lidong Yan via GitGitGadget wrote:\n>\n> > From: Lidong Yan <502024330056@smail.nju.edu.cn>\n> >\n> > In pack-bitmap.c:load_bitmap_entries_v1, the function `read_bitmap_1`\n> > allocates a bitmap and reads index data into it. However, if any of\n> > the validation checks following the allocation fail, the allocated bitmap\n> > is not freed, resulting in a memory leak. To avoid this, the validation\n> > checks should be performed before the bitmap is allocated.\n>\n> Thanks, this looks correct to me.\n\nIt looks correct to me as well, and is a strict improvement. But I think\nthere is a leak outside of this function as well that is not touched by\nthis patch.\n\n> > @@ -388,10 +388,6 @@ static int load_bitmap_entries_v1(struct bitmap_index *index)\n> > \t\t\treturn error(_(\"corrupt ewah bitmap: commit index %u out of range\"),\n> > \t\t\t\t (unsigned)commit_idx_pos);\n> >\n> > -\t\tbitmap = read_bitmap_1(index);\n> > -\t\tif (!bitmap)\n> > -\t\t\treturn -1;\n> > -\n> > \t\tif (xor_offset > MAX_XOR_OFFSET || xor_offset > i)\n> > \t\t\treturn error(_(\"corrupted bitmap pack index\"));\n>\n> I noticed that this code is also within a loop, so we could still return\n> early on the next loop iteration. But by that point we will have called\n> store_bitmap() on the result, so we only have to worry about leaking the\n> bitmap from the current loop iteration.\n\nThat's right, though I think there is still a leak here.\n\nAfter going through the \"failed\" label, load_bitmap() will return -1,\nand its caller (either prepare_bitmap_walk() or prepare_bitmap_git())\nwill then call free_bitmap_index().\n\nThat function would have done:\n\n struct stored_bitmap *sb;\n kh_foreach_value(b->bitmaps, sb {\n ewah_pool_free(sb->root);\n free(sb);\n });\n\n, but won't since load_bitmap() already called kh_destroy_oid_map() and\nNULL'd the \"bitmaps\" pointer from within its \"failed\" label.\n\nSo I think if you got part of the way through loading bitmap entries and\nthen failed, you would leak all of the previous entries that you were\nable to load successfully.\n\nI suspect the fix looks something like:\n\n--- 8< ---\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex 5299f49d59..7f28532a69 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -631,41 +631,28 @@ static int load_bitmap(struct repository *r, struct bitmap_index *bitmap_git,\n \tbitmap_git->ext_index.positions = kh_init_oid_pos();\n\n \tif (load_reverse_index(r, bitmap_git))\n-\t\tgoto failed;\n+\t\treturn -1;\n\n \tif (!(bitmap_git->commits = read_bitmap_1(bitmap_git)) ||\n \t\t!(bitmap_git->trees = read_bitmap_1(bitmap_git)) ||\n \t\t!(bitmap_git->blobs = read_bitmap_1(bitmap_git)) ||\n \t\t!(bitmap_git->tags = read_bitmap_1(bitmap_git)))\n-\t\tgoto failed;\n+\t\treturn -1;\n\n \tif (!bitmap_git->table_lookup && load_bitmap_entries_v1(bitmap_git) < 0)\n-\t\tgoto failed;\n+\t\treturn -1;\n\n \tif (bitmap_git->base) {\n \t\tif (!bitmap_is_midx(bitmap_git))\n \t\t\tBUG(\"non-MIDX bitmap has non-NULL base bitmap index\");\n \t\tif (load_bitmap(r, bitmap_git->base, 1) < 0)\n-\t\t\tgoto failed;\n+\t\t\treturn -1;\n \t}\n\n \tif (!recursing)\n \t\tload_all_type_bitmaps(bitmap_git);\n\n \treturn 0;\n-\n-failed:\n-\tmunmap(bitmap_git->map, bitmap_git->map_size);\n-\tbitmap_git->map = NULL;\n-\tbitmap_git->map_size = 0;\n-\n-\tkh_destroy_oid_map(bitmap_git->bitmaps);\n-\tbitmap_git->bitmaps = NULL;\n-\n-\tkh_destroy_oid_pos(bitmap_git->ext_index.positions);\n-\tbitmap_git->ext_index.positions = NULL;\n-\n-\treturn -1;\n }\n\n static int open_pack_bitmap(struct repository *r,\n--- >8 ---\n\n, since all callers of load_bitmap() will themselves call\nfree_bitmap_index(), so there is no need for us to open-code a portion\nof that function's implementation ourselves.\n\nThanks,\nTaylor\n"},{"id":"518035","messageId":"xmqqcycbcou7.fsf@gitster.g","threadId":"63447","inReplyTo":"aCOFqYdnPp1Lne4Y@nand.local","subject":"Re: [PATCH] pack-bitmap: fix memory leak if `load_bitmap_entries_v1` failed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-14T13:18:56Z","receivedAt":"2025-05-14T13:19:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> After going through the \"failed\" label, load_bitmap() will return -1,\n> and its caller (either prepare_bitmap_walk() or prepare_bitmap_git())\n> will then call free_bitmap_index().\n>\n> That function would have done:\n>\n> struct stored_bitmap *sb;\n> kh_foreach_value(b->bitmaps, sb {\n> ewah_pool_free(sb->root);\n> free(sb);\n> });\n>\n> , but won't since load_bitmap() already called kh_destroy_oid_map() and\n> NULL'd the \"bitmaps\" pointer from within its \"failed\" label.\n\nYikes.\n\n> So I think if you got part of the way through loading bitmap entries and\n> then failed, you would leak all of the previous entries that you were\n> able to load successfully.\n>\n> I suspect the fix looks something like:\n> ...\n> --- 8< ---\n> diff --git a/pack-bitmap.c b/pack-bitmap.c\n> index 5299f49d59..7f28532a69 100644\n> --- a/pack-bitmap.c\n> +++ b/pack-bitmap.c\n> @@ -631,41 +631,28 @@ static int load_bitmap(struct repository *r, struct bitmap_index *bitmap_git,\n> \tbitmap_git->ext_index.positions = kh_init_oid_pos();\n>\n> \tif (load_reverse_index(r, bitmap_git))\n> -\t\tgoto failed;\n> +\t\treturn -1;\n\n(a lot of changes that simplifies the code snipped)\n\n> -failed:\n> -\tmunmap(bitmap_git->map, bitmap_git->map_size);\n> -\tbitmap_git->map = NULL;\n> -\tbitmap_git->map_size = 0;\n> -\n> -\tkh_destroy_oid_map(bitmap_git->bitmaps);\n> -\tbitmap_git->bitmaps = NULL;\n> -\n> -\tkh_destroy_oid_pos(bitmap_git->ext_index.positions);\n> -\tbitmap_git->ext_index.positions = NULL;\n> -\n> -\treturn -1;\n> }\n>\n> static int open_pack_bitmap(struct repository *r,\n> --- >8 ---\n>\n> , since all callers of load_bitmap() will themselves call\n> free_bitmap_index(), so there is no need for us to open-code a portion\n> of that function's implementation ourselves.\n\nIt is rare for a fix to be removing and simplifying this much code\n;-)\n"},{"id":"518062","messageId":"20250514180325.GB2196784@coredump.intra.peff.net","threadId":"63447","inReplyTo":"aCOFqYdnPp1Lne4Y@nand.local","subject":"Re: [PATCH] pack-bitmap: fix memory leak if `load_bitmap_entries_v1` failed","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-05-14T18:03:25Z","receivedAt":"2025-05-14T18:03:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 13, 2025 at 01:47:21PM -0400, Taylor Blau wrote:\n\n> > > In pack-bitmap.c:load_bitmap_entries_v1, the function `read_bitmap_1`\n> > > allocates a bitmap and reads index data into it. However, if any of\n> > > the validation checks following the allocation fail, the allocated bitmap\n> > > is not freed, resulting in a memory leak. To avoid this, the validation\n> > > checks should be performed before the bitmap is allocated.\n> >\n> > Thanks, this looks correct to me.\n> \n> It looks correct to me as well, and is a strict improvement. But I think\n> there is a leak outside of this function as well that is not touched by\n> this patch.\n\nGood catch, and your analysis looks correct to me. I don't think that\nchanges anything for this patch, which is fixing a more \"inner\" issue of\nthe allocated memory hitting store_bitmap() at all.\n\nSo I think this can graduate independently, and then you can prepare\nyour fix on top (but no rush).\n\nIt would be nice if we triggered these cases in the test suite so that\nLSan could confirm that all leaks are covered. But I suspect it may not\nbe worth the effort to craft a bitmap file that is broken in such\nparticular ways.\n\n> I suspect the fix looks something like:\n> [...]\n> , since all callers of load_bitmap() will themselves call\n> free_bitmap_index(), so there is no need for us to open-code a portion\n> of that function's implementation ourselves.\n\nDeleting that extra code would be doubly satisfying.\n\n-Peff\n"},{"id":"518080","messageId":"99DD81A9-DDF2-4D13-BDEC-9DD83C8E8423@smail.nju.edu.cn","threadId":"63447","inReplyTo":"20250514180325.GB2196784@coredump.intra.peff.net","subject":"Re: [PATCH] pack-bitmap: fix memory leak if `load_bitmap_entries_v1` failed","fromName":"lidongyan","fromEmail":"502024330056@smail.nju.edu.cn","sentAt":"2025-05-15T01:37:39Z","receivedAt":"2025-05-15T01:38:24Z","isPatch":true,"sender":{"key":"502024330056@smail.nju.edu.cn","avatar":"https://avatars.githubusercontent.com/u/77328395?v=4"},"body":"On 15/5/2025 at 02:03，Jeff King 写道：\n\n> It would be nice if we triggered these cases in the test suite so that\n> LSan could confirm that all leaks are covered. But I suspect it may not\n> be worth the effort to craft a bitmap file that is broken in such\n> particular ways.\n\nI'd like to try coming up with some test cases for this."},{"id":"518474","messageId":"pull.1962.v2.git.git.1747732991.gitgitgadget@gmail.com","threadId":"63447","inReplyTo":"pull.1962.git.git.1747052530271.gitgitgadget@gmail.com","subject":"[PATCH v2 0/3] pack-bitmap: fix memory leak if load_bitmap_entries_v1 failed","fromName":"Lidong Yan via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-05-20T09:23:07Z","receivedAt":"2025-05-20T09:23:14Z","isPatch":true,"sender":{"key":"yldhome2d2@gmail.com","avatar":"https://avatars.githubusercontent.com/u/77328395?v=4"},"body":"In pack-bitmap.c:load_bitmap_entries_v1, the function read_bitmap_1\nallocates a bitmap and reads index data into it. However, if any of the\nvalidation checks following the allocation fail, the allocated bitmap is not\nfreed, resulting in a memory leak. To avoid this, the validation checks\nshould be performed before the bitmap is allocated.\n\nLidong Yan (2):\n pack-bitmap: fix memory leak if `load_bitmap_entries_v1` failed\n pack-bitmap: add loading corrupt bitmap_index test\n\nTaylor Blau (1):\n pack-bitmap: fix memory leak if `load_bitmap_entries_v1` failed\n\n pack-bitmap.c | 29 +++++++-----------------\n t/t5310-pack-bitmaps.sh | 50 +++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 58 insertions(+), 21 deletions(-)\n\n\nbase-commit: cb96e1697ad6e54d11fc920c95f82977f8e438f8\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1962%2Fbrandb97%2Ffix-pack-bitmap-leak-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1962/brandb97/fix-pack-bitmap-leak-v2\nPull-Request: https://github.com/git/git/pull/1962\n\nRange-diff vs v1:\n\n 1: 00168766edf = 1: 130c3dc5dcd pack-bitmap: fix memory leak if `load_bitmap_entries_v1` failed\n -: ----------- > 2: b515c278a8f pack-bitmap: fix memory leak if `load_bitmap_entries_v1` failed\n -: ----------- > 3: 5be22d563af pack-bitmap: add loading corrupt bitmap_index test\n\n-- \ngitgitgadget\n"},{"id":"518475","messageId":"130c3dc5dcddf9a0b124a7d6f6d50b9787f389fb.1747732991.git.gitgitgadget@gmail.com","threadId":"63447","inReplyTo":"pull.1962.v2.git.git.1747732991.gitgitgadget@gmail.com","subject":"[PATCH v2 1/3] pack-bitmap: fix memory leak if `load_bitmap_entries_v1` failed","fromName":"Lidong Yan via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-05-20T09:23:08Z","receivedAt":"2025-05-20T09:23:15Z","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\nIn pack-bitmap.c:load_bitmap_entries_v1, the function `read_bitmap_1`\nallocates a bitmap and reads index data into it. However, if any of\nthe validation checks following the allocation fail, the allocated bitmap\nis not freed, resulting in a memory leak. To avoid this, the validation\nchecks should be performed before the bitmap is allocated.\n\nSigned-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>\n---\n pack-bitmap.c | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex b9f1d866046b..ac6d62b980c5 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -388,10 +388,6 @@ static int load_bitmap_entries_v1(struct bitmap_index *index)\n \t\t\treturn error(_(\"corrupt ewah bitmap: commit index %u out of range\"),\n \t\t\t\t (unsigned)commit_idx_pos);\n \n-\t\tbitmap = read_bitmap_1(index);\n-\t\tif (!bitmap)\n-\t\t\treturn -1;\n-\n \t\tif (xor_offset > MAX_XOR_OFFSET || xor_offset > i)\n \t\t\treturn error(_(\"corrupted bitmap pack index\"));\n \n@@ -402,6 +398,10 @@ static int load_bitmap_entries_v1(struct bitmap_index *index)\n \t\t\t\treturn error(_(\"invalid XOR offset in bitmap pack index\"));\n \t\t}\n \n+\t\tbitmap = read_bitmap_1(index);\n+\t\tif (!bitmap)\n+\t\t\treturn -1;\n+\n \t\trecent_bitmaps[i % MAX_XOR_OFFSET] = store_bitmap(\n \t\t\tindex, bitmap, &oid, xor_bitmap, flags);\n \t}\n-- \ngitgitgadget\n\n"},{"id":"518476","messageId":"b515c278a8fec6c2ab9d11a49261f44fe0f37bf5.1747732991.git.gitgitgadget@gmail.com","threadId":"63447","inReplyTo":"pull.1962.v2.git.git.1747732991.gitgitgadget@gmail.com","subject":"[PATCH v2 2/3] pack-bitmap: fix memory leak if `load_bitmap_entries_v1` failed","fromName":"Taylor Blau via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-05-20T09:23:09Z","receivedAt":"2025-05-20T09:23:16Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"From: Taylor Blau \n\nAfter going through the \"failed\" label, load_bitmap() will return -1,\nand its caller (either prepare_bitmap_walk() or prepare_bitmap_git())\nwill then call free_bitmap_index().\n\nThat function would have done:\n\n struct stored_bitmap *sb;\n kh_foreach_value(b->bitmaps, sb {\n ewah_pool_free(sb->root);\n free(sb);\n });\n\n, but won't since load_bitmap() already called kh_destroy_oid_map() and\nNULL'd the \"bitmaps\" pointer from within its \"failed\" label.\n\nSo I think if you got part of the way through loading bitmap entries and\nthen failed, you would leak all of the previous entries that you were\nable to load successfully.\n\nThe solution is to remove the error handling code in load_bitmap(), because\nits caller will always call free_bitmap_index() in case of an error.\n\nSigned-off-by: Taylor Blau \n---\n pack-bitmap.c | 21 ++++-----------------\n 1 file changed, 4 insertions(+), 17 deletions(-)\n\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex ac6d62b980c5..fd19c2255163 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -630,41 +630,28 @@ static int load_bitmap(struct repository *r, struct bitmap_index *bitmap_git,\n \tbitmap_git->ext_index.positions = kh_init_oid_pos();\n \n \tif (load_reverse_index(r, bitmap_git))\n-\t\tgoto failed;\n+\t\treturn -1;\n \n \tif (!(bitmap_git->commits = read_bitmap_1(bitmap_git)) ||\n \t\t!(bitmap_git->trees = read_bitmap_1(bitmap_git)) ||\n \t\t!(bitmap_git->blobs = read_bitmap_1(bitmap_git)) ||\n \t\t!(bitmap_git->tags = read_bitmap_1(bitmap_git)))\n-\t\tgoto failed;\n+\t\treturn -1;\n \n \tif (!bitmap_git->table_lookup && load_bitmap_entries_v1(bitmap_git) < 0)\n-\t\tgoto failed;\n+\t\treturn -1;\n \n \tif (bitmap_git->base) {\n \t\tif (!bitmap_is_midx(bitmap_git))\n \t\t\tBUG(\"non-MIDX bitmap has non-NULL base bitmap index\");\n \t\tif (load_bitmap(r, bitmap_git->base, 1) < 0)\n-\t\t\tgoto failed;\n+\t\t\treturn -1;\n \t}\n \n \tif (!recursing)\n \t\tload_all_type_bitmaps(bitmap_git);\n \n \treturn 0;\n-\n-failed:\n-\tmunmap(bitmap_git->map, bitmap_git->map_size);\n-\tbitmap_git->map = NULL;\n-\tbitmap_git->map_size = 0;\n-\n-\tkh_destroy_oid_map(bitmap_git->bitmaps);\n-\tbitmap_git->bitmaps = NULL;\n-\n-\tkh_destroy_oid_pos(bitmap_git->ext_index.positions);\n-\tbitmap_git->ext_index.positions = NULL;\n-\n-\treturn -1;\n }\n \n static int open_pack_bitmap(struct repository *r,\n-- \ngitgitgadget\n\n"},{"id":"518477","messageId":"5be22d563af714ebb902506f12b4468a5348896c.1747732991.git.gitgitgadget@gmail.com","threadId":"63447","inReplyTo":"pull.1962.v2.git.git.1747732991.gitgitgadget@gmail.com","subject":"[PATCH v2 3/3] pack-bitmap: add loading corrupt bitmap_index test","fromName":"Lidong Yan via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-05-20T09:23:10Z","receivedAt":"2025-05-20T09:23:17Z","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 \"load corrupt bitmap\" test case in t5310-pack-bitmaps.sh.\n\nThis test case intentionally corrupt the \"xor_offset\" field of the first\nentry. To find position of first entry in *.bitmap, we need to skip 4\newah_bitmaps before entries. And I add a function `skip_ewah_bitmap()`\nto do this.\n\nSigned-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>\n---\n t/t5310-pack-bitmaps.sh | 50 +++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 50 insertions(+)\n\ndiff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh\nindex a62b463eaf09..537a507957bb 100755\n--- a/t/t5310-pack-bitmaps.sh\n+++ b/t/t5310-pack-bitmaps.sh\n@@ -26,6 +26,18 @@ has_any () {\n \tgrep -Ff \"$1\" \"$2\"\n }\n \n+skip_ewah_bitmap() {\n+\tlocal bitmap=\"$1\" &&\n+\tlocal offset=\"$2\" &&\n+\tlocal size= &&\n+\n+\toffset=$(($offset + 4)) &&\n+\tsize=0x$(od -An -v -t x1 -j $offset -N 4 $bitmap | tr -d ' \\n') &&\n+\tsize=$(($size * 8)) &&\n+\toffset=$(($offset + 4 + $size + 4)) &&\n+\techo $offset\n+}\n+\n # Since name-hash values are stored in the .bitmap files, add a test\n # that checks that the name-hash calculations are stable across versions.\n # Not exhaustive, but these hashing algorithms would be hard to change\n@@ -486,6 +498,44 @@ test_bitmap_cases () {\n \t\t\tgrep \"ignoring extra bitmap\" trace2.txt\n \t\t)\n \t'\n+\n+\t# A `.bitmap` file has the following structure:\n+\t# | Header | Commits | Trees | Blobs | Tags | Entries... |\n+\t#\n+\t# - The header is 32 bytes long when using SHA-1.\n+\t# - Commits, Trees, Blobs, and Tags are all stored as EWAH bitmaps.\n+\t#\n+\t# This test intentionally corrupts the `xor_offset` field of the first entry\n+\t# to verify robustness against malformed bitmap data.\n+\ttest_expect_success 'load corrupt bitmap' '\n+\t\trm -fr repo &&\n+\t\tgit init repo &&\n+\t\ttest_when_finished \"rm -fr repo\" &&\n+\t\t(\n+\t\t\tcd repo &&\n+\t\t\tgit config pack.writeBitmapLookupTable '\"$writeLookupTable\"' &&\n+\n+\t\t\ttest_commit base &&\n+\n+\t\t\tgit repack -adb &&\n+\t\t\tbitmap=\"$(ls .git/objects/pack/pack-*.bitmap)\" &&\n+\t\t\tchmod +w \"$bitmap\" &&\n+\n+\t\t\thdr_sz=$((12 + $(test_oid rawsz))) &&\n+\t\t\toffset=$(skip_ewah_bitmap $bitmap $hdr_sz) &&\n+\t\t\toffset=$(skip_ewah_bitmap $bitmap $offset) &&\n+\t\t\toffset=$(skip_ewah_bitmap $bitmap $offset) &&\n+\t\t\toffset=$(skip_ewah_bitmap $bitmap $offset) &&\n+\t\t\toffset=$((offset + 4)) &&\n+\n+\t\t\tprintf '\\161' |\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 test_bitmap_cases\n-- \ngitgitgadget\n"},{"id":"518633","messageId":"aC5nxa0uTb+ieiML@nand.local","threadId":"63447","inReplyTo":"b515c278a8fec6c2ab9d11a49261f44fe0f37bf5.1747732991.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 2/3] pack-bitmap: fix memory leak if `load_bitmap_entries_v1` failed","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-05-21T23:54:45Z","receivedAt":"2025-05-21T23:54:52Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, May 20, 2025 at 09:23:09AM +0000, Taylor Blau via GitGitGadget wrote:\n> Signed-off-by: Taylor Blau \n\nThis commit forges my Signed-off-by, but I am happy with the result\nhere.\n\nI do think the series is structured a little awkwardly as a result of\nadding this patch to it. That this and the previous patch have the\nsubject \"pack-bitmap: fix memory leak if `load_bitmap_entries_v1`\nfailed\" make the series not quite as clear as it could be.\n\nI think there are a couple of things going on:\n\n - This patch is a bug fix that could be applied independently of the\n first one. The rationale there would be that we shouldn't be leaking\n the EWAH bitmaps in 'b->bitmaps', but we are as a result of NULL'ing\n the pointer in the \"failed\" label. That patch can stand alone.\n\n - The first patch (yours) is no longer fixing a leak, at least after\n this patch. But it does delay reading the bitmap until we have\n validated its XOR offset for sanity, which is a good thing mostly\n from a performance perspective.\n\nI would probably swap the two patches around so that yours applies on\ntop of mine, and then rewords the patch message in yours to reflect that\nit is no longer fixing a leak.\n\nThat all said, if you feel strongly that the structure is fine/better\nas-is, I'd be more than happy to discuss it further.\n\nThanks,\nTaylor\n"},{"id":"518634","messageId":"aC5rCRJd3GaTNgL5@nand.local","threadId":"63447","inReplyTo":"5be22d563af714ebb902506f12b4468a5348896c.1747732991.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 3/3] pack-bitmap: add loading corrupt bitmap_index test","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-05-22T00:08:41Z","receivedAt":"2025-05-22T00:08:43Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, May 20, 2025 at 09:23:10AM +0000, Lidong Yan via GitGitGadget wrote:\n> From: Lidong Yan <502024330056@smail.nju.edu.cn>\n>\n> This patch add \"load corrupt bitmap\" test case in t5310-pack-bitmaps.sh.\n>\n> This test case intentionally corrupt the \"xor_offset\" field of the first\n> entry. To find position of first entry in *.bitmap, we need to skip 4\n> ewah_bitmaps before entries. And I add a function `skip_ewah_bitmap()`\n> to do this.\n\nI'm going to avoid commenting on the message itself, since I think we\nmay be able to drop this patch entirely, see below.\n\n> Signed-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>\n> ---\n> t/t5310-pack-bitmaps.sh | 50 +++++++++++++++++++++++++++++++++++++++++\n> 1 file changed, 50 insertions(+)\n>\n> diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh\n> index a62b463eaf09..537a507957bb 100755\n> --- a/t/t5310-pack-bitmaps.sh\n> +++ b/t/t5310-pack-bitmaps.sh\n> @@ -26,6 +26,18 @@ has_any () {\n> \tgrep -Ff \"$1\" \"$2\"\n> }\n>\n> +skip_ewah_bitmap() {\n> +\tlocal bitmap=\"$1\" &&\n> +\tlocal offset=\"$2\" &&\n> +\tlocal size= &&\n> +\n> +\toffset=$(($offset + 4)) &&\n> +\tsize=0x$(od -An -v -t x1 -j $offset -N 4 $bitmap | tr -d ' \\n') &&\n> +\tsize=$(($size * 8)) &&\n> +\toffset=$(($offset + 4 + $size + 4)) &&\n> +\techo $offset\n> +}\n> +\n> # Since name-hash values are stored in the .bitmap files, add a test\n> # that checks that the name-hash calculations are stable across versions.\n> # Not exhaustive, but these hashing algorithms would be hard to change\n> @@ -486,6 +498,44 @@ test_bitmap_cases () {\n> \t\t\tgrep \"ignoring extra bitmap\" trace2.txt\n> \t\t)\n> \t'\n> +\n> +\t# A `.bitmap` file has the following structure:\n> +\t# | Header | Commits | Trees | Blobs | Tags | Entries... |\n> +\t#\n> +\t# - The header is 32 bytes long when using SHA-1.\n> +\t# - Commits, Trees, Blobs, and Tags are all stored as EWAH bitmaps.\n> +\t#\n> +\t# This test intentionally corrupts the `xor_offset` field of the first entry\n> +\t# to verify robustness against malformed bitmap data.\n> +\ttest_expect_success 'load corrupt bitmap' '\n\nI am not totally following what this case is supposed to be testing.\nLet me think aloud for a moment...\n\n> +\t\trm -fr repo &&\n> +\t\tgit init repo &&\n> +\t\ttest_when_finished \"rm -fr repo\" &&\n> +\t\t(\n> +\t\t\tcd repo &&\n> +\t\t\tgit config pack.writeBitmapLookupTable '\"$writeLookupTable\"' &&\n\nFirst we set up a temporary repository, change into it, and enable\nbitmap lookup tables. Makes sense.\n\n> +\t\t\ttest_commit base &&\n> +\n> +\t\t\tgit repack -adb &&\n> +\t\t\tbitmap=\"$(ls .git/objects/pack/pack-*.bitmap)\" &&\n> +\t\t\tchmod +w \"$bitmap\" &&\n\nThen we make a commit, and write a bitmap containing the objects from\nthe commit we just made. Good.\n\n> +\t\t\thdr_sz=$((12 + $(test_oid rawsz))) &&\n> +\t\t\toffset=$(skip_ewah_bitmap $bitmap $hdr_sz) &&\n> +\t\t\toffset=$(skip_ewah_bitmap $bitmap $offset) &&\n> +\t\t\toffset=$(skip_ewah_bitmap $bitmap $offset) &&\n> +\t\t\toffset=$(skip_ewah_bitmap $bitmap $offset) &&\n\nThen we read past the header and four type bitmaps. Makes sense.\n\n> +\t\t\toffset=$((offset + 4)) &&\n\nNow we land at the bitmap for the commit we just wrote.\n\n(As an aside unrelated to this part of the test, this skip_ewah_bitmap()\nfunction seems awfully fragile. I wonder if it would make more sense to\nimplement this as a test helper that can dump the offsets of EWAH\nbitmaps in a *.bitmap file by object ID rather than trying to parse the\nfile ourselves?\n\nWe don't currently store an offset for each stored_bitmap that we\nmaintain, but doing so would be pretty straightforward (add it as a\nfield to the structure, and store the value of bitmap_git->map_pos from\nimmediately before reading the actual bitmap).)\n\n> +\t\t\tprintf '\\161' |\n> +\t\t\t\tdd of=$bitmap count=1 bs=1 conv=notrunc seek=$offset &&\n\nOK. Now we break the XOR offset field of this bitmap by writing garbage\ninto it.\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\n...and then we make sure that we still get the correct result.\n\nHmmph. I don't think this is quite testing what we want, since this test\npasses with or without your first patch. And that makes sense, we have\ntests elsewhere in this script that verify we can still fall back to\nclassic traversal when the bitmap index can't be read. (For some\nexamples, see: \"truncated bitmap fails gracefully (ewah)\" and \"truncated\nbitmap fails gracefully (cache)\".)\n\nI think what we're really testing here is the absence of a memory leak,\nwhich we are as of 1fc7ddf35b (test-lib: unconditionally enable leak\nchecking, 2024-11-20). I wonder whether or not we need this test at all?\n\nThanks,\nTaylor\n"},{"id":"518659","messageId":"013153DA-8314-429B-8408-9A79A3304013@smail.nju.edu.cn","threadId":"63447","inReplyTo":"aC5rCRJd3GaTNgL5@nand.local","subject":"Re: [PATCH v2 3/3] pack-bitmap: add loading corrupt bitmap_index test","fromName":"lidongyan","fromEmail":"502024330056@smail.nju.edu.cn","sentAt":"2025-05-22T15:05:56Z","receivedAt":"2025-05-22T15:06:39Z","isPatch":true,"sender":{"key":"502024330056@smail.nju.edu.cn","avatar":"https://avatars.githubusercontent.com/u/77328395?v=4"},"body":"2025年5月22日 08:08，Taylor Blau 写道：\n> \n> On Tue, May 20, 2025 at 09:23:10AM +0000, Lidong Yan via GitGitGadget wrote:\n>> From: Lidong Yan <502024330056@smail.nju.edu.cn>\n>> \n>> This patch add \"load corrupt bitmap\" test case in t5310-pack-bitmaps.sh.\n>> \n>> This test case intentionally corrupt the \"xor_offset\" field of the first\n>> entry. To find position of first entry in *.bitmap, we need to skip 4\n>> ewah_bitmaps before entries. And I add a function `skip_ewah_bitmap()`\n>> to do this.\n> \n> I'm going to avoid commenting on the message itself, since I think we\n> may be able to drop this patch entirely, see below.\n> \n>> Signed-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>\n>> ---\n>> t/t5310-pack-bitmaps.sh | 50 +++++++++++++++++++++++++++++++++++++++++\n>> 1 file changed, 50 insertions(+)\n>> \n>> diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh\n>> index a62b463eaf09..537a507957bb 100755\n>> --- a/t/t5310-pack-bitmaps.sh\n>> +++ b/t/t5310-pack-bitmaps.sh\n>> @@ -26,6 +26,18 @@ has_any () {\n>> grep -Ff \"$1\" \"$2\"\n>> }\n>> \n>> +skip_ewah_bitmap() {\n>> + local bitmap=\"$1\" &&\n>> + local offset=\"$2\" &&\n>> + local size= &&\n>> +\n>> + offset=$(($offset + 4)) &&\n>> + size=0x$(od -An -v -t x1 -j $offset -N 4 $bitmap | tr -d ' \\n') &&\n>> + size=$(($size * 8)) &&\n>> + offset=$(($offset + 4 + $size + 4)) &&\n>> + echo $offset\n>> +}\n>> +\n>> # Since name-hash values are stored in the .bitmap files, add a test\n>> # that checks that the name-hash calculations are stable across versions.\n>> # Not exhaustive, but these hashing algorithms would be hard to change\n>> @@ -486,6 +498,44 @@ test_bitmap_cases () {\n>> grep \"ignoring extra bitmap\" trace2.txt\n>> )\n>> '\n>> +\n>> + # A `.bitmap` file has the following structure:\n>> + # | Header | Commits | Trees | Blobs | Tags | Entries... |\n>> + #\n>> + # - The header is 32 bytes long when using SHA-1.\n>> + # - Commits, Trees, Blobs, and Tags are all stored as EWAH bitmaps.\n>> + #\n>> + # This test intentionally corrupts the `xor_offset` field of the first entry\n>> + # to verify robustness against malformed bitmap data.\n>> + test_expect_success 'load corrupt bitmap' '\n> \n> I am not totally following what this case is supposed to be testing.\n> Let me think aloud for a moment...\n> \n>> + rm -fr repo &&\n>> + git init repo &&\n>> + test_when_finished \"rm -fr repo\" &&\n>> + (\n>> + cd repo &&\n>> + git config pack.writeBitmapLookupTable '\"$writeLookupTable\"' &&\n> \n> First we set up a temporary repository, change into it, and enable\n> bitmap lookup tables. Makes sense.\n> \n>> + test_commit base &&\n>> +\n>> + git repack -adb &&\n>> + bitmap=\"$(ls .git/objects/pack/pack-*.bitmap)\" &&\n>> + chmod +w \"$bitmap\" &&\n> \n> Then we make a commit, and write a bitmap containing the objects from\n> the commit we just made. Good.\n> \n>> + hdr_sz=$((12 + $(test_oid rawsz))) &&\n>> + offset=$(skip_ewah_bitmap $bitmap $hdr_sz) &&\n>> + offset=$(skip_ewah_bitmap $bitmap $offset) &&\n>> + offset=$(skip_ewah_bitmap $bitmap $offset) &&\n>> + offset=$(skip_ewah_bitmap $bitmap $offset) &&\n> \n> Then we read past the header and four type bitmaps. Makes sense.\n> \n>> + offset=$((offset + 4)) &&\n> \n> Now we land at the bitmap for the commit we just wrote.\n> \n> (As an aside unrelated to this part of the test, this skip_ewah_bitmap()\n> function seems awfully fragile. I wonder if it would make more sense to\n> implement this as a test helper that can dump the offsets of EWAH\n> bitmaps in a *.bitmap file by object ID rather than trying to parse the\n> file ourselves?\n> \n\nI am actually replaying the pack-bitmap.c:prepare_bitmap() here. Also I have had\nwrite a test helper version once. And since I want to use prepare_bitmap()\nI have to put the code in pack-bitmap.c. It looks like this\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:\nreturn 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+ struct bitmap_index *bitmap_git;\n+ unsigned char *map;\n+\n+ if (!(bitmap_git = prepare_bitmap_git(r)))\n+ die(_(\"failed to prepare bitmap indexes\"));\n+ /*\n+ * If the table lookup extension is not used,\n+ * prepare_bitmap_git has already called load_bitmap_entries_v1(),\n+ * making it impossible to corrupt the bitmap.\n+ */\n+ if (!bitmap_git->table_lookup)\n+ return 0;\n+\n+ /*\n+ * bitmap_git->map is read-only;\n+ * to corrupt it, we need a writable memory block.\n+ */\n+ map = bitmap_git->map;\n+ bitmap_git->map = xmalloc(bitmap_git->map_size);\n+ if (!bitmap_git->map)\n+ return 0;\n+ memcpy(bitmap_git->map, map, bitmap_git->map_size);\n+\n+ do_corrupt(bitmap_git);\n+ if (!load_bitmap_entries_v1(bitmap_git))\n+ die(_(\"load corrupt bitmap successfully\"));\n+\n+ free(bitmap_git->map);\n+ bitmap_git->map = map;\n+ free_bitmap_index(bitmap_git);\n+\n+ return 0;\n+}\n+\n+static void do_corrupt_commit_pos(struct bitmap_index *bitmap_git)\n+{\n+ uint32_t *commit_pos_ptr;\n+\n+ commit_pos_ptr = (uint32_t *)(bitmap_git->map + bitmap_git->map_pos);\n+ *commit_pos_ptr = (uint32_t)-1;\n+}\n+\n+static void do_corrupt_xor_offset(struct bitmap_index *bitmap_git)\n+{\n+ uint8_t *xor_offset_ptr;\n+\n+ xor_offset_ptr = (uint8_t *)(bitmap_git->map + bitmap_git->map_pos +\n+ sizeof(uint32_t));\n+ *xor_offset_ptr = MAX_XOR_OFFSET + 1;\n+}\n+\n+int test_bitmap_load_corrupt(struct repository *r)\n+{\n+ int res = 0;\n+ if ((res = bitmap_corrupt_then_load(r, do_corrupt_commit_pos)))\n+ return res;\n+ if ((res = bitmap_corrupt_then_load(r, do_corrupt_xor_offset)))\n+ return res;\n+ return res;\n+}\n+\nint rebuild_bitmap(const uint32_t *reposition,\n struct ewah_bitmap *source,\n struct bitmap *dest)\n\n> We don't currently store an offset for each stored_bitmap that we\n> maintain, but doing so would be pretty straightforward (add it as a\n> field to the structure, and store the value of bitmap_git->map_pos from\n> immediately before reading the actual bitmap).)\n> \n>> + printf '\\161' |\n>> + dd of=$bitmap count=1 bs=1 conv=notrunc seek=$offset &&\n> \n> OK. Now we break the XOR offset field of this bitmap by writing garbage\n> into it.\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> ...and then we make sure that we still get the correct result.\n> \n> Hmmph. I don't think this is quite testing what we want, since this test\n> passes with or without your first patch. And that makes sense, we have\n> tests elsewhere in this script that verify we can still fall back to\n> classic traversal when the bitmap index can't be read. (For some\n> examples, see: \"truncated bitmap fails gracefully (ewah)\" and \"truncated\n> bitmap fails gracefully (cache)\".)\n\nI want to *test* for a memory leak here, not whether git can load a corrupt bitmap.\nSince git ci linux-leak test runs each test script with ASAN_OPTIONS=detect_leaks=1, I’m \nincluding this test case specifically to check whether it triggers a crash when \n`SANITIZE_LEAK` is enabled. And I do find if without the first patch, leak sanitizer\nrunning this test script would output error message.\n\n> I think what we're really testing here is the absence of a memory leak,\n> which we are as of 1fc7ddf35b (test-lib: unconditionally enable leak\n> checking, 2024-11-20). I wonder whether or not we need this test at all?\n> \n> Thanks,\n> Taylor\n\nI am not truly following what are you talking here. But If you think it’s unnecessary to\ncheck for potential leaks in load_bitmap() or load_bitmap_entries_v1(). Or this test\nscript shouldn’t be put in this way. I’m happy to drop the final patch.\n\nThanks\nLidong Yan"},{"id":"518660","messageId":"457DC23C-A052-416A-B181-A1EC48AD91A1@smail.nju.edu.cn","threadId":"63447","inReplyTo":"aC5nxa0uTb+ieiML@nand.local","subject":"Re: [PATCH v2 2/3] pack-bitmap: fix memory leak if `load_bitmap_entries_v1` failed","fromName":"lidongyan","fromEmail":"502024330056@smail.nju.edu.cn","sentAt":"2025-05-22T15:15:48Z","receivedAt":"2025-05-22T15:16:36Z","isPatch":true,"sender":{"key":"502024330056@smail.nju.edu.cn","avatar":"https://avatars.githubusercontent.com/u/77328395?v=4"},"body":"2025年5月22日 07:54，Taylor Blau 写道：\n> \n> On Tue, May 20, 2025 at 09:23:09AM +0000, Taylor Blau via GitGitGadget wrote:\n>> Signed-off-by: Taylor Blau \n> \n> This commit forges my Signed-off-by, but I am happy with the result\n> here.\n> \n> I do think the series is structured a little awkwardly as a result of\n> adding this patch to it. That this and the previous patch have the\n> subject \"pack-bitmap: fix memory leak if `load_bitmap_entries_v1`\n> failed\" make the series not quite as clear as it could be.\n> \n\nAgreed. I’ve definitely learned a lot about how to write commit messages\n and cover letters through this process\n\n> I think there are a couple of things going on:\n> \n> - This patch is a bug fix that could be applied independently of the\n> first one. The rationale there would be that we shouldn't be leaking\n> the EWAH bitmaps in 'b->bitmaps', but we are as a result of NULL'ing\n> the pointer in the \"failed\" label. That patch can stand alone.\n> \n> - The first patch (yours) is no longer fixing a leak, at least after\n> this patch. But it does delay reading the bitmap until we have\n> validated its XOR offset for sanity, which is a good thing mostly\n> from a performance perspective.\n> \n> I would probably swap the two patches around so that yours applies on\n> top of mine, and then rewords the patch message in yours to reflect that\n> it is no longer fixing a leak.\n> \n> That all said, if you feel strongly that the structure is fine/better\n> as-is, I'd be more than happy to discuss it further.\n> \n> Thanks,\n> Taylor\n> \n\nI think I can do this in third version, and I have to submit patch v3 after\nwe decide if patch v2 3/3 in this series should live or not. "},{"id":"518719","messageId":"xmqq4ixc49yp.fsf@gitster.g","threadId":"63447","inReplyTo":"aC5nxa0uTb+ieiML@nand.local","subject":"Re: [PATCH v2 2/3] pack-bitmap: fix memory leak if `load_bitmap_entries_v1` failed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-22T21:22:22Z","receivedAt":"2025-05-22T21:22:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> On Tue, May 20, 2025 at 09:23:09AM +0000, Taylor Blau via GitGitGadget wrote:\n>> Signed-off-by: Taylor Blau \n>\n> This commit forges my Signed-off-by, but I am happy with the result\n> here.\n>\n> I do think the series is structured a little awkwardly as a result of\n> adding this patch to it. That this and the previous patch have the\n> subject \"pack-bitmap: fix memory leak if `load_bitmap_entries_v1`\n> failed\" make the series not quite as clear as it could be.\n>\n> I think there are a couple of things going on:\n>\n> - This patch is a bug fix that could be applied independently of the\n> first one. The rationale there would be that we shouldn't be leaking\n> the EWAH bitmaps in 'b->bitmaps', but we are as a result of NULL'ing\n> the pointer in the \"failed\" label. That patch can stand alone.\n>\n> - The first patch (yours) is no longer fixing a leak, at least after\n> this patch. But it does delay reading the bitmap until we have\n> validated its XOR offset for sanity, which is a good thing mostly\n> from a performance perspective.\n>\n> I would probably swap the two patches around so that yours applies on\n> top of mine, and then rewords the patch message in yours to reflect that\n> it is no longer fixing a leak.\n\nSounds like a plausible structure.\n"},{"id":"518734","messageId":"aC/B21ZYCixgFSfe@nand.local","threadId":"63447","inReplyTo":"013153DA-8314-429B-8408-9A79A3304013@smail.nju.edu.cn","subject":"Re: [PATCH v2 3/3] pack-bitmap: add loading corrupt bitmap_index test","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-05-23T00:31:23Z","receivedAt":"2025-05-23T00:31:25Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, May 22, 2025 at 11:05:56PM +0800, lidongyan wrote:\n> > (As an aside unrelated to this part of the test, this skip_ewah_bitmap()\n> > function seems awfully fragile. I wonder if it would make more sense to\n> > implement this as a test helper that can dump the offsets of EWAH\n> > bitmaps in a *.bitmap file by object ID rather than trying to parse the\n> > file ourselves?\n> >\n>\n> I am actually replaying the pack-bitmap.c:prepare_bitmap() here. Also I have had\n> write a test helper version once. And since I want to use prepare_bitmap()\n> I have to put the code in pack-bitmap.c. It looks like this\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> [...]\n\nYeah, since the pack_bitmap struct is defined locally within the\npack-bitmap.c compilation unit, any test helper that performs any\nnon-trivial operation would likely need to be defined in that file.\n\nThe \"test helper\" code would be a little shim into the real\nfunctionality within pack-bitmap.c. See the following for an example:\n\n - t/helper/test-bitmap.c::bitmap_list_commits()\n - pack-bitmap.c::test_bitmap_commits()\n\nHere the former dispatches a single call to the latter, where all of the\nreal functionality is.\n\nBut the (elided) code below isn't quite what I was thinking. I think the\n\"write garbage data\" part is fine as-is and can continue to be written\nin shell. We have lots of examples of using dd to write garbage data\ninto files (see for e.g., the \"corrupt_data()\" function in t5319).\n\nWhat I was thinking is the test helper would print (via some new mode,\nor bolted onto \"list-commits\") line-delimited output like the following:\n\n $COMMIT_OID $BITMAP_OFFSET $FLAGS $XOR_OFFSET\n\nor similar. Then you could use the output of that to determine the\nlocation (replacing everything up to the actual \"printf | dd\nof=$bitmap ...\", which is the most fragile in my opinion).\n\n> > Hmmph. I don't think this is quite testing what we want, since this test\n> > passes with or without your first patch. And that makes sense, we have\n> > tests elsewhere in this script that verify we can still fall back to\n> > classic traversal when the bitmap index can't be read. (For some\n> > examples, see: \"truncated bitmap fails gracefully (ewah)\" and \"truncated\n> > bitmap fails gracefully (cache)\".)\n>\n> I want to *test* for a memory leak here, not whether git can load a corrupt bitmap.\n> Since git ci linux-leak test runs each test script with ASAN_OPTIONS=detect_leaks=1, I’m\n> including this test case specifically to check whether it triggers a crash when\n> `SANITIZE_LEAK` is enabled. And I do find if without the first patch, leak sanitizer\n> running this test script would output error message.\n\nMakes sense.\n\n> > I think what we're really testing here is the absence of a memory leak,\n> > which we are as of 1fc7ddf35b (test-lib: unconditionally enable leak\n> > checking, 2024-11-20). I wonder whether or not we need this test at all?\n> >\n> > Thanks,\n> > Taylor\n>\n> I am not truly following what are you talking here. But If you think it’s unnecessary to\n> check for potential leaks in load_bitmap() or load_bitmap_entries_v1(). Or this test\n> script shouldn’t be put in this way. I’m happy to drop the final patch.\n\nI think the above scenario (writing a test that would have leaked memory\notherwise behind a SANITIZE_LEAK prerequisite) is reasonable.\n\nThanks,\nTaylor\n"},{"id":"518750","messageId":"CD2E6414-76C3-4A0B-A625-C3146BEF2686@smail.nju.edu.cn","threadId":"63447","inReplyTo":"aC/B21ZYCixgFSfe@nand.local","subject":"Re: [PATCH v2 3/3] pack-bitmap: add loading corrupt bitmap_index test","fromName":"lidongyan","fromEmail":"502024330056@smail.nju.edu.cn","sentAt":"2025-05-23T07:17:05Z","receivedAt":"2025-05-23T07:17:47Z","isPatch":true,"sender":{"key":"502024330056@smail.nju.edu.cn","avatar":"https://avatars.githubusercontent.com/u/77328395?v=4"},"body":"2025年5月23日 08:31，Taylor Blau 写道：\n> But the (elided) code below isn't quite what I was thinking. I think the\n> \"write garbage data\" part is fine as-is and can continue to be written\n> in shell. We have lots of examples of using dd to write garbage data\n> into files (see for e.g., the \"corrupt_data()\" function in t5319).\n> \n> What I was thinking is the test helper would print (via some new mode,\n> or bolted onto \"list-commits\") line-delimited output like the following:\n> \n> $COMMIT_OID $BITMAP_OFFSET $FLAGS $XOR_OFFSET\n> \n> or similar. Then you could use the output of that to determine the\n> location (replacing everything up to the actual \"printf | dd\n> of=$bitmap ...\", which is the most fragile in my opinion).\n\nAgreed, I would add a `test-tool bitmap dump-entries` helper which dumps\nthe output you suggest.\n\n> I think the above scenario (writing a test that would have leaked memory\n> otherwise behind a SANITIZE_LEAK prerequisite) is reasonable.\n\nI will submit patch v3 with better structure and cover letter soon.\n\nThanks,\nLidong\n\n"},{"id":"518849","messageId":"pull.1962.v3.git.git.1748138764.gitgitgadget@gmail.com","threadId":"63447","inReplyTo":"pull.1962.v2.git.git.1747732991.gitgitgadget@gmail.com","subject":"[PATCH v3 0/2] pack-bitmap: fix memory leak if load_bitmap_entries_v1 failed","fromName":"Lidong Yan via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-05-25T02:06:02Z","receivedAt":"2025-05-25T02:06:08Z","isPatch":true,"sender":{"key":"yldhome2d2@gmail.com","avatar":"https://avatars.githubusercontent.com/u/77328395?v=4"},"body":"In pack-bitmap.c:load_bitmap_entries_v1, the function read_bitmap_1\nallocates a bitmap and reads index data into it. However, if any of the\nvalidation checks following the allocation fail, the allocated bitmap is not\nfreed, resulting in a memory leak. To avoid this, the validation checks\nshould be performed before the bitmap is allocated.\n\nLidong Yan (1):\n pack-bitmap: add load corrupt bitmap test\n\nTaylor Blau (1):\n pack-bitmap: fix memory leak if `load_bitmap_entries_v1` failed\n\n pack-bitmap.c | 94 +++++++++++++++++++++++++++++++----------\n pack-bitmap.h | 1 +\n t/helper/test-bitmap.c | 8 ++++\n t/t5310-pack-bitmaps.sh | 27 ++++++++++++\n 4 files changed, 107 insertions(+), 23 deletions(-)\n\n\nbase-commit: 845c48a16a7f7b2c44d8cb137b16a4a1f0140229\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1962%2Fbrandb97%2Ffix-pack-bitmap-leak-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1962/brandb97/fix-pack-bitmap-leak-v3\nPull-Request: https://github.com/git/git/pull/1962\n\nRange-diff vs v2:\n\n 1: 130c3dc5dcd < -: ----------- pack-bitmap: fix memory leak if `load_bitmap_entries_v1` failed\n 2: b515c278a8f = 1: cf87aad7c99 pack-bitmap: fix memory leak if `load_bitmap_entries_v1` failed\n 3: 5be22d563af ! 2: f5371d7daa9 pack-bitmap: add loading corrupt bitmap_index test\n @@ Metadata\n Author: Lidong Yan <502024330056@smail.nju.edu.cn>\n \n ## Commit message ##\n - pack-bitmap: add loading corrupt bitmap_index test\n + pack-bitmap: add load corrupt bitmap test\n \n - This patch add \"load corrupt bitmap\" test case in t5310-pack-bitmaps.sh.\n + This patch add test_bitmap_list_commits_offset() in patch-bitmap.c,\n + a new test helper command `test-tool bitmap list-commits-offset`,\n + and a `load corrupt bitmap` test case in t5310.\n \n - This test case intentionally corrupt the \"xor_offset\" field of the first\n - entry. To find position of first entry in *.bitmap, we need to skip 4\n - ewah_bitmaps before entries. And I add a function `skip_ewah_bitmap()`\n - to do this.\n + The `load corrupt bitmap` test case intentionally corrupt the\n + \"xor_offset\" field of the first entry. And the newly added helper\n + can help to find position of \"xor_offset\" in bitmap file.\n \n Signed-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>\n \n - ## t/t5310-pack-bitmaps.sh ##\n -@@ t/t5310-pack-bitmaps.sh: has_any () {\n - \tgrep -Ff \"$1\" \"$2\"\n + ## pack-bitmap.c ##\n +@@ pack-bitmap.c: struct stored_bitmap {\n + \tint flags;\n + };\n + \n ++struct stored_bitmap_tag_pos {\n ++\tstruct stored_bitmap stored;\n ++\tsize_t map_pos;\n ++};\n ++\n + /*\n + * The active bitmap index for a repository. By design, repositories only have\n + * a single bitmap index available (the index for the biggest packfile in\n +@@ pack-bitmap.c: static int existing_bitmaps_hits_nr;\n + static int existing_bitmaps_misses_nr;\n + static int roots_with_bitmaps_nr;\n + static int roots_without_bitmaps_nr;\n ++static int tag_pos_on_bitmap;\n + \n + static struct ewah_bitmap *lookup_stored_bitmap(struct stored_bitmap *st)\n + {\n +@@ pack-bitmap.c: static struct stored_bitmap *store_bitmap(struct bitmap_index *index,\n + \t\t\t\t\t struct ewah_bitmap *root,\n + \t\t\t\t\t const struct object_id *oid,\n + \t\t\t\t\t struct stored_bitmap *xor_with,\n +-\t\t\t\t\t int flags)\n ++\t\t\t\t\t int flags, size_t map_pos)\n + {\n + \tstruct stored_bitmap *stored;\n ++\tstruct stored_bitmap_tag_pos *tagged;\n + \tkhiter_t hash_pos;\n + \tint ret;\n + \n +-\tstored = xmalloc(sizeof(struct stored_bitmap));\n ++\ttagged = xmalloc(tag_pos_on_bitmap ? sizeof(struct stored_bitmap_tag_pos) :\n ++\t\t\t\t\t sizeof(struct stored_bitmap));\n ++\tstored = &tagged->stored;\n ++\tif (tag_pos_on_bitmap)\n ++\t\ttagged->map_pos = map_pos;\n + \tstored->root = root;\n + \tstored->xor = xor_with;\n + \tstored->flags = flags;\n +@@ pack-bitmap.c: static int load_bitmap_entries_v1(struct bitmap_index *index)\n + \t\tstruct stored_bitmap *xor_bitmap = NULL;\n + \t\tuint32_t commit_idx_pos;\n + \t\tstruct object_id oid;\n ++\t\tsize_t entry_map_pos;\n + \n + \t\tif (index->map_size - index->map_pos < 6)\n + \t\t\treturn error(_(\"corrupt ewah bitmap: truncated header for entry %d\"), i);\n + \n ++\t\tentry_map_pos = index->map_pos;\n + \t\tcommit_idx_pos = read_be32(index->map, &index->map_pos);\n + \t\txor_offset = read_u8(index->map, &index->map_pos);\n + \t\tflags = read_u8(index->map, &index->map_pos);\n +@@ pack-bitmap.c: static int load_bitmap_entries_v1(struct bitmap_index *index)\n + \t\tif (!bitmap)\n + \t\t\treturn -1;\n + \n +-\t\trecent_bitmaps[i % MAX_XOR_OFFSET] = store_bitmap(\n +-\t\t\tindex, bitmap, &oid, xor_bitmap, flags);\n ++\t\trecent_bitmaps[i % MAX_XOR_OFFSET] =\n ++\t\t\tstore_bitmap(index, bitmap, &oid, xor_bitmap, flags,\n ++\t\t\t\t entry_map_pos);\n + \t}\n + \n + \treturn 0;\n +@@ pack-bitmap.c: static struct stored_bitmap *lazy_bitmap_for_commit(struct bitmap_index *bitmap_\n + \tint xor_flags;\n + \tkhiter_t hash_pos;\n + \tstruct bitmap_lookup_table_xor_item *xor_item;\n ++\tsize_t entry_map_pos;\n + \n + \tif (is_corrupt)\n + \t\treturn NULL;\n +@@ pack-bitmap.c: static struct stored_bitmap *lazy_bitmap_for_commit(struct bitmap_index *bitmap_\n + \t\t\tgoto corrupt;\n + \t\t}\n + \n ++\t\tentry_map_pos = bitmap_git->map_pos;\n + \t\tbitmap_git->map_pos += sizeof(uint32_t) + sizeof(uint8_t);\n + \t\txor_flags = read_u8(bitmap_git->map, &bitmap_git->map_pos);\n + \t\tbitmap = read_bitmap_1(bitmap_git);\n +@@ pack-bitmap.c: static struct stored_bitmap *lazy_bitmap_for_commit(struct bitmap_index *bitmap_\n + \t\tif (!bitmap)\n + \t\t\tgoto corrupt;\n + \n +-\t\txor_bitmap = store_bitmap(bitmap_git, bitmap, &xor_item->oid, xor_bitmap, xor_flags);\n ++\t\txor_bitmap = store_bitmap(bitmap_git, bitmap, &xor_item->oid,\n ++\t\t\t\t\t xor_bitmap, xor_flags, entry_map_pos);\n + \t\txor_items_nr--;\n + \t}\n + \n +@@ pack-bitmap.c: static struct stored_bitmap *lazy_bitmap_for_commit(struct bitmap_index *bitmap_\n + \t * Instead, we can skip ahead and immediately read the flags and\n + \t * ewah bitmap.\n + \t */\n ++\tentry_map_pos = bitmap_git->map_pos;\n + \tbitmap_git->map_pos += sizeof(uint32_t) + sizeof(uint8_t);\n + \tflags = read_u8(bitmap_git->map, &bitmap_git->map_pos);\n + \tbitmap = read_bitmap_1(bitmap_git);\n +@@ pack-bitmap.c: static struct stored_bitmap *lazy_bitmap_for_commit(struct bitmap_index *bitmap_\n + \tif (!bitmap)\n + \t\tgoto corrupt;\n + \n +-\treturn store_bitmap(bitmap_git, bitmap, oid, xor_bitmap, flags);\n ++\treturn store_bitmap(bitmap_git, bitmap, oid, xor_bitmap, flags,\n ++\t\t\t entry_map_pos);\n + \n + corrupt:\n + \tfree(xor_items);\n +@@ pack-bitmap.c: int test_bitmap_commits(struct repository *r)\n + \treturn 0;\n }\n \n -+skip_ewah_bitmap() {\n -+\tlocal bitmap=\"$1\" &&\n -+\tlocal offset=\"$2\" &&\n -+\tlocal size= &&\n ++int test_bitmap_commits_offset(struct repository *r)\n ++{\n ++\tstruct object_id oid;\n ++\tstruct stored_bitmap_tag_pos *tagged;\n ++\tstruct bitmap_index *bitmap_git;\n ++\tsize_t commit_idx_pos_map_pos, xor_offset_map_pos, flag_map_pos,\n ++\t\tewah_bitmap_map_pos;\n ++\n ++\ttag_pos_on_bitmap = 1;\n ++\tbitmap_git = prepare_bitmap_git(r);\n ++\tif (!bitmap_git)\n ++\t\tdie(_(\"failed to load bitmap indexes\"));\n +\n -+\toffset=$(($offset + 4)) &&\n -+\tsize=0x$(od -An -v -t x1 -j $offset -N 4 $bitmap | tr -d ' \\n') &&\n -+\tsize=$(($size * 8)) &&\n -+\toffset=$(($offset + 4 + $size + 4)) &&\n -+\techo $offset\n ++\t/*\n ++\t * As this function is only used to print bitmap selected\n ++\t * commits, we don't have to read the commit table.\n ++\t */\n ++\tif (bitmap_git->table_lookup) {\n ++\t\tif (load_bitmap_entries_v1(bitmap_git) < 0)\n ++\t\t\tdie(_(\"failed to load bitmap indexes\"));\n ++\t}\n ++\n ++\tkh_foreach (bitmap_git->bitmaps, oid, tagged, {\n ++\t\tcommit_idx_pos_map_pos = tagged->map_pos;\n ++\t\txor_offset_map_pos = tagged->map_pos + sizeof(uint32_t);\n ++\t\tflag_map_pos = xor_offset_map_pos + sizeof(uint8_t);\n ++\t\tewah_bitmap_map_pos = flag_map_pos + sizeof(uint8_t);\n ++\n ++\t\tprintf_ln(\"%s %\"PRIuMAX\" %\"PRIuMAX\" %\"PRIuMAX\" %\"PRIuMAX,\n ++\t\t\t oid_to_hex(&oid),\n ++\t\t\t (uintmax_t)commit_idx_pos_map_pos,\n ++\t\t\t (uintmax_t)xor_offset_map_pos,\n ++\t\t\t (uintmax_t)flag_map_pos,\n ++\t\t\t (uintmax_t)ewah_bitmap_map_pos);\n ++\t})\n ++\t\t;\n ++\n ++\tfree_bitmap_index(bitmap_git);\n ++\n ++\treturn 0;\n ++}\n ++\n + int test_bitmap_hashes(struct repository *r)\n + {\n + \tstruct bitmap_index *bitmap_git = prepare_bitmap_git(r);\n +\n + ## pack-bitmap.h ##\n +@@ pack-bitmap.h: void traverse_bitmap_commit_list(struct bitmap_index *,\n + \t\t\t\t show_reachable_fn show_reachable);\n + void test_bitmap_walk(struct rev_info *revs);\n + int test_bitmap_commits(struct repository *r);\n ++int test_bitmap_commits_offset(struct repository *r);\n + 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 +\n + ## t/helper/test-bitmap.c ##\n +@@ t/helper/test-bitmap.c: static int bitmap_list_commits(void)\n + \treturn test_bitmap_commits(the_repository);\n + }\n + \n ++static int bitmap_list_commits_offset(void)\n ++{\n ++\treturn test_bitmap_commits_offset(the_repository);\n +}\n +\n - # Since name-hash values are stored in the .bitmap files, add a test\n - # that checks that the name-hash calculations are stable across versions.\n - # Not exhaustive, but these hashing algorithms would be hard to change\n + static int bitmap_dump_hashes(void)\n + {\n + \treturn test_bitmap_hashes(the_repository);\n +@@ t/helper/test-bitmap.c: int cmd__bitmap(int argc, const char **argv)\n + \n + \tif (argc == 2 && !strcmp(argv[1], \"list-commits\"))\n + \t\treturn bitmap_list_commits();\n ++\tif (argc == 2 && !strcmp(argv[1], \"list-commits-offset\"))\n ++\t\treturn bitmap_list_commits_offset();\n + \tif (argc == 2 && !strcmp(argv[1], \"dump-hashes\"))\n + \t\treturn bitmap_dump_hashes();\n + \tif (argc == 2 && !strcmp(argv[1], \"dump-pseudo-merges\"))\n +@@ t/helper/test-bitmap.c: int cmd__bitmap(int argc, const char **argv)\n + \t\treturn bitmap_dump_pseudo_merge_objects(atoi(argv[2]));\n + \n + \tusage(\"\\ttest-tool bitmap list-commits\\n\"\n ++\t \"\\ttest-tool bitmap list-commits-offset\\n\"\n + \t \"\\ttest-tool bitmap dump-hashes\\n\"\n + \t \"\\ttest-tool bitmap dump-pseudo-merges\\n\"\n + \t \"\\ttest-tool bitmap dump-pseudo-merge-commits \\n\"\n +\n + ## t/t5310-pack-bitmaps.sh ##\n @@ t/t5310-pack-bitmaps.sh: test_bitmap_cases () {\n \t\t\tgrep \"ignoring extra bitmap\" trace2.txt\n \t\t)\n \t'\n +\n -+\t# A `.bitmap` file has the following structure:\n -+\t# | Header | Commits | Trees | Blobs | Tags | Entries... |\n -+\t#\n -+\t# - The header is 32 bytes long when using SHA-1.\n -+\t# - Commits, Trees, Blobs, and Tags are all stored as EWAH bitmaps.\n -+\t#\n -+\t# This test intentionally corrupts the `xor_offset` field of the first entry\n -+\t# to verify robustness against malformed bitmap data.\n +\ttest_expect_success 'load corrupt bitmap' '\n +\t\trm -fr repo &&\n +\t\tgit init repo &&\n @@ t/t5310-pack-bitmaps.sh: test_bitmap_cases () {\n +\n +\t\t\tgit repack -adb &&\n +\t\t\tbitmap=\"$(ls .git/objects/pack/pack-*.bitmap)\" &&\n -+\t\t\tchmod +w \"$bitmap\" &&\n -+\n -+\t\t\thdr_sz=$((12 + $(test_oid rawsz))) &&\n -+\t\t\toffset=$(skip_ewah_bitmap $bitmap $hdr_sz) &&\n -+\t\t\toffset=$(skip_ewah_bitmap $bitmap $offset) &&\n -+\t\t\toffset=$(skip_ewah_bitmap $bitmap $offset) &&\n -+\t\t\toffset=$(skip_ewah_bitmap $bitmap $offset) &&\n -+\t\t\toffset=$((offset + 4)) &&\n ++\t\t\tchmod +w $bitmap &&\n +\n ++\t\t\tread oid commit_off xor_off flag_off ewah_off <<-EOF &&\n ++\t\t\t\t$(test-tool bitmap list-commits-offset | head -n 1)\n ++\t\t\tEOF\n +\t\t\tprintf '\\161' |\n -+\t\t\t\tdd of=$bitmap count=1 bs=1 conv=notrunc seek=$offset &&\n ++\t\t\t\tdd of=$bitmap count=1 bs=1 conv=notrunc seek=$xor_off &&\n ++\n +\n +\t\t\tgit rev-list --count HEAD > expect &&\n +\t\t\tgit rev-list --use-bitmap-index --count HEAD > actual &&\n\n-- \ngitgitgadget\n"},{"id":"518850","messageId":"cf87aad7c998995bfcb778eba26492614db1181a.1748138764.git.gitgitgadget@gmail.com","threadId":"63447","inReplyTo":"pull.1962.v3.git.git.1748138764.gitgitgadget@gmail.com","subject":"[PATCH v3 1/2] pack-bitmap: fix memory leak if `load_bitmap_entries_v1` failed","fromName":"Taylor Blau via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-05-25T02:06:03Z","receivedAt":"2025-05-25T02:06:09Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"From: Taylor Blau \n\nAfter going through the \"failed\" label, load_bitmap() will return -1,\nand its caller (either prepare_bitmap_walk() or prepare_bitmap_git())\nwill then call free_bitmap_index().\n\nThat function would have done:\n\n struct stored_bitmap *sb;\n kh_foreach_value(b->bitmaps, sb {\n ewah_pool_free(sb->root);\n free(sb);\n });\n\n, but won't since load_bitmap() already called kh_destroy_oid_map() and\nNULL'd the \"bitmaps\" pointer from within its \"failed\" label.\n\nSo I think if you got part of the way through loading bitmap entries and\nthen failed, you would leak all of the previous entries that you were\nable to load successfully.\n\nThe solution is to remove the error handling code in load_bitmap(), because\nits caller will always call free_bitmap_index() in case of an error.\n\nSigned-off-by: Taylor Blau \n---\n pack-bitmap.c | 21 ++++-----------------\n 1 file changed, 4 insertions(+), 17 deletions(-)\n\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex ac6d62b980c5..fd19c2255163 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -630,41 +630,28 @@ static int load_bitmap(struct repository *r, struct bitmap_index *bitmap_git,\n \tbitmap_git->ext_index.positions = kh_init_oid_pos();\n \n \tif (load_reverse_index(r, bitmap_git))\n-\t\tgoto failed;\n+\t\treturn -1;\n \n \tif (!(bitmap_git->commits = read_bitmap_1(bitmap_git)) ||\n \t\t!(bitmap_git->trees = read_bitmap_1(bitmap_git)) ||\n \t\t!(bitmap_git->blobs = read_bitmap_1(bitmap_git)) ||\n \t\t!(bitmap_git->tags = read_bitmap_1(bitmap_git)))\n-\t\tgoto failed;\n+\t\treturn -1;\n \n \tif (!bitmap_git->table_lookup && load_bitmap_entries_v1(bitmap_git) < 0)\n-\t\tgoto failed;\n+\t\treturn -1;\n \n \tif (bitmap_git->base) {\n \t\tif (!bitmap_is_midx(bitmap_git))\n \t\t\tBUG(\"non-MIDX bitmap has non-NULL base bitmap index\");\n \t\tif (load_bitmap(r, bitmap_git->base, 1) < 0)\n-\t\t\tgoto failed;\n+\t\t\treturn -1;\n \t}\n \n \tif (!recursing)\n \t\tload_all_type_bitmaps(bitmap_git);\n \n \treturn 0;\n-\n-failed:\n-\tmunmap(bitmap_git->map, bitmap_git->map_size);\n-\tbitmap_git->map = NULL;\n-\tbitmap_git->map_size = 0;\n-\n-\tkh_destroy_oid_map(bitmap_git->bitmaps);\n-\tbitmap_git->bitmaps = NULL;\n-\n-\tkh_destroy_oid_pos(bitmap_git->ext_index.positions);\n-\tbitmap_git->ext_index.positions = NULL;\n-\n-\treturn -1;\n }\n \n static int open_pack_bitmap(struct repository *r,\n-- \ngitgitgadget\n\n"},{"id":"518851","messageId":"f5371d7daa94242c13d98e2a003b9d05d4ef52a9.1748138764.git.gitgitgadget@gmail.com","threadId":"63447","inReplyTo":"pull.1962.v3.git.git.1748138764.gitgitgadget@gmail.com","subject":"[PATCH v3 2/2] pack-bitmap: add load corrupt bitmap test","fromName":"Lidong Yan via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-05-25T02:06:04Z","receivedAt":"2025-05-25T02:06:09Z","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 test_bitmap_list_commits_offset() in patch-bitmap.c,\na new test helper command `test-tool bitmap list-commits-offset`,\nand a `load corrupt bitmap` test case in t5310.\n\nThe `load corrupt bitmap` test case intentionally corrupt the\n\"xor_offset\" field of the first entry. And the newly added helper\ncan help to find position of \"xor_offset\" in bitmap file.\n\nSigned-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>\n---\n pack-bitmap.c | 73 +++++++++++++++++++++++++++++++++++++----\n pack-bitmap.h | 1 +\n t/helper/test-bitmap.c | 8 +++++\n t/t5310-pack-bitmaps.sh | 27 +++++++++++++++\n 4 files changed, 103 insertions(+), 6 deletions(-)\n\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex fd19c2255163..39c1c1bc4ce1 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -34,6 +34,11 @@ struct stored_bitmap {\n \tint flags;\n };\n \n+struct stored_bitmap_tag_pos {\n+\tstruct stored_bitmap stored;\n+\tsize_t map_pos;\n+};\n+\n /*\n * The active bitmap index for a repository. By design, repositories only have\n * a single bitmap index available (the index for the biggest packfile in\n@@ -148,6 +153,7 @@ static int existing_bitmaps_hits_nr;\n static int existing_bitmaps_misses_nr;\n static int roots_with_bitmaps_nr;\n static int roots_without_bitmaps_nr;\n+static int tag_pos_on_bitmap;\n \n static struct ewah_bitmap *lookup_stored_bitmap(struct stored_bitmap *st)\n {\n@@ -314,13 +320,18 @@ static struct stored_bitmap *store_bitmap(struct bitmap_index *index,\n \t\t\t\t\t struct ewah_bitmap *root,\n \t\t\t\t\t const struct object_id *oid,\n \t\t\t\t\t struct stored_bitmap *xor_with,\n-\t\t\t\t\t int flags)\n+\t\t\t\t\t int flags, size_t map_pos)\n {\n \tstruct stored_bitmap *stored;\n+\tstruct stored_bitmap_tag_pos *tagged;\n \tkhiter_t hash_pos;\n \tint ret;\n \n-\tstored = xmalloc(sizeof(struct stored_bitmap));\n+\ttagged = xmalloc(tag_pos_on_bitmap ? sizeof(struct stored_bitmap_tag_pos) :\n+\t\t\t\t\t sizeof(struct stored_bitmap));\n+\tstored = &tagged->stored;\n+\tif (tag_pos_on_bitmap)\n+\t\ttagged->map_pos = map_pos;\n \tstored->root = root;\n \tstored->xor = xor_with;\n \tstored->flags = flags;\n@@ -376,10 +387,12 @@ static int load_bitmap_entries_v1(struct bitmap_index *index)\n \t\tstruct stored_bitmap *xor_bitmap = NULL;\n \t\tuint32_t commit_idx_pos;\n \t\tstruct object_id oid;\n+\t\tsize_t entry_map_pos;\n \n \t\tif (index->map_size - index->map_pos < 6)\n \t\t\treturn error(_(\"corrupt ewah bitmap: truncated header for entry %d\"), i);\n \n+\t\tentry_map_pos = index->map_pos;\n \t\tcommit_idx_pos = read_be32(index->map, &index->map_pos);\n \t\txor_offset = read_u8(index->map, &index->map_pos);\n \t\tflags = read_u8(index->map, &index->map_pos);\n@@ -402,8 +415,9 @@ static int load_bitmap_entries_v1(struct bitmap_index *index)\n \t\tif (!bitmap)\n \t\t\treturn -1;\n \n-\t\trecent_bitmaps[i % MAX_XOR_OFFSET] = store_bitmap(\n-\t\t\tindex, bitmap, &oid, xor_bitmap, flags);\n+\t\trecent_bitmaps[i % MAX_XOR_OFFSET] =\n+\t\t\tstore_bitmap(index, bitmap, &oid, xor_bitmap, flags,\n+\t\t\t\t entry_map_pos);\n \t}\n \n \treturn 0;\n@@ -869,6 +883,7 @@ static struct stored_bitmap *lazy_bitmap_for_commit(struct bitmap_index *bitmap_\n \tint xor_flags;\n \tkhiter_t hash_pos;\n \tstruct bitmap_lookup_table_xor_item *xor_item;\n+\tsize_t entry_map_pos;\n \n \tif (is_corrupt)\n \t\treturn NULL;\n@@ -928,6 +943,7 @@ static struct stored_bitmap *lazy_bitmap_for_commit(struct bitmap_index *bitmap_\n \t\t\tgoto corrupt;\n \t\t}\n \n+\t\tentry_map_pos = bitmap_git->map_pos;\n \t\tbitmap_git->map_pos += sizeof(uint32_t) + sizeof(uint8_t);\n \t\txor_flags = read_u8(bitmap_git->map, &bitmap_git->map_pos);\n \t\tbitmap = read_bitmap_1(bitmap_git);\n@@ -935,7 +951,8 @@ static struct stored_bitmap *lazy_bitmap_for_commit(struct bitmap_index *bitmap_\n \t\tif (!bitmap)\n \t\t\tgoto corrupt;\n \n-\t\txor_bitmap = store_bitmap(bitmap_git, bitmap, &xor_item->oid, xor_bitmap, xor_flags);\n+\t\txor_bitmap = store_bitmap(bitmap_git, bitmap, &xor_item->oid,\n+\t\t\t\t\t xor_bitmap, xor_flags, entry_map_pos);\n \t\txor_items_nr--;\n \t}\n \n@@ -969,6 +986,7 @@ static struct stored_bitmap *lazy_bitmap_for_commit(struct bitmap_index *bitmap_\n \t * Instead, we can skip ahead and immediately read the flags and\n \t * ewah bitmap.\n \t */\n+\tentry_map_pos = bitmap_git->map_pos;\n \tbitmap_git->map_pos += sizeof(uint32_t) + sizeof(uint8_t);\n \tflags = read_u8(bitmap_git->map, &bitmap_git->map_pos);\n \tbitmap = read_bitmap_1(bitmap_git);\n@@ -976,7 +994,8 @@ static struct stored_bitmap *lazy_bitmap_for_commit(struct bitmap_index *bitmap_\n \tif (!bitmap)\n \t\tgoto corrupt;\n \n-\treturn store_bitmap(bitmap_git, bitmap, oid, xor_bitmap, flags);\n+\treturn store_bitmap(bitmap_git, bitmap, oid, xor_bitmap, flags,\n+\t\t\t entry_map_pos);\n \n corrupt:\n \tfree(xor_items);\n@@ -2856,6 +2875,48 @@ int test_bitmap_commits(struct repository *r)\n \treturn 0;\n }\n \n+int test_bitmap_commits_offset(struct repository *r)\n+{\n+\tstruct object_id oid;\n+\tstruct stored_bitmap_tag_pos *tagged;\n+\tstruct bitmap_index *bitmap_git;\n+\tsize_t commit_idx_pos_map_pos, xor_offset_map_pos, flag_map_pos,\n+\t\tewah_bitmap_map_pos;\n+\n+\ttag_pos_on_bitmap = 1;\n+\tbitmap_git = prepare_bitmap_git(r);\n+\tif (!bitmap_git)\n+\t\tdie(_(\"failed to load bitmap indexes\"));\n+\n+\t/*\n+\t * As this function is only used to print bitmap selected\n+\t * commits, we don't have to read the commit table.\n+\t */\n+\tif (bitmap_git->table_lookup) {\n+\t\tif (load_bitmap_entries_v1(bitmap_git) < 0)\n+\t\t\tdie(_(\"failed to load bitmap indexes\"));\n+\t}\n+\n+\tkh_foreach (bitmap_git->bitmaps, oid, tagged, {\n+\t\tcommit_idx_pos_map_pos = tagged->map_pos;\n+\t\txor_offset_map_pos = tagged->map_pos + sizeof(uint32_t);\n+\t\tflag_map_pos = xor_offset_map_pos + sizeof(uint8_t);\n+\t\tewah_bitmap_map_pos = flag_map_pos + sizeof(uint8_t);\n+\n+\t\tprintf_ln(\"%s %\"PRIuMAX\" %\"PRIuMAX\" %\"PRIuMAX\" %\"PRIuMAX,\n+\t\t\t oid_to_hex(&oid),\n+\t\t\t (uintmax_t)commit_idx_pos_map_pos,\n+\t\t\t (uintmax_t)xor_offset_map_pos,\n+\t\t\t (uintmax_t)flag_map_pos,\n+\t\t\t (uintmax_t)ewah_bitmap_map_pos);\n+\t})\n+\t\t;\n+\n+\tfree_bitmap_index(bitmap_git);\n+\n+\treturn 0;\n+}\n+\n int test_bitmap_hashes(struct repository *r)\n {\n \tstruct bitmap_index *bitmap_git = prepare_bitmap_git(r);\ndiff --git a/pack-bitmap.h b/pack-bitmap.h\nindex 382d39499af2..96880ba3d72d 100644\n--- a/pack-bitmap.h\n+++ b/pack-bitmap.h\n@@ -81,6 +81,7 @@ void traverse_bitmap_commit_list(struct bitmap_index *,\n \t\t\t\t show_reachable_fn show_reachable);\n void test_bitmap_walk(struct rev_info *revs);\n int test_bitmap_commits(struct repository *r);\n+int test_bitmap_commits_offset(struct repository *r);\n 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);\ndiff --git a/t/helper/test-bitmap.c b/t/helper/test-bitmap.c\nindex 3f23f2107268..65a1ab29192b 100644\n--- a/t/helper/test-bitmap.c\n+++ b/t/helper/test-bitmap.c\n@@ -10,6 +10,11 @@ static int bitmap_list_commits(void)\n \treturn test_bitmap_commits(the_repository);\n }\n \n+static int bitmap_list_commits_offset(void)\n+{\n+\treturn test_bitmap_commits_offset(the_repository);\n+}\n+\n static int bitmap_dump_hashes(void)\n {\n \treturn test_bitmap_hashes(the_repository);\n@@ -36,6 +41,8 @@ int cmd__bitmap(int argc, const char **argv)\n \n \tif (argc == 2 && !strcmp(argv[1], \"list-commits\"))\n \t\treturn bitmap_list_commits();\n+\tif (argc == 2 && !strcmp(argv[1], \"list-commits-offset\"))\n+\t\treturn bitmap_list_commits_offset();\n \tif (argc == 2 && !strcmp(argv[1], \"dump-hashes\"))\n \t\treturn bitmap_dump_hashes();\n \tif (argc == 2 && !strcmp(argv[1], \"dump-pseudo-merges\"))\n@@ -46,6 +53,7 @@ int cmd__bitmap(int argc, const char **argv)\n \t\treturn bitmap_dump_pseudo_merge_objects(atoi(argv[2]));\n \n \tusage(\"\\ttest-tool bitmap list-commits\\n\"\n+\t \"\\ttest-tool bitmap list-commits-offset\\n\"\n \t \"\\ttest-tool bitmap dump-hashes\\n\"\n \t \"\\ttest-tool bitmap dump-pseudo-merges\\n\"\n \t \"\\ttest-tool bitmap dump-pseudo-merge-commits \\n\"\ndiff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh\nindex a62b463eaf09..ef4c5fbaae83 100755\n--- a/t/t5310-pack-bitmaps.sh\n+++ b/t/t5310-pack-bitmaps.sh\n@@ -486,6 +486,33 @@ 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\trm -fr repo &&\n+\t\tgit init repo &&\n+\t\ttest_when_finished \"rm -fr repo\" &&\n+\t\t(\n+\t\t\tcd repo &&\n+\t\t\tgit config pack.writeBitmapLookupTable '\"$writeLookupTable\"' &&\n+\n+\t\t\ttest_commit base &&\n+\n+\t\t\tgit repack -adb &&\n+\t\t\tbitmap=\"$(ls .git/objects/pack/pack-*.bitmap)\" &&\n+\t\t\tchmod +w $bitmap &&\n+\n+\t\t\tread oid commit_off xor_off flag_off ewah_off <<-EOF &&\n+\t\t\t\t$(test-tool bitmap list-commits-offset | head -n 1)\n+\t\t\tEOF\n+\t\t\tprintf '\\161' |\n+\t\t\t\tdd of=$bitmap count=1 bs=1 conv=notrunc seek=$xor_off &&\n+\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 test_bitmap_cases\n-- \ngitgitgadget\n"},{"id":"518852","messageId":"pull.1962.v4.git.git.1748140983.gitgitgadget@gmail.com","threadId":"63447","inReplyTo":"pull.1962.v3.git.git.1748138764.gitgitgadget@gmail.com","subject":"[PATCH v4 0/2] pack-bitmap: fix memory leak if load_bitmap failed","fromName":"Lidong Yan via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-05-25T02:43:01Z","receivedAt":"2025-05-25T02:43:06Z","isPatch":true,"sender":{"key":"yldhome2d2@gmail.com","avatar":"https://avatars.githubusercontent.com/u/77328395?v=4"},"body":"This patch prevents pack-bitmap.c:load_bitmap() from nulling\nbitmap_git->bitmap when loading failed thus eliminates memory leak. This\npatch also add a test case in t5310 which use clang leak sanitizer to detect\nwhether leak happens when loading failed.\n\nLidong Yan (1):\n pack-bitmap: add load corrupt bitmap test\n\nTaylor Blau (1):\n pack-bitmap: fix memory leak if load_bitmap() failed\n\n pack-bitmap.c | 94 +++++++++++++++++++++++++++++++----------\n pack-bitmap.h | 1 +\n t/helper/test-bitmap.c | 8 ++++\n t/t5310-pack-bitmaps.sh | 27 ++++++++++++\n 4 files changed, 107 insertions(+), 23 deletions(-)\n\n\nbase-commit: 845c48a16a7f7b2c44d8cb137b16a4a1f0140229\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1962%2Fbrandb97%2Ffix-pack-bitmap-leak-v4\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1962/brandb97/fix-pack-bitmap-leak-v4\nPull-Request: https://github.com/git/git/pull/1962\n\nRange-diff vs v3:\n\n 1: cf87aad7c99 ! 1: b6b3a83a224 pack-bitmap: fix memory leak if `load_bitmap_entries_v1` failed\n @@ Metadata\n Author: Taylor Blau \n \n ## Commit message ##\n - pack-bitmap: fix memory leak if `load_bitmap_entries_v1` failed\n + pack-bitmap: fix memory leak if load_bitmap() failed\n \n After going through the \"failed\" label, load_bitmap() will return -1,\n and its caller (either prepare_bitmap_walk() or prepare_bitmap_git())\n 2: f5371d7daa9 = 2: 7876d9a9014 pack-bitmap: add load corrupt bitmap test\n\n-- \ngitgitgadget\n"},{"id":"518853","messageId":"b6b3a83a22486d0c104c494d1950fdaa2f2a658c.1748140983.git.gitgitgadget@gmail.com","threadId":"63447","inReplyTo":"pull.1962.v4.git.git.1748140983.gitgitgadget@gmail.com","subject":"[PATCH v4 1/2] pack-bitmap: fix memory leak if load_bitmap() failed","fromName":"Taylor Blau via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-05-25T02:43:02Z","receivedAt":"2025-05-25T02:43:07Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"From: Taylor Blau \n\nAfter going through the \"failed\" label, load_bitmap() will return -1,\nand its caller (either prepare_bitmap_walk() or prepare_bitmap_git())\nwill then call free_bitmap_index().\n\nThat function would have done:\n\n struct stored_bitmap *sb;\n kh_foreach_value(b->bitmaps, sb {\n ewah_pool_free(sb->root);\n free(sb);\n });\n\n, but won't since load_bitmap() already called kh_destroy_oid_map() and\nNULL'd the \"bitmaps\" pointer from within its \"failed\" label.\n\nSo I think if you got part of the way through loading bitmap entries and\nthen failed, you would leak all of the previous entries that you were\nable to load successfully.\n\nThe solution is to remove the error handling code in load_bitmap(), because\nits caller will always call free_bitmap_index() in case of an error.\n\nSigned-off-by: Taylor Blau \n---\n pack-bitmap.c | 21 ++++-----------------\n 1 file changed, 4 insertions(+), 17 deletions(-)\n\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex ac6d62b980c5..fd19c2255163 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -630,41 +630,28 @@ static int load_bitmap(struct repository *r, struct bitmap_index *bitmap_git,\n \tbitmap_git->ext_index.positions = kh_init_oid_pos();\n \n \tif (load_reverse_index(r, bitmap_git))\n-\t\tgoto failed;\n+\t\treturn -1;\n \n \tif (!(bitmap_git->commits = read_bitmap_1(bitmap_git)) ||\n \t\t!(bitmap_git->trees = read_bitmap_1(bitmap_git)) ||\n \t\t!(bitmap_git->blobs = read_bitmap_1(bitmap_git)) ||\n \t\t!(bitmap_git->tags = read_bitmap_1(bitmap_git)))\n-\t\tgoto failed;\n+\t\treturn -1;\n \n \tif (!bitmap_git->table_lookup && load_bitmap_entries_v1(bitmap_git) < 0)\n-\t\tgoto failed;\n+\t\treturn -1;\n \n \tif (bitmap_git->base) {\n \t\tif (!bitmap_is_midx(bitmap_git))\n \t\t\tBUG(\"non-MIDX bitmap has non-NULL base bitmap index\");\n \t\tif (load_bitmap(r, bitmap_git->base, 1) < 0)\n-\t\t\tgoto failed;\n+\t\t\treturn -1;\n \t}\n \n \tif (!recursing)\n \t\tload_all_type_bitmaps(bitmap_git);\n \n \treturn 0;\n-\n-failed:\n-\tmunmap(bitmap_git->map, bitmap_git->map_size);\n-\tbitmap_git->map = NULL;\n-\tbitmap_git->map_size = 0;\n-\n-\tkh_destroy_oid_map(bitmap_git->bitmaps);\n-\tbitmap_git->bitmaps = NULL;\n-\n-\tkh_destroy_oid_pos(bitmap_git->ext_index.positions);\n-\tbitmap_git->ext_index.positions = NULL;\n-\n-\treturn -1;\n }\n \n static int open_pack_bitmap(struct repository *r,\n-- \ngitgitgadget\n\n"},{"id":"518854","messageId":"7876d9a9014ea6a0657f440f7fa1efd496a4a15a.1748140983.git.gitgitgadget@gmail.com","threadId":"63447","inReplyTo":"pull.1962.v4.git.git.1748140983.gitgitgadget@gmail.com","subject":"[PATCH v4 2/2] pack-bitmap: add load corrupt bitmap test","fromName":"Lidong Yan via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-05-25T02:43:03Z","receivedAt":"2025-05-25T02:43:11Z","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 test_bitmap_list_commits_offset() in patch-bitmap.c,\na new test helper command `test-tool bitmap list-commits-offset`,\nand a `load corrupt bitmap` test case in t5310.\n\nThe `load corrupt bitmap` test case intentionally corrupt the\n\"xor_offset\" field of the first entry. And the newly added helper\ncan help to find position of \"xor_offset\" in bitmap file.\n\nSigned-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>\n---\n pack-bitmap.c | 73 +++++++++++++++++++++++++++++++++++++----\n pack-bitmap.h | 1 +\n t/helper/test-bitmap.c | 8 +++++\n t/t5310-pack-bitmaps.sh | 27 +++++++++++++++\n 4 files changed, 103 insertions(+), 6 deletions(-)\n\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex fd19c2255163..39c1c1bc4ce1 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -34,6 +34,11 @@ struct stored_bitmap {\n \tint flags;\n };\n \n+struct stored_bitmap_tag_pos {\n+\tstruct stored_bitmap stored;\n+\tsize_t map_pos;\n+};\n+\n /*\n * The active bitmap index for a repository. By design, repositories only have\n * a single bitmap index available (the index for the biggest packfile in\n@@ -148,6 +153,7 @@ static int existing_bitmaps_hits_nr;\n static int existing_bitmaps_misses_nr;\n static int roots_with_bitmaps_nr;\n static int roots_without_bitmaps_nr;\n+static int tag_pos_on_bitmap;\n \n static struct ewah_bitmap *lookup_stored_bitmap(struct stored_bitmap *st)\n {\n@@ -314,13 +320,18 @@ static struct stored_bitmap *store_bitmap(struct bitmap_index *index,\n \t\t\t\t\t struct ewah_bitmap *root,\n \t\t\t\t\t const struct object_id *oid,\n \t\t\t\t\t struct stored_bitmap *xor_with,\n-\t\t\t\t\t int flags)\n+\t\t\t\t\t int flags, size_t map_pos)\n {\n \tstruct stored_bitmap *stored;\n+\tstruct stored_bitmap_tag_pos *tagged;\n \tkhiter_t hash_pos;\n \tint ret;\n \n-\tstored = xmalloc(sizeof(struct stored_bitmap));\n+\ttagged = xmalloc(tag_pos_on_bitmap ? sizeof(struct stored_bitmap_tag_pos) :\n+\t\t\t\t\t sizeof(struct stored_bitmap));\n+\tstored = &tagged->stored;\n+\tif (tag_pos_on_bitmap)\n+\t\ttagged->map_pos = map_pos;\n \tstored->root = root;\n \tstored->xor = xor_with;\n \tstored->flags = flags;\n@@ -376,10 +387,12 @@ static int load_bitmap_entries_v1(struct bitmap_index *index)\n \t\tstruct stored_bitmap *xor_bitmap = NULL;\n \t\tuint32_t commit_idx_pos;\n \t\tstruct object_id oid;\n+\t\tsize_t entry_map_pos;\n \n \t\tif (index->map_size - index->map_pos < 6)\n \t\t\treturn error(_(\"corrupt ewah bitmap: truncated header for entry %d\"), i);\n \n+\t\tentry_map_pos = index->map_pos;\n \t\tcommit_idx_pos = read_be32(index->map, &index->map_pos);\n \t\txor_offset = read_u8(index->map, &index->map_pos);\n \t\tflags = read_u8(index->map, &index->map_pos);\n@@ -402,8 +415,9 @@ static int load_bitmap_entries_v1(struct bitmap_index *index)\n \t\tif (!bitmap)\n \t\t\treturn -1;\n \n-\t\trecent_bitmaps[i % MAX_XOR_OFFSET] = store_bitmap(\n-\t\t\tindex, bitmap, &oid, xor_bitmap, flags);\n+\t\trecent_bitmaps[i % MAX_XOR_OFFSET] =\n+\t\t\tstore_bitmap(index, bitmap, &oid, xor_bitmap, flags,\n+\t\t\t\t entry_map_pos);\n \t}\n \n \treturn 0;\n@@ -869,6 +883,7 @@ static struct stored_bitmap *lazy_bitmap_for_commit(struct bitmap_index *bitmap_\n \tint xor_flags;\n \tkhiter_t hash_pos;\n \tstruct bitmap_lookup_table_xor_item *xor_item;\n+\tsize_t entry_map_pos;\n \n \tif (is_corrupt)\n \t\treturn NULL;\n@@ -928,6 +943,7 @@ static struct stored_bitmap *lazy_bitmap_for_commit(struct bitmap_index *bitmap_\n \t\t\tgoto corrupt;\n \t\t}\n \n+\t\tentry_map_pos = bitmap_git->map_pos;\n \t\tbitmap_git->map_pos += sizeof(uint32_t) + sizeof(uint8_t);\n \t\txor_flags = read_u8(bitmap_git->map, &bitmap_git->map_pos);\n \t\tbitmap = read_bitmap_1(bitmap_git);\n@@ -935,7 +951,8 @@ static struct stored_bitmap *lazy_bitmap_for_commit(struct bitmap_index *bitmap_\n \t\tif (!bitmap)\n \t\t\tgoto corrupt;\n \n-\t\txor_bitmap = store_bitmap(bitmap_git, bitmap, &xor_item->oid, xor_bitmap, xor_flags);\n+\t\txor_bitmap = store_bitmap(bitmap_git, bitmap, &xor_item->oid,\n+\t\t\t\t\t xor_bitmap, xor_flags, entry_map_pos);\n \t\txor_items_nr--;\n \t}\n \n@@ -969,6 +986,7 @@ static struct stored_bitmap *lazy_bitmap_for_commit(struct bitmap_index *bitmap_\n \t * Instead, we can skip ahead and immediately read the flags and\n \t * ewah bitmap.\n \t */\n+\tentry_map_pos = bitmap_git->map_pos;\n \tbitmap_git->map_pos += sizeof(uint32_t) + sizeof(uint8_t);\n \tflags = read_u8(bitmap_git->map, &bitmap_git->map_pos);\n \tbitmap = read_bitmap_1(bitmap_git);\n@@ -976,7 +994,8 @@ static struct stored_bitmap *lazy_bitmap_for_commit(struct bitmap_index *bitmap_\n \tif (!bitmap)\n \t\tgoto corrupt;\n \n-\treturn store_bitmap(bitmap_git, bitmap, oid, xor_bitmap, flags);\n+\treturn store_bitmap(bitmap_git, bitmap, oid, xor_bitmap, flags,\n+\t\t\t entry_map_pos);\n \n corrupt:\n \tfree(xor_items);\n@@ -2856,6 +2875,48 @@ int test_bitmap_commits(struct repository *r)\n \treturn 0;\n }\n \n+int test_bitmap_commits_offset(struct repository *r)\n+{\n+\tstruct object_id oid;\n+\tstruct stored_bitmap_tag_pos *tagged;\n+\tstruct bitmap_index *bitmap_git;\n+\tsize_t commit_idx_pos_map_pos, xor_offset_map_pos, flag_map_pos,\n+\t\tewah_bitmap_map_pos;\n+\n+\ttag_pos_on_bitmap = 1;\n+\tbitmap_git = prepare_bitmap_git(r);\n+\tif (!bitmap_git)\n+\t\tdie(_(\"failed to load bitmap indexes\"));\n+\n+\t/*\n+\t * As this function is only used to print bitmap selected\n+\t * commits, we don't have to read the commit table.\n+\t */\n+\tif (bitmap_git->table_lookup) {\n+\t\tif (load_bitmap_entries_v1(bitmap_git) < 0)\n+\t\t\tdie(_(\"failed to load bitmap indexes\"));\n+\t}\n+\n+\tkh_foreach (bitmap_git->bitmaps, oid, tagged, {\n+\t\tcommit_idx_pos_map_pos = tagged->map_pos;\n+\t\txor_offset_map_pos = tagged->map_pos + sizeof(uint32_t);\n+\t\tflag_map_pos = xor_offset_map_pos + sizeof(uint8_t);\n+\t\tewah_bitmap_map_pos = flag_map_pos + sizeof(uint8_t);\n+\n+\t\tprintf_ln(\"%s %\"PRIuMAX\" %\"PRIuMAX\" %\"PRIuMAX\" %\"PRIuMAX,\n+\t\t\t oid_to_hex(&oid),\n+\t\t\t (uintmax_t)commit_idx_pos_map_pos,\n+\t\t\t (uintmax_t)xor_offset_map_pos,\n+\t\t\t (uintmax_t)flag_map_pos,\n+\t\t\t (uintmax_t)ewah_bitmap_map_pos);\n+\t})\n+\t\t;\n+\n+\tfree_bitmap_index(bitmap_git);\n+\n+\treturn 0;\n+}\n+\n int test_bitmap_hashes(struct repository *r)\n {\n \tstruct bitmap_index *bitmap_git = prepare_bitmap_git(r);\ndiff --git a/pack-bitmap.h b/pack-bitmap.h\nindex 382d39499af2..96880ba3d72d 100644\n--- a/pack-bitmap.h\n+++ b/pack-bitmap.h\n@@ -81,6 +81,7 @@ void traverse_bitmap_commit_list(struct bitmap_index *,\n \t\t\t\t show_reachable_fn show_reachable);\n void test_bitmap_walk(struct rev_info *revs);\n int test_bitmap_commits(struct repository *r);\n+int test_bitmap_commits_offset(struct repository *r);\n 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);\ndiff --git a/t/helper/test-bitmap.c b/t/helper/test-bitmap.c\nindex 3f23f2107268..65a1ab29192b 100644\n--- a/t/helper/test-bitmap.c\n+++ b/t/helper/test-bitmap.c\n@@ -10,6 +10,11 @@ static int bitmap_list_commits(void)\n \treturn test_bitmap_commits(the_repository);\n }\n \n+static int bitmap_list_commits_offset(void)\n+{\n+\treturn test_bitmap_commits_offset(the_repository);\n+}\n+\n static int bitmap_dump_hashes(void)\n {\n \treturn test_bitmap_hashes(the_repository);\n@@ -36,6 +41,8 @@ int cmd__bitmap(int argc, const char **argv)\n \n \tif (argc == 2 && !strcmp(argv[1], \"list-commits\"))\n \t\treturn bitmap_list_commits();\n+\tif (argc == 2 && !strcmp(argv[1], \"list-commits-offset\"))\n+\t\treturn bitmap_list_commits_offset();\n \tif (argc == 2 && !strcmp(argv[1], \"dump-hashes\"))\n \t\treturn bitmap_dump_hashes();\n \tif (argc == 2 && !strcmp(argv[1], \"dump-pseudo-merges\"))\n@@ -46,6 +53,7 @@ int cmd__bitmap(int argc, const char **argv)\n \t\treturn bitmap_dump_pseudo_merge_objects(atoi(argv[2]));\n \n \tusage(\"\\ttest-tool bitmap list-commits\\n\"\n+\t \"\\ttest-tool bitmap list-commits-offset\\n\"\n \t \"\\ttest-tool bitmap dump-hashes\\n\"\n \t \"\\ttest-tool bitmap dump-pseudo-merges\\n\"\n \t \"\\ttest-tool bitmap dump-pseudo-merge-commits \\n\"\ndiff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh\nindex a62b463eaf09..ef4c5fbaae83 100755\n--- a/t/t5310-pack-bitmaps.sh\n+++ b/t/t5310-pack-bitmaps.sh\n@@ -486,6 +486,33 @@ 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\trm -fr repo &&\n+\t\tgit init repo &&\n+\t\ttest_when_finished \"rm -fr repo\" &&\n+\t\t(\n+\t\t\tcd repo &&\n+\t\t\tgit config pack.writeBitmapLookupTable '\"$writeLookupTable\"' &&\n+\n+\t\t\ttest_commit base &&\n+\n+\t\t\tgit repack -adb &&\n+\t\t\tbitmap=\"$(ls .git/objects/pack/pack-*.bitmap)\" &&\n+\t\t\tchmod +w $bitmap &&\n+\n+\t\t\tread oid commit_off xor_off flag_off ewah_off <<-EOF &&\n+\t\t\t\t$(test-tool bitmap list-commits-offset | head -n 1)\n+\t\t\tEOF\n+\t\t\tprintf '\\161' |\n+\t\t\t\tdd of=$bitmap count=1 bs=1 conv=notrunc seek=$xor_off &&\n+\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 test_bitmap_cases\n-- \ngitgitgadget\n"},{"id":"519145","messageId":"xmqqjz5zmnxy.fsf@gitster.g","threadId":"63447","inReplyTo":"b6b3a83a22486d0c104c494d1950fdaa2f2a658c.1748140983.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v4 1/2] pack-bitmap: fix memory leak if load_bitmap() failed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-29T15:33:29Z","receivedAt":"2025-05-29T15:33:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Taylor Blau via GitGitGadget\" writes:\n\n> From: Taylor Blau \n>\n> After going through the \"failed\" label, load_bitmap() will return -1,\n> and its caller (either prepare_bitmap_walk() or prepare_bitmap_git())\n> will then call free_bitmap_index().\n> ...\n> The solution is to remove the error handling code in load_bitmap(), because\n> its caller will always call free_bitmap_index() in case of an error.\n>\n> Signed-off-by: Taylor Blau \n> ---\n\nAs this is Lidong relaying that Taylor\nsent to the list, shouldn't Lidong's sign-off be after Taylor's?\n"},{"id":"519146","messageId":"xmqqbjrbmndn.fsf@gitster.g","threadId":"63447","inReplyTo":"7876d9a9014ea6a0657f440f7fa1efd496a4a15a.1748140983.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v4 2/2] pack-bitmap: add load corrupt bitmap test","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-29T15:45:40Z","receivedAt":"2025-05-29T15:45:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Lidong Yan via GitGitGadget\" writes:\n\n> From: Lidong Yan <502024330056@smail.nju.edu.cn>\n>\n> This patch add test_bitmap_list_commits_offset() in patch-bitmap.c,\n\n\"pack-bitmap.c\"?\n\n> a new test helper command `test-tool bitmap list-commits-offset`,\n> and a `load corrupt bitmap` test case in t5310.\n>\n> The `load corrupt bitmap` test case intentionally corrupt the\n> \"xor_offset\" field of the first entry. And the newly added helper\n> can help to find position of \"xor_offset\" in bitmap file.\n\n[the structure of a log message]\n\nThe usual way to compose a log message of this project is to\n\n - Give an observation on how the current system works in the\n present tense (so no need to say \"Currently X is Y\", or\n \"Previously X was Y\" to describe the state before your change;\n just \"X is Y\" is enough), and discuss what you perceive as a\n problem in it.\n\n - Propose a solution (optional---often, problem description\n trivially leads to an obvious solution in reader's minds).\n\n - Give commands to somebody editing the codebase to \"make it so\".\n\nin this order.\n\nThe proposed log message lacks the motivation and only talks about\nwhat the patch does. We add a test-only code in a file, intermixed\nwith production code. Let's explain why it is the best arrangement.\n\n> Signed-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>\n> ---\n> pack-bitmap.c | 73 +++++++++++++++++++++++++++++++++++++----\n> pack-bitmap.h | 1 +\n> t/helper/test-bitmap.c | 8 +++++\n> t/t5310-pack-bitmaps.sh | 27 +++++++++++++++\n> 4 files changed, 103 insertions(+), 6 deletions(-)\n\nAfter the second round of the series, no review comments seem to\nhave been sent to the list. Is everybody happy with the latest\niteration?\n\nThanks.\n\n> diff --git a/pack-bitmap.c b/pack-bitmap.c\n> index fd19c2255163..39c1c1bc4ce1 100644\n> --- a/pack-bitmap.c\n> +++ b/pack-bitmap.c\n> @@ -34,6 +34,11 @@ struct stored_bitmap {\n> \tint flags;\n> };\n> \n> +struct stored_bitmap_tag_pos {\n> +\tstruct stored_bitmap stored;\n> +\tsize_t map_pos;\n> +};\n> +\n> /*\n> * The active bitmap index for a repository. By design, repositories only have\n> * a single bitmap index available (the index for the biggest packfile in\n> @@ -148,6 +153,7 @@ static int existing_bitmaps_hits_nr;\n> static int existing_bitmaps_misses_nr;\n> static int roots_with_bitmaps_nr;\n> static int roots_without_bitmaps_nr;\n> +static int tag_pos_on_bitmap;\n> \n> static struct ewah_bitmap *lookup_stored_bitmap(struct stored_bitmap *st)\n> {\n> @@ -314,13 +320,18 @@ static struct stored_bitmap *store_bitmap(struct bitmap_index *index,\n> \t\t\t\t\t struct ewah_bitmap *root,\n> \t\t\t\t\t const struct object_id *oid,\n> \t\t\t\t\t struct stored_bitmap *xor_with,\n> -\t\t\t\t\t int flags)\n> +\t\t\t\t\t int flags, size_t map_pos)\n> {\n> \tstruct stored_bitmap *stored;\n> +\tstruct stored_bitmap_tag_pos *tagged;\n> \tkhiter_t hash_pos;\n> \tint ret;\n> \n> -\tstored = xmalloc(sizeof(struct stored_bitmap));\n> +\ttagged = xmalloc(tag_pos_on_bitmap ? sizeof(struct stored_bitmap_tag_pos) :\n> +\t\t\t\t\t sizeof(struct stored_bitmap));\n> +\tstored = &tagged->stored;\n> +\tif (tag_pos_on_bitmap)\n> +\t\ttagged->map_pos = map_pos;\n> \tstored->root = root;\n> \tstored->xor = xor_with;\n> \tstored->flags = flags;\n> @@ -376,10 +387,12 @@ static int load_bitmap_entries_v1(struct bitmap_index *index)\n> \t\tstruct stored_bitmap *xor_bitmap = NULL;\n> \t\tuint32_t commit_idx_pos;\n> \t\tstruct object_id oid;\n> +\t\tsize_t entry_map_pos;\n> \n> \t\tif (index->map_size - index->map_pos < 6)\n> \t\t\treturn error(_(\"corrupt ewah bitmap: truncated header for entry %d\"), i);\n> \n> +\t\tentry_map_pos = index->map_pos;\n> \t\tcommit_idx_pos = read_be32(index->map, &index->map_pos);\n> \t\txor_offset = read_u8(index->map, &index->map_pos);\n> \t\tflags = read_u8(index->map, &index->map_pos);\n> @@ -402,8 +415,9 @@ static int load_bitmap_entries_v1(struct bitmap_index *index)\n> \t\tif (!bitmap)\n> \t\t\treturn -1;\n> \n> -\t\trecent_bitmaps[i % MAX_XOR_OFFSET] = store_bitmap(\n> -\t\t\tindex, bitmap, &oid, xor_bitmap, flags);\n> +\t\trecent_bitmaps[i % MAX_XOR_OFFSET] =\n> +\t\t\tstore_bitmap(index, bitmap, &oid, xor_bitmap, flags,\n> +\t\t\t\t entry_map_pos);\n> \t}\n> \n> \treturn 0;\n> @@ -869,6 +883,7 @@ static struct stored_bitmap *lazy_bitmap_for_commit(struct bitmap_index *bitmap_\n> \tint xor_flags;\n> \tkhiter_t hash_pos;\n> \tstruct bitmap_lookup_table_xor_item *xor_item;\n> +\tsize_t entry_map_pos;\n> \n> \tif (is_corrupt)\n> \t\treturn NULL;\n> @@ -928,6 +943,7 @@ static struct stored_bitmap *lazy_bitmap_for_commit(struct bitmap_index *bitmap_\n> \t\t\tgoto corrupt;\n> \t\t}\n> \n> +\t\tentry_map_pos = bitmap_git->map_pos;\n> \t\tbitmap_git->map_pos += sizeof(uint32_t) + sizeof(uint8_t);\n> \t\txor_flags = read_u8(bitmap_git->map, &bitmap_git->map_pos);\n> \t\tbitmap = read_bitmap_1(bitmap_git);\n> @@ -935,7 +951,8 @@ static struct stored_bitmap *lazy_bitmap_for_commit(struct bitmap_index *bitmap_\n> \t\tif (!bitmap)\n> \t\t\tgoto corrupt;\n> \n> -\t\txor_bitmap = store_bitmap(bitmap_git, bitmap, &xor_item->oid, xor_bitmap, xor_flags);\n> +\t\txor_bitmap = store_bitmap(bitmap_git, bitmap, &xor_item->oid,\n> +\t\t\t\t\t xor_bitmap, xor_flags, entry_map_pos);\n> \t\txor_items_nr--;\n> \t}\n> \n> @@ -969,6 +986,7 @@ static struct stored_bitmap *lazy_bitmap_for_commit(struct bitmap_index *bitmap_\n> \t * Instead, we can skip ahead and immediately read the flags and\n> \t * ewah bitmap.\n> \t */\n> +\tentry_map_pos = bitmap_git->map_pos;\n> \tbitmap_git->map_pos += sizeof(uint32_t) + sizeof(uint8_t);\n> \tflags = read_u8(bitmap_git->map, &bitmap_git->map_pos);\n> \tbitmap = read_bitmap_1(bitmap_git);\n> @@ -976,7 +994,8 @@ static struct stored_bitmap *lazy_bitmap_for_commit(struct bitmap_index *bitmap_\n> \tif (!bitmap)\n> \t\tgoto corrupt;\n> \n> -\treturn store_bitmap(bitmap_git, bitmap, oid, xor_bitmap, flags);\n> +\treturn store_bitmap(bitmap_git, bitmap, oid, xor_bitmap, flags,\n> +\t\t\t entry_map_pos);\n> \n> corrupt:\n> \tfree(xor_items);\n> @@ -2856,6 +2875,48 @@ int test_bitmap_commits(struct repository *r)\n> \treturn 0;\n> }\n> \n> +int test_bitmap_commits_offset(struct repository *r)\n> +{\n> +\tstruct object_id oid;\n> +\tstruct stored_bitmap_tag_pos *tagged;\n> +\tstruct bitmap_index *bitmap_git;\n> +\tsize_t commit_idx_pos_map_pos, xor_offset_map_pos, flag_map_pos,\n> +\t\tewah_bitmap_map_pos;\n> +\n> +\ttag_pos_on_bitmap = 1;\n> +\tbitmap_git = prepare_bitmap_git(r);\n> +\tif (!bitmap_git)\n> +\t\tdie(_(\"failed to load bitmap indexes\"));\n> +\n> +\t/*\n> +\t * As this function is only used to print bitmap selected\n> +\t * commits, we don't have to read the commit table.\n> +\t */\n> +\tif (bitmap_git->table_lookup) {\n> +\t\tif (load_bitmap_entries_v1(bitmap_git) < 0)\n> +\t\t\tdie(_(\"failed to load bitmap indexes\"));\n> +\t}\n> +\n> +\tkh_foreach (bitmap_git->bitmaps, oid, tagged, {\n> +\t\tcommit_idx_pos_map_pos = tagged->map_pos;\n> +\t\txor_offset_map_pos = tagged->map_pos + sizeof(uint32_t);\n> +\t\tflag_map_pos = xor_offset_map_pos + sizeof(uint8_t);\n> +\t\tewah_bitmap_map_pos = flag_map_pos + sizeof(uint8_t);\n> +\n> +\t\tprintf_ln(\"%s %\"PRIuMAX\" %\"PRIuMAX\" %\"PRIuMAX\" %\"PRIuMAX,\n> +\t\t\t oid_to_hex(&oid),\n> +\t\t\t (uintmax_t)commit_idx_pos_map_pos,\n> +\t\t\t (uintmax_t)xor_offset_map_pos,\n> +\t\t\t (uintmax_t)flag_map_pos,\n> +\t\t\t (uintmax_t)ewah_bitmap_map_pos);\n> +\t})\n> +\t\t;\n> +\n> +\tfree_bitmap_index(bitmap_git);\n> +\n> +\treturn 0;\n> +}\n> +\n> int test_bitmap_hashes(struct repository *r)\n> {\n> \tstruct bitmap_index *bitmap_git = prepare_bitmap_git(r);\n> diff --git a/pack-bitmap.h b/pack-bitmap.h\n> index 382d39499af2..96880ba3d72d 100644\n> --- a/pack-bitmap.h\n> +++ b/pack-bitmap.h\n> @@ -81,6 +81,7 @@ void traverse_bitmap_commit_list(struct bitmap_index *,\n> \t\t\t\t show_reachable_fn show_reachable);\n> void test_bitmap_walk(struct rev_info *revs);\n> int test_bitmap_commits(struct repository *r);\n> +int test_bitmap_commits_offset(struct repository *r);\n> 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> diff --git a/t/helper/test-bitmap.c b/t/helper/test-bitmap.c\n> index 3f23f2107268..65a1ab29192b 100644\n> --- a/t/helper/test-bitmap.c\n> +++ b/t/helper/test-bitmap.c\n> @@ -10,6 +10,11 @@ static int bitmap_list_commits(void)\n> \treturn test_bitmap_commits(the_repository);\n> }\n> \n> +static int bitmap_list_commits_offset(void)\n> +{\n> +\treturn test_bitmap_commits_offset(the_repository);\n> +}\n> +\n> static int bitmap_dump_hashes(void)\n> {\n> \treturn test_bitmap_hashes(the_repository);\n> @@ -36,6 +41,8 @@ int cmd__bitmap(int argc, const char **argv)\n> \n> \tif (argc == 2 && !strcmp(argv[1], \"list-commits\"))\n> \t\treturn bitmap_list_commits();\n> +\tif (argc == 2 && !strcmp(argv[1], \"list-commits-offset\"))\n> +\t\treturn bitmap_list_commits_offset();\n> \tif (argc == 2 && !strcmp(argv[1], \"dump-hashes\"))\n> \t\treturn bitmap_dump_hashes();\n> \tif (argc == 2 && !strcmp(argv[1], \"dump-pseudo-merges\"))\n> @@ -46,6 +53,7 @@ int cmd__bitmap(int argc, const char **argv)\n> \t\treturn bitmap_dump_pseudo_merge_objects(atoi(argv[2]));\n> \n> \tusage(\"\\ttest-tool bitmap list-commits\\n\"\n> +\t \"\\ttest-tool bitmap list-commits-offset\\n\"\n> \t \"\\ttest-tool bitmap dump-hashes\\n\"\n> \t \"\\ttest-tool bitmap dump-pseudo-merges\\n\"\n> \t \"\\ttest-tool bitmap dump-pseudo-merge-commits \\n\"\n> diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh\n> index a62b463eaf09..ef4c5fbaae83 100755\n> --- a/t/t5310-pack-bitmaps.sh\n> +++ b/t/t5310-pack-bitmaps.sh\n> @@ -486,6 +486,33 @@ 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\trm -fr repo &&\n> +\t\tgit init repo &&\n> +\t\ttest_when_finished \"rm -fr repo\" &&\n> +\t\t(\n> +\t\t\tcd repo &&\n> +\t\t\tgit config pack.writeBitmapLookupTable '\"$writeLookupTable\"' &&\n> +\n> +\t\t\ttest_commit base &&\n> +\n> +\t\t\tgit repack -adb &&\n> +\t\t\tbitmap=\"$(ls .git/objects/pack/pack-*.bitmap)\" &&\n> +\t\t\tchmod +w $bitmap &&\n> +\n> +\t\t\tread oid commit_off xor_off flag_off ewah_off <<-EOF &&\n> +\t\t\t\t$(test-tool bitmap list-commits-offset | head -n 1)\n> +\t\t\tEOF\n> +\t\t\tprintf '\\161' |\n> +\t\t\t\tdd of=$bitmap count=1 bs=1 conv=notrunc seek=$xor_off &&\n> +\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> test_bitmap_cases\n"},{"id":"519173","messageId":"aDi8OD08I6+6BLja@nand.local","threadId":"63447","inReplyTo":"xmqqjz5zmnxy.fsf@gitster.g","subject":"Re: [PATCH v4 1/2] pack-bitmap: fix memory leak if load_bitmap() failed","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-05-29T19:57:44Z","receivedAt":"2025-05-29T19:57:52Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, May 29, 2025 at 08:33:29AM -0700, Junio C Hamano wrote:\n> \"Taylor Blau via GitGitGadget\" writes:\n>\n> > From: Taylor Blau \n> >\n> > After going through the \"failed\" label, load_bitmap() will return -1,\n> > and its caller (either prepare_bitmap_walk() or prepare_bitmap_git())\n> > will then call free_bitmap_index().\n> > ...\n> > The solution is to remove the error handling code in load_bitmap(), because\n> > its caller will always call free_bitmap_index() in case of an error.\n> >\n> > Signed-off-by: Taylor Blau \n> > ---\n>\n> As this is Lidong relaying that Taylor\n> sent to the list, shouldn't Lidong's sign-off be after Taylor's?\n\nI've always assumed the answer here was \"yes\", but I don't know that our\ndocumentation suggests the same.\n\nIn c11c3b5681 (Documentation/SubmittingPatches: What's Acked-by and\nTested-by?, 2008-02-03) you added:\n\n Notice that you can place your own Signed-off-by: line when\n forwarding somebody else's patch [...]. Indeed you are encouraged\n to do so. [...]\n\nand that text survives into the current version of SubmittingPatches.\nSo I think that while our documentation encourages people to add their\nown S-o-b to others' patches sent on their behalf, it doesn't\nexplicitly require it.\n\nThanks,\nTaylor\n"},{"id":"519179","messageId":"aDjPsMqyYSm+b2Ap@nand.local","threadId":"63447","inReplyTo":"7876d9a9014ea6a0657f440f7fa1efd496a4a15a.1748140983.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v4 2/2] pack-bitmap: add load corrupt bitmap test","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-05-29T21:20:48Z","receivedAt":"2025-05-29T21:20:51Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Sun, May 25, 2025 at 02:43:03AM +0000, Lidong Yan via GitGitGadget wrote:\n> diff --git a/pack-bitmap.c b/pack-bitmap.c\n> index fd19c2255163..39c1c1bc4ce1 100644\n> --- a/pack-bitmap.c\n> +++ b/pack-bitmap.c\n> @@ -34,6 +34,11 @@ struct stored_bitmap {\n> \tint flags;\n> };\n>\n> +struct stored_bitmap_tag_pos {\n> +\tstruct stored_bitmap stored;\n> +\tsize_t map_pos;\n> +};\n> +\n\nHmm. I was expecting you to add a new member to the stored_bitmap\nstructure, not a new structure entirely. Let's read on...\n\n> /*\n> * The active bitmap index for a repository. By design, repositories only have\n> * a single bitmap index available (the index for the biggest packfile in\n> @@ -148,6 +153,7 @@ static int existing_bitmaps_hits_nr;\n> static int existing_bitmaps_misses_nr;\n> static int roots_with_bitmaps_nr;\n> static int roots_without_bitmaps_nr;\n> +static int tag_pos_on_bitmap;\n\nWhy are we only sometimes tagging bitmaps with their position?\n\n>\n> static struct ewah_bitmap *lookup_stored_bitmap(struct stored_bitmap *st)\n> {\n> @@ -314,13 +320,18 @@ static struct stored_bitmap *store_bitmap(struct bitmap_index *index,\n> \t\t\t\t\t struct ewah_bitmap *root,\n> \t\t\t\t\t const struct object_id *oid,\n> \t\t\t\t\t struct stored_bitmap *xor_with,\n> -\t\t\t\t\t int flags)\n> +\t\t\t\t\t int flags, size_t map_pos)\n> {\n> \tstruct stored_bitmap *stored;\n> +\tstruct stored_bitmap_tag_pos *tagged;\n\nOK.\n\n> \tkhiter_t hash_pos;\n> \tint ret;\n>\n> -\tstored = xmalloc(sizeof(struct stored_bitmap));\n> +\ttagged = xmalloc(tag_pos_on_bitmap ? sizeof(struct stored_bitmap_tag_pos) :\n> +\t\t\t\t\t sizeof(struct stored_bitmap));\n> +\tstored = &tagged->stored;\n> +\tif (tag_pos_on_bitmap)\n> +\t\ttagged->map_pos = map_pos;\n\nI am quite worried about this portion of the diff.\n\nHere you allocate memory for \"tagged\" which is a stored_bitmap_tag_pos.\nBut the amount of bytes you allocate depends on whether the global\nvariable tag_pos_on_bitmap is set or not. If it isn't, then you don't\nallocate enough memory here to hold an entire stored_bitmap_tag_pos\nstructure.\n\nI think within this function you're OK, since you only write into that\nfield when tag_pos_on_bitmap is set. But this seems like a recipe for\ndisaster if you ever try to read or write into the tagged->map_pos field\nwhen tag_pos_on_bitmap *isn't* set.\n\nThis happens to work because of where the pointer to the stored_bitmap\nstructure lives within the stored_bitmap_tag_pos structure. But this\nseems *extremely* fragile to only save 4 bytes of allocated memory per\nbitmap. Even on a repository with ~1,000 bitmaps (which is rare from my\nexperience), you're only saving ~3.91 KiB.\n\nI would expect something more like the following (based on top of your\npatch here):\n\n--- 8< ---\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex 39c1c1bc4c..4c3829dba9 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -31,12 +31,8 @@ struct stored_bitmap {\n \tstruct object_id oid;\n \tstruct ewah_bitmap *root;\n \tstruct stored_bitmap *xor;\n-\tint flags;\n-};\n-\n-struct stored_bitmap_tag_pos {\n-\tstruct stored_bitmap stored;\n \tsize_t map_pos;\n+\tint flags;\n };\n\n /*\n@@ -153,7 +149,6 @@ static int existing_bitmaps_hits_nr;\n static int existing_bitmaps_misses_nr;\n static int roots_with_bitmaps_nr;\n static int roots_without_bitmaps_nr;\n-static int tag_pos_on_bitmap;\n\n static struct ewah_bitmap *lookup_stored_bitmap(struct stored_bitmap *st)\n {\n@@ -323,17 +318,13 @@ static struct stored_bitmap *store_bitmap(struct bitmap_index *index,\n \t\t\t\t\t int flags, size_t map_pos)\n {\n \tstruct stored_bitmap *stored;\n-\tstruct stored_bitmap_tag_pos *tagged;\n \tkhiter_t hash_pos;\n \tint ret;\n\n-\ttagged = xmalloc(tag_pos_on_bitmap ? sizeof(struct stored_bitmap_tag_pos) :\n-\t\t\t\t\t sizeof(struct stored_bitmap));\n-\tstored = &tagged->stored;\n-\tif (tag_pos_on_bitmap)\n-\t\ttagged->map_pos = map_pos;\n+\tstored = xmalloc(sizeof(struct stored_bitmap));\n \tstored->root = root;\n \tstored->xor = xor_with;\n+\tstored->map_pos = map_pos;\n \tstored->flags = flags;\n \toidcpy(&stored->oid, oid);\n\n@@ -2878,12 +2869,11 @@ int test_bitmap_commits(struct repository *r)\n int test_bitmap_commits_offset(struct repository *r)\n {\n \tstruct object_id oid;\n-\tstruct stored_bitmap_tag_pos *tagged;\n+\tstruct stored_bitmap *bitmap;\n \tstruct bitmap_index *bitmap_git;\n \tsize_t commit_idx_pos_map_pos, xor_offset_map_pos, flag_map_pos,\n \t\tewah_bitmap_map_pos;\n\n-\ttag_pos_on_bitmap = 1;\n \tbitmap_git = prepare_bitmap_git(r);\n \tif (!bitmap_git)\n \t\tdie(_(\"failed to load bitmap indexes\"));\n@@ -2897,9 +2887,9 @@ int test_bitmap_commits_offset(struct repository *r)\n \t\t\tdie(_(\"failed to load bitmap indexes\"));\n \t}\n\n-\tkh_foreach (bitmap_git->bitmaps, oid, tagged, {\n-\t\tcommit_idx_pos_map_pos = tagged->map_pos;\n-\t\txor_offset_map_pos = tagged->map_pos + sizeof(uint32_t);\n+\tkh_foreach (bitmap_git->bitmaps, oid, bitmap, {\n+\t\tcommit_idx_pos_map_pos = bitmap->map_pos;\n+\t\txor_offset_map_pos = bitmap->map_pos + sizeof(uint32_t);\n \t\tflag_map_pos = xor_offset_map_pos + sizeof(uint8_t);\n \t\tewah_bitmap_map_pos = flag_map_pos + sizeof(uint8_t);\n--- >8 ---\n\n> \tstored->root = root;\n> \tstored->xor = xor_with;\n> \tstored->flags = flags;\n> @@ -376,10 +387,12 @@ static int load_bitmap_entries_v1(struct bitmap_index *index)\n> \t\tstruct stored_bitmap *xor_bitmap = NULL;\n> \t\tuint32_t commit_idx_pos;\n> \t\tstruct object_id oid;\n> +\t\tsize_t entry_map_pos;\n>\n> \t\tif (index->map_size - index->map_pos < 6)\n> \t\t\treturn error(_(\"corrupt ewah bitmap: truncated header for entry %d\"), i);\n>\n> +\t\tentry_map_pos = index->map_pos;\n\nGood. This is important since the read_be32() and read_u8() calls below\nboth adjust the value of index->map_pos past the beginning of the bitmap.\n\n> @@ -869,6 +883,7 @@ static struct stored_bitmap *lazy_bitmap_for_commit(struct bitmap_index *bitmap_\n> \tint xor_flags;\n> \tkhiter_t hash_pos;\n> \tstruct bitmap_lookup_table_xor_item *xor_item;\n> +\tsize_t entry_map_pos;\n>\n> \tif (is_corrupt)\n> \t\treturn NULL;\n> @@ -928,6 +943,7 @@ static struct stored_bitmap *lazy_bitmap_for_commit(struct bitmap_index *bitmap_\n> \t\t\tgoto corrupt;\n> \t\t}\n>\n> +\t\tentry_map_pos = bitmap_git->map_pos;\n\nSame here.\n\n> @@ -969,6 +986,7 @@ static struct stored_bitmap *lazy_bitmap_for_commit(struct bitmap_index *bitmap_\n> \t * Instead, we can skip ahead and immediately read the flags and\n> \t * ewah bitmap.\n> \t */\n> +\tentry_map_pos = bitmap_git->map_pos;\n\nAnd here.\n\n> +int test_bitmap_commits_offset(struct repository *r)\n> +{\n> +\tstruct object_id oid;\n> +\tstruct stored_bitmap_tag_pos *tagged;\n> +\tstruct bitmap_index *bitmap_git;\n> +\tsize_t commit_idx_pos_map_pos, xor_offset_map_pos, flag_map_pos,\n> +\t\tewah_bitmap_map_pos;\n> +\n> +\ttag_pos_on_bitmap = 1;\n> +\tbitmap_git = prepare_bitmap_git(r);\n> +\tif (!bitmap_git)\n> +\t\tdie(_(\"failed to load bitmap indexes\"));\n> +\n\nIf we either forgot to set this variable here or did so after calling\nprepare_bitmap_git(), then we wouldn't allocate enough memory to store\nthe map_pos field in the stored_bitmap_tag_pos structure. When we then\nwould try and read that field below, we'd read garbage heap data outside\nof our structure.\n\n> +\t/*\n> +\t * As this function is only used to print bitmap selected\n> +\t * commits, we don't have to read the commit table.\n> +\t */\n> +\tif (bitmap_git->table_lookup) {\n> +\t\tif (load_bitmap_entries_v1(bitmap_git) < 0)\n> +\t\t\tdie(_(\"failed to load bitmap indexes\"));\n> +\t}\n\nThis comment suggests that we can avoid reading the commit table\naltogether. Indeed, calling load_bitmap_entries_v1() here does that,\nsince it is not called when loading a bitmap that has a lookup table.\n\nSo I think the behavior here is correct, but the comment is misleading.\nI suspect that the confusion would be resolved by instead writing:\n\n /*\n * Since this function needs to know the position of each individual\n * bitmap, bypass the commit lookup table (if one exists) by forcing\n * the bitmap to eagerly load its entries.\n */\n\nI think this is copy-and-paste from 28cd730680 (pack-bitmap: prepare to\nread lookup table extension, 2022-08-14) via the 'test_bitmap_commits()'\nfunction immediately above this one. I think both would benefit from\nsome clean-up, since this comment is equally misleading in that\nfunction.\n\nFor your purposes, I would either:\n\n - remove or (preferably) reword the comment in your new function,\n leaving the one in test_bitmap_commits() as-is, or\n\n - reword the comment in test_bitmap_commits() to be more like the one\n above, via a preparatory commit, and then introduce the new function\n using the same wording.\n\nBetween the two, I think the latter is preferable.\n\nAs an aside, I think that for bitmaps that do have a commit lookup\ntable, you could go slightly faster here by walking over that portion of\nthe *.bitmap file, since it directly encodes the information you're\ninterested in here. But I would avoid doing that, since it too seems\nbrittle and I would like to avoid having two separate spots that each\nimplement reading the commit table format.\n\n> +\tkh_foreach (bitmap_git->bitmaps, oid, tagged, {\n> +\t\tcommit_idx_pos_map_pos = tagged->map_pos;\n\nOK, and here's where we pull out the actual position of the selected\ncommit's bitmap.\n\n> +\t\txor_offset_map_pos = tagged->map_pos + sizeof(uint32_t);\n> +\t\tflag_map_pos = xor_offset_map_pos + sizeof(uint8_t);\n> +\t\tewah_bitmap_map_pos = flag_map_pos + sizeof(uint8_t);\n> +\n> +\t\tprintf_ln(\"%s %\"PRIuMAX\" %\"PRIuMAX\" %\"PRIuMAX\" %\"PRIuMAX,\n> +\t\t\t oid_to_hex(&oid),\n> +\t\t\t (uintmax_t)commit_idx_pos_map_pos,\n> +\t\t\t (uintmax_t)xor_offset_map_pos,\n> +\t\t\t (uintmax_t)flag_map_pos,\n> +\t\t\t (uintmax_t)ewah_bitmap_map_pos);\n\nHmm. We print more information here than just the map_pos. This is\nbrittle if the on-disk format changes (e.g., to store the XOR offsets in\nsome other part of the bitmap). But hopefully future updates to the\nbitmap format will come with updates to this function as well ;-).\n\nIt feels somewhat unsatisfying to print output like:\n\n $COMMIT_OID \n\n, but I think it makes sense here for a couple of reasons:\n\n - If we just print the , then the test is responsible for\n knowing the distance between that and the XOR offset, which extends\n the brittleness to the test code\n\n - likewise, if we print out just the map_pos and the positions of the\n XOR offset, it feels strange to omit the others.\n\n> diff --git a/pack-bitmap.h b/pack-bitmap.h\n> index 382d39499af2..96880ba3d72d 100644\n> --- a/pack-bitmap.h\n> +++ b/pack-bitmap.h\n> @@ -81,6 +81,7 @@ void traverse_bitmap_commit_list(struct bitmap_index *,\n> \t\t\t\t show_reachable_fn show_reachable);\n> void test_bitmap_walk(struct rev_info *revs);\n> int test_bitmap_commits(struct repository *r);\n> +int test_bitmap_commits_offset(struct repository *r);\n> 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> diff --git a/t/helper/test-bitmap.c b/t/helper/test-bitmap.c\n> index 3f23f2107268..65a1ab29192b 100644\n> --- a/t/helper/test-bitmap.c\n> +++ b/t/helper/test-bitmap.c\n> @@ -10,6 +10,11 @@ static int bitmap_list_commits(void)\n> \treturn test_bitmap_commits(the_repository);\n> }\n>\n> +static int bitmap_list_commits_offset(void)\n> +{\n> +\treturn test_bitmap_commits_offset(the_repository);\n> +}\n> +\n> static int bitmap_dump_hashes(void)\n> {\n> \treturn test_bitmap_hashes(the_repository);\n> @@ -36,6 +41,8 @@ int cmd__bitmap(int argc, const char **argv)\n>\n> \tif (argc == 2 && !strcmp(argv[1], \"list-commits\"))\n> \t\treturn bitmap_list_commits();\n> +\tif (argc == 2 && !strcmp(argv[1], \"list-commits-offset\"))\n> +\t\treturn bitmap_list_commits_offset();\n\nAll of the scaffolding here looks good.\n\nThis new mode reads a little awkwardly to me (but may not to others, in\nwhich case I am happy to back away from the following suggestion). Can\nwe either call this 'list-commits-with-offset' or 'list-commit-offsets'?\nI have a vague preference towards the former since the new mode has a\nstring prefix matching the existing mode.\n\n> +\ttest_expect_success 'load corrupt bitmap' '\n> +\t\trm -fr repo &&\n> +\t\tgit init repo &&\n> +\t\ttest_when_finished \"rm -fr repo\" &&\n> +\t\t(\n> +\t\t\tcd repo &&\n> +\t\t\tgit config pack.writeBitmapLookupTable '\"$writeLookupTable\"' &&\n> +\n> +\t\t\ttest_commit base &&\n> +\n> +\t\t\tgit repack -adb &&\n> +\t\t\tbitmap=\"$(ls .git/objects/pack/pack-*.bitmap)\" &&\n> +\t\t\tchmod +w $bitmap &&\n> +\n> +\t\t\tread oid commit_off xor_off flag_off ewah_off <<-EOF &&\n> +\t\t\t\t$(test-tool bitmap list-commits-offset | head -n 1)\n\nWe avoid putting 'git' or 'test-tool' on the left-hand side of a pipe,\nsince we want to avoid squelching any errors / segfaults from our code.\n\nHow about (assuming the rename above):\n\n test-tool bitmap list-commit-offsets >offsets &&\n xor_off=\"$(head -n1 offsets | awk '{print $3}')\" &&\n ...\n\n?\n\n> +\t\t\tprintf '\\161' |\n> +\t\t\t\tdd of=$bitmap count=1 bs=1 conv=notrunc seek=$xor_off &&\n> +\n> +\n> +\t\t\tgit rev-list --count HEAD > expect &&\n> +\t\t\tgit rev-list --use-bitmap-index --count HEAD > actual &&\n\nUsing --count can mask failures in the bitmap code since it only guards\nyou against getting the wrong number of objects, but doesn't guard you\nagainst getting the right amount of objects in a permuted order. I think\nwe'd want just 'git rev-list --objects' here, but note that you have to\nuse '--no-object-names' on the non-bitmap side, since rev-list does not\nprint out paths when using bitmaps[^1].\n\nHow about:\n\n git rev-list --objects --no-object-names HEAD >expect.raw &&\n git rev-list --objects --use-bitmap-index --no-object-names HEAD \\\n >actual.raw &&\n\n sort expect.raw >expect &&\n sort actual.raw >actual &&\n\n test_cmp expect actual\n\n(Note that you also have to sort the output here since rev-list does not\noutput objects in a meaningful order when using bitmaps.)\n\nThanks,\nTaylor\n\n[^1]: Not that it matters here since we don't expect to load the bitmap\n anyway, but it's worth doing regardless.\n"},{"id":"519180","messageId":"aDjP7GiNvflWepAL@nand.local","threadId":"63447","inReplyTo":"xmqqbjrbmndn.fsf@gitster.g","subject":"Re: [PATCH v4 2/2] pack-bitmap: add load corrupt bitmap test","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-05-29T21:21:48Z","receivedAt":"2025-05-29T21:21:51Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, May 29, 2025 at 08:45:40AM -0700, Junio C Hamano wrote:\n> > Signed-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>\n> > ---\n> > pack-bitmap.c | 73 +++++++++++++++++++++++++++++++++++++----\n> > pack-bitmap.h | 1 +\n> > t/helper/test-bitmap.c | 8 +++++\n> > t/t5310-pack-bitmaps.sh | 27 +++++++++++++++\n> > 4 files changed, 103 insertions(+), 6 deletions(-)\n>\n> After the second round of the series, no review comments seem to\n> have been sent to the list. Is everybody happy with the latest\n> iteration?\n\nSorry for missing this one from earlier this week. I left a few comments\non the latest round. I think we are getting there, but I do not feel\ncomfortable merging down the series just yet.\n\nThanks,\nTaylor\n"},{"id":"519184","messageId":"xmqqjz5zhy4i.fsf@gitster.g","threadId":"63447","inReplyTo":"aDi8OD08I6+6BLja@nand.local","subject":"Re: [PATCH v4 1/2] pack-bitmap: fix memory leak if load_bitmap() failed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-29T22:04:45Z","receivedAt":"2025-05-29T22:04:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> In c11c3b5681 (Documentation/SubmittingPatches: What's Acked-by and\n> Tested-by?, 2008-02-03) you added:\n>\n> Notice that you can place your own Signed-off-by: line when\n> forwarding somebody else's patch [...]. Indeed you are encouraged\n> to do so. [...]\n>\n> and that text survives into the current version of SubmittingPatches.\n> So I think that while our documentation encourages people to add their\n> own S-o-b to others' patches sent on their behalf, it doesn't\n> explicitly require it.\n\nIt would not hurt that much if I pretended that I ignored what\nLidong forwarded and picked up your patch directly from the list,\nonly because what was forwarded was public. If it were a privately\nshared patch, sign-off is required, so we'd need to tighten the\nlanguage in the SubmittingPatches document, I think.\n\nThanks.\n"},{"id":"519193","messageId":"68A128BD-DB63-403F-82DF-B8B6C78D5308@smail.nju.edu.cn","threadId":"63447","inReplyTo":"xmqqjz5zmnxy.fsf@gitster.g","subject":"Re: [PATCH v4 1/2] pack-bitmap: fix memory leak if load_bitmap() failed","fromName":"lidongyan","fromEmail":"502024330056@smail.nju.edu.cn","sentAt":"2025-05-30T03:50:12Z","receivedAt":"2025-05-30T03:51:14Z","isPatch":true,"sender":{"key":"502024330056@smail.nju.edu.cn","avatar":"https://avatars.githubusercontent.com/u/77328395?v=4"},"body":"Got it, I will add my sign-off.\n\n> 2025年5月29日 23:33，Junio C Hamano 写道：\n> \n> \"Taylor Blau via GitGitGadget\" writes:\n> \n>> From: Taylor Blau \n>> \n>> After going through the \"failed\" label, load_bitmap() will return -1,\n>> and its caller (either prepare_bitmap_walk() or prepare_bitmap_git())\n>> will then call free_bitmap_index().\n>> ...\n>> The solution is to remove the error handling code in load_bitmap(), because\n>> its caller will always call free_bitmap_index() in case of an error.\n>> \n>> Signed-off-by: Taylor Blau \n>> ---\n> \n> As this is Lidong relaying that Taylor\n> sent to the list, shouldn't Lidong's sign-off be after Taylor's?\n> \n\n"},{"id":"519194","messageId":"B77763AE-316A-405A-B11F-C08CB44A734B@smail.nju.edu.cn","threadId":"63447","inReplyTo":"xmqqbjrbmndn.fsf@gitster.g","subject":"Re: [PATCH v4 2/2] pack-bitmap: add load corrupt bitmap test","fromName":"lidongyan","fromEmail":"502024330056@smail.nju.edu.cn","sentAt":"2025-05-30T03:53:55Z","receivedAt":"2025-05-30T03:54:38Z","isPatch":true,"sender":{"key":"502024330056@smail.nju.edu.cn","avatar":"https://avatars.githubusercontent.com/u/77328395?v=4"},"body":"\n\n> 2025年5月29日 23:45，Junio C Hamano 写道：\n> \n> \"Lidong Yan via GitGitGadget\" writes:\n> \n>> From: Lidong Yan <502024330056@smail.nju.edu.cn>\n>> \n>> This patch add test_bitmap_list_commits_offset() in patch-bitmap.c,\n> \n> \"pack-bitmap.c\"?\n> \n>> a new test helper command `test-tool bitmap list-commits-offset`,\n>> and a `load corrupt bitmap` test case in t5310.\n>> \n>> The `load corrupt bitmap` test case intentionally corrupt the\n>> \"xor_offset\" field of the first entry. And the newly added helper\n>> can help to find position of \"xor_offset\" in bitmap file.\n> \n> [the structure of a log message]\n> \n> The usual way to compose a log message of this project is to\n> \n> - Give an observation on how the current system works in the\n> present tense (so no need to say \"Currently X is Y\", or\n> \"Previously X was Y\" to describe the state before your change;\n> just \"X is Y\" is enough), and discuss what you perceive as a\n> problem in it.\n> \n> - Propose a solution (optional---often, problem description\n> trivially leads to an obvious solution in reader's minds).\n> \n> - Give commands to somebody editing the codebase to \"make it so\".\n> \n> in this order.\n> \n> The proposed log message lacks the motivation and only talks about\n> what the patch does. We add a test-only code in a file, intermixed\n> with production code. Let's explain why it is the best arrangement.\n\nI see. I am trying to validate these patch series and test further patch won’t leak\nmemory under the condition that bitmap is corrupted, Anyway I will pay attention\nto motivation in my following log messages.\n\n> \n>> Signed-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>\n>> ---\n>> pack-bitmap.c | 73 +++++++++++++++++++++++++++++++++++++----\n>> pack-bitmap.h | 1 +\n>> t/helper/test-bitmap.c | 8 +++++\n>> t/t5310-pack-bitmaps.sh | 27 +++++++++++++++\n>> 4 files changed, 103 insertions(+), 6 deletions(-)\n> \n> After the second round of the series, no review comments seem to\n> have been sent to the list. Is everybody happy with the latest\n> iteration?\n> \n> Thanks.\n> \n>> diff --git a/pack-bitmap.c b/pack-bitmap.c\n>> index fd19c2255163..39c1c1bc4ce1 100644\n>> --- a/pack-bitmap.c\n>> +++ b/pack-bitmap.c\n>> @@ -34,6 +34,11 @@ struct stored_bitmap {\n>> int flags;\n>> };\n>> \n>> +struct stored_bitmap_tag_pos {\n>> + struct stored_bitmap stored;\n>> + size_t map_pos;\n>> +};\n>> +\n>> /*\n>> * The active bitmap index for a repository. By design, repositories only have\n>> * a single bitmap index available (the index for the biggest packfile in\n>> @@ -148,6 +153,7 @@ static int existing_bitmaps_hits_nr;\n>> static int existing_bitmaps_misses_nr;\n>> static int roots_with_bitmaps_nr;\n>> static int roots_without_bitmaps_nr;\n>> +static int tag_pos_on_bitmap;\n>> \n>> static struct ewah_bitmap *lookup_stored_bitmap(struct stored_bitmap *st)\n>> {\n>> @@ -314,13 +320,18 @@ static struct stored_bitmap *store_bitmap(struct bitmap_index *index,\n>> struct ewah_bitmap *root,\n>> const struct object_id *oid,\n>> struct stored_bitmap *xor_with,\n>> - int flags)\n>> + int flags, size_t map_pos)\n>> {\n>> struct stored_bitmap *stored;\n>> + struct stored_bitmap_tag_pos *tagged;\n>> khiter_t hash_pos;\n>> int ret;\n>> \n>> - stored = xmalloc(sizeof(struct stored_bitmap));\n>> + tagged = xmalloc(tag_pos_on_bitmap ? sizeof(struct stored_bitmap_tag_pos) :\n>> + sizeof(struct stored_bitmap));\n>> + stored = &tagged->stored;\n>> + if (tag_pos_on_bitmap)\n>> + tagged->map_pos = map_pos;\n>> stored->root = root;\n>> stored->xor = xor_with;\n>> stored->flags = flags;\n>> @@ -376,10 +387,12 @@ static int load_bitmap_entries_v1(struct bitmap_index *index)\n>> struct stored_bitmap *xor_bitmap = NULL;\n>> uint32_t commit_idx_pos;\n>> struct object_id oid;\n>> + size_t entry_map_pos;\n>> \n>> if (index->map_size - index->map_pos < 6)\n>> return error(_(\"corrupt ewah bitmap: truncated header for entry %d\"), i);\n>> \n>> + entry_map_pos = index->map_pos;\n>> commit_idx_pos = read_be32(index->map, &index->map_pos);\n>> xor_offset = read_u8(index->map, &index->map_pos);\n>> flags = read_u8(index->map, &index->map_pos);\n>> @@ -402,8 +415,9 @@ static int load_bitmap_entries_v1(struct bitmap_index *index)\n>> if (!bitmap)\n>> return -1;\n>> \n>> - recent_bitmaps[i % MAX_XOR_OFFSET] = store_bitmap(\n>> - index, bitmap, &oid, xor_bitmap, flags);\n>> + recent_bitmaps[i % MAX_XOR_OFFSET] =\n>> + store_bitmap(index, bitmap, &oid, xor_bitmap, flags,\n>> + entry_map_pos);\n>> }\n>> \n>> return 0;\n>> @@ -869,6 +883,7 @@ static struct stored_bitmap *lazy_bitmap_for_commit(struct bitmap_index *bitmap_\n>> int xor_flags;\n>> khiter_t hash_pos;\n>> struct bitmap_lookup_table_xor_item *xor_item;\n>> + size_t entry_map_pos;\n>> \n>> if (is_corrupt)\n>> return NULL;\n>> @@ -928,6 +943,7 @@ static struct stored_bitmap *lazy_bitmap_for_commit(struct bitmap_index *bitmap_\n>> goto corrupt;\n>> }\n>> \n>> + entry_map_pos = bitmap_git->map_pos;\n>> bitmap_git->map_pos += sizeof(uint32_t) + sizeof(uint8_t);\n>> xor_flags = read_u8(bitmap_git->map, &bitmap_git->map_pos);\n>> bitmap = read_bitmap_1(bitmap_git);\n>> @@ -935,7 +951,8 @@ static struct stored_bitmap *lazy_bitmap_for_commit(struct bitmap_index *bitmap_\n>> if (!bitmap)\n>> goto corrupt;\n>> \n>> - xor_bitmap = store_bitmap(bitmap_git, bitmap, &xor_item->oid, xor_bitmap, xor_flags);\n>> + xor_bitmap = store_bitmap(bitmap_git, bitmap, &xor_item->oid,\n>> + xor_bitmap, xor_flags, entry_map_pos);\n>> xor_items_nr--;\n>> }\n>> \n>> @@ -969,6 +986,7 @@ static struct stored_bitmap *lazy_bitmap_for_commit(struct bitmap_index *bitmap_\n>> * Instead, we can skip ahead and immediately read the flags and\n>> * ewah bitmap.\n>> */\n>> + entry_map_pos = bitmap_git->map_pos;\n>> bitmap_git->map_pos += sizeof(uint32_t) + sizeof(uint8_t);\n>> flags = read_u8(bitmap_git->map, &bitmap_git->map_pos);\n>> bitmap = read_bitmap_1(bitmap_git);\n>> @@ -976,7 +994,8 @@ static struct stored_bitmap *lazy_bitmap_for_commit(struct bitmap_index *bitmap_\n>> if (!bitmap)\n>> goto corrupt;\n>> \n>> - return store_bitmap(bitmap_git, bitmap, oid, xor_bitmap, flags);\n>> + return store_bitmap(bitmap_git, bitmap, oid, xor_bitmap, flags,\n>> + entry_map_pos);\n>> \n>> corrupt:\n>> free(xor_items);\n>> @@ -2856,6 +2875,48 @@ int test_bitmap_commits(struct repository *r)\n>> return 0;\n>> }\n>> \n>> +int test_bitmap_commits_offset(struct repository *r)\n>> +{\n>> + struct object_id oid;\n>> + struct stored_bitmap_tag_pos *tagged;\n>> + struct bitmap_index *bitmap_git;\n>> + size_t commit_idx_pos_map_pos, xor_offset_map_pos, flag_map_pos,\n>> + ewah_bitmap_map_pos;\n>> +\n>> + tag_pos_on_bitmap = 1;\n>> + bitmap_git = prepare_bitmap_git(r);\n>> + if (!bitmap_git)\n>> + die(_(\"failed to load bitmap indexes\"));\n>> +\n>> + /*\n>> + * As this function is only used to print bitmap selected\n>> + * commits, we don't have to read the commit table.\n>> + */\n>> + if (bitmap_git->table_lookup) {\n>> + if (load_bitmap_entries_v1(bitmap_git) < 0)\n>> + die(_(\"failed to load bitmap indexes\"));\n>> + }\n>> +\n>> + kh_foreach (bitmap_git->bitmaps, oid, tagged, {\n>> + commit_idx_pos_map_pos = tagged->map_pos;\n>> + xor_offset_map_pos = tagged->map_pos + sizeof(uint32_t);\n>> + flag_map_pos = xor_offset_map_pos + sizeof(uint8_t);\n>> + ewah_bitmap_map_pos = flag_map_pos + sizeof(uint8_t);\n>> +\n>> + printf_ln(\"%s %\"PRIuMAX\" %\"PRIuMAX\" %\"PRIuMAX\" %\"PRIuMAX,\n>> + oid_to_hex(&oid),\n>> + (uintmax_t)commit_idx_pos_map_pos,\n>> + (uintmax_t)xor_offset_map_pos,\n>> + (uintmax_t)flag_map_pos,\n>> + (uintmax_t)ewah_bitmap_map_pos);\n>> + })\n>> + ;\n>> +\n>> + free_bitmap_index(bitmap_git);\n>> +\n>> + return 0;\n>> +}\n>> +\n>> int test_bitmap_hashes(struct repository *r)\n>> {\n>> struct bitmap_index *bitmap_git = prepare_bitmap_git(r);\n>> diff --git a/pack-bitmap.h b/pack-bitmap.h\n>> index 382d39499af2..96880ba3d72d 100644\n>> --- a/pack-bitmap.h\n>> +++ b/pack-bitmap.h\n>> @@ -81,6 +81,7 @@ void traverse_bitmap_commit_list(struct bitmap_index *,\n>> show_reachable_fn show_reachable);\n>> void test_bitmap_walk(struct rev_info *revs);\n>> int test_bitmap_commits(struct repository *r);\n>> +int test_bitmap_commits_offset(struct repository *r);\n>> 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>> diff --git a/t/helper/test-bitmap.c b/t/helper/test-bitmap.c\n>> index 3f23f2107268..65a1ab29192b 100644\n>> --- a/t/helper/test-bitmap.c\n>> +++ b/t/helper/test-bitmap.c\n>> @@ -10,6 +10,11 @@ static int bitmap_list_commits(void)\n>> return test_bitmap_commits(the_repository);\n>> }\n>> \n>> +static int bitmap_list_commits_offset(void)\n>> +{\n>> + return test_bitmap_commits_offset(the_repository);\n>> +}\n>> +\n>> static int bitmap_dump_hashes(void)\n>> {\n>> return test_bitmap_hashes(the_repository);\n>> @@ -36,6 +41,8 @@ int cmd__bitmap(int argc, const char **argv)\n>> \n>> if (argc == 2 && !strcmp(argv[1], \"list-commits\"))\n>> return bitmap_list_commits();\n>> + if (argc == 2 && !strcmp(argv[1], \"list-commits-offset\"))\n>> + return bitmap_list_commits_offset();\n>> if (argc == 2 && !strcmp(argv[1], \"dump-hashes\"))\n>> return bitmap_dump_hashes();\n>> if (argc == 2 && !strcmp(argv[1], \"dump-pseudo-merges\"))\n>> @@ -46,6 +53,7 @@ int cmd__bitmap(int argc, const char **argv)\n>> return bitmap_dump_pseudo_merge_objects(atoi(argv[2]));\n>> \n>> usage(\"\\ttest-tool bitmap list-commits\\n\"\n>> + \"\\ttest-tool bitmap list-commits-offset\\n\"\n>> \"\\ttest-tool bitmap dump-hashes\\n\"\n>> \"\\ttest-tool bitmap dump-pseudo-merges\\n\"\n>> \"\\ttest-tool bitmap dump-pseudo-merge-commits \\n\"\n>> diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh\n>> index a62b463eaf09..ef4c5fbaae83 100755\n>> --- a/t/t5310-pack-bitmaps.sh\n>> +++ b/t/t5310-pack-bitmaps.sh\n>> @@ -486,6 +486,33 @@ test_bitmap_cases () {\n>> grep \"ignoring extra bitmap\" trace2.txt\n>> )\n>> '\n>> +\n>> + test_expect_success 'load corrupt bitmap' '\n>> + rm -fr repo &&\n>> + git init repo &&\n>> + test_when_finished \"rm -fr repo\" &&\n>> + (\n>> + cd repo &&\n>> + git config pack.writeBitmapLookupTable '\"$writeLookupTable\"' &&\n>> +\n>> + test_commit base &&\n>> +\n>> + git repack -adb &&\n>> + bitmap=\"$(ls .git/objects/pack/pack-*.bitmap)\" &&\n>> + chmod +w $bitmap &&\n>> +\n>> + read oid commit_off xor_off flag_off ewah_off <<-EOF &&\n>> + $(test-tool bitmap list-commits-offset | head -n 1)\n>> + EOF\n>> + printf '\\161' |\n>> + dd of=$bitmap count=1 bs=1 conv=notrunc seek=$xor_off &&\n>> +\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>> test_bitmap_cases\n> \n\n"},{"id":"519195","messageId":"963BE708-67DE-4DBA-B1E3-754ADFCD9C26@smail.nju.edu.cn","threadId":"63447","inReplyTo":"aDjPsMqyYSm+b2Ap@nand.local","subject":"Re: [PATCH v4 2/2] pack-bitmap: add load corrupt bitmap test","fromName":"lidongyan","fromEmail":"502024330056@smail.nju.edu.cn","sentAt":"2025-05-30T04:03:13Z","receivedAt":"2025-05-30T04:03:56Z","isPatch":true,"sender":{"key":"502024330056@smail.nju.edu.cn","avatar":"https://avatars.githubusercontent.com/u/77328395?v=4"},"body":"2025年5月30日 05:20，Taylor Blau 写道：\n> \n> On Sun, May 25, 2025 at 02:43:03AM +0000, Lidong Yan via GitGitGadget wrote:\n>> diff --git a/pack-bitmap.c b/pack-bitmap.c\n>> index fd19c2255163..39c1c1bc4ce1 100644\n>> --- a/pack-bitmap.c\n>> +++ b/pack-bitmap.c\n>> @@ -34,6 +34,11 @@ struct stored_bitmap {\n>> int flags;\n>> };\n>> \n>> +struct stored_bitmap_tag_pos {\n>> + struct stored_bitmap stored;\n>> + size_t map_pos;\n>> +};\n>> +\n> \n> Hmm. I was expecting you to add a new member to the stored_bitmap\n> structure, not a new structure entirely. Let's read on...\n> \n>> /*\n>> * The active bitmap index for a repository. By design, repositories only have\n>> * a single bitmap index available (the index for the biggest packfile in\n>> @@ -148,6 +153,7 @@ static int existing_bitmaps_hits_nr;\n>> static int existing_bitmaps_misses_nr;\n>> static int roots_with_bitmaps_nr;\n>> static int roots_without_bitmaps_nr;\n>> +static int tag_pos_on_bitmap;\n> \n> Why are we only sometimes tagging bitmaps with their position?\n> \n>> \n>> static struct ewah_bitmap *lookup_stored_bitmap(struct stored_bitmap *st)\n>> {\n>> @@ -314,13 +320,18 @@ static struct stored_bitmap *store_bitmap(struct bitmap_index *index,\n>> struct ewah_bitmap *root,\n>> const struct object_id *oid,\n>> struct stored_bitmap *xor_with,\n>> - int flags)\n>> + int flags, size_t map_pos)\n>> {\n>> struct stored_bitmap *stored;\n>> + struct stored_bitmap_tag_pos *tagged;\n> \n> OK.\n> \n>> khiter_t hash_pos;\n>> int ret;\n>> \n>> - stored = xmalloc(sizeof(struct stored_bitmap));\n>> + tagged = xmalloc(tag_pos_on_bitmap ? sizeof(struct stored_bitmap_tag_pos) :\n>> + sizeof(struct stored_bitmap));\n>> + stored = &tagged->stored;\n>> + if (tag_pos_on_bitmap)\n>> + tagged->map_pos = map_pos;\n> \n> I am quite worried about this portion of the diff.\n> \n> Here you allocate memory for \"tagged\" which is a stored_bitmap_tag_pos.\n> But the amount of bytes you allocate depends on whether the global\n> variable tag_pos_on_bitmap is set or not. If it isn't, then you don't\n> allocate enough memory here to hold an entire stored_bitmap_tag_pos\n> structure.\n> \n> I think within this function you're OK, since you only write into that\n> field when tag_pos_on_bitmap is set. But this seems like a recipe for\n> disaster if you ever try to read or write into the tagged->map_pos field\n> when tag_pos_on_bitmap *isn't* set.\n> \n> This happens to work because of where the pointer to the stored_bitmap\n> structure lives within the stored_bitmap_tag_pos structure. But this\n> seems *extremely* fragile to only save 4 bytes of allocated memory per\n> bitmap. Even on a repository with ~1,000 bitmaps (which is rare from my\n> experience), you're only saving ~3.91 KiB.\n\nInteresting calculation indeed — I hadn't thought about the actual memory\nsavings that way. I agree it’s not worth the added fragility just to save ~4 KiB\nin such rare cases. I’ll go ahead and remove the struct stored_bitmap_tagged_pos.\n\n> \n> I would expect something more like the following (based on top of your\n> patch here):\n> \n> --- 8< ---\n> diff --git a/pack-bitmap.c b/pack-bitmap.c\n> index 39c1c1bc4c..4c3829dba9 100644\n> --- a/pack-bitmap.c\n> +++ b/pack-bitmap.c\n> @@ -31,12 +31,8 @@ struct stored_bitmap {\n> struct object_id oid;\n> struct ewah_bitmap *root;\n> struct stored_bitmap *xor;\n> - int flags;\n> -};\n> -\n> -struct stored_bitmap_tag_pos {\n> - struct stored_bitmap stored;\n> size_t map_pos;\n> + int flags;\n> };\n> \n> /*\n> @@ -153,7 +149,6 @@ static int existing_bitmaps_hits_nr;\n> static int existing_bitmaps_misses_nr;\n> static int roots_with_bitmaps_nr;\n> static int roots_without_bitmaps_nr;\n> -static int tag_pos_on_bitmap;\n> \n> static struct ewah_bitmap *lookup_stored_bitmap(struct stored_bitmap *st)\n> {\n> @@ -323,17 +318,13 @@ static struct stored_bitmap *store_bitmap(struct bitmap_index *index,\n> int flags, size_t map_pos)\n> {\n> struct stored_bitmap *stored;\n> - struct stored_bitmap_tag_pos *tagged;\n> khiter_t hash_pos;\n> int ret;\n> \n> - tagged = xmalloc(tag_pos_on_bitmap ? sizeof(struct stored_bitmap_tag_pos) :\n> - sizeof(struct stored_bitmap));\n> - stored = &tagged->stored;\n> - if (tag_pos_on_bitmap)\n> - tagged->map_pos = map_pos;\n> + stored = xmalloc(sizeof(struct stored_bitmap));\n> stored->root = root;\n> stored->xor = xor_with;\n> + stored->map_pos = map_pos;\n> stored->flags = flags;\n> oidcpy(&stored->oid, oid);\n> \n> @@ -2878,12 +2869,11 @@ int test_bitmap_commits(struct repository *r)\n> int test_bitmap_commits_offset(struct repository *r)\n> {\n> struct object_id oid;\n> - struct stored_bitmap_tag_pos *tagged;\n> + struct stored_bitmap *bitmap;\n> struct bitmap_index *bitmap_git;\n> size_t commit_idx_pos_map_pos, xor_offset_map_pos, flag_map_pos,\n> ewah_bitmap_map_pos;\n> \n> - tag_pos_on_bitmap = 1;\n> bitmap_git = prepare_bitmap_git(r);\n> if (!bitmap_git)\n> die(_(\"failed to load bitmap indexes\"));\n> @@ -2897,9 +2887,9 @@ int test_bitmap_commits_offset(struct repository *r)\n> die(_(\"failed to load bitmap indexes\"));\n> }\n> \n> - kh_foreach (bitmap_git->bitmaps, oid, tagged, {\n> - commit_idx_pos_map_pos = tagged->map_pos;\n> - xor_offset_map_pos = tagged->map_pos + sizeof(uint32_t);\n> + kh_foreach (bitmap_git->bitmaps, oid, bitmap, {\n> + commit_idx_pos_map_pos = bitmap->map_pos;\n> + xor_offset_map_pos = bitmap->map_pos + sizeof(uint32_t);\n> flag_map_pos = xor_offset_map_pos + sizeof(uint8_t);\n> ewah_bitmap_map_pos = flag_map_pos + sizeof(uint8_t);\n> --- >8 ---\n> \n>> stored->root = root;\n>> stored->xor = xor_with;\n>> stored->flags = flags;\n>> @@ -376,10 +387,12 @@ static int load_bitmap_entries_v1(struct bitmap_index *index)\n>> struct stored_bitmap *xor_bitmap = NULL;\n>> uint32_t commit_idx_pos;\n>> struct object_id oid;\n>> + size_t entry_map_pos;\n>> \n>> if (index->map_size - index->map_pos < 6)\n>> return error(_(\"corrupt ewah bitmap: truncated header for entry %d\"), i);\n>> \n>> + entry_map_pos = index->map_pos;\n> \n> Good. This is important since the read_be32() and read_u8() calls below\n> both adjust the value of index->map_pos past the beginning of the bitmap.\n> \n>> @@ -869,6 +883,7 @@ static struct stored_bitmap *lazy_bitmap_for_commit(struct bitmap_index *bitmap_\n>> int xor_flags;\n>> khiter_t hash_pos;\n>> struct bitmap_lookup_table_xor_item *xor_item;\n>> + size_t entry_map_pos;\n>> \n>> if (is_corrupt)\n>> return NULL;\n>> @@ -928,6 +943,7 @@ static struct stored_bitmap *lazy_bitmap_for_commit(struct bitmap_index *bitmap_\n>> goto corrupt;\n>> }\n>> \n>> + entry_map_pos = bitmap_git->map_pos;\n> \n> Same here.\n> \n>> @@ -969,6 +986,7 @@ static struct stored_bitmap *lazy_bitmap_for_commit(struct bitmap_index *bitmap_\n>> * Instead, we can skip ahead and immediately read the flags and\n>> * ewah bitmap.\n>> */\n>> + entry_map_pos = bitmap_git->map_pos;\n> \n> And here.\n> \n>> +int test_bitmap_commits_offset(struct repository *r)\n>> +{\n>> + struct object_id oid;\n>> + struct stored_bitmap_tag_pos *tagged;\n>> + struct bitmap_index *bitmap_git;\n>> + size_t commit_idx_pos_map_pos, xor_offset_map_pos, flag_map_pos,\n>> + ewah_bitmap_map_pos;\n>> +\n>> + tag_pos_on_bitmap = 1;\n>> + bitmap_git = prepare_bitmap_git(r);\n>> + if (!bitmap_git)\n>> + die(_(\"failed to load bitmap indexes\"));\n>> +\n> \n> If we either forgot to set this variable here or did so after calling\n> prepare_bitmap_git(), then we wouldn't allocate enough memory to store\n> the map_pos field in the stored_bitmap_tag_pos structure. When we then\n> would try and read that field below, we'd read garbage heap data outside\n> of our structure.\n> \n>> + /*\n>> + * As this function is only used to print bitmap selected\n>> + * commits, we don't have to read the commit table.\n>> + */\n>> + if (bitmap_git->table_lookup) {\n>> + if (load_bitmap_entries_v1(bitmap_git) < 0)\n>> + die(_(\"failed to load bitmap indexes\"));\n>> + }\n> \n> This comment suggests that we can avoid reading the commit table\n> altogether. Indeed, calling load_bitmap_entries_v1() here does that,\n> since it is not called when loading a bitmap that has a lookup table.\n> \n> So I think the behavior here is correct, but the comment is misleading.\n> I suspect that the confusion would be resolved by instead writing:\n> \n> /*\n> * Since this function needs to know the position of each individual\n> * bitmap, bypass the commit lookup table (if one exists) by forcing\n> * the bitmap to eagerly load its entries.\n> */\n> \n> I think this is copy-and-paste from 28cd730680 (pack-bitmap: prepare to\n> read lookup table extension, 2022-08-14) via the 'test_bitmap_commits()'\n> function immediately above this one. I think both would benefit from\n> some clean-up, since this comment is equally misleading in that\n> function.\n> \n> For your purposes, I would either:\n> \n> - remove or (preferably) reword the comment in your new function,\n> leaving the one in test_bitmap_commits() as-is, or\n> \n> - reword the comment in test_bitmap_commits() to be more like the one\n> above, via a preparatory commit, and then introduce the new function\n> using the same wording.\n> \n> Between the two, I think the latter is preferable.\n\nI will add a new commit reword the comment before the last addt-test-case commit.\n\n> \n> As an aside, I think that for bitmaps that do have a commit lookup\n> table, you could go slightly faster here by walking over that portion of\n> the *.bitmap file, since it directly encodes the information you're\n> interested in here. But I would avoid doing that, since it too seems\n> brittle and I would like to avoid having two separate spots that each\n> implement reading the commit table format.\n> \n>> + kh_foreach (bitmap_git->bitmaps, oid, tagged, {\n>> + commit_idx_pos_map_pos = tagged->map_pos;\n> \n> OK, and here's where we pull out the actual position of the selected\n> commit's bitmap.\n> \n>> + xor_offset_map_pos = tagged->map_pos + sizeof(uint32_t);\n>> + flag_map_pos = xor_offset_map_pos + sizeof(uint8_t);\n>> + ewah_bitmap_map_pos = flag_map_pos + sizeof(uint8_t);\n>> +\n>> + printf_ln(\"%s %\"PRIuMAX\" %\"PRIuMAX\" %\"PRIuMAX\" %\"PRIuMAX,\n>> + oid_to_hex(&oid),\n>> + (uintmax_t)commit_idx_pos_map_pos,\n>> + (uintmax_t)xor_offset_map_pos,\n>> + (uintmax_t)flag_map_pos,\n>> + (uintmax_t)ewah_bitmap_map_pos);\n> \n> Hmm. We print more information here than just the map_pos. This is\n> brittle if the on-disk format changes (e.g., to store the XOR offsets in\n> some other part of the bitmap). But hopefully future updates to the\n> bitmap format will come with updates to this function as well ;-).\n> \n> It feels somewhat unsatisfying to print output like:\n> \n> $COMMIT_OID \n> \n> , but I think it makes sense here for a couple of reasons:\n> \n> - If we just print the , then the test is responsible for\n> knowing the distance between that and the XOR offset, which extends\n> the brittleness to the test code\n> \n> - likewise, if we print out just the map_pos and the positions of the\n> XOR offset, it feels strange to omit the others.\n> \n>> diff --git a/pack-bitmap.h b/pack-bitmap.h\n>> index 382d39499af2..96880ba3d72d 100644\n>> --- a/pack-bitmap.h\n>> +++ b/pack-bitmap.h\n>> @@ -81,6 +81,7 @@ void traverse_bitmap_commit_list(struct bitmap_index *,\n>> show_reachable_fn show_reachable);\n>> void test_bitmap_walk(struct rev_info *revs);\n>> int test_bitmap_commits(struct repository *r);\n>> +int test_bitmap_commits_offset(struct repository *r);\n>> 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>> diff --git a/t/helper/test-bitmap.c b/t/helper/test-bitmap.c\n>> index 3f23f2107268..65a1ab29192b 100644\n>> --- a/t/helper/test-bitmap.c\n>> +++ b/t/helper/test-bitmap.c\n>> @@ -10,6 +10,11 @@ static int bitmap_list_commits(void)\n>> return test_bitmap_commits(the_repository);\n>> }\n>> \n>> +static int bitmap_list_commits_offset(void)\n>> +{\n>> + return test_bitmap_commits_offset(the_repository);\n>> +}\n>> +\n>> static int bitmap_dump_hashes(void)\n>> {\n>> return test_bitmap_hashes(the_repository);\n>> @@ -36,6 +41,8 @@ int cmd__bitmap(int argc, const char **argv)\n>> \n>> if (argc == 2 && !strcmp(argv[1], \"list-commits\"))\n>> return bitmap_list_commits();\n>> + if (argc == 2 && !strcmp(argv[1], \"list-commits-offset\"))\n>> + return bitmap_list_commits_offset();\n> \n> All of the scaffolding here looks good.\n> \n> This new mode reads a little awkwardly to me (but may not to others, in\n> which case I am happy to back away from the following suggestion). Can\n> we either call this 'list-commits-with-offset' or 'list-commit-offsets'?\n> I have a vague preference towards the former since the new mode has a\n> string prefix matching the existing mode.\n> \n>> + test_expect_success 'load corrupt bitmap' '\n>> + rm -fr repo &&\n>> + git init repo &&\n>> + test_when_finished \"rm -fr repo\" &&\n>> + (\n>> + cd repo &&\n>> + git config pack.writeBitmapLookupTable '\"$writeLookupTable\"' &&\n>> +\n>> + test_commit base &&\n>> +\n>> + git repack -adb &&\n>> + bitmap=\"$(ls .git/objects/pack/pack-*.bitmap)\" &&\n>> + chmod +w $bitmap &&\n>> +\n>> + read oid commit_off xor_off flag_off ewah_off <<-EOF &&\n>> + $(test-tool bitmap list-commits-offset | head -n 1)\n> \n> We avoid putting 'git' or 'test-tool' on the left-hand side of a pipe,\n> since we want to avoid squelching any errors / segfaults from our code.\n> \n> How about (assuming the rename above):\n> \n> test-tool bitmap list-commit-offsets >offsets &&\n> xor_off=\"$(head -n1 offsets | awk '{print $3}')\" &&\n> ...\n> \n> ?\n> \n>> + printf '\\161' |\n>> + dd of=$bitmap count=1 bs=1 conv=notrunc seek=$xor_off &&\n>> +\n>> +\n>> + git rev-list --count HEAD > expect &&\n>> + git rev-list --use-bitmap-index --count HEAD > actual &&\n> \n> Using --count can mask failures in the bitmap code since it only guards\n> you against getting the wrong number of objects, but doesn't guard you\n> against getting the right amount of objects in a permuted order. I think\n> we'd want just 'git rev-list --objects' here, but note that you have to\n> use '--no-object-names' on the non-bitmap side, since rev-list does not\n> print out paths when using bitmaps[^1].\n> \n> How about:\n> \n> git rev-list --objects --no-object-names HEAD >expect.raw &&\n> git rev-list --objects --use-bitmap-index --no-object-names HEAD \\\n>> actual.raw &&\n> \n> sort expect.raw >expect &&\n> sort actual.raw >actual &&\n> \n> test_cmp expect actual\n> \n> (Note that you also have to sort the output here since rev-list does not\n> output objects in a meaningful order when using bitmaps.)\n> \n> Thanks,\n> Taylor\n> \n> [^1]: Not that it matters here since we don't expect to load the bitmap\n> anyway, but it's worth doing regardless.\n> \n\nGood advice, Thanks\nLidong"},{"id":"519525","messageId":"pull.1962.v5.git.git.1748920444.gitgitgadget@gmail.com","threadId":"63447","inReplyTo":"pull.1962.v4.git.git.1748140983.gitgitgadget@gmail.com","subject":"[PATCH v5 0/3] pack-bitmap: fix memory leak if load_bitmap failed","fromName":"Lidong Yan via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-06-03T03:14:01Z","receivedAt":"2025-06-03T03:14:09Z","isPatch":true,"sender":{"key":"yldhome2d2@gmail.com","avatar":"https://avatars.githubusercontent.com/u/77328395?v=4"},"body":"This patch prevents pack-bitmap.c:load_bitmap() from nulling\nbitmap_git->bitmap when loading failed thus eliminates memory leak. This\npatch also add a test case in t5310 which use clang leak sanitizer to detect\nwhether leak happens when loading failed.\n\nLidong Yan (2):\n pack-bitmap: reword comments in test_bitmap_commits()\n pack-bitmap: add load corrupt bitmap test\n\nTaylor Blau (1):\n pack-bitmap: fix memory leak if load_bitmap() failed\n\n pack-bitmap.c | 88 ++++++++++++++++++++++++++++++-----------\n pack-bitmap.h | 1 +\n t/helper/test-bitmap.c | 8 ++++\n t/t5310-pack-bitmaps.sh | 30 ++++++++++++++\n 4 files changed, 103 insertions(+), 24 deletions(-)\n\n\nbase-commit: 845c48a16a7f7b2c44d8cb137b16a4a1f0140229\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1962%2Fbrandb97%2Ffix-pack-bitmap-leak-v5\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1962/brandb97/fix-pack-bitmap-leak-v5\nPull-Request: https://github.com/git/git/pull/1962\n\nRange-diff vs v4:\n\n 1: b6b3a83a224 ! 1: 9ce2135df2a pack-bitmap: fix memory leak if load_bitmap() failed\n @@ Commit message\n its caller will always call free_bitmap_index() in case of an error.\n \n Signed-off-by: Taylor Blau \n + Signed-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>\n \n ## pack-bitmap.c ##\n @@ pack-bitmap.c: static int load_bitmap(struct repository *r, struct bitmap_index *bitmap_git,\n -: ----------- > 2: a75d0a3cc7f pack-bitmap: reword comments in test_bitmap_commits()\n 2: 7876d9a9014 ! 3: 05140e2171d pack-bitmap: add load corrupt bitmap test\n @@ Commit message\n \n ## pack-bitmap.c ##\n @@ pack-bitmap.c: struct stored_bitmap {\n + \tstruct object_id oid;\n + \tstruct ewah_bitmap *root;\n + \tstruct stored_bitmap *xor;\n ++\tsize_t map_pos;\n \tint flags;\n };\n \n -+struct stored_bitmap_tag_pos {\n -+\tstruct stored_bitmap stored;\n -+\tsize_t map_pos;\n -+};\n -+\n - /*\n - * The active bitmap index for a repository. By design, repositories only have\n - * a single bitmap index available (the index for the biggest packfile in\n -@@ pack-bitmap.c: static int existing_bitmaps_hits_nr;\n - static int existing_bitmaps_misses_nr;\n - static int roots_with_bitmaps_nr;\n - static int roots_without_bitmaps_nr;\n -+static int tag_pos_on_bitmap;\n - \n - static struct ewah_bitmap *lookup_stored_bitmap(struct stored_bitmap *st)\n - {\n @@ pack-bitmap.c: static struct stored_bitmap *store_bitmap(struct bitmap_index *index,\n \t\t\t\t\t struct ewah_bitmap *root,\n \t\t\t\t\t const struct object_id *oid,\n @@ pack-bitmap.c: static struct stored_bitmap *store_bitmap(struct bitmap_index *in\n +\t\t\t\t\t int flags, size_t map_pos)\n {\n \tstruct stored_bitmap *stored;\n -+\tstruct stored_bitmap_tag_pos *tagged;\n \tkhiter_t hash_pos;\n \tint ret;\n \n --\tstored = xmalloc(sizeof(struct stored_bitmap));\n -+\ttagged = xmalloc(tag_pos_on_bitmap ? sizeof(struct stored_bitmap_tag_pos) :\n -+\t\t\t\t\t sizeof(struct stored_bitmap));\n -+\tstored = &tagged->stored;\n -+\tif (tag_pos_on_bitmap)\n -+\t\ttagged->map_pos = map_pos;\n + \tstored = xmalloc(sizeof(struct stored_bitmap));\n ++\tstored->map_pos = map_pos;\n \tstored->root = root;\n \tstored->xor = xor_with;\n \tstored->flags = flags;\n @@ pack-bitmap.c: int test_bitmap_commits(struct repository *r)\n \treturn 0;\n }\n \n -+int test_bitmap_commits_offset(struct repository *r)\n ++int test_bitmap_commits_with_offset(struct repository *r)\n +{\n +\tstruct object_id oid;\n -+\tstruct stored_bitmap_tag_pos *tagged;\n ++\tstruct stored_bitmap *stored;\n +\tstruct bitmap_index *bitmap_git;\n +\tsize_t commit_idx_pos_map_pos, xor_offset_map_pos, flag_map_pos,\n +\t\tewah_bitmap_map_pos;\n +\n -+\ttag_pos_on_bitmap = 1;\n +\tbitmap_git = prepare_bitmap_git(r);\n +\tif (!bitmap_git)\n +\t\tdie(_(\"failed to load bitmap indexes\"));\n +\n +\t/*\n -+\t * As this function is only used to print bitmap selected\n -+\t * commits, we don't have to read the commit table.\n ++\t * Since this function needs to know the position of each individual\n ++\t * bitmap, bypass the commit lookup table (if one exists) by forcing\n ++\t * the bitmap to eagerly load its entries.\n +\t */\n +\tif (bitmap_git->table_lookup) {\n +\t\tif (load_bitmap_entries_v1(bitmap_git) < 0)\n +\t\t\tdie(_(\"failed to load bitmap indexes\"));\n +\t}\n +\n -+\tkh_foreach (bitmap_git->bitmaps, oid, tagged, {\n -+\t\tcommit_idx_pos_map_pos = tagged->map_pos;\n -+\t\txor_offset_map_pos = tagged->map_pos + sizeof(uint32_t);\n ++\tkh_foreach (bitmap_git->bitmaps, oid, stored, {\n ++\t\tcommit_idx_pos_map_pos = stored->map_pos;\n ++\t\txor_offset_map_pos = stored->map_pos + sizeof(uint32_t);\n +\t\tflag_map_pos = xor_offset_map_pos + sizeof(uint8_t);\n +\t\tewah_bitmap_map_pos = flag_map_pos + sizeof(uint8_t);\n +\n @@ pack-bitmap.h: void traverse_bitmap_commit_list(struct bitmap_index *,\n \t\t\t\t show_reachable_fn show_reachable);\n void test_bitmap_walk(struct rev_info *revs);\n int test_bitmap_commits(struct repository *r);\n -+int test_bitmap_commits_offset(struct repository *r);\n ++int test_bitmap_commits_with_offset(struct repository *r);\n 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 @@ t/helper/test-bitmap.c: static int bitmap_list_commits(void)\n \treturn test_bitmap_commits(the_repository);\n }\n \n -+static int bitmap_list_commits_offset(void)\n ++static int bitmap_list_commits_with_offset(void)\n +{\n -+\treturn test_bitmap_commits_offset(the_repository);\n ++\treturn test_bitmap_commits_with_offset(the_repository);\n +}\n +\n static int bitmap_dump_hashes(void)\n @@ t/helper/test-bitmap.c: int cmd__bitmap(int argc, const char **argv)\n \n \tif (argc == 2 && !strcmp(argv[1], \"list-commits\"))\n \t\treturn bitmap_list_commits();\n -+\tif (argc == 2 && !strcmp(argv[1], \"list-commits-offset\"))\n -+\t\treturn bitmap_list_commits_offset();\n ++\tif (argc == 2 && !strcmp(argv[1], \"list-commits-with-offset\"))\n ++\t\treturn bitmap_list_commits_with_offset();\n \tif (argc == 2 && !strcmp(argv[1], \"dump-hashes\"))\n \t\treturn bitmap_dump_hashes();\n \tif (argc == 2 && !strcmp(argv[1], \"dump-pseudo-merges\"))\n @@ t/helper/test-bitmap.c: int cmd__bitmap(int argc, const char **argv)\n \t\treturn bitmap_dump_pseudo_merge_objects(atoi(argv[2]));\n \n \tusage(\"\\ttest-tool bitmap list-commits\\n\"\n -+\t \"\\ttest-tool bitmap list-commits-offset\\n\"\n ++\t \"\\ttest-tool bitmap list-commits-with-offset\\n\"\n \t \"\\ttest-tool bitmap dump-hashes\\n\"\n \t \"\\ttest-tool bitmap dump-pseudo-merges\\n\"\n \t \"\\ttest-tool bitmap dump-pseudo-merge-commits \\n\"\n @@ t/t5310-pack-bitmaps.sh: test_bitmap_cases () {\n +\t\t\tbitmap=\"$(ls .git/objects/pack/pack-*.bitmap)\" &&\n +\t\t\tchmod +w $bitmap &&\n +\n -+\t\t\tread oid commit_off xor_off flag_off ewah_off <<-EOF &&\n -+\t\t\t\t$(test-tool bitmap list-commits-offset | head -n 1)\n -+\t\t\tEOF\n ++\t\t\ttest-tool bitmap list-commits-with-offset >offsets &&\n ++\t\t\txor_off=$(head -n1 offsets | awk \"{print \\$3}\") &&\n +\t\t\tprintf '\\161' |\n +\t\t\t\tdd of=$bitmap count=1 bs=1 conv=notrunc seek=$xor_off &&\n +\n ++\t\t\tgit rev-list --objects --no-object-names HEAD >expect.raw &&\n ++\t\t\tgit rev-list --objects --use-bitmap-index --no-object-names HEAD \\\n ++\t\t\t\t>actual.raw &&\n ++\n ++\t\t\tsort expect.raw >expect &&\n ++\t\t\tsort actual.raw >actual &&\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 test_cmp expect actual\n +\t\t)\n +\t'\n }\n\n-- \ngitgitgadget\n"},{"id":"519526","messageId":"9ce2135df2a1f728fd24b99f171f3d6dfe8dc350.1748920444.git.gitgitgadget@gmail.com","threadId":"63447","inReplyTo":"pull.1962.v5.git.git.1748920444.gitgitgadget@gmail.com","subject":"[PATCH v5 1/3] pack-bitmap: fix memory leak if load_bitmap() failed","fromName":"Taylor Blau via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-06-03T03:14:02Z","receivedAt":"2025-06-03T03:14:09Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"From: Taylor Blau \n\nAfter going through the \"failed\" label, load_bitmap() will return -1,\nand its caller (either prepare_bitmap_walk() or prepare_bitmap_git())\nwill then call free_bitmap_index().\n\nThat function would have done:\n\n struct stored_bitmap *sb;\n kh_foreach_value(b->bitmaps, sb {\n ewah_pool_free(sb->root);\n free(sb);\n });\n\n, but won't since load_bitmap() already called kh_destroy_oid_map() and\nNULL'd the \"bitmaps\" pointer from within its \"failed\" label.\n\nSo I think if you got part of the way through loading bitmap entries and\nthen failed, you would leak all of the previous entries that you were\nable to load successfully.\n\nThe solution is to remove the error handling code in load_bitmap(), because\nits caller will always call free_bitmap_index() in case of an error.\n\nSigned-off-by: Taylor Blau \nSigned-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>\n---\n pack-bitmap.c | 21 ++++-----------------\n 1 file changed, 4 insertions(+), 17 deletions(-)\n\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex ac6d62b980c5..fd19c2255163 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -630,41 +630,28 @@ static int load_bitmap(struct repository *r, struct bitmap_index *bitmap_git,\n \tbitmap_git->ext_index.positions = kh_init_oid_pos();\n \n \tif (load_reverse_index(r, bitmap_git))\n-\t\tgoto failed;\n+\t\treturn -1;\n \n \tif (!(bitmap_git->commits = read_bitmap_1(bitmap_git)) ||\n \t\t!(bitmap_git->trees = read_bitmap_1(bitmap_git)) ||\n \t\t!(bitmap_git->blobs = read_bitmap_1(bitmap_git)) ||\n \t\t!(bitmap_git->tags = read_bitmap_1(bitmap_git)))\n-\t\tgoto failed;\n+\t\treturn -1;\n \n \tif (!bitmap_git->table_lookup && load_bitmap_entries_v1(bitmap_git) < 0)\n-\t\tgoto failed;\n+\t\treturn -1;\n \n \tif (bitmap_git->base) {\n \t\tif (!bitmap_is_midx(bitmap_git))\n \t\t\tBUG(\"non-MIDX bitmap has non-NULL base bitmap index\");\n \t\tif (load_bitmap(r, bitmap_git->base, 1) < 0)\n-\t\t\tgoto failed;\n+\t\t\treturn -1;\n \t}\n \n \tif (!recursing)\n \t\tload_all_type_bitmaps(bitmap_git);\n \n \treturn 0;\n-\n-failed:\n-\tmunmap(bitmap_git->map, bitmap_git->map_size);\n-\tbitmap_git->map = NULL;\n-\tbitmap_git->map_size = 0;\n-\n-\tkh_destroy_oid_map(bitmap_git->bitmaps);\n-\tbitmap_git->bitmaps = NULL;\n-\n-\tkh_destroy_oid_pos(bitmap_git->ext_index.positions);\n-\tbitmap_git->ext_index.positions = NULL;\n-\n-\treturn -1;\n }\n \n static int open_pack_bitmap(struct repository *r,\n-- \ngitgitgadget\n\n"},{"id":"519527","messageId":"a75d0a3cc7fc78d13e7703bd02a7e30fbd601831.1748920445.git.gitgitgadget@gmail.com","threadId":"63447","inReplyTo":"pull.1962.v5.git.git.1748920444.gitgitgadget@gmail.com","subject":"[PATCH v5 2/3] pack-bitmap: reword comments in test_bitmap_commits()","fromName":"Lidong Yan via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-06-03T03:14:03Z","receivedAt":"2025-06-03T03:14:11Z","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\nIn pack-bitmap.c:test_bitmap_commits(), it comments\n\n /*\n * As this function is only used to print bitmap selected\n * commits, we don't have to read the commit table.\n */\n\nThis suggests that we can avoid reading the commit table altogether.\nHowever, this comment is misleading. The reason we load bitmap entries here\nis because test_bitmap_commits() needs to print the commit IDs from the\nbitmap, and we must read the bitmap entries to obtain those commit IDs.\nSo reword this comment.\n\nSigned-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>\n---\n pack-bitmap.c | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex fd19c2255163..e514c9da239b 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -2839,8 +2839,9 @@ int test_bitmap_commits(struct repository *r)\n \t\tdie(_(\"failed to load bitmap indexes\"));\n \n \t/*\n-\t * As this function is only used to print bitmap selected\n-\t * commits, we don't have to read the commit table.\n+\t * Since this function needs to print bitmap selected\n+\t * commits, bypass the commit lookup table (if one exists)\n+\t * by forcing the bitmap to eagerly load its entries.\n \t */\n \tif (bitmap_git->table_lookup) {\n \t\tif (load_bitmap_entries_v1(bitmap_git) < 0)\n-- \ngitgitgadget\n\n"},{"id":"519528","messageId":"05140e2171d393b89faf8b47ee3bcceac3d7b2ff.1748920445.git.gitgitgadget@gmail.com","threadId":"63447","inReplyTo":"pull.1962.v5.git.git.1748920444.gitgitgadget@gmail.com","subject":"[PATCH v5 3/3] pack-bitmap: add load corrupt bitmap test","fromName":"Lidong Yan via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-06-03T03:14:04Z","receivedAt":"2025-06-03T03:14:11Z","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 test_bitmap_list_commits_offset() in patch-bitmap.c,\na new test helper command `test-tool bitmap list-commits-offset`,\nand a `load corrupt bitmap` test case in t5310.\n\nThe `load corrupt bitmap` test case intentionally corrupt the\n\"xor_offset\" field of the first entry. And the newly added helper\ncan help to find position of \"xor_offset\" in bitmap file.\n\nSigned-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>\n---\n pack-bitmap.c | 62 +++++++++++++++++++++++++++++++++++++----\n pack-bitmap.h | 1 +\n t/helper/test-bitmap.c | 8 ++++++\n t/t5310-pack-bitmaps.sh | 30 ++++++++++++++++++++\n 4 files changed, 96 insertions(+), 5 deletions(-)\n\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex e514c9da239b..0825129b58f3 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -31,6 +31,7 @@ struct stored_bitmap {\n \tstruct object_id oid;\n \tstruct ewah_bitmap *root;\n \tstruct stored_bitmap *xor;\n+\tsize_t map_pos;\n \tint flags;\n };\n \n@@ -314,13 +315,14 @@ static struct stored_bitmap *store_bitmap(struct bitmap_index *index,\n \t\t\t\t\t struct ewah_bitmap *root,\n \t\t\t\t\t const struct object_id *oid,\n \t\t\t\t\t struct stored_bitmap *xor_with,\n-\t\t\t\t\t int flags)\n+\t\t\t\t\t int flags, size_t map_pos)\n {\n \tstruct stored_bitmap *stored;\n \tkhiter_t hash_pos;\n \tint ret;\n \n \tstored = xmalloc(sizeof(struct stored_bitmap));\n+\tstored->map_pos = map_pos;\n \tstored->root = root;\n \tstored->xor = xor_with;\n \tstored->flags = flags;\n@@ -376,10 +378,12 @@ static int load_bitmap_entries_v1(struct bitmap_index *index)\n \t\tstruct stored_bitmap *xor_bitmap = NULL;\n \t\tuint32_t commit_idx_pos;\n \t\tstruct object_id oid;\n+\t\tsize_t entry_map_pos;\n \n \t\tif (index->map_size - index->map_pos < 6)\n \t\t\treturn error(_(\"corrupt ewah bitmap: truncated header for entry %d\"), i);\n \n+\t\tentry_map_pos = index->map_pos;\n \t\tcommit_idx_pos = read_be32(index->map, &index->map_pos);\n \t\txor_offset = read_u8(index->map, &index->map_pos);\n \t\tflags = read_u8(index->map, &index->map_pos);\n@@ -402,8 +406,9 @@ static int load_bitmap_entries_v1(struct bitmap_index *index)\n \t\tif (!bitmap)\n \t\t\treturn -1;\n \n-\t\trecent_bitmaps[i % MAX_XOR_OFFSET] = store_bitmap(\n-\t\t\tindex, bitmap, &oid, xor_bitmap, flags);\n+\t\trecent_bitmaps[i % MAX_XOR_OFFSET] =\n+\t\t\tstore_bitmap(index, bitmap, &oid, xor_bitmap, flags,\n+\t\t\t\t entry_map_pos);\n \t}\n \n \treturn 0;\n@@ -869,6 +874,7 @@ static struct stored_bitmap *lazy_bitmap_for_commit(struct bitmap_index *bitmap_\n \tint xor_flags;\n \tkhiter_t hash_pos;\n \tstruct bitmap_lookup_table_xor_item *xor_item;\n+\tsize_t entry_map_pos;\n \n \tif (is_corrupt)\n \t\treturn NULL;\n@@ -928,6 +934,7 @@ static struct stored_bitmap *lazy_bitmap_for_commit(struct bitmap_index *bitmap_\n \t\t\tgoto corrupt;\n \t\t}\n \n+\t\tentry_map_pos = bitmap_git->map_pos;\n \t\tbitmap_git->map_pos += sizeof(uint32_t) + sizeof(uint8_t);\n \t\txor_flags = read_u8(bitmap_git->map, &bitmap_git->map_pos);\n \t\tbitmap = read_bitmap_1(bitmap_git);\n@@ -935,7 +942,8 @@ static struct stored_bitmap *lazy_bitmap_for_commit(struct bitmap_index *bitmap_\n \t\tif (!bitmap)\n \t\t\tgoto corrupt;\n \n-\t\txor_bitmap = store_bitmap(bitmap_git, bitmap, &xor_item->oid, xor_bitmap, xor_flags);\n+\t\txor_bitmap = store_bitmap(bitmap_git, bitmap, &xor_item->oid,\n+\t\t\t\t\t xor_bitmap, xor_flags, entry_map_pos);\n \t\txor_items_nr--;\n \t}\n \n@@ -969,6 +977,7 @@ static struct stored_bitmap *lazy_bitmap_for_commit(struct bitmap_index *bitmap_\n \t * Instead, we can skip ahead and immediately read the flags and\n \t * ewah bitmap.\n \t */\n+\tentry_map_pos = bitmap_git->map_pos;\n \tbitmap_git->map_pos += sizeof(uint32_t) + sizeof(uint8_t);\n \tflags = read_u8(bitmap_git->map, &bitmap_git->map_pos);\n \tbitmap = read_bitmap_1(bitmap_git);\n@@ -976,7 +985,8 @@ static struct stored_bitmap *lazy_bitmap_for_commit(struct bitmap_index *bitmap_\n \tif (!bitmap)\n \t\tgoto corrupt;\n \n-\treturn store_bitmap(bitmap_git, bitmap, oid, xor_bitmap, flags);\n+\treturn store_bitmap(bitmap_git, bitmap, oid, xor_bitmap, flags,\n+\t\t\t entry_map_pos);\n \n corrupt:\n \tfree(xor_items);\n@@ -2857,6 +2867,48 @@ int test_bitmap_commits(struct repository *r)\n \treturn 0;\n }\n \n+int test_bitmap_commits_with_offset(struct repository *r)\n+{\n+\tstruct object_id oid;\n+\tstruct stored_bitmap *stored;\n+\tstruct bitmap_index *bitmap_git;\n+\tsize_t commit_idx_pos_map_pos, xor_offset_map_pos, flag_map_pos,\n+\t\tewah_bitmap_map_pos;\n+\n+\tbitmap_git = prepare_bitmap_git(r);\n+\tif (!bitmap_git)\n+\t\tdie(_(\"failed to load bitmap indexes\"));\n+\n+\t/*\n+\t * Since this function needs to know the position of each individual\n+\t * bitmap, bypass the commit lookup table (if one exists) by forcing\n+\t * the bitmap to eagerly load its entries.\n+\t */\n+\tif (bitmap_git->table_lookup) {\n+\t\tif (load_bitmap_entries_v1(bitmap_git) < 0)\n+\t\t\tdie(_(\"failed to load bitmap indexes\"));\n+\t}\n+\n+\tkh_foreach (bitmap_git->bitmaps, oid, stored, {\n+\t\tcommit_idx_pos_map_pos = stored->map_pos;\n+\t\txor_offset_map_pos = stored->map_pos + sizeof(uint32_t);\n+\t\tflag_map_pos = xor_offset_map_pos + sizeof(uint8_t);\n+\t\tewah_bitmap_map_pos = flag_map_pos + sizeof(uint8_t);\n+\n+\t\tprintf_ln(\"%s %\"PRIuMAX\" %\"PRIuMAX\" %\"PRIuMAX\" %\"PRIuMAX,\n+\t\t\t oid_to_hex(&oid),\n+\t\t\t (uintmax_t)commit_idx_pos_map_pos,\n+\t\t\t (uintmax_t)xor_offset_map_pos,\n+\t\t\t (uintmax_t)flag_map_pos,\n+\t\t\t (uintmax_t)ewah_bitmap_map_pos);\n+\t})\n+\t\t;\n+\n+\tfree_bitmap_index(bitmap_git);\n+\n+\treturn 0;\n+}\n+\n int test_bitmap_hashes(struct repository *r)\n {\n \tstruct bitmap_index *bitmap_git = prepare_bitmap_git(r);\ndiff --git a/pack-bitmap.h b/pack-bitmap.h\nindex 382d39499af2..1bd7a791e2a0 100644\n--- a/pack-bitmap.h\n+++ b/pack-bitmap.h\n@@ -81,6 +81,7 @@ void traverse_bitmap_commit_list(struct bitmap_index *,\n \t\t\t\t show_reachable_fn show_reachable);\n void test_bitmap_walk(struct rev_info *revs);\n int test_bitmap_commits(struct repository *r);\n+int test_bitmap_commits_with_offset(struct repository *r);\n 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);\ndiff --git a/t/helper/test-bitmap.c b/t/helper/test-bitmap.c\nindex 3f23f2107268..16a01669e414 100644\n--- a/t/helper/test-bitmap.c\n+++ b/t/helper/test-bitmap.c\n@@ -10,6 +10,11 @@ static int bitmap_list_commits(void)\n \treturn test_bitmap_commits(the_repository);\n }\n \n+static int bitmap_list_commits_with_offset(void)\n+{\n+\treturn test_bitmap_commits_with_offset(the_repository);\n+}\n+\n static int bitmap_dump_hashes(void)\n {\n \treturn test_bitmap_hashes(the_repository);\n@@ -36,6 +41,8 @@ int cmd__bitmap(int argc, const char **argv)\n \n \tif (argc == 2 && !strcmp(argv[1], \"list-commits\"))\n \t\treturn bitmap_list_commits();\n+\tif (argc == 2 && !strcmp(argv[1], \"list-commits-with-offset\"))\n+\t\treturn bitmap_list_commits_with_offset();\n \tif (argc == 2 && !strcmp(argv[1], \"dump-hashes\"))\n \t\treturn bitmap_dump_hashes();\n \tif (argc == 2 && !strcmp(argv[1], \"dump-pseudo-merges\"))\n@@ -46,6 +53,7 @@ int cmd__bitmap(int argc, const char **argv)\n \t\treturn bitmap_dump_pseudo_merge_objects(atoi(argv[2]));\n \n \tusage(\"\\ttest-tool bitmap list-commits\\n\"\n+\t \"\\ttest-tool bitmap list-commits-with-offset\\n\"\n \t \"\\ttest-tool bitmap dump-hashes\\n\"\n \t \"\\ttest-tool bitmap dump-pseudo-merges\\n\"\n \t \"\\ttest-tool bitmap dump-pseudo-merge-commits \\n\"\ndiff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh\nindex a62b463eaf09..df05d7419185 100755\n--- a/t/t5310-pack-bitmaps.sh\n+++ b/t/t5310-pack-bitmaps.sh\n@@ -486,6 +486,36 @@ 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\trm -fr repo &&\n+\t\tgit init repo &&\n+\t\ttest_when_finished \"rm -fr repo\" &&\n+\t\t(\n+\t\t\tcd repo &&\n+\t\t\tgit config pack.writeBitmapLookupTable '\"$writeLookupTable\"' &&\n+\n+\t\t\ttest_commit base &&\n+\n+\t\t\tgit repack -adb &&\n+\t\t\tbitmap=\"$(ls .git/objects/pack/pack-*.bitmap)\" &&\n+\t\t\tchmod +w $bitmap &&\n+\n+\t\t\ttest-tool bitmap list-commits-with-offset >offsets &&\n+\t\t\txor_off=$(head -n1 offsets | awk \"{print \\$3}\") &&\n+\t\t\tprintf '\\161' |\n+\t\t\t\tdd of=$bitmap count=1 bs=1 conv=notrunc seek=$xor_off &&\n+\n+\t\t\tgit rev-list --objects --no-object-names HEAD >expect.raw &&\n+\t\t\tgit rev-list --objects --use-bitmap-index --no-object-names HEAD \\\n+\t\t\t\t>actual.raw &&\n+\n+\t\t\tsort expect.raw >expect &&\n+\t\t\tsort actual.raw >actual &&\n+\n+\t\t test_cmp expect actual\n+\t\t)\n+\t'\n }\n \n test_bitmap_cases\n-- \ngitgitgadget\n"},{"id":"519602","messageId":"aD9zo30dXflldlGt@nand.local","threadId":"63447","inReplyTo":"a75d0a3cc7fc78d13e7703bd02a7e30fbd601831.1748920445.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v5 2/3] pack-bitmap: reword comments in test_bitmap_commits()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-06-03T22:13:55Z","receivedAt":"2025-06-03T22:13:57Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Jun 03, 2025 at 03:14:03AM +0000, Lidong Yan via GitGitGadget wrote:\n> From: Lidong Yan <502024330056@smail.nju.edu.cn>\n>\n> In pack-bitmap.c:test_bitmap_commits(), it comments\n>\n> /*\n> * As this function is only used to print bitmap selected\n> * commits, we don't have to read the commit table.\n> */\n>\n\nThere is no need to include the original comment here, since it is clear\nfrom the patch below what you're referring to.\n\nI don't think this alone is worth rerolling the series, but others may\nfeel differently.\n\n> This suggests that we can avoid reading the commit table altogether.\n> However, this comment is misleading. The reason we load bitmap entries here\n> is because test_bitmap_commits() needs to print the commit IDs from the\n> bitmap, and we must read the bitmap entries to obtain those commit IDs.\n> So reword this comment.\n>\n> Signed-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>\n> ---\n> pack-bitmap.c | 5 +++--\n> 1 file changed, 3 insertions(+), 2 deletions(-)\n>\n> diff --git a/pack-bitmap.c b/pack-bitmap.c\n> index fd19c2255163..e514c9da239b 100644\n> --- a/pack-bitmap.c\n> +++ b/pack-bitmap.c\n> @@ -2839,8 +2839,9 @@ int test_bitmap_commits(struct repository *r)\n> \t\tdie(_(\"failed to load bitmap indexes\"));\n>\n> \t/*\n> -\t * As this function is only used to print bitmap selected\n> -\t * commits, we don't have to read the commit table.\n> +\t * Since this function needs to print bitmap selected\n\nThe phrase \"bitmap selected commits\" is a little awkward. I might have\nwritten either \"the bitmapped commits\", or \"the set of commits which\nhave bitmaps\".\n\n> +\t * commits, bypass the commit lookup table (if one exists)\n> +\t * by forcing the bitmap to eagerly load its entries.\n> \t */\n> \tif (bitmap_git->table_lookup) {\n> \t\tif (load_bitmap_entries_v1(bitmap_git) < 0)\n> --\n> gitgitgadget\n>\nThanks,\nTaylor\n"},{"id":"519603","messageId":"aD9z4bVNSLi0TuRq@nand.local","threadId":"63447","inReplyTo":"pull.1962.v5.git.git.1748920444.gitgitgadget@gmail.com","subject":"Re: [PATCH v5 0/3] pack-bitmap: fix memory leak if load_bitmap failed","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-06-03T22:14:57Z","receivedAt":"2025-06-03T22:14:59Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Jun 03, 2025 at 03:14:01AM +0000, Lidong Yan via GitGitGadget wrote:\n> Lidong Yan (2):\n> pack-bitmap: reword comments in test_bitmap_commits()\n> pack-bitmap: add load corrupt bitmap test\n>\n> Taylor Blau (1):\n> pack-bitmap: fix memory leak if load_bitmap() failed\n\nThis version looks pretty good to me. There's a pair of minor\nsuggestions that I left on the second patch, but otherwise I think the\nresult is ready to start merging down.\n\nThanks,\nTaylor\n"},{"id":"520982","messageId":"pull.1962.v6.git.git.1751347929.gitgitgadget@gmail.com","threadId":"63447","inReplyTo":"pull.1962.v5.git.git.1748920444.gitgitgadget@gmail.com","subject":"[PATCH v6 0/3] pack-bitmap: fix memory leak if load_bitmap failed","fromName":"Lidong Yan via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-07-01T05:32:06Z","receivedAt":"2025-07-01T05:32:12Z","isPatch":true,"sender":{"key":"yldhome2d2@gmail.com","avatar":"https://avatars.githubusercontent.com/u/77328395?v=4"},"body":"Since it seems this patch has been inactive for some time, I have revised\nthe comments according to Taylor's feedback and submitted a new version.\n\nThis patch prevents pack-bitmap.c:load_bitmap() from nulling\nbitmap_git->bitmap when loading failed. Thus eliminates memory leak. This\npatch also add a test case in t5310 which use clang leak sanitizer to detect\nwhether leak happens when loading failed.\n\nLidong Yan (2):\n pack-bitmap: reword comments in test_bitmap_commits()\n pack-bitmap: add load corrupt bitmap test\n\nTaylor Blau (1):\n pack-bitmap: fix memory leak if load_bitmap() failed\n\n pack-bitmap.c | 88 ++++++++++++++++++++++++++++++-----------\n pack-bitmap.h | 1 +\n t/helper/test-bitmap.c | 8 ++++\n t/t5310-pack-bitmaps.sh | 30 ++++++++++++++\n 4 files changed, 103 insertions(+), 24 deletions(-)\n\n\nbase-commit: f0135a9047ca37d4d117dcf21f7e3e89fad85d00\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1962%2Fbrandb97%2Ffix-pack-bitmap-leak-v6\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1962/brandb97/fix-pack-bitmap-leak-v6\nPull-Request: https://github.com/git/git/pull/1962\n\nRange-diff vs v5:\n\n 1: 9ce2135df2a ! 1: 3d70e14e415 pack-bitmap: fix memory leak if load_bitmap() failed\n @@ Commit message\n });\n \n , but won't since load_bitmap() already called kh_destroy_oid_map() and\n - NULL'd the \"bitmaps\" pointer from within its \"failed\" label.\n -\n - So I think if you got part of the way through loading bitmap entries and\n - then failed, you would leak all of the previous entries that you were\n - able to load successfully.\n + NULL'd the \"bitmaps\" pointer from within its \"failed\" label. Thus if you\n + got part of the way through loading bitmap entries and then failed, you\n + would leak all of the previous entries that you were able to load\n + successfully.\n \n The solution is to remove the error handling code in load_bitmap(), because\n its caller will always call free_bitmap_index() in case of an error.\n 2: a75d0a3cc7f ! 2: 6a082930ea3 pack-bitmap: reword comments in test_bitmap_commits()\n @@ Metadata\n ## Commit message ##\n pack-bitmap: reword comments in test_bitmap_commits()\n \n - In pack-bitmap.c:test_bitmap_commits(), it comments\n -\n - /*\n - * As this function is only used to print bitmap selected\n - * commits, we don't have to read the commit table.\n - */\n -\n - This suggests that we can avoid reading the commit table altogether.\n - However, this comment is misleading. The reason we load bitmap entries here\n - is because test_bitmap_commits() needs to print the commit IDs from the\n + The comment in pack-bitmap.c:test_bitmap_commits(), suggests that\n + we can avoid reading the commit table altogether. However, this\n + comment is misleading. The reason we load bitmap entries here is\n + because test_bitmap_commits() needs to print the commit IDs from the\n bitmap, and we must read the bitmap entries to obtain those commit IDs.\n So reword this comment.\n \n @@ pack-bitmap.c: int test_bitmap_commits(struct repository *r)\n \t/*\n -\t * As this function is only used to print bitmap selected\n -\t * commits, we don't have to read the commit table.\n -+\t * Since this function needs to print bitmap selected\n ++\t * Since this function needs to print the bitmapped\n +\t * commits, bypass the commit lookup table (if one exists)\n +\t * by forcing the bitmap to eagerly load its entries.\n \t */\n 3: 05140e2171d ! 3: c1b5d030133 pack-bitmap: add load corrupt bitmap test\n @@ Metadata\n ## Commit message ##\n pack-bitmap: add load corrupt bitmap test\n \n - This patch add test_bitmap_list_commits_offset() in patch-bitmap.c,\n - a new test helper command `test-tool bitmap list-commits-offset`,\n - and a `load corrupt bitmap` test case in t5310.\n -\n - The `load corrupt bitmap` test case intentionally corrupt the\n - \"xor_offset\" field of the first entry. And the newly added helper\n - can help to find position of \"xor_offset\" in bitmap file.\n + t5310 lacks a test to ensure git works correctly when commit bitmap\n + data is corrupted. So this patch add test helper in pack-bitmap.c to\n + list each commit bitmap position in bitmap file and `load corrupt bitmap`\n + test case in t/t5310 to corrupt a commit bitmap before loading it.\n \n Signed-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>\n \n\n-- \ngitgitgadget\n"},{"id":"520983","messageId":"3d70e14e415f7f13864d4d3d2d5d4395f6e14bb3.1751347929.git.gitgitgadget@gmail.com","threadId":"63447","inReplyTo":"pull.1962.v6.git.git.1751347929.gitgitgadget@gmail.com","subject":"[PATCH v6 1/3] pack-bitmap: fix memory leak if load_bitmap() failed","fromName":"Taylor Blau via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-07-01T05:32:07Z","receivedAt":"2025-07-01T05:32:13Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"From: Taylor Blau \n\nAfter going through the \"failed\" label, load_bitmap() will return -1,\nand its caller (either prepare_bitmap_walk() or prepare_bitmap_git())\nwill then call free_bitmap_index().\n\nThat function would have done:\n\n struct stored_bitmap *sb;\n kh_foreach_value(b->bitmaps, sb {\n ewah_pool_free(sb->root);\n free(sb);\n });\n\n, but won't since load_bitmap() already called kh_destroy_oid_map() and\nNULL'd the \"bitmaps\" pointer from within its \"failed\" label. Thus if you\ngot part of the way through loading bitmap entries and then failed, you\nwould leak all of the previous entries that you were able to load\nsuccessfully.\n\nThe solution is to remove the error handling code in load_bitmap(), because\nits caller will always call free_bitmap_index() in case of an error.\n\nSigned-off-by: Taylor Blau \nSigned-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>\n---\n pack-bitmap.c | 21 ++++-----------------\n 1 file changed, 4 insertions(+), 17 deletions(-)\n\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex 8727f316de92..38588b4aec01 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -630,41 +630,28 @@ static int load_bitmap(struct repository *r, struct bitmap_index *bitmap_git,\n \tbitmap_git->ext_index.positions = kh_init_oid_pos();\n \n \tif (load_reverse_index(r, bitmap_git))\n-\t\tgoto failed;\n+\t\treturn -1;\n \n \tif (!(bitmap_git->commits = read_bitmap_1(bitmap_git)) ||\n \t\t!(bitmap_git->trees = read_bitmap_1(bitmap_git)) ||\n \t\t!(bitmap_git->blobs = read_bitmap_1(bitmap_git)) ||\n \t\t!(bitmap_git->tags = read_bitmap_1(bitmap_git)))\n-\t\tgoto failed;\n+\t\treturn -1;\n \n \tif (!bitmap_git->table_lookup && load_bitmap_entries_v1(bitmap_git) < 0)\n-\t\tgoto failed;\n+\t\treturn -1;\n \n \tif (bitmap_git->base) {\n \t\tif (!bitmap_is_midx(bitmap_git))\n \t\t\tBUG(\"non-MIDX bitmap has non-NULL base bitmap index\");\n \t\tif (load_bitmap(r, bitmap_git->base, 1) < 0)\n-\t\t\tgoto failed;\n+\t\t\treturn -1;\n \t}\n \n \tif (!recursing)\n \t\tload_all_type_bitmaps(bitmap_git);\n \n \treturn 0;\n-\n-failed:\n-\tmunmap(bitmap_git->map, bitmap_git->map_size);\n-\tbitmap_git->map = NULL;\n-\tbitmap_git->map_size = 0;\n-\n-\tkh_destroy_oid_map(bitmap_git->bitmaps);\n-\tbitmap_git->bitmaps = NULL;\n-\n-\tkh_destroy_oid_pos(bitmap_git->ext_index.positions);\n-\tbitmap_git->ext_index.positions = NULL;\n-\n-\treturn -1;\n }\n \n static int open_pack_bitmap(struct repository *r,\n-- \ngitgitgadget\n\n"},{"id":"520984","messageId":"6a082930ea3afaae03aaf87a861da8806799301a.1751347929.git.gitgitgadget@gmail.com","threadId":"63447","inReplyTo":"pull.1962.v6.git.git.1751347929.gitgitgadget@gmail.com","subject":"[PATCH v6 2/3] pack-bitmap: reword comments in test_bitmap_commits()","fromName":"Lidong Yan via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-07-01T05:32:08Z","receivedAt":"2025-07-01T05:32:14Z","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\nThe comment in pack-bitmap.c:test_bitmap_commits(), suggests that\nwe can avoid reading the commit table altogether. However, this\ncomment is misleading. The reason we load bitmap entries here is\nbecause test_bitmap_commits() needs to print the commit IDs from the\nbitmap, and we must read the bitmap entries to obtain those commit IDs.\nSo reword this comment.\n\nSigned-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>\n---\n pack-bitmap.c | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex 38588b4aec01..330f07609835 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -2839,8 +2839,9 @@ int test_bitmap_commits(struct repository *r)\n \t\tdie(_(\"failed to load bitmap indexes\"));\n \n \t/*\n-\t * As this function is only used to print bitmap selected\n-\t * commits, we don't have to read the commit table.\n+\t * Since this function needs to print the bitmapped\n+\t * commits, bypass the commit lookup table (if one exists)\n+\t * by forcing the bitmap to eagerly load its entries.\n \t */\n \tif (bitmap_git->table_lookup) {\n \t\tif (load_bitmap_entries_v1(bitmap_git) < 0)\n-- \ngitgitgadget\n\n"},{"id":"520985","messageId":"c1b5d030133fafc6adaa70facf13027b37edf632.1751347929.git.gitgitgadget@gmail.com","threadId":"63447","inReplyTo":"pull.1962.v6.git.git.1751347929.gitgitgadget@gmail.com","subject":"[PATCH v6 3/3] pack-bitmap: add load corrupt bitmap test","fromName":"Lidong Yan via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-07-01T05:32:09Z","receivedAt":"2025-07-01T05:32:14Z","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\nt5310 lacks a test to ensure git works correctly when commit bitmap\ndata is corrupted. So this patch add test helper in pack-bitmap.c to\nlist each commit bitmap position in bitmap file and `load corrupt bitmap`\ntest case in t/t5310 to corrupt a commit bitmap before loading it.\n\nSigned-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>\n---\n pack-bitmap.c | 62 +++++++++++++++++++++++++++++++++++++----\n pack-bitmap.h | 1 +\n t/helper/test-bitmap.c | 8 ++++++\n t/t5310-pack-bitmaps.sh | 30 ++++++++++++++++++++\n 4 files changed, 96 insertions(+), 5 deletions(-)\n\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex 330f07609835..499d77a1d368 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -31,6 +31,7 @@ struct stored_bitmap {\n \tstruct object_id oid;\n \tstruct ewah_bitmap *root;\n \tstruct stored_bitmap *xor;\n+\tsize_t map_pos;\n \tint flags;\n };\n \n@@ -314,13 +315,14 @@ static struct stored_bitmap *store_bitmap(struct bitmap_index *index,\n \t\t\t\t\t struct ewah_bitmap *root,\n \t\t\t\t\t const struct object_id *oid,\n \t\t\t\t\t struct stored_bitmap *xor_with,\n-\t\t\t\t\t int flags)\n+\t\t\t\t\t int flags, size_t map_pos)\n {\n \tstruct stored_bitmap *stored;\n \tkhiter_t hash_pos;\n \tint ret;\n \n \tstored = xmalloc(sizeof(struct stored_bitmap));\n+\tstored->map_pos = map_pos;\n \tstored->root = root;\n \tstored->xor = xor_with;\n \tstored->flags = flags;\n@@ -376,10 +378,12 @@ static int load_bitmap_entries_v1(struct bitmap_index *index)\n \t\tstruct stored_bitmap *xor_bitmap = NULL;\n \t\tuint32_t commit_idx_pos;\n \t\tstruct object_id oid;\n+\t\tsize_t entry_map_pos;\n \n \t\tif (index->map_size - index->map_pos < 6)\n \t\t\treturn error(_(\"corrupt ewah bitmap: truncated header for entry %d\"), i);\n \n+\t\tentry_map_pos = index->map_pos;\n \t\tcommit_idx_pos = read_be32(index->map, &index->map_pos);\n \t\txor_offset = read_u8(index->map, &index->map_pos);\n \t\tflags = read_u8(index->map, &index->map_pos);\n@@ -402,8 +406,9 @@ static int load_bitmap_entries_v1(struct bitmap_index *index)\n \t\tif (!bitmap)\n \t\t\treturn -1;\n \n-\t\trecent_bitmaps[i % MAX_XOR_OFFSET] = store_bitmap(\n-\t\t\tindex, bitmap, &oid, xor_bitmap, flags);\n+\t\trecent_bitmaps[i % MAX_XOR_OFFSET] =\n+\t\t\tstore_bitmap(index, bitmap, &oid, xor_bitmap, flags,\n+\t\t\t\t entry_map_pos);\n \t}\n \n \treturn 0;\n@@ -869,6 +874,7 @@ static struct stored_bitmap *lazy_bitmap_for_commit(struct bitmap_index *bitmap_\n \tint xor_flags;\n \tkhiter_t hash_pos;\n \tstruct bitmap_lookup_table_xor_item *xor_item;\n+\tsize_t entry_map_pos;\n \n \tif (is_corrupt)\n \t\treturn NULL;\n@@ -928,6 +934,7 @@ static struct stored_bitmap *lazy_bitmap_for_commit(struct bitmap_index *bitmap_\n \t\t\tgoto corrupt;\n \t\t}\n \n+\t\tentry_map_pos = bitmap_git->map_pos;\n \t\tbitmap_git->map_pos += sizeof(uint32_t) + sizeof(uint8_t);\n \t\txor_flags = read_u8(bitmap_git->map, &bitmap_git->map_pos);\n \t\tbitmap = read_bitmap_1(bitmap_git);\n@@ -935,7 +942,8 @@ static struct stored_bitmap *lazy_bitmap_for_commit(struct bitmap_index *bitmap_\n \t\tif (!bitmap)\n \t\t\tgoto corrupt;\n \n-\t\txor_bitmap = store_bitmap(bitmap_git, bitmap, &xor_item->oid, xor_bitmap, xor_flags);\n+\t\txor_bitmap = store_bitmap(bitmap_git, bitmap, &xor_item->oid,\n+\t\t\t\t\t xor_bitmap, xor_flags, entry_map_pos);\n \t\txor_items_nr--;\n \t}\n \n@@ -969,6 +977,7 @@ static struct stored_bitmap *lazy_bitmap_for_commit(struct bitmap_index *bitmap_\n \t * Instead, we can skip ahead and immediately read the flags and\n \t * ewah bitmap.\n \t */\n+\tentry_map_pos = bitmap_git->map_pos;\n \tbitmap_git->map_pos += sizeof(uint32_t) + sizeof(uint8_t);\n \tflags = read_u8(bitmap_git->map, &bitmap_git->map_pos);\n \tbitmap = read_bitmap_1(bitmap_git);\n@@ -976,7 +985,8 @@ static struct stored_bitmap *lazy_bitmap_for_commit(struct bitmap_index *bitmap_\n \tif (!bitmap)\n \t\tgoto corrupt;\n \n-\treturn store_bitmap(bitmap_git, bitmap, oid, xor_bitmap, flags);\n+\treturn store_bitmap(bitmap_git, bitmap, oid, xor_bitmap, flags,\n+\t\t\t entry_map_pos);\n \n corrupt:\n \tfree(xor_items);\n@@ -2857,6 +2867,48 @@ int test_bitmap_commits(struct repository *r)\n \treturn 0;\n }\n \n+int test_bitmap_commits_with_offset(struct repository *r)\n+{\n+\tstruct object_id oid;\n+\tstruct stored_bitmap *stored;\n+\tstruct bitmap_index *bitmap_git;\n+\tsize_t commit_idx_pos_map_pos, xor_offset_map_pos, flag_map_pos,\n+\t\tewah_bitmap_map_pos;\n+\n+\tbitmap_git = prepare_bitmap_git(r);\n+\tif (!bitmap_git)\n+\t\tdie(_(\"failed to load bitmap indexes\"));\n+\n+\t/*\n+\t * Since this function needs to know the position of each individual\n+\t * bitmap, bypass the commit lookup table (if one exists) by forcing\n+\t * the bitmap to eagerly load its entries.\n+\t */\n+\tif (bitmap_git->table_lookup) {\n+\t\tif (load_bitmap_entries_v1(bitmap_git) < 0)\n+\t\t\tdie(_(\"failed to load bitmap indexes\"));\n+\t}\n+\n+\tkh_foreach (bitmap_git->bitmaps, oid, stored, {\n+\t\tcommit_idx_pos_map_pos = stored->map_pos;\n+\t\txor_offset_map_pos = stored->map_pos + sizeof(uint32_t);\n+\t\tflag_map_pos = xor_offset_map_pos + sizeof(uint8_t);\n+\t\tewah_bitmap_map_pos = flag_map_pos + sizeof(uint8_t);\n+\n+\t\tprintf_ln(\"%s %\"PRIuMAX\" %\"PRIuMAX\" %\"PRIuMAX\" %\"PRIuMAX,\n+\t\t\t oid_to_hex(&oid),\n+\t\t\t (uintmax_t)commit_idx_pos_map_pos,\n+\t\t\t (uintmax_t)xor_offset_map_pos,\n+\t\t\t (uintmax_t)flag_map_pos,\n+\t\t\t (uintmax_t)ewah_bitmap_map_pos);\n+\t})\n+\t\t;\n+\n+\tfree_bitmap_index(bitmap_git);\n+\n+\treturn 0;\n+}\n+\n int test_bitmap_hashes(struct repository *r)\n {\n \tstruct bitmap_index *bitmap_git = prepare_bitmap_git(r);\ndiff --git a/pack-bitmap.h b/pack-bitmap.h\nindex 382d39499af2..1bd7a791e2a0 100644\n--- a/pack-bitmap.h\n+++ b/pack-bitmap.h\n@@ -81,6 +81,7 @@ void traverse_bitmap_commit_list(struct bitmap_index *,\n \t\t\t\t show_reachable_fn show_reachable);\n void test_bitmap_walk(struct rev_info *revs);\n int test_bitmap_commits(struct repository *r);\n+int test_bitmap_commits_with_offset(struct repository *r);\n 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);\ndiff --git a/t/helper/test-bitmap.c b/t/helper/test-bitmap.c\nindex 3f23f2107268..16a01669e414 100644\n--- a/t/helper/test-bitmap.c\n+++ b/t/helper/test-bitmap.c\n@@ -10,6 +10,11 @@ static int bitmap_list_commits(void)\n \treturn test_bitmap_commits(the_repository);\n }\n \n+static int bitmap_list_commits_with_offset(void)\n+{\n+\treturn test_bitmap_commits_with_offset(the_repository);\n+}\n+\n static int bitmap_dump_hashes(void)\n {\n \treturn test_bitmap_hashes(the_repository);\n@@ -36,6 +41,8 @@ int cmd__bitmap(int argc, const char **argv)\n \n \tif (argc == 2 && !strcmp(argv[1], \"list-commits\"))\n \t\treturn bitmap_list_commits();\n+\tif (argc == 2 && !strcmp(argv[1], \"list-commits-with-offset\"))\n+\t\treturn bitmap_list_commits_with_offset();\n \tif (argc == 2 && !strcmp(argv[1], \"dump-hashes\"))\n \t\treturn bitmap_dump_hashes();\n \tif (argc == 2 && !strcmp(argv[1], \"dump-pseudo-merges\"))\n@@ -46,6 +53,7 @@ int cmd__bitmap(int argc, const char **argv)\n \t\treturn bitmap_dump_pseudo_merge_objects(atoi(argv[2]));\n \n \tusage(\"\\ttest-tool bitmap list-commits\\n\"\n+\t \"\\ttest-tool bitmap list-commits-with-offset\\n\"\n \t \"\\ttest-tool bitmap dump-hashes\\n\"\n \t \"\\ttest-tool bitmap dump-pseudo-merges\\n\"\n \t \"\\ttest-tool bitmap dump-pseudo-merge-commits \\n\"\ndiff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh\nindex b6926f102708..6718fb98c057 100755\n--- a/t/t5310-pack-bitmaps.sh\n+++ b/t/t5310-pack-bitmaps.sh\n@@ -495,6 +495,36 @@ 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\trm -fr repo &&\n+\t\tgit init repo &&\n+\t\ttest_when_finished \"rm -fr repo\" &&\n+\t\t(\n+\t\t\tcd repo &&\n+\t\t\tgit config pack.writeBitmapLookupTable '\"$writeLookupTable\"' &&\n+\n+\t\t\ttest_commit base &&\n+\n+\t\t\tgit repack -adb &&\n+\t\t\tbitmap=\"$(ls .git/objects/pack/pack-*.bitmap)\" &&\n+\t\t\tchmod +w $bitmap &&\n+\n+\t\t\ttest-tool bitmap list-commits-with-offset >offsets &&\n+\t\t\txor_off=$(head -n1 offsets | awk \"{print \\$3}\") &&\n+\t\t\tprintf '\\161' |\n+\t\t\t\tdd of=$bitmap count=1 bs=1 conv=notrunc seek=$xor_off &&\n+\n+\t\t\tgit rev-list --objects --no-object-names HEAD >expect.raw &&\n+\t\t\tgit rev-list --objects --use-bitmap-index --no-object-names HEAD \\\n+\t\t\t\t>actual.raw &&\n+\n+\t\t\tsort expect.raw >expect &&\n+\t\t\tsort actual.raw >actual &&\n+\n+\t\t test_cmp expect actual\n+\t\t)\n+\t'\n }\n \n test_bitmap_cases\n-- \ngitgitgadget\n"},{"id":"521483","messageId":"xmqqfrf71ull.fsf@gitster.g","threadId":"63447","inReplyTo":"pull.1962.v6.git.git.1751347929.gitgitgadget@gmail.com","subject":"Re: [PATCH v6 0/3] pack-bitmap: fix memory leak if load_bitmap failed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-07-07T22:53:10Z","receivedAt":"2025-07-07T22:53:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Lidong Yan via GitGitGadget\" writes:\n\n> Since it seems this patch has been inactive for some time, I have revised\n> the comments according to Taylor's feedback and submitted a new version.\n>\n> This patch prevents pack-bitmap.c:load_bitmap() from nulling\n> bitmap_git->bitmap when loading failed. Thus eliminates memory leak. This\n> patch also add a test case in t5310 which use clang leak sanitizer to detect\n> whether leak happens when loading failed.\n>\n> Lidong Yan (2):\n> pack-bitmap: reword comments in test_bitmap_commits()\n> pack-bitmap: add load corrupt bitmap test\n>\n> Taylor Blau (1):\n> pack-bitmap: fix memory leak if load_bitmap() failed\n\nOK, now, how does this iteration look to folks? We haven't heard\nanybody say yet. Is it ready to be marked for 'next' yet?\n\nThanks.\n"},{"id":"521583","messageId":"aG2XZYamUv5FWq/W@nand.local","threadId":"63447","inReplyTo":"xmqqfrf71ull.fsf@gitster.g","subject":"Re: [PATCH v6 0/3] pack-bitmap: fix memory leak if load_bitmap failed","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-07-08T22:10:45Z","receivedAt":"2025-07-08T22:10:52Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Jul 07, 2025 at 03:53:10PM -0700, Junio C Hamano wrote:\n> \"Lidong Yan via GitGitGadget\" writes:\n>\n> > Since it seems this patch has been inactive for some time, I have revised\n> > the comments according to Taylor's feedback and submitted a new version.\n> >\n> > This patch prevents pack-bitmap.c:load_bitmap() from nulling\n> > bitmap_git->bitmap when loading failed. Thus eliminates memory leak. This\n> > patch also add a test case in t5310 which use clang leak sanitizer to detect\n> > whether leak happens when loading failed.\n> >\n> > Lidong Yan (2):\n> > pack-bitmap: reword comments in test_bitmap_commits()\n> > pack-bitmap: add load corrupt bitmap test\n> >\n> > Taylor Blau (1):\n> > pack-bitmap: fix memory leak if load_bitmap() failed\n>\n> OK, now, how does this iteration look to folks? We haven't heard\n> anybody say yet. Is it ready to be marked for 'next' yet?\n\nOops, this fell off of my review queue. This version looks great to me.\nThanks, Lidong!\n\nThanks,\nTaylor\n"},{"id":"521584","messageId":"xmqqms9es43b.fsf@gitster.g","threadId":"63447","inReplyTo":"aG2XZYamUv5FWq/W@nand.local","subject":"Re: [PATCH v6 0/3] pack-bitmap: fix memory leak if load_bitmap failed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-07-08T22:35:52Z","receivedAt":"2025-07-08T22:35:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> On Mon, Jul 07, 2025 at 03:53:10PM -0700, Junio C Hamano wrote:\n>> \"Lidong Yan via GitGitGadget\" writes:\n>>\n>> > Since it seems this patch has been inactive for some time, I have revised\n>> > the comments according to Taylor's feedback and submitted a new version.\n>> >\n>> > This patch prevents pack-bitmap.c:load_bitmap() from nulling\n>> > bitmap_git->bitmap when loading failed. Thus eliminates memory leak. This\n>> > patch also add a test case in t5310 which use clang leak sanitizer to detect\n>> > whether leak happens when loading failed.\n>> >\n>> > Lidong Yan (2):\n>> > pack-bitmap: reword comments in test_bitmap_commits()\n>> > pack-bitmap: add load corrupt bitmap test\n>> >\n>> > Taylor Blau (1):\n>> > pack-bitmap: fix memory leak if load_bitmap() failed\n>>\n>> OK, now, how does this iteration look to folks? We haven't heard\n>> anybody say yet. Is it ready to be marked for 'next' yet?\n>\n> Oops, this fell off of my review queue. This version looks great to me.\n> Thanks, Lidong!\n\nThanks.\n"}]}