On Sat, May 17, 2025 at 02:26:22PM +0000, Lidong Yan via GitGitGadget wrote:
> From: Lidong Yan <502024330056@xxxxxxxxxxxxxxxx>
>
> 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
