Subject: Re: [PATCH 02/17] chunk-format: create chunk format write API

From: Derrick Stolee <[email-masked]>
Date: Wed, 27 Jan 2021 08:49:16 -0500

On 1/26/2021 9:42 PM, Taylor Blau wrote:
> On Tue, Jan 26, 2021 at 04:01:11PM +0000, Derrick Stolee via GitGitGadget wrote:
>> +/*
>> + * When writing a chunk-based file format, collect the chunks in
>> + * an array of chunk_info structs. The size stores the _expected_
>> + * amount of data that will be written by write_fn.
>> + */
>> +struct chunk_info {
>> +	uint32_t id;
>> +	uint64_t size;
> 
> Hmm. Would we not want an off_t to indicate the size here?
> 
> I wondered briefly if we even needed a size field at all, since calling
> write_fn would tell us the number of bytes written. But I suppose you
> want to know ahead of time so that you can write the file in one pass
> (beginning with the table of contents, which certainly needs to know the
> size).

Is off_t 64-bits on a 32-bit machine? This is intentionally typed
to be "64 bits no matter what" because it correlates with the file
format's size for the chunk offsets.

>> +		if (cf->f->total + cf->f->offset != start_offset + cf->chunks[i].size)
> 
> I don't think this is a practical concern, but a malicious caller could
> overflow this by passing a bogus "size" parameter. Maybe:
> 
>     uint64_t end_offset = ...;
> 
>     if (end_offset - start_offset != cf->chunks[i].size)
>       BUG(...)

Sure.

Thanks,
-Stolee