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

[Patrick Steinhardt <ps@pks.im>]

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