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

From: Jeff King <peff@peff.net>
Date: Nov 10, 2023, 21:57 UTC
Message-ID: <20231110215747.GG2758295@coredump.intra.peff.net>
In-Reply-To: <xmqqedgyw6jv.fsf@gitster.g>

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

Show 30 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.
> 
> 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.

Yes, the distinction does matter. If pair_chunk_expect_fn() used st_mult(), then it would die on overflow, rather than returning an error. For commit-graph files this is propagated up, and we continue the operation by falling back to the non-graph code. There's a test in t5318 that will fail in this case. See patches 1 and 6 in my series for more discussion.

One of those patches calls out the truncating division issue, but to summarize: IMHO this is OK, as what we really want to know is "is it big enough that we can always ask for NR records of size ELEM", which division gives us. If we do want to be more precise, but also avoid die(), we'd need a variant of st_mult() that returns a boolean. I didn't think it was worth it for this case (but arguably it is something that would be useful to have in general).

-Peff