From: Phillip Wood Date: Fri, 14 Jul 2023 09:55:10 GMT Subject: Re: [PATCH 02/20] packfile.c: prevent overflow in `load_idx()` Message-ID: <7b5d695e-07da-adba-d9ff-5551102881e1@gmail.com> In-Reply-To: On 13/07/2023 15:24, Taylor Blau wrote: > On Thu, Jul 13, 2023 at 09:21:55AM +0100, Phillip Wood wrote: >>> diff --git a/packfile.c b/packfile.c >>> index 89220f0e03..70acf1694b 100644 >>> --- a/packfile.c >>> +++ b/packfile.c >>> @@ -186,7 +186,7 @@ int load_idx(const char *path, const unsigned int hashsz, void *idx_map, >>> */ >>> (sizeof(off_t) <= 4)) >>> return error("pack too large for current definition of off_t in %s", path); >>> - p->crc_offset = 8 + 4 * 256 + nr * hashsz; >>> + p->crc_offset = st_add(8 + 4 * 256, st_mult(nr, hashsz)); >> >> p->crc_offset is a uint32_t so we're still prone to truncation here unless >> we change the crc_offset member of struct packed_git to be a size_t. I >> haven't checked if the other users of crc_offset would need adjusting if we >> change its type. > > Thanks for spotting. Luckily, this should be a straightforward change: > > $ git grep crc_offset > builtin/index-pack.c: idx1 = (((const uint32_t *)((const uint8_t *)p->index_data + p->crc_offset)) > object-store-ll.h: uint32_t crc_offset; > packfile.c: p->crc_offset = st_add(8 + 4 * 256, st_mult(nr, hashsz)); > > The single usage in index-pack is OK, so we only need to change its type > to a size_t. That's good, it is nice it is such a simple change > I could see an argument that this should be an off_t, since it is an > offset into a file. But since we memory map the whole thing anyway, I > think we are equally OK to treat it as a pointer offset. A similar > argument is made in f86f769550e (compute pack .idx byte offsets using > size_t, 2020-11-13), so I am content to leave this as a size_t. If we're already using size_t where one could argue in favor of using off_t I think that makes sense. Best Wishes Phillip