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

From: Taylor Blau <me@ttaylorr.com>
Date: Nov 10, 2023, 16:27 UTC
Message-ID: <ZU5Z/Z4PcdNP5U1r@nand.local>
In-Reply-To: <xmqqedgyw6jv.fsf@gitster.g>

On Fri, Nov 10, 2023 at 01:55:48PM +0900, Junio C Hamano wrote:

Show 25 quoted lines

> Taylor Blau <me@ttaylorr.com> writes:
>
> > +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.

Hmm. I was thinking of Peff's "commit-graph: handle overflow in chunk_size checks", but I think that I was overly eager in applying the same reasoning to the MIDX code.

The important piece of the rationale in that patch is as follows:

    In the current code this is only possible for the CDAT chunk, but
    the reasons are quite subtle. We compute g->num_commits by dividing
    the size of the OIDL chunk by the hash length (since it consists of
    a bunch of hashes). So we know that any size_t multiplication that
    uses a value smaller than the hash length cannot overflow. And the
    CDAT records are the only ones that are larger (the others are just
    4-byte records). So it's worth fixing all of these, to make it clear
    that they're not subject to overflow (without having to reason about
    seemingly unrelated code).

In particular, that g->num_commits is computed by dividing the length of the OIDL chunk by the hash length, thus any size_t multiplication of g->num_commits with a value smaller than that hash length cannot overflow.

But I don't think we enjoy the same benefits in the MIDX scenario. In this case, the num_objects field is just:

    m->num_objects = ntohl(m->chunk_oid_fanout[255])

so I don't think we can make the same guarantees about what is and isn't save to compute under size_t arithmetic.

I'd be curious what Peff has to say, but if he agrees with me, I'd recommend taking the first five patches, and dropping the two MIDX-related ones.

Thanks, Taylor