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

From: Linus Arver <linusa@google.com>
Date: Jan 15, 2024, 22:31 UTC
Message-ID: <owly8r4qi5zj.fsf@fine.c.googlers.com>
In-Reply-To: <af5fe3b7237caeba8f970e967933db96c83a230e.1699569246.git.me@ttaylorr.com>

Taylor Blau <me@ttaylorr.com> writes:

Show 11 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 don't think this function should assume anything about the inputs (chunk_size, record_size, nor record_nr). If we are asking the helper to be the common tool for multiple locations, it should assume even less about what these inputs could look like.

For example, if record_size is 0 this will lead to a divide-by-zero. Likewise, if record_nr is zero it doesn't make sense to check if chunk_size fits into record_size * record_nr.

And even if there are no (unexpected) zero-values involved, shouldn't we also check for nonsensical comparisons, such as if chunk_size is smaller than record_size?

I think there are several possibilities:

CHUNK_MISSING (chunk_size == 0) CHUNK_TOO_SMALL (chunk_size < record_size) CHUNK_OK (chunk_size == record_nr * record_size) CHUNK_TOO_BIG (chunk_size > record_size, record_nr * record_size < chunk_size)

And for the CHUNK_TOO_BIG case there are two possibilities --- the
leftover parts of chunk_size that are not accounted by "record_nr *
record_size" are (or are not) "aligned" such that increasing the
record_size by some positive increment could exactly match the
chunk_size. For example, chunk_size = 128, record_size = 8, record_nr =
8. In this case the records account for 64 bytes so we have 128 - 64 =
64 bytes left over, and simply increasing record_nr to 16 could account
for every bytes in chunk_size. If chunk_size in this example was 120 or
130, there would be bytes left over that cannot be accounted for by
adjusting record_size. This divisibility-of-leftover-bytes check could
be done with '%' as mentioned already by others.

So in summary there appear to be the following possibilities:

CHUNK_MISSING CHUNK_TOO_SMALL CHUNK_OK CHUNK_TOO_BIG_ALIGNED CHUNK_TOO_BIG_MISALIGNED

(on top of the cases where record_* inputs are zero).

And it seems prudent to treat each of the not-OK cases differently (including how we report error messages) once we know which category we fall into.