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