git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Patrick Steinhardt <ps@pks.im>
Date
May 19, 2025, 06:02 UTC
Message-ID
<aCrJcK6ml4r4S-mF@pks.im>
In-Reply-To
<pull.1967.git.git.1747491983066.gitgitgadget@gmail.com>
On Sat, May 17, 2025 at 02:26:22PM +0000, Lidong Yan via GitGitGadget wrote:
Show 12 quoted lines
> 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_.

Show 73 quoted lines
> 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
Previous: Lidong Yan via GitGitGadgetNext: lidongyan
Message 2 of 7 in “pack-bitmap: add loading corrupt bitmap_index test”
  1. pack-bitmap: add loading corrupt bitmap_index testLidong Yan via GitGitGadget, May 17, 2025
  2. Patrick SteinhardtMay 19, 2025
  3. lidongyanMay 19, 2025
  4. Patrick SteinhardtMay 19, 2025
  5. lidongyanMay 19, 2025
  6. Jeff KingMay 19, 2025
  7. lidongyanMay 19, 2025

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.