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
