Re: [PATCH 1/7] chunk-format: introduce `pair_chunk_expect()` helper

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 10, 2023, 04:55 UTC
Message-ID: <xmqqedgyw6jv.fsf@gitster.g>
In-Reply-To: <af5fe3b7237caeba8f970e967933db96c83a230e.1699569246.git.me@ttaylorr.com>

Taylor Blau <me@ttaylorr.com> writes:

Show 10 quoted lines

> +static int pair_chunk_expect_fn(const unsigned char *chunk_start,
> +				size_t chunk_size,
> +				void *data)
> +{
> +	struct pair_chunk_data *pcd = data;
> +	if (chunk_size / pcd->record_size != pcd->record_nr)
> +		return -1;
> +	*pcd->p = chunk_start;
> +	return 0;
> +}

I know one of the original places did the "divide the whole by per-record size and see if it matches the number of records", the same as we see above, but the check in the above could also be

	if (chunk_size != st_mult(pcd->record_size, pcd->record_nr))
		return -1;

which would also catch the case where chunk_size is not a multiple of the record size. Your conversion of OOFF in midx.c loses this protection as the original uses the multiplication-and-compare, but the rewrite to call pair_chunk_expect would call the above and checks with the truncating-divide-and-compare.

Does the distinction matter? I dunno. If the record/chunk alignment is asserted elsewhere, then the distinction should not matter, but even if it were, seeing a truncating division used in any validation makes my skin tingle.

Other than that, the series was a pleasant read.

Thanks.