pack-bitmap: add loading corrupt bitmap_index test

PATCH In review Lidong Yan via GitGitGadget 4 participants 7 messages May 17, 2025 – May 19, 2025

lore

Share & tools

lore

Lidong Yan via GitGitGadget

author

2025-05-17 14:26 UTC #0 lore raw

PATCH 1/1 pack-bitmap: add loading corrupt bitmap_index test

From: Lidong Yan <[email protected]>

This patch add a test function test_bitmap_load_corrupt in patch-bitmap.c , a load corrupt bitmap test case in t5310-pack-bitmaps.sh and a new command load-corrupt for test-tool in t/helper/test-bitmap.c.

To make sure we are loading a corrupt bitmap, we need enable bitmap table lookup so that prepare_bitmap() won't call load_bitmap_entries_v1(). So to test corrupt bitmap_index, we first call prepare_bitmap() to set everything up but bitmap_index->bitmaps for us. Then we do any corruption we want to the bitmap_index. Finally we can test loading corrupt bitmap by calling load_bitmap_entries_v1().

Signed-off-by Lidong Yan <[email protected]>

---
pack-bitmap: add loading corrupt bitmap_index test

This patch add a test function test_bitmap_load_corrupt in patch-bitmap.c , a load corrupt bitmap test case in t5310-pack-bitmaps.sh and a new command load-corrupt for test-tool in t/helper/test-bitmap.c.

To make sure we are loading a corrupt bitmap, we need enable bitmap table lookup so that prepare_bitmap() won't call load_bitmap_entries_v1(). So to test corrupt bitmap_index, we first prepare_bitmap() to set everything up but bitmap_index->bitmaps for us. Then we do any corruption we want to the bitmap_index. Finally we call load_bitmap_entries_v1() to test loading corrupt bitmap.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1967%2Fbrandb97%2Fcorrupt-bitmap-test-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1967/brandb97/corrupt-bitmap-test-v1
Pull-Request: https://github.com/git/git/pull/1967

 pack-bitmap.c           | 65 +++++++++++++++++++++++++++++++++++++++++
 pack-bitmap.h           |  1 +
 t/helper/test-bitmap.c  |  8 +++++
 t/t5310-pack-bitmaps.sh | 15 ++++++++++
 4 files changed, 89 insertions(+)

pack-bitmap.c +65 -0

index b9f1d866046..9642a06b3fe 100644
@@ -3022,6 +3022,71 @@ cleanup:
 	return ret;
 }
 
+typedef void(corrupt_fn)(struct bitmap_index *);
+
+static int bitmap_corrupt_then_load(struct repository *r, corrupt_fn *do_corrupt)
+{
+	struct bitmap_index *bitmap_git;
+	unsigned char *map;
+
+	if (!(bitmap_git = prepare_bitmap_git(r)))
+		die(_("failed to prepare bitmap indexes"));
+	/*
+	 * If the table lookup extension is not used,
+	 * prepare_bitmap_git has already called load_bitmap_entries_v1(),
+	 * making it impossible to corrupt the bitmap.
+	 */
+	if (!bitmap_git->table_lookup)
+		return 0;
+
+	/*
+	 * bitmap_git->map is read-only;
+	 * to corrupt it, we need a writable memory block.
+	 */
+	map = bitmap_git->map;
+	bitmap_git->map = xmalloc(bitmap_git->map_size);
+	if (!bitmap_git->map)
+		return 0;
+	memcpy(bitmap_git->map, map, bitmap_git->map_size);
+
+	do_corrupt(bitmap_git);
+	if (!load_bitmap_entries_v1(bitmap_git))
+		die(_("load corrupt bitmap successfully"));
+
+	free(bitmap_git->map);
+	bitmap_git->map = map;
+	free_bitmap_index(bitmap_git);
+
+	return 0;
+}
+
+static void do_corrupt_commit_pos(struct bitmap_index *bitmap_git)
+{
+	uint32_t *commit_pos_ptr;
+
+	commit_pos_ptr = (uint32_t *)(bitmap_git->map + bitmap_git->map_pos);
+	*commit_pos_ptr = (uint32_t)-1;
+}
+
+static void do_corrupt_xor_offset(struct bitmap_index *bitmap_git)
+{
+	uint8_t *xor_offset_ptr;
+
+	xor_offset_ptr = (uint8_t *)(bitmap_git->map + bitmap_git->map_pos +
+				     sizeof(uint32_t));
+	*xor_offset_ptr = MAX_XOR_OFFSET + 1;
+}
+
+int test_bitmap_load_corrupt(struct repository *r)
+{
+	int res = 0;
+	if ((res = bitmap_corrupt_then_load(r, do_corrupt_commit_pos)))
+		return res;
+	if ((res = bitmap_corrupt_then_load(r, do_corrupt_xor_offset)))
+		return res;
+	return res;
+}
+
 int rebuild_bitmap(const uint32_t *reposition,
 		   struct ewah_bitmap *source,
 		   struct bitmap *dest)

