Re: [PATCH 1/9] commit-graph: handle overflow in chunk_size checks

From: Taylor Blau <me@ttaylorr.com>
Date: Nov 9, 2023, 21:13 UTC
Message-ID: <ZU1LbV0/ciDdO1aD@nand.local>
In-Reply-To: <20231109070948.GA2698043@coredump.intra.peff.net>

On Thu, Nov 09, 2023 at 02:09:48AM -0500, Jeff King wrote:

Show 11 quoted lines

> So instead, we can do a division like this:
>
>   if (chunk_size / GRAPH_DATA_WIDTH != g->num_commits)
>
> where there's no possibility of overflow. We do lose a little bit of
> precision; due to integer division truncation we'd allow up to an extra
> GRAPH_DATA_WIDTH-1 bytes of data in the chunk. That's OK. Our main goal
> here is making sure we don't have too _few_ bytes, which would cause an
> out-of-bounds read (we could actually replace our "!=" with "<", but I
> think it's worth being a little pedantic, as a large mismatch could be a
> sign of other problems).

This is wonderfully explained, and the patch below follows trivially from what you wrote here.

So everything in this patch makes sense and looks good to me. It does make me think about the pair_chunk_expect() function that I proposed elsewhere. I haven't yet read the rest of the series, so it may be a totally useless direction by the end of this series ;-).

But I wonder if the interface we want is something like:

    int pair_chunk_expect(struct chunkfile *cf, uint32_t chunk_id,
                          const unsigned char **p,
                          size_t record_size, size_t record_nr);

So we can then grab the size of the chunk, divide it by "record_size" and ensure that end up with "record_nr" as a result.

Again, this is perhaps totally useless by the end of your series, but just having looked at the first patch I wonder if this is a productive direction to consider...

Thanks, Taylor