Re: [PATCH] test_bitmap_hashes(): handle repository without bitmaps

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 5, 2021, 18:52 UTC
Message-ID: <xmqq35oaxwnz.fsf@gitster.g>
In-Reply-To: <YYTy6+DG5guzJIO7@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 21 quoted lines

> If prepare_bitmap_git() returns NULL (one easy-to-trigger cause being
> that the repository does not have bitmaps at all), then we'll segfault
> accessing bitmap_git->hashes:
>
>   $ t/helper/test-tool bitmap dump-hashes
>   Segmentation fault
>
> We should treat this the same as a repository with bitmaps but no
> name-hashes, and quietly produce an empty output. The later call to
> free_bitmap_index() in the cleanup label is OK, as it treats a NULL
> pointer as a noop.
>
> This isn't a big deal in practice, as this function is intended for and
> used only by test-tool. It's probably worth fixing to avoid confusion,
> but not worth adding coverage for this to the test suite.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> This is new in the v2.34.0 cycle, but it's so low impact it doesn't
> matter much if we ship with the bug. OTOH, it's pretty low-risk since it
> is only run by the test suite.

;-)

I wonder how you found it. Diagnosing a repository that did not seem healthy? What I am getting at is if we want a new option to make a plumbing command, other than the test-tool, that calls this function, as the latter is usually not deployed in the field.

Will queue.  Thanks.

Show 16 quoted lines

>  pack-bitmap.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/pack-bitmap.c b/pack-bitmap.c
> index a56ceb9441..f772d3cb7f 100644
> --- a/pack-bitmap.c
> +++ b/pack-bitmap.c
> @@ -1759,7 +1759,7 @@ int test_bitmap_hashes(struct repository *r)
>  	struct object_id oid;
>  	uint32_t i, index_pos;
>  
> -	if (!bitmap_git->hashes)
> +	if (!bitmap_git || !bitmap_git->hashes)
>  		goto cleanup;
>  
>  	for (i = 0; i < bitmap_num_objects(bitmap_git); i++) {