Re: [PATCH 3/4] fsck: check rev-index position values

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 17, 2023, 22:01 UTC
Message-ID: <xmqq1qkijhho.fsf@gitster.g>
In-Reply-To: <adbe9c8ee90e087e864bd9e0c67338974b5dbc8d.1681748502.git.gitgitgadget@gmail.com>

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 10 quoted lines

>  	if (!hashfile_checksum_valid((const unsigned char *)p->revindex_map, p->revindex_size)) {
>  		error(_("invalid checksum"));
> -		return -1;
> +		res = -1;
>  	}
>  
> -	return 0;
> +	/* This may fail due to a broken .idx. */
> +	if (create_pack_revindex_in_memory(p))
> +		return res;

Here, if the revindex file itself is self consistent, res would still be 0, so we will end up silently returning. Is the idea that while whatever causes in-memory revindex fail to be computed is a concern from the point of view of the repository's health, it would be caught elsewhere as a problem for the <pack,idx> pair we are seeing here?

Show 25 quoted lines

> +	for (size_t i = 0; i < p->num_objects; i++) {
> +		uint32_t nr = p->revindex[i].nr;
> +		uint32_t rev_val = get_be32(p->revindex_data + i);
> +
> +		if (nr != rev_val) {
> +			error(_("invalid rev-index position at %"PRIu64": %"PRIu32" != %"PRIu32""),
> +			      (uint64_t)i, nr, rev_val);
> +			res = -1;
> +		}
> +	}
> +
> +	return res;
>  }
>  
>  int load_midx_revindex(struct multi_pack_index *m)
> diff --git a/t/t5325-reverse-index.sh b/t/t5325-reverse-index.sh
> index 6b7c709a1f6..5c3c80f88f0 100755
> --- a/t/t5325-reverse-index.sh
> +++ b/t/t5325-reverse-index.sh
> @@ -185,4 +185,9 @@ test_expect_success 'fsck catches invalid checksum' '
>  		"invalid checksum"
>  '
>  
> +test_expect_success 'fsck catches invalid row position' '
> +	corrupt_rev_and_verify 14 "\07" \

;-)

I wondered how the patch made this "\07" portably available; it is fed as the format string to printf in the helper, which is clever but obviously correct way to do this. Nice.

> +		"invalid rev-index position"
> +'
> +
>  test_done