Re: [PATCH v2 3/3] pack-bitmap: add loading corrupt bitmap_index test
- From
Taylor Blau <me@ttaylorr.com>
- Date
- May 22, 2025, 00:08 UTC
- Message-ID
- <aC5rCRJd3GaTNgL5@nand.local>
- In-Reply-To
- <5be22d563af714ebb902506f12b4468a5348896c.1747732991.git.gitgitgadget@gmail.com>
On Tue, May 20, 2025 at 09:23:10AM +0000, Lidong Yan via GitGitGadget wrote:
Show 8 quoted lines
> From: Lidong Yan <502024330056@smail.nju.edu.cn> > > This patch add "load corrupt bitmap" test case in t5310-pack-bitmaps.sh. > > This test case intentionally corrupt the "xor_offset" field of the first > entry. To find position of first entry in *.bitmap, we need to skip 4 > ewah_bitmaps before entries. And I add a function `skip_ewah_bitmap()` > to do this.
I'm going to avoid commenting on the message itself, since I think we may be able to drop this patch entirely, see below.
Show 42 quoted lines
> Signed-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>
> ---
> t/t5310-pack-bitmaps.sh | 50 +++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 50 insertions(+)
>
> diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh
> index a62b463eaf09..537a507957bb 100755
> --- a/t/t5310-pack-bitmaps.sh
> +++ b/t/t5310-pack-bitmaps.sh
> @@ -26,6 +26,18 @@ has_any () {
> grep -Ff "$1" "$2"
> }
>
> +skip_ewah_bitmap() {
> + local bitmap="$1" &&
> + local offset="$2" &&
> + local size= &&
> +
> + offset=$(($offset + 4)) &&
> + size=0x$(od -An -v -t x1 -j $offset -N 4 $bitmap | tr -d ' \n') &&
> + size=$(($size * 8)) &&
> + offset=$(($offset + 4 + $size + 4)) &&
> + echo $offset
> +}
> +
> # Since name-hash values are stored in the .bitmap files, add a test
> # that checks that the name-hash calculations are stable across versions.
> # Not exhaustive, but these hashing algorithms would be hard to change
> @@ -486,6 +498,44 @@ test_bitmap_cases () {
> grep "ignoring extra bitmap" trace2.txt
> )
> '
> +
> + # A `.bitmap` file has the following structure:
> + # | Header | Commits | Trees | Blobs | Tags | Entries... |
> + #
> + # - The header is 32 bytes long when using SHA-1.
> + # - Commits, Trees, Blobs, and Tags are all stored as EWAH bitmaps.
> + #
> + # This test intentionally corrupts the `xor_offset` field of the first entry
> + # to verify robustness against malformed bitmap data.
> + test_expect_success 'load corrupt bitmap' 'I am not totally following what this case is supposed to be testing. Let me think aloud for a moment...
Show 6 quoted lines
> + rm -fr repo && > + git init repo && > + test_when_finished "rm -fr repo" && > + ( > + cd repo && > + git config pack.writeBitmapLookupTable '"$writeLookupTable"' &&
First we set up a temporary repository, change into it, and enable bitmap lookup tables. Makes sense.
Show 5 quoted lines
> + test_commit base && > + > + git repack -adb && > + bitmap="$(ls .git/objects/pack/pack-*.bitmap)" && > + chmod +w "$bitmap" &&
Then we make a commit, and write a bitmap containing the objects from the commit we just made. Good.
Show 5 quoted lines
> + hdr_sz=$((12 + $(test_oid rawsz))) && > + offset=$(skip_ewah_bitmap $bitmap $hdr_sz) && > + offset=$(skip_ewah_bitmap $bitmap $offset) && > + offset=$(skip_ewah_bitmap $bitmap $offset) && > + offset=$(skip_ewah_bitmap $bitmap $offset) &&
Then we read past the header and four type bitmaps. Makes sense.
> + offset=$((offset + 4)) &&
Now we land at the bitmap for the commit we just wrote.
(As an aside unrelated to this part of the test, this skip_ewah_bitmap() function seems awfully fragile. I wonder if it would make more sense to implement this as a test helper that can dump the offsets of EWAH bitmaps in a *.bitmap file by object ID rather than trying to parse the file ourselves?
We don't currently store an offset for each stored_bitmap that we maintain, but doing so would be pretty straightforward (add it as a field to the structure, and store the value of bitmap_git->map_pos from immediately before reading the actual bitmap).)
> + printf '\161' | > + dd of=$bitmap count=1 bs=1 conv=notrunc seek=$offset &&
OK. Now we break the XOR offset field of this bitmap by writing garbage into it.
> + git rev-list --count HEAD > expect && > + git rev-list --use-bitmap-index --count HEAD > actual && > + test_cmp expect actual
...and then we make sure that we still get the correct result.
Hmmph. I don't think this is quite testing what we want, since this test passes with or without your first patch. And that makes sense, we have tests elsewhere in this script that verify we can still fall back to classic traversal when the bitmap index can't be read. (For some examples, see: "truncated bitmap fails gracefully (ewah)" and "truncated bitmap fails gracefully (cache)".)
I think what we're really testing here is the absence of a memory leak, which we are as of 1fc7ddf35b (test-lib: unconditionally enable leak checking, 2024-11-20). I wonder whether or not we need this test at all?
Thanks, Taylor