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

From: Taylor Blau <me@ttaylorr.com>
Date: May 23, 2025, 00:31 UTC
Message-ID: <aC/B21ZYCixgFSfe@nand.local>
In-Reply-To: <013153DA-8314-429B-8408-9A79A3304013@smail.nju.edu.cn>

On Thu, May 22, 2025 at 11:05:56PM +0800, lidongyan wrote:

Show 16 quoted lines

> > (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?
> >
>
> I am actually replaying the pack-bitmap.c:prepare_bitmap() here. Also I have had
> write a test helper version once. And since I want to use prepare_bitmap()
> I have to put the code in pack-bitmap.c. It looks like this
>
> diff --git a/pack-bitmap.c b/pack-bitmap.c
> index b9f1d866046..9642a06b3fe 100644
> --- a/pack-bitmap.c
> +++ b/pack-bitmap.c
> [...]

Yeah, since the pack_bitmap struct is defined locally within the pack-bitmap.c compilation unit, any test helper that performs any non-trivial operation would likely need to be defined in that file.

The "test helper" code would be a little shim into the real functionality within pack-bitmap.c. See the following for an example:

    - t/helper/test-bitmap.c::bitmap_list_commits()
    - pack-bitmap.c::test_bitmap_commits()

Here the former dispatches a single call to the latter, where all of the real functionality is.

But the (elided) code below isn't quite what I was thinking. I think the "write garbage data" part is fine as-is and can continue to be written in shell. We have lots of examples of using dd to write garbage data into files (see for e.g., the "corrupt_data()" function in t5319).

What I was thinking is the test helper would print (via some new mode, or bolted onto "list-commits") line-delimited output like the following:

    $COMMIT_OID $BITMAP_OFFSET $FLAGS $XOR_OFFSET

or similar. Then you could use the output of that to determine the location (replacing everything up to the actual "printf | dd of=$bitmap ...", which is the most fragile in my opinion).

Show 12 quoted lines

> > 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 want to *test* for a memory leak here, not whether git can load a corrupt bitmap.
> Since git ci linux-leak test runs each test script with ASAN_OPTIONS=detect_leaks=1, I’m
> including this test case specifically to check whether it triggers a crash when
> `SANITIZE_LEAK` is enabled. And I do find if without the first patch, leak sanitizer
> running this test script would output error message.

Makes sense.

Show 10 quoted lines

> > 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
>
> I am not truly following what are you talking here. But If you think it’s unnecessary to
> check for potential leaks in load_bitmap() or load_bitmap_entries_v1(). Or this test
> script shouldn’t be put in this way. I’m happy to drop the final patch.

I think the above scenario (writing a test that would have leaked memory otherwise behind a SANITIZE_LEAK prerequisite) is reasonable.

Thanks, Taylor