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

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

[Lidong Yan via GitGitGadget]

From: Lidong Yan <502024330056@smail.nju.edu.cn>

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 <502024330056@smail.nju.edu.cn>
---
    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(+)

diff --git a/pack-bitmap.c b/pack-bitmap.c
index b9f1d866046..9642a06b3fe 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -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)
diff --git a/pack-bitmap.h b/pack-bitmap.h
index 382d39499af..7770abe6bff 100644
--- a/pack-bitmap.h
+++ b/pack-bitmap.h
@@ -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;
 
diff --git a/t/helper/test-bitmap.c b/t/helper/test-bitmap.c
index 3f23f210726..7aabcfed2f9 100644
--- a/t/helper/test-bitmap.c
+++ b/t/helper/test-bitmap.c
@@ -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>");
 
diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh
index a62b463eaf0..042f62f16ea 100755
--- a/t/t5310-pack-bitmaps.sh
+++ b/t/t5310-pack-bitmaps.sh
@@ -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
-- 
gitgitgadget

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

[Patrick Steinhardt]

On Sat, May 17, 2025 at 02:26:22PM +0000, Lidong Yan via GitGitGadget wrote:
> From: Lidong Yan <502024330056@smail.nju.edu.cn>
> 
> 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()`.

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

> diff --git a/pack-bitmap.c b/pack-bitmap.c
> index b9f1d866046..9642a06b3fe 100644
> --- a/pack-bitmap.c
> +++ b/pack-bitmap.c
> @@ -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;
> +}
> +

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

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

[lidongyan]

2025年5月19日 14:02，Patrick Steinhardt <ps@pks.im> 写道：
> Okay. We _can_ do that now, but the patch doesn't explain why we
> _should_.

The main purpose of this patch is to provide a test case to check whether
 a memory leak occurs when loading a corrupt index file as requested here
https://lore.kernel.org/git/20250514180325.GB2196784@coredump.intra.peff.net.
A potential memory leak is mentioned in this patch here https://lore.kernel.org/git/pull.1962.git.git.1747052530271.gitgitgadget@gmail.com/.

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

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

[Patrick Steinhardt]

On Mon, May 19, 2025 at 02:44:22PM +0800, lidongyan wrote:
> 2025年5月19日 14:02，Patrick Steinhardt <ps@pks.im> 写道：
> > Okay. We _can_ do that now, but the patch doesn't explain why we
> > _should_.
> 
> The main purpose of this patch is to provide a test case to check whether
>  a memory leak occurs when loading a corrupt index file as requested here
> https://lore.kernel.org/git/20250514180325.GB2196784@coredump.intra.peff.net.
> A potential memory leak is mentioned in this patch here https://lore.kernel.org/git/pull.1962.git.git.1747052530271.gitgitgadget@gmail.com/.

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

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

[lidongyan]

> 2025年5月19日 15:27，Patrick Steinhardt <ps@pks.im> 写道：
> 
> On Mon, May 19, 2025 at 02:44:22PM +0800, lidongyan wrote:
>> 2025年5月19日 14:02，Patrick Steinhardt <ps@pks.im> 写道：
>>> Okay. We _can_ do that now, but the patch doesn't explain why we
>>> _should_.
>> 
>> The main purpose of this patch is to provide a test case to check whether
>> a memory leak occurs when loading a corrupt index file as requested here
>> https://lore.kernel.org/git/20250514180325.GB2196784@coredump.intra.peff.net.
>> A potential memory leak is mentioned in this patch here https://lore.kernel.org/git/pull.1962.git.git.1747052530271.gitgitgadget@gmail.com/.
> 
> 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.

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

[Jeff King]

On Mon, May 19, 2025 at 02:44:22PM +0800, lidongyan wrote:

> 2025年5月19日 14:02，Patrick Steinhardt <ps@pks.im> 写道：
> > Okay. We _can_ do that now, but the patch doesn't explain why we
> > _should_.
> 
> The main purpose of this patch is to provide a test case to check whether
>  a memory leak occurs when loading a corrupt index file as requested here
> https://lore.kernel.org/git/20250514180325.GB2196784@coredump.intra.peff.net.
> A potential memory leak is mentioned in this patch here https://lore.kernel.org/git/pull.1962.git.git.1747052530271.gitgitgadget@gmail.com/.

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

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

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

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

Something like:

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

-Peff

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

[lidongyan]

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

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