Re: [PATCH 0/20] bounds-checks for chunk-based files

From: Jeff King <peff@peff.net>
Date: Oct 11, 2023, 23:31 UTC
Message-ID: <20231011233121.GN518221@coredump.intra.peff.net>
In-Reply-To: <ZSb1QAaLX+xcZK4a@nand.local>

On Wed, Oct 11, 2023 at 03:19:28PM -0400, Taylor Blau wrote:

Show 7 quoted lines

> I reviewed this carefully (well, except for the new Perl script, for
> obvious[^1] reasons ;-)).
> 
> Everything mostly looks good to me, though I
> had a handful of review comments throughout. Many of them are trivial
> (e.g. a number of warning() and error() strings should be marked for
> translation, etc.), but a couple of them I think are worth looking at.

Thanks for taking a look. I think it may make sense to come back on top and adjust a few of the commit messages, along with adding a few st_mult() overflow checks that you suggest.

Show 5 quoted lines

> Most notably, I think that by the end of the series, I was convinced
> that having some kind of 'pair_chunk_expectsz()' or similar would be
> useful and eliminate a good chunk of the boilerplate you have to write
> to check the chunk size against an expected value when using
> read_chunk().

This I'm less convinced by. In fact, I _almost_ just dropped pair_chunk() entirely. Adding an out-parameter for the size at least forces the caller to consider what to do with the size. But really, I think the right mindset is "we should be sanity-checking this chunk as we load it". And having a callback, even if it is a little bit of boilerplate, helps set that frame of mind.

I dunno. Maybe that is all just programmer pseudo-psychology. But I also don't like that about half the calls to pair_chunk() can't do a size check (so we need two functions, or to make the "expect" parameter optional).

-Peff