pack-bitmap.h +1 -0

index 382d39499af..7770abe6bff 100644
@@ -85,6 +85,7 @@ int test_bitmap_hashes(struct repository *r);
 int test_bitmap_pseudo_merges(struct repository *r);
 int test_bitmap_pseudo_merge_commits(struct repository *r, uint32_t n);
 int test_bitmap_pseudo_merge_objects(struct repository *r, uint32_t n);
+int test_bitmap_load_corrupt(struct repository *r);
 
 struct list_objects_filter_options;

t/helper/test-bitmap.c +8 -0

index 3f23f210726..7aabcfed2f9 100644
@@ -20,6 +20,11 @@ static int bitmap_dump_pseudo_merges(void)
 	return test_bitmap_pseudo_merges(the_repository);
 }
 
+static int bitmap_load_corrupt(void)
+{
+	return test_bitmap_load_corrupt(the_repository);
+}
+
 static int bitmap_dump_pseudo_merge_commits(uint32_t n)
 {
 	return test_bitmap_pseudo_merge_commits(the_repository, n);

@@ -40,6 +45,8 @@ int cmd__bitmap(int argc, const char **argv)
 		return bitmap_dump_hashes();
 	if (argc == 2 && !strcmp(argv[1], "dump-pseudo-merges"))
 		return bitmap_dump_pseudo_merges();
+	if (argc == 2 && !strcmp(argv[1], "load-corrupt"))
+		return bitmap_load_corrupt();
 	if (argc == 3 && !strcmp(argv[1], "dump-pseudo-merge-commits"))
 		return bitmap_dump_pseudo_merge_commits(atoi(argv[2]));
 	if (argc == 3 && !strcmp(argv[1], "dump-pseudo-merge-objects"))

@@ -48,6 +55,7 @@ int cmd__bitmap(int argc, const char **argv)
 	usage("\ttest-tool bitmap list-commits\n"
 	      "\ttest-tool bitmap dump-hashes\n"
 	      "\ttest-tool bitmap dump-pseudo-merges\n"
+	      "\ttest-tool bitmap load-corrupt\n"
 	      "\ttest-tool bitmap dump-pseudo-merge-commits <n>\n"
 	      "\ttest-tool bitmap dump-pseudo-merge-objects <n>");

t/t5310-pack-bitmaps.sh +15 -0

index a62b463eaf0..042f62f16ea 100755
@@ -486,6 +486,21 @@ test_bitmap_cases () {
 			grep "ignoring extra bitmap" trace2.txt
 		)
 	'
+
+	test_expect_success 'load corrupt bitmap' '
+		git init repo &&
+		test_when_finished "rm -fr repo" && (
+			cd repo &&
+			git config pack.writeBitmapLookupTable '"$writeLookupTable"' &&
+
+			echo "Hello world" > hello_world.txt &&
+			git add hello_world.txt &&
+			git commit -am "add hello_world.txt" &&
+
+			git repack -adb &&
+			test-tool bitmap load-corrupt
+		)
+	'
 }
 
 test_bitmap_cases

base-commit: 6f84262c44a89851c3ae5a6e4c1a9d06b2068d75

Signature · 1 line

gitgitgadget

Patrick Steinhardt

2025-05-19 06:02 UTC #1 lore raw

Quoted 6 lines · Lidong Yan via GitGitGadget

From: Lidong Yan <[email protected]>
This patch add a test function test_bitmap_load_corrupt in patch-bitmap.c , a load corrupt bitmap test case in t5310-pack-bitmaps.sh and a new command load-corrupt for test-tool in t/helper/test-bitmap.c.

Quoted 6 lines

To make sure we are loading a corrupt bitmap, we need enable bitmap table lookup so that prepare_bitmap() won't call load_bitmap_entries_v1(). So to test corrupt bitmap_index, we first call prepare_bitmap() to set everything up but bitmap_index->bitmaps for us. Then we do any corruption we want to the bitmap_index. Finally we can test loading corrupt bitmap by calling load_bitmap_entries_v1().

Okay. We _can_ do that now, but the patch doesn't explain why we _should_.

Quoted 63 lines

pack-bitmap.c +55 -0

index b9f1d866046..9642a06b3fe 100644
@@ -3022,6 +3022,71 @@ cleanup:
 	return ret;
 }
 
