On Fri, 13 Nov 2020 at 05:57, Jeff King <peff@xxxxxxxx> wrote: > > On Thu, Nov 12, 2020 at 06:47:09PM +0100, Martin Ågren wrote: > > > > + uint32_t cache_size = st_mult(index->pack->num_objects, sizeof(uint32_t)); > > > > Hmm. If `sizeof(size_t)` is 8, then this multiplication can't possibly > > overflow. A huge value of `num_objects` (say, 0xffffffff) would give a > > huge return value (0xffffffff<<2) which would be truncated (0xfffffffc). > > I think? > > Yeah, `cache_size` should absolutely be a `size_t`. If you have more > than a billion objects, obviously your cache is going to be bigger than > that. But most importantly, somebody can _claim_ to have a huge number > of objects and foil the size checks by wrapping around. > > > Do we want a `u32_mult()`? > > Nah, we should be doing this as a size_t in the first place. There are > similar problems with the .idx format, btw. I have a series to deal with > that which I've been meaning to post. Yes, that makes sense! > > if (cache_size > index_end - index->map - header_size) > > Yes, I agree this should be done as a subtraction as you showed to avoid > integer overflow. > > - if (index->map_size < sizeof(*header) + the_hash_algo->rawsz) > > + if (index->map_size < header_size + the_hash_algo->rawsz) > Right. I think that's right, and the previous patch is just buggy. Martin