+typedef void(corrupt_fn)(struct bitmap_index *);
+
+static int bitmap_corrupt_then_load(struct repository *r, corrupt_fn *do_corrupt)
+{
+	struct bitmap_index *bitmap_git;
+	unsigned char *map;
+
+	if (!(bitmap_git = prepare_bitmap_git(r)))
+		die(_("failed to prepare bitmap indexes"));
+	/*
+	 * If the table lookup extension is not used,
+	 * prepare_bitmap_git has already called load_bitmap_entries_v1(),
+	 * making it impossible to corrupt the bitmap.
+	 */
+	if (!bitmap_git->table_lookup)
+		return 0;
+
+	/*
+	 * bitmap_git->map is read-only;
+	 * to corrupt it, we need a writable memory block.
+	 */
+	map = bitmap_git->map;
+	bitmap_git->map = xmalloc(bitmap_git->map_size);
+	if (!bitmap_git->map)
+		return 0;
+	memcpy(bitmap_git->map, map, bitmap_git->map_size);
+
+	do_corrupt(bitmap_git);
+	if (!load_bitmap_entries_v1(bitmap_git))
+		die(_("load corrupt bitmap successfully"));
+
+	free(bitmap_git->map);
+	bitmap_git->map = map;
+	free_bitmap_index(bitmap_git);
+
+	return 0;
+}
+
+static void do_corrupt_commit_pos(struct bitmap_index *bitmap_git)
+{
+	uint32_t *commit_pos_ptr;
+
+	commit_pos_ptr = (uint32_t *)(bitmap_git->map + bitmap_git->map_pos);
+	*commit_pos_ptr = (uint32_t)-1;
+}
+
+static void do_corrupt_xor_offset(struct bitmap_index *bitmap_git)
+{
+	uint8_t *xor_offset_ptr;
+
+	xor_offset_ptr = (uint8_t *)(bitmap_git->map + bitmap_git->map_pos +
+				     sizeof(uint32_t));
+	*xor_offset_ptr = MAX_XOR_OFFSET + 1;
+}
+

pack-bitmap.c cleanup: · line 3089

Quoted 10 lines

pack-bitmap.c +10 -0

+int test_bitmap_load_corrupt(struct repository *r)
+{
+	int res = 0;
+	if ((res = bitmap_corrupt_then_load(r, do_corrupt_commit_pos)))
+		return res;
+	if ((res = bitmap_corrupt_then_load(r, do_corrupt_xor_offset)))
+		return res;
+	return res;
+}
+

Does all of this logic really have to be part of "pack-bitmap.c"? It would generally preferable to not have our production logic be cluttered with test logic. Sometimes we don't have a better way to do this, but you should explain why we cannot host the logic elsewhere in that case.

My proposal would be to either move the logic into "test-bitmap.c", or to even better to write a unit test in "t/unit-tests/". After all, we expect that the code should fail gracefully, so a unit test might be a good fit after all.

Patrick

lidongyan

2025-05-19 06:44 UTC #2 lore raw

Quoted 2 lines · 2025年5月19日 14:02，Patrick Steinhardt <[email protected]> 写道：

Okay. We _can_ do that now, but the patch doesn't explain why we _should_.

The main purpose of this patch is to provide a test case to check whether
a memory leak occurs when loading a corrupt index file as requested here
https://lore.kernel.org/git/[email protected].
A potential memory leak is mentioned in this patch here https://lore.kernel.org/git/[email protected]/.

Quoted 11 lines

Does all of this logic really have to be part of "pack-bitmap.c"? It would generally preferable to not have our production logic be cluttered with test logic. Sometimes we don't have a better way to do this, but you should explain why we cannot host the logic elsewhere in that case.
My proposal would be to either move the logic into "test-bitmap.c", or to even better to write a unit test in "t/unit-tests/". After all, we expect that the code should fail gracefully, so a unit test might be a good fit after all.
Patrick

I found that the header size of an index file depends only on the type of hash algorithm. To trigger the condition for the memory leak, I need to corrupt a few bytes right after the index file header size. It's more convenient to implement this functionality in pack-bitmap.c. However, I think I can place the test itself under t/unit-tests/.

Patrick Steinhardt

2025-05-19 07:27 UTC #3 lore raw

Quoted 8 lines · lidongyan

Quoted 2 lines · 2025年5月19日 14:02，Patrick Steinhardt <[email protected]> 写道：
Okay. We _can_ do that now, but the patch doesn't explain why we _should_.
The main purpose of this patch is to provide a test case to check whether
a memory leak occurs when loading a corrupt index file as requested here
https://lore.kernel.org/git/[email protected].
A potential memory leak is mentioned in this patch here https://lore.kernel.org/git/[email protected]/.

Wouldn't it make sense to include this in a v2 of that patch series then? That makes it way easier for the reviewer to draw the connection and allows you to draft commit messages in a way that they refer to one another.

Patrick

lidongyan

2025-05-19 07:38 UTC #4 lore raw

Quoted 13 lines

2025年5月19日 15:27，Patrick Steinhardt <[email protected]> 写道：
Quoted 8 lines · lidongyan
Quoted 2 lines · 2025年5月19日 14:02，Patrick Steinhardt <[email protected]> 写道：
Okay. We _can_ do that now, but the patch doesn't explain why we _should_.
The main purpose of this patch is to provide a test case to check whether
a memory leak occurs when loading a corrupt index file as requested here
https://lore.kernel.org/git/[email protected].
A potential memory leak is mentioned in this patch here https://lore.kernel.org/git/[email protected]/.

Quoted 7 lines

Wouldn't it make sense to include this in a v2 of that patch series then? That makes it way easier for the reviewer to draw the connection and allows you to draft commit messages in a way that they refer to one another.
Patrick

Thanks for the suggestion, including it in v2 sounds like the right approach. I’ll update the series and revise the commit messages accordingly.

Jeff King

2025-05-19 07:39 UTC #5 lore raw

In reply to lidongyan

Quoted 8 lines · lidongyan

Quoted 2 lines · 2025年5月19日 14:02，Patrick Steinhardt <[email protected]> 写道：
Okay. We _can_ do that now, but the patch doesn't explain why we _should_.
The main purpose of this patch is to provide a test case to check whether
a memory leak occurs when loading a corrupt index file as requested here
https://lore.kernel.org/git/[email protected].
A potential memory leak is mentioned in this patch here https://lore.kernel.org/git/[email protected]/.

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

Quoted 9 lines

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

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

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

Something like:

t/t5310-pack-bitmaps.sh +11 -1

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

-Peff

lidongyan

2025-05-19 07:58 UTC #6 lore raw

Quoted 64 lines · 2025年5月19日 15:39，Jeff King <[email protected]> 写道：

Quoted 8 lines · lidongyan
Quoted 2 lines · 2025年5月19日 14:02，Patrick Steinhardt <[email protected]> 写道：
Okay. We _can_ do that now, but the patch doesn't explain why we _should_.
The main purpose of this patch is to provide a test case to check whether
a memory leak occurs when loading a corrupt index file as requested here
https://lore.kernel.org/git/[email protected].
A potential memory leak is mentioned in this patch here https://lore.kernel.org/git/[email protected]/.
I think we'd need to mark it with the !SANITIZE_LEAK prereq until the leaks are actually fixed. Or simpler, just apply this on top of the leak fixes once they are ready. That ordering needs to be communicated to the maintainer, and the simplest way to do that is probably to just post a 3-patch series: your initial leak fix, a polished version of the one from Taylor, and then the test on top.
Quoted 9 lines
Quoted 4 lines
My proposal would be to either move the logic into "test-bitmap.c", or to even better to write a unit test in "t/unit-tests/". After all, we expect that the code should fail gracefully, so a unit test might be a good fit after all.
I found that the header size of an index file depends only on the type of hash algorithm. To trigger the condition for the memory leak, I need to corrupt a few bytes right after the index file header size. It's more convenient to implement this functionality in pack-bitmap.c. However, I think I can place the test itself under t/unit-tests/.
I don't think you can do a prereq for a unit-test file (though I might be wrong, as I have not really paid attention to that area).
If the corruption offsets are easy-ish to compute statically (and it sounds like they mostly just depend on the hash algorithm size), then I'd actually prefer to just do it with "dd". That avoids extra C code, and simulates a real on-disk corruption more exactly (using real commands).
Something like:
t/t5310-pack-bitmaps.sh +0 -0
index 042f62f16e..16bd607654 100755
@@ -498,7 +498,17 @@ test_bitmap_cases () {
git commit -am "add hello_world.txt" &&
git repack -adb &&
- test-tool bitmap load-corrupt
+ bitmap=$(ls .git/objects/pack/pack-*.bitmap) &&
+ chmod +w "$bitmap" &&
t/t5310-pack-bitmaps.sh +9 -0
+
+ # this matches the xor offset
+ offset=$((120 + $(test_oid rawsz))) &&
+ printf "\241" |
+ dd of=$bitmap count=1 bs=1 conv=notrunc seek=$offset &&
+
+ git rev-list --count HEAD >expect &&
+ git rev-list --use-bitmap-index --count HEAD >actual &&
+ test_cmp expect actual
)
'
}

Quoted 2 lines

-Peff

Got it, using dd is indeed simpler and better. Thanks a lot!