{"thread":{"id":"59978","subject":"[PATCH 00/20] guard object lookups against 32-bit overflow","startedAt":"2023-07-12T23:37:30Z","lastAt":"2023-07-14T16:29:47Z","messageCount":28,"participants":["Taylor Blau","Phillip Wood","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":20},"messages":[{"id":"479427","messageId":"cover.1689205042.git.me@ttaylorr.com","threadId":"59978","inReplyTo":null,"subject":"[PATCH 00/20] guard object lookups against 32-bit overflow","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-07-12T23:37:24Z","receivedAt":"2023-07-12T23:37:30Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"This is a series I wrote over the last week or so to address a handful\nof spots where we may overflow a computation, and produce incorrect\nresults.\n\nMost of these share a common theme, which is that many of our\ndata structures (packfiles, bitmaps, commit-graph, MIDX) all tolerate\nup to 2^32-1 objects. But when we, say, multiply the number of objects\nby some constant or other value which isn't a 64-bit unsigned, we'll\noverflow.\n\nOften times the overflows occur when looking up an object / bit\nposition / commit / etc which is at position 2^32-1/20 or greater,\ncausing us to exceed the maximum 32-bit unsigned value when we multiply\nby 20 (or the_hash_algo->rawsz, if stored as an \"unsigned int\").\n\nThere are a handful of instances where the existing implementation won't\noverflow (i.e. because one of the operands is already a size_t, or an\nexplicit cast is made, etc.). These instances are also replaced with\ntheir corresponding checked functions (st_add(), st_mult(), etc.) to\nmore clearly express that these operations are supposed to be computed\nwith 64-bit values.\n\nThis series regrettably does not contain any tests. There was some\ndiscussion internally about using a purpose-built test helper to\ngenerate raw packs without going through fast-import. I looked around\nfor prior examples of how we handled testing (or not) when writing these\nkinds of patches with the following script:\n\n    $ git log --format='%H' -G 'st_(mult|add[234]?)\\(' |\n        xargs -I {} sh -c 'echo \"==> {}\" && git -P diff {}^ {} -- t'\n\nAnd looking at each such commit which mentions st_mult(), st_add(), or\none of its variants, I could not find any examples of us constructing a\ngigantic pack/MIDX/commit-graph/bitmap/etc specifically to trigger an\noverflow.\n\nTaylor Blau (20):\n  packfile.c: prevent overflow in `nth_packed_object_id()`\n  packfile.c: prevent overflow in `load_idx()`\n  packfile.c: use checked arithmetic in `nth_packed_object_offset()`\n  midx.c: use `size_t`'s for fanout nr and alloc\n  midx.c: prevent overflow in `nth_midxed_object_oid()`\n  midx.c: prevent overflow in `nth_midxed_offset()`\n  midx.c: store `nr`, `alloc` variables as `size_t`'s\n  midx.c: prevent overflow in `write_midx_internal()`\n  midx.c: prevent overflow in `fill_included_packs_batch()`\n  pack-bitmap.c: ensure that eindex lookups don't overflow\n  commit-graph.c: prevent overflow in `write_commit_graph_file()`\n  commit-graph.c: prevent overflow in add_graph_to_chain()\n  commit-graph.c: prevent overflow in `load_oid_from_graph()`\n  commit-graph.c: prevent overflow in `fill_commit_graph_info()`\n  commit-graph.c: prevent overflow in `fill_commit_in_graph()`\n  commit-graph.c: prevent overflow in `load_tree_for_commit()`\n  commit-graph.c: prevent overflow in `split_graph_merge_strategy()`\n  commit-graph.c: prevent overflow in `merge_commit_graph()`\n  commit-graph.c: prevent overflow in `write_commit_graph()`\n  commit-graph.c: prevent overflow in `verify_commit_graph()`\n\n commit-graph.c | 63 ++++++++++++++++++++++++++++++++------------------\n midx.c         | 42 ++++++++++++++++++---------------\n pack-bitmap.c  | 12 ++++++----\n packfile.c     | 15 ++++++------\n 4 files changed, 79 insertions(+), 53 deletions(-)\n\n-- \n2.41.0.347.g7b976b8871f\n"},{"id":"479428","messageId":"5e92582e2912806e0068af97c265fb50e8bbe54f.1689205042.git.me@ttaylorr.com","threadId":"59978","inReplyTo":"cover.1689205042.git.me@ttaylorr.com","subject":"[PATCH 01/20] packfile.c: prevent overflow in `nth_packed_object_id()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-07-12T23:37:27Z","receivedAt":"2023-07-12T23:37:36Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In 37fec86a83 (packfile: abstract away hash constant values,\n2018-05-02), `nth_packed_object_id()` started using the variable\n`the_hash_algo->rawsz` instead of a fixed constant when trying to\ncompute an offset into the \".idx\" file for some object position.\n\nThis can lead to surprising truncation when looking for an object\ntowards the end of a large enough pack, like the following:\n\n    (gdb) p hashsz\n    $1 = 20\n    (gdb) p n\n    $2 = 215043814\n    (gdb) p hashsz * n\n    $3 = 5908984\n\n, which is a debugger session broken on a known-bad call to the\n`nth_packed_object_id()` function.\n\nThis behavior predates 37fec86a83, and is original to the v2 index\nformat, via: 74e34e1fca (sha1_file.c: learn about index version 2,\n2007-04-09).\n\nThis is due to §6.4.4.1 of the C99 standard, which states that an\nuntyped integer constant will take the first type in which the value can\nbe accurately represented, among `int`, `long int`, and `long long int`.\n\nSince 20 can be represented as an `int`, and `n` is a 32-bit unsigned\ninteger, the resulting computation is defined by §6.3.1.8, and the\n(signed) integer value representing `n` is converted to an unsigned\ntype, meaning that `20 * n` (for `n` having type `uint32_t`) is\nequivalent to a multiplication between two unsigned 32-bit integers.\n\nWhen multiplying a sufficiently large `n`, the resulting value can\nexceed 2^32-1, wrapping around and producing an invalid result. Let's\nfollow the example in f86f769550e (compute pack .idx byte offsets using\nsize_t, 2020-11-13) and replace this computation with `st_mult()`, which\nwill ensure that the computation is done using 64-bits.\n\nWhile here, guard the corresponding computation for packs with v1\nindexes, too. Though the likelihood of seeing a bug there is much\nsmaller, since (a) v1 indexes are generated far less frequently than v2\nindexes, and (b) they all correspond to packs no larger than 2 GiB, so\nhaving enough objects to trigger this overflow is unlikely if not\nimpossible.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n packfile.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/packfile.c b/packfile.c\nindex c2e753ef8f..89220f0e03 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -1920,10 +1920,10 @@ int nth_packed_object_id(struct object_id *oid,\n \t\treturn -1;\n \tindex += 4 * 256;\n \tif (p->index_version == 1) {\n-\t\toidread(oid, index + (hashsz + 4) * n + 4);\n+\t\toidread(oid, index + st_add(st_mult(hashsz + 4, n), 4));\n \t} else {\n \t\tindex += 8;\n-\t\toidread(oid, index + hashsz * n);\n+\t\toidread(oid, index + st_mult(hashsz, n));\n \t}\n \treturn 0;\n }\n-- \n2.41.0.347.g7b976b8871f\n\n"},{"id":"479429","messageId":"d6902cd9e7f7f2a6b8044c8fb782a28c23e15600.1689205042.git.me@ttaylorr.com","threadId":"59978","inReplyTo":"cover.1689205042.git.me@ttaylorr.com","subject":"[PATCH 02/20] packfile.c: prevent overflow in `load_idx()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-07-12T23:37:30Z","receivedAt":"2023-07-12T23:37:38Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Prevent an overflow when locating a pack's CRC offset when the number\nof packed items is greater than 2^32-1/hashsz by guarding the\ncomputation with an `st_mult()`.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n packfile.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/packfile.c b/packfile.c\nindex 89220f0e03..70acf1694b 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -186,7 +186,7 @@ int load_idx(const char *path, const unsigned int hashsz, void *idx_map,\n \t\t     */\n \t\t    (sizeof(off_t) <= 4))\n \t\t\treturn error(\"pack too large for current definition of off_t in %s\", path);\n-\t\tp->crc_offset = 8 + 4 * 256 + nr * hashsz;\n+\t\tp->crc_offset = st_add(8 + 4 * 256, st_mult(nr, hashsz));\n \t}\n \n \tp->index_version = version;\n-- \n2.41.0.347.g7b976b8871f\n\n"},{"id":"479430","messageId":"750d38cbeba42416d98895d6e0004d1ca6329ce5.1689205042.git.me@ttaylorr.com","threadId":"59978","inReplyTo":"cover.1689205042.git.me@ttaylorr.com","subject":"[PATCH 03/20] packfile.c: use checked arithmetic in `nth_packed_object_offset()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-07-12T23:37:32Z","receivedAt":"2023-07-12T23:37:40Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In a similar spirit as the previous commits, ensure that we use\n`st_add()` or `st_mult()` when computing values that may overflow the\n32-bit unsigned limit.\n\nNote that in each of these instances, we prevent 32-bit overflow\nalready since we have explicit casts to `size_t`.\n\nSo this code is OK as-is, but let's clarify it by using the `st_xyz()`\nhelpers to make it obvious that we are performing the relevant\ncomputations using 64 bits.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n packfile.c | 9 +++++----\n 1 file changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git a/packfile.c b/packfile.c\nindex 70acf1694b..e8e01e348e 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -1948,14 +1948,15 @@ off_t nth_packed_object_offset(const struct packed_git *p, uint32_t n)\n \tconst unsigned int hashsz = the_hash_algo->rawsz;\n \tindex += 4 * 256;\n \tif (p->index_version == 1) {\n-\t\treturn ntohl(*((uint32_t *)(index + (hashsz + 4) * (size_t)n)));\n+\t\treturn ntohl(*((uint32_t *)(index + st_mult(hashsz + 4, n))));\n \t} else {\n \t\tuint32_t off;\n-\t\tindex += 8 + (size_t)p->num_objects * (hashsz + 4);\n-\t\toff = ntohl(*((uint32_t *)(index + 4 * n)));\n+\t\tindex += st_add(8, st_mult(p->num_objects, hashsz + 4));\n+\t\toff = ntohl(*((uint32_t *)(index + st_mult(4, n))));\n \t\tif (!(off & 0x80000000))\n \t\t\treturn off;\n-\t\tindex += (size_t)p->num_objects * 4 + (off & 0x7fffffff) * 8;\n+\t\tindex += st_add(st_mult(p->num_objects, 4),\n+\t\t\t\tst_mult(off & 0x7fffffff, 8));\n \t\tcheck_pack_index_ptr(p, index);\n \t\treturn get_be64(index);\n \t}\n-- \n2.41.0.347.g7b976b8871f\n\n"},{"id":"479431","messageId":"bdad7286046e5f4ae43d4b2571fab2d3e31912c7.1689205042.git.me@ttaylorr.com","threadId":"59978","inReplyTo":"cover.1689205042.git.me@ttaylorr.com","subject":"[PATCH 04/20] midx.c: use `size_t`'s for fanout nr and alloc","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-07-12T23:37:36Z","receivedAt":"2023-07-12T23:38:00Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"The `midx_fanout` struct is used to keep track of a set of OIDs\ncorresponding to each layer of the MIDX's fanout table. It stores an\narray of entries, along with the number of entries in the table, and the\nallocated size of the array.\n\nBoth `nr` and `alloc` are stored as 32-bit unsigned integers. In\npractice, this should never cause any problems, since most packs have\nfar fewer than 2^32-1 objects.\n\nBut storing these as `size_t`'s is more appropriate, and prevents us\nfrom accidentally overflowing some result when multiplying or adding to\neither of these values. Update these struct members to be `size_t`'s as\nappropriate.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n midx.c | 8 +++++---\n 1 file changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/midx.c b/midx.c\nindex db459e448b..449c10289c 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -584,12 +584,14 @@ static void fill_pack_entry(uint32_t pack_int_id,\n \n struct midx_fanout {\n \tstruct pack_midx_entry *entries;\n-\tuint32_t nr;\n-\tuint32_t alloc;\n+\tsize_t nr, alloc;\n };\n \n-static void midx_fanout_grow(struct midx_fanout *fanout, uint32_t nr)\n+static void midx_fanout_grow(struct midx_fanout *fanout, size_t nr)\n {\n+\tif (nr < fanout->nr)\n+\t\tBUG(\"negative growth in midx_fanout_grow() (%\"PRIuMAX\" < %\"PRIuMAX\")\",\n+\t\t    (uintmax_t)nr, (uintmax_t)fanout->nr);\n \tALLOC_GROW(fanout->entries, nr, fanout->alloc);\n }\n \n-- \n2.41.0.347.g7b976b8871f\n\n"},{"id":"479432","messageId":"1a60b79296c567da33cb8e7001ff21c484836989.1689205042.git.me@ttaylorr.com","threadId":"59978","inReplyTo":"cover.1689205042.git.me@ttaylorr.com","subject":"[PATCH 05/20] midx.c: prevent overflow in `nth_midxed_object_oid()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-07-12T23:37:38Z","receivedAt":"2023-07-12T23:38:01Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In a similar spirit as previous commits, avoid overflow when looking up\nan object's OID in a MIDX when its position is greater than\n`2^32-1/m->hash_len`.\n\nAs usual, it is perfectly OK for a MIDX to have as many as 2^32-1\nobjects (since we use 32-bit fields to count the number of objects at\neach fanout layer). But if we have more than `2^32-1/m->hash_len` number\nof objects, we will incorrectly perform the computation using 32-bit\nintegers, overflowing the result.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n midx.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/midx.c b/midx.c\nindex 449c10289c..dbc63c0d42 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -254,7 +254,7 @@ struct object_id *nth_midxed_object_oid(struct object_id *oid,\n \tif (n >= m->num_objects)\n \t\treturn NULL;\n \n-\toidread(oid, m->chunk_oid_lookup + m->hash_len * n);\n+\toidread(oid, m->chunk_oid_lookup + st_mult(m->hash_len, n));\n \treturn oid;\n }\n \n-- \n2.41.0.347.g7b976b8871f\n\n"},{"id":"479433","messageId":"7973822d87d571d472be336a3e102ce2b3c85fc2.1689205042.git.me@ttaylorr.com","threadId":"59978","inReplyTo":"cover.1689205042.git.me@ttaylorr.com","subject":"[PATCH 06/20] midx.c: prevent overflow in `nth_midxed_offset()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-07-12T23:37:41Z","receivedAt":"2023-07-12T23:38:03Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In a similar spirit as previous patches, avoid an overflow when looking\nup object offsets in the MIDX's large offset table by guarding the\ncomputation via `st_mult()`.\n\nThis instance is also OK as-is, since the left operand is the result of\n`sizeof(...)`, which is already a `size_t`. But use `st_mult()` instead\nhere to make it explicit that this computation is to be performed using\n64-bit unsigned integers.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n midx.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/midx.c b/midx.c\nindex dbc63c0d42..a5a4ff4398 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -271,7 +271,8 @@ off_t nth_midxed_offset(struct multi_pack_index *m, uint32_t pos)\n \t\t\tdie(_(\"multi-pack-index stores a 64-bit offset, but off_t is too small\"));\n \n \t\toffset32 ^= MIDX_LARGE_OFFSET_NEEDED;\n-\t\treturn get_be64(m->chunk_large_offsets + sizeof(uint64_t) * offset32);\n+\t\treturn get_be64(m->chunk_large_offsets +\n+\t\t\t\tst_mult(sizeof(uint64_t), offset32));\n \t}\n \n \treturn offset32;\n-- \n2.41.0.347.g7b976b8871f\n\n"},{"id":"479434","messageId":"09bd84700bd1904cd5340d5c4dbf7bd84ae1922d.1689205042.git.me@ttaylorr.com","threadId":"59978","inReplyTo":"cover.1689205042.git.me@ttaylorr.com","subject":"[PATCH 08/20] midx.c: prevent overflow in `write_midx_internal()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-07-12T23:37:46Z","receivedAt":"2023-07-12T23:38:05Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When writing a MIDX, we use the chunk-format API to write out each\nindividual chunk of the MIDX. Each chunk of the MIDX is tracked via a\ncall to `add_chunk()`, along with the expected size of that chunk.\n\nGuard against overflow when dealing with a MIDX with a large number of\nentries (and consequently, large chunks within the MIDX file itself) to\navoid corrupting the contents of the MIDX itself.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n midx.c | 9 +++++----\n 1 file changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git a/midx.c b/midx.c\nindex b176745df1..57c53dbd4a 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -1501,21 +1501,22 @@ static int write_midx_internal(const char *object_dir,\n \tadd_chunk(cf, MIDX_CHUNKID_OIDFANOUT, MIDX_CHUNK_FANOUT_SIZE,\n \t\t  write_midx_oid_fanout);\n \tadd_chunk(cf, MIDX_CHUNKID_OIDLOOKUP,\n-\t\t  (size_t)ctx.entries_nr * the_hash_algo->rawsz,\n+\t\t  st_mult(ctx.entries_nr, the_hash_algo->rawsz),\n \t\t  write_midx_oid_lookup);\n \tadd_chunk(cf, MIDX_CHUNKID_OBJECTOFFSETS,\n-\t\t  (size_t)ctx.entries_nr * MIDX_CHUNK_OFFSET_WIDTH,\n+\t\t  st_mult(ctx.entries_nr, MIDX_CHUNK_OFFSET_WIDTH),\n \t\t  write_midx_object_offsets);\n \n \tif (ctx.large_offsets_needed)\n \t\tadd_chunk(cf, MIDX_CHUNKID_LARGEOFFSETS,\n-\t\t\t(size_t)ctx.num_large_offsets * MIDX_CHUNK_LARGE_OFFSET_WIDTH,\n+\t\t\tst_mult(ctx.num_large_offsets,\n+\t\t\t\tMIDX_CHUNK_LARGE_OFFSET_WIDTH),\n \t\t\twrite_midx_large_offsets);\n \n \tif (flags & (MIDX_WRITE_REV_INDEX | MIDX_WRITE_BITMAP)) {\n \t\tctx.pack_order = midx_pack_order(&ctx);\n \t\tadd_chunk(cf, MIDX_CHUNKID_REVINDEX,\n-\t\t\t  ctx.entries_nr * sizeof(uint32_t),\n+\t\t\t  st_mult(ctx.entries_nr, sizeof(uint32_t)),\n \t\t\t  write_midx_revindex);\n \t}\n \n-- \n2.41.0.347.g7b976b8871f\n\n"},{"id":"479436","messageId":"80c79f6fea2fdafb00e6c2866ae1756c1956d16e.1689205042.git.me@ttaylorr.com","threadId":"59978","inReplyTo":"cover.1689205042.git.me@ttaylorr.com","subject":"[PATCH 09/20] midx.c: prevent overflow in `fill_included_packs_batch()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-07-12T23:37:49Z","receivedAt":"2023-07-12T23:38:07Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In a similar spirit as in previous commits, avoid an integer overflow\nwhen computing the expected size of a MIDX.\n\n(Note that this is also OK as-is, since `p->pack_size` is an `off_t`, so\nthis computation should already be done as 64-bit integers. But again,\nlet's use `st_mult()` to make this fact clear).\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n midx.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/midx.c b/midx.c\nindex 57c53dbd4a..a5e4094340 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -1994,8 +1994,8 @@ static int fill_included_packs_batch(struct repository *r,\n \t\tif (open_pack_index(p) || !p->num_objects)\n \t\t\tcontinue;\n \n-\t\texpected_size = (size_t)(p->pack_size\n-\t\t\t\t\t * pack_info[i].referenced_objects);\n+\t\texpected_size = st_mult(p->pack_size,\n+\t\t\t\t\tpack_info[i].referenced_objects);\n \t\texpected_size /= p->num_objects;\n \n \t\tif (expected_size >= batch_size)\n-- \n2.41.0.347.g7b976b8871f\n\n"},{"id":"479435","messageId":"822cb19da4e7dac5b1d4d4c7e115d7e4bcb7c976.1689205042.git.me@ttaylorr.com","threadId":"59978","inReplyTo":"cover.1689205042.git.me@ttaylorr.com","subject":"[PATCH 10/20] pack-bitmap.c: ensure that eindex lookups don't overflow","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-07-12T23:37:52Z","receivedAt":"2023-07-12T23:38:09Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When a bitmap is used to answer some reachability query, it creates a\npseudo-bitmap called the \"extended index\" on top of any existing bitmaps\nto store objects that are relevant to the query, but not mentioned in\nthe bitmap.\n\nWhen looking up the ith object in the extended index in a bitmap, it is\ncommon to write something like:\n\n    bitmap_get(result, i + bitmap_num_objects(bitmap_git))\n\n, indicating that we want the ith object following all other objects\nmentioned in the bitmap_git.\n\nSince the type of `i` and the return type of `bitmap_num_objects()` are\nboth `uint32_t`s,  But if there are either a large number of objects in\nthe bitmap, or a large number of objects in the extended index (or\nboth), this addition can overflow when the sum is greater than 2^32-1.\n\nHaving that large of a bitmap position is entirely acceptable, but we\nneed to ensure that the computed bitmap position for that object is\nperformed using 64-bits and doesn't overflow.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n pack-bitmap.c | 12 +++++++-----\n 1 file changed, 7 insertions(+), 5 deletions(-)\n\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex 7367f62bb6..7ddb465c20 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -1294,7 +1294,7 @@ static void show_extended_objects(struct bitmap_index *bitmap_git,\n \tfor (i = 0; i < eindex->count; ++i) {\n \t\tstruct object *obj;\n \n-\t\tif (!bitmap_get(objects, bitmap_num_objects(bitmap_git) + i))\n+\t\tif (!bitmap_get(objects, st_add(bitmap_num_objects(bitmap_git), i)))\n \t\t\tcontinue;\n \n \t\tobj = eindex->objects[i];\n@@ -1473,7 +1473,7 @@ static void filter_bitmap_exclude_type(struct bitmap_index *bitmap_git,\n \t * them individually.\n \t */\n \tfor (i = 0; i < eindex->count; i++) {\n-\t\tuint32_t pos = i + bitmap_num_objects(bitmap_git);\n+\t\tsize_t pos = st_add(i, bitmap_num_objects(bitmap_git));\n \t\tif (eindex->objects[i]->type == type &&\n \t\t    bitmap_get(to_filter, pos) &&\n \t\t    !bitmap_get(tips, pos))\n@@ -1564,7 +1564,7 @@ static void filter_bitmap_blob_limit(struct bitmap_index *bitmap_git,\n \t}\n \n \tfor (i = 0; i < eindex->count; i++) {\n-\t\tuint32_t pos = i + bitmap_num_objects(bitmap_git);\n+\t\tsize_t pos = st_add(i, bitmap_num_objects(bitmap_git));\n \t\tif (eindex->objects[i]->type == OBJ_BLOB &&\n \t\t    bitmap_get(to_filter, pos) &&\n \t\t    !bitmap_get(tips, pos) &&\n@@ -2038,7 +2038,8 @@ static uint32_t count_object_type(struct bitmap_index *bitmap_git,\n \n \tfor (i = 0; i < eindex->count; ++i) {\n \t\tif (eindex->objects[i]->type == type &&\n-\t\t\tbitmap_get(objects, bitmap_num_objects(bitmap_git) + i))\n+\t\t    bitmap_get(objects,\n+\t\t\t       st_add(bitmap_num_objects(bitmap_git), i)))\n \t\t\tcount++;\n \t}\n \n@@ -2452,7 +2453,8 @@ static off_t get_disk_usage_for_extended(struct bitmap_index *bitmap_git)\n \tfor (i = 0; i < eindex->count; i++) {\n \t\tstruct object *obj = eindex->objects[i];\n \n-\t\tif (!bitmap_get(result, bitmap_num_objects(bitmap_git) + i))\n+\t\tif (!bitmap_get(result,\n+\t\t\t\tst_add(bitmap_num_objects(bitmap_git), i)))\n \t\t\tcontinue;\n \n \t\tif (oid_object_info_extended(the_repository, &obj->oid, &oi, 0) < 0)\n-- \n2.41.0.347.g7b976b8871f\n\n"},{"id":"479438","messageId":"66e57bf359dad6c83e103f2b6ffa51d82f604d8d.1689205042.git.me@ttaylorr.com","threadId":"59978","inReplyTo":"cover.1689205042.git.me@ttaylorr.com","subject":"[PATCH 11/20] commit-graph.c: prevent overflow in `write_commit_graph_file()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-07-12T23:37:54Z","receivedAt":"2023-07-12T23:38:11Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When writing a commit-graph, we use the chunk-format API to write out\neach individual chunk of the commit-graph. Each chunk of the\ncommit-graph is tracked via a call to `add_chunk()`, along with the\nexpected size of that chunk.\n\nSimilar to an earlier commit which handled the identical issue in the\nMIDX machinery, guard against overflow when dealing with a commit-graph\nwith a large number of entries to avoid corrupting the contents of the\ncommit-graph itself.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n commit-graph.c | 20 ++++++++++----------\n 1 file changed, 10 insertions(+), 10 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex f70afccada..538f96b27a 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -1952,35 +1952,35 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)\n \n \tadd_chunk(cf, GRAPH_CHUNKID_OIDFANOUT, GRAPH_FANOUT_SIZE,\n \t\t  write_graph_chunk_fanout);\n-\tadd_chunk(cf, GRAPH_CHUNKID_OIDLOOKUP, hashsz * ctx->commits.nr,\n+\tadd_chunk(cf, GRAPH_CHUNKID_OIDLOOKUP, st_mult(hashsz, ctx->commits.nr),\n \t\t  write_graph_chunk_oids);\n-\tadd_chunk(cf, GRAPH_CHUNKID_DATA, (hashsz + 16) * ctx->commits.nr,\n+\tadd_chunk(cf, GRAPH_CHUNKID_DATA, st_mult(hashsz + 16, ctx->commits.nr),\n \t\t  write_graph_chunk_data);\n \n \tif (ctx->write_generation_data)\n \t\tadd_chunk(cf, GRAPH_CHUNKID_GENERATION_DATA,\n-\t\t\t  sizeof(uint32_t) * ctx->commits.nr,\n+\t\t\t  st_mult(sizeof(uint32_t), ctx->commits.nr),\n \t\t\t  write_graph_chunk_generation_data);\n \tif (ctx->num_generation_data_overflows)\n \t\tadd_chunk(cf, GRAPH_CHUNKID_GENERATION_DATA_OVERFLOW,\n-\t\t\t  sizeof(timestamp_t) * ctx->num_generation_data_overflows,\n+\t\t\t  st_mult(sizeof(timestamp_t), ctx->num_generation_data_overflows),\n \t\t\t  write_graph_chunk_generation_data_overflow);\n \tif (ctx->num_extra_edges)\n \t\tadd_chunk(cf, GRAPH_CHUNKID_EXTRAEDGES,\n-\t\t\t  4 * ctx->num_extra_edges,\n+\t\t\t  st_mult(4, ctx->num_extra_edges),\n \t\t\t  write_graph_chunk_extra_edges);\n \tif (ctx->changed_paths) {\n \t\tadd_chunk(cf, GRAPH_CHUNKID_BLOOMINDEXES,\n-\t\t\t  sizeof(uint32_t) * ctx->commits.nr,\n+\t\t\t  st_mult(sizeof(uint32_t), ctx->commits.nr),\n \t\t\t  write_graph_chunk_bloom_indexes);\n \t\tadd_chunk(cf, GRAPH_CHUNKID_BLOOMDATA,\n-\t\t\t  sizeof(uint32_t) * 3\n-\t\t\t\t+ ctx->total_bloom_filter_data_size,\n+\t\t\t  st_add(sizeof(uint32_t) * 3,\n+\t\t\t\t ctx->total_bloom_filter_data_size),\n \t\t\t  write_graph_chunk_bloom_data);\n \t}\n \tif (ctx->num_commit_graphs_after > 1)\n \t\tadd_chunk(cf, GRAPH_CHUNKID_BASE,\n-\t\t\t  hashsz * (ctx->num_commit_graphs_after - 1),\n+\t\t\t  st_mult(hashsz, ctx->num_commit_graphs_after - 1),\n \t\t\t  write_graph_chunk_base);\n \n \thashwrite_be32(f, GRAPH_SIGNATURE);\n@@ -1998,7 +1998,7 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)\n \t\t\t    get_num_chunks(cf));\n \t\tctx->progress = start_delayed_progress(\n \t\t\tprogress_title.buf,\n-\t\t\tget_num_chunks(cf) * ctx->commits.nr);\n+\t\t\tst_mult(get_num_chunks(cf), ctx->commits.nr));\n \t}\n \n \twrite_chunkfile(cf, ctx);\n-- \n2.41.0.347.g7b976b8871f\n\n"},{"id":"479437","messageId":"34edcef888f0cdb25cb88d1d736e99e338317573.1689205042.git.me@ttaylorr.com","threadId":"59978","inReplyTo":"cover.1689205042.git.me@ttaylorr.com","subject":"[PATCH 12/20] commit-graph.c: prevent overflow in add_graph_to_chain()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-07-12T23:37:57Z","receivedAt":"2023-07-12T23:38:12Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"The commit-graph uses a fanout table with 4-byte entries to store the\nnumber of commits at each shard of the commit-graph. So it is OK to have\na commit graph with as many as 2^32-1 stored commits. But we risk\noverflowing any computation which may exceed the 32-bit (unsigned)\nmaximum when those computations are (incorrectly) performed using 32-bit\noperands.\n\nThere are a couple of spots in `add_graph_to_chain()` where we could\npotentially overflow the result:\n\n  - First, when comparing the list of existing entries in the\n    commit-graph chain. It is unlikely that this should ever overflow,\n    since it would require having roughly 2^32-1/g->hash_len\n    commit-graphs in the chain. But let's guard that computation with a\n    `st_mult()` just to be safe.\n\n  - Second, when computing the number of commits in the graph added to\n    the front of the chain. This value is also a 32-bit unsigned, but we\n    should make sure that it does not grow beyond the maximum value.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n commit-graph.c | 11 +++++++++--\n 1 file changed, 9 insertions(+), 2 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 538f96b27a..99af73e40a 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -481,7 +481,7 @@ static int add_graph_to_chain(struct commit_graph *g,\n \n \t\tif (!cur_g ||\n \t\t    !oideq(&oids[n], &cur_g->oid) ||\n-\t\t    !hasheq(oids[n].hash, g->chunk_base_graphs + g->hash_len * n)) {\n+\t\t    !hasheq(oids[n].hash, g->chunk_base_graphs + st_mult(g->hash_len, n))) {\n \t\t\twarning(_(\"commit-graph chain does not match\"));\n \t\t\treturn 0;\n \t\t}\n@@ -491,8 +491,15 @@ static int add_graph_to_chain(struct commit_graph *g,\n \n \tg->base_graph = chain;\n \n-\tif (chain)\n+\tif (chain) {\n+\t\tif (unsigned_add_overflows(chain->num_commits,\n+\t\t\t\t\t   chain->num_commits_in_base)) {\n+\t\t\twarning(_(\"commit count in base graph too high: %\"PRIuMAX),\n+\t\t\t\t(uintmax_t)chain->num_commits_in_base);\n+\t\t\treturn 0;\n+\t\t}\n \t\tg->num_commits_in_base = chain->num_commits + chain->num_commits_in_base;\n+\t}\n \n \treturn 1;\n }\n-- \n2.41.0.347.g7b976b8871f\n\n"},{"id":"479439","messageId":"0b7aabc23b10c1c3e260848dd8ac500ee7e62d8c.1689205042.git.me@ttaylorr.com","threadId":"59978","inReplyTo":"cover.1689205042.git.me@ttaylorr.com","subject":"[PATCH 13/20] commit-graph.c: prevent overflow in `load_oid_from_graph()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-07-12T23:38:00Z","receivedAt":"2023-07-12T23:38:14Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In a similar spirit as previous commits, ensure that we don't overflow\nwhen trying to compute an offset into the `chunk_oid_lookup` table when\nthe `lex_index` of the item we're trying to look up exceeds\n`2^32-1/g->hash_len`.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n commit-graph.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 99af73e40a..1b70bdb07e 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -753,7 +753,7 @@ static void load_oid_from_graph(struct commit_graph *g,\n \n \tlex_index = pos - g->num_commits_in_base;\n \n-\toidread(oid, g->chunk_oid_lookup + g->hash_len * lex_index);\n+\toidread(oid, g->chunk_oid_lookup + st_mult(g->hash_len, lex_index));\n }\n \n static struct commit_list **insert_parent_or_die(struct repository *r,\n-- \n2.41.0.347.g7b976b8871f\n\n"},{"id":"479440","messageId":"62a3e40b9714bb8739ab32eceda8816b073ba5de.1689205042.git.me@ttaylorr.com","threadId":"59978","inReplyTo":"cover.1689205042.git.me@ttaylorr.com","subject":"[PATCH 14/20] commit-graph.c: prevent overflow in `fill_commit_graph_info()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-07-12T23:38:03Z","receivedAt":"2023-07-12T23:38:35Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In a similar spirit as previous commits, ensure that we don't overflow\nin a few spots within `fill_commit_graph_info()`:\n\n  - First, when computing an offset into the commit data chunk, which\n    can occur when the `lex_index` of the item we're looking up exceeds\n    2^32-1/GRAPH_DATA_WIDTH.\n\n  - A similar issue when computing the generation date offset for\n    commits with `lex_index` greater than 2^32-1/4. Note that in\n    practice this will never overflow, since the left-hand operand is\n    from calling `sizeof(...)` and is thus already a `size_t`. But wrap\n    that in an `st_mult()` to make it clear that we intend to perform\n    this computation using 64-bit operands.\n\n  - Finally, a nearly identical issue as above when computing an offset\n    into the `generation_data_overflow` chunk.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n commit-graph.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 1b70bdb07e..ceaeb8b785 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -789,7 +789,7 @@ static void fill_commit_graph_info(struct commit *item, struct commit_graph *g,\n \t\tdie(_(\"invalid commit position. commit-graph is likely corrupt\"));\n \n \tlex_index = pos - g->num_commits_in_base;\n-\tcommit_data = g->chunk_commit_data + GRAPH_DATA_WIDTH * lex_index;\n+\tcommit_data = g->chunk_commit_data + st_mult(GRAPH_DATA_WIDTH, lex_index);\n \n \tgraph_data = commit_graph_data_at(item);\n \tgraph_data->graph_pos = pos;\n@@ -799,14 +799,14 @@ static void fill_commit_graph_info(struct commit *item, struct commit_graph *g,\n \titem->date = (timestamp_t)((date_high << 32) | date_low);\n \n \tif (g->read_generation_data) {\n-\t\toffset = (timestamp_t)get_be32(g->chunk_generation_data + sizeof(uint32_t) * lex_index);\n+\t\toffset = (timestamp_t)get_be32(g->chunk_generation_data + st_mult(sizeof(uint32_t), lex_index));\n \n \t\tif (offset & CORRECTED_COMMIT_DATE_OFFSET_OVERFLOW) {\n \t\t\tif (!g->chunk_generation_data_overflow)\n \t\t\t\tdie(_(\"commit-graph requires overflow generation data but has none\"));\n \n \t\t\toffset_pos = offset ^ CORRECTED_COMMIT_DATE_OFFSET_OVERFLOW;\n-\t\t\tgraph_data->generation = item->date + get_be64(g->chunk_generation_data_overflow + 8 * offset_pos);\n+\t\t\tgraph_data->generation = item->date + get_be64(g->chunk_generation_data_overflow + st_mult(8, offset_pos));\n \t\t} else\n \t\t\tgraph_data->generation = item->date + offset;\n \t} else\n-- \n2.41.0.347.g7b976b8871f\n\n"},{"id":"479441","messageId":"d49bedb1be00b76629dbfb6494d4f8cd45af1d02.1689205042.git.me@ttaylorr.com","threadId":"59978","inReplyTo":"cover.1689205042.git.me@ttaylorr.com","subject":"[PATCH 15/20] commit-graph.c: prevent overflow in `fill_commit_in_graph()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-07-12T23:38:05Z","receivedAt":"2023-07-12T23:38:38Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In a similar spirit as previous commits, ensure that we don't overflow\nwhen the lex_index of the commit we are trying to fill out exceeds\n2^32-1/(g->hash_len+16).\n\nThe other hunk touched in this patch is not susceptible to overflow,\nsince an explicit cast is made to a 64-bit unsigned value. For clarity\nand consistency with the rest of the commits in this series, avoid a\ntricky to reason about cast, and use `st_mult()` directly.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n commit-graph.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex ceaeb8b785..ca1d997516 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -837,7 +837,7 @@ static int fill_commit_in_graph(struct repository *r,\n \tfill_commit_graph_info(item, g, pos);\n \n \tlex_index = pos - g->num_commits_in_base;\n-\tcommit_data = g->chunk_commit_data + (g->hash_len + 16) * lex_index;\n+\tcommit_data = g->chunk_commit_data + st_mult(g->hash_len + 16, lex_index);\n \n \titem->object.parsed = 1;\n \n@@ -859,7 +859,7 @@ static int fill_commit_in_graph(struct repository *r,\n \t}\n \n \tparent_data_ptr = (uint32_t*)(g->chunk_extra_edges +\n-\t\t\t  4 * (uint64_t)(edge_value & GRAPH_EDGE_LAST_MASK));\n+\t\t\t  st_mult(4, edge_value & GRAPH_EDGE_LAST_MASK));\n \tdo {\n \t\tedge_value = get_be32(parent_data_ptr);\n \t\tpptr = insert_parent_or_die(r, g,\n-- \n2.41.0.347.g7b976b8871f\n\n"},{"id":"479443","messageId":"32c046e684acd27df254b8e500e0b64ec76b197d.1689205042.git.me@ttaylorr.com","threadId":"59978","inReplyTo":"cover.1689205042.git.me@ttaylorr.com","subject":"[PATCH 16/20] commit-graph.c: prevent overflow in `load_tree_for_commit()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-07-12T23:38:08Z","receivedAt":"2023-07-12T23:38:47Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In a similar spirit as previous commits, ensure that we don't overflow\nwhen computing an offset into the commit_data chunk when the (relative)\ngraph position exceeds 2^32-1/GRAPH_DATA_WIDTH.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n commit-graph.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex ca1d997516..35f700273b 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -979,7 +979,7 @@ static struct tree *load_tree_for_commit(struct repository *r,\n \t\tg = g->base_graph;\n \n \tcommit_data = g->chunk_commit_data +\n-\t\t\tGRAPH_DATA_WIDTH * (graph_pos - g->num_commits_in_base);\n+\t\t\tst_mult(GRAPH_DATA_WIDTH, graph_pos - g->num_commits_in_base);\n \n \toidread(&oid, commit_data);\n \tset_commit_tree(c, lookup_tree(r, &oid));\n-- \n2.41.0.347.g7b976b8871f\n\n"},{"id":"479442","messageId":"8d43e3bab557a85f5c22abb750ebbe9885444690.1689205042.git.me@ttaylorr.com","threadId":"59978","inReplyTo":"cover.1689205042.git.me@ttaylorr.com","subject":"[PATCH 17/20] commit-graph.c: prevent overflow in `split_graph_merge_strategy()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-07-12T23:38:11Z","receivedAt":"2023-07-12T23:38:49Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In a similar spirit as previous commits, ensure that we don't overflow\nwhen choosing how to split and merge different layers of the\ncommit-graph.\n\nIn particular, avoid a potential overflow between `size_mult` and\n`num_commits`, as well as a potential overflow between the number of\ncommits currently in the merged graph, and the number of commits in the\ngraph about to be merged.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n commit-graph.c | 7 ++++++-\n 1 file changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 35f700273b..8010e0763e 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -2111,11 +2111,16 @@ static void split_graph_merge_strategy(struct write_commit_graph_context *ctx)\n \n \tif (flags != COMMIT_GRAPH_SPLIT_MERGE_PROHIBITED &&\n \t    flags != COMMIT_GRAPH_SPLIT_REPLACE) {\n-\t\twhile (g && (g->num_commits <= size_mult * num_commits ||\n+\t\twhile (g && (g->num_commits <= st_mult(size_mult, num_commits) ||\n \t\t\t    (max_commits && num_commits > max_commits))) {\n \t\t\tif (g->odb != ctx->odb)\n \t\t\t\tbreak;\n \n+\t\t\tif (unsigned_add_overflows(num_commits, g->num_commits))\n+\t\t\t\tdie(_(\"cannot merge graphs with %\"PRIuMAX\", \"\n+\t\t\t\t      \"%\"PRIuMAX\" commits\"),\n+\t\t\t\t    (uintmax_t)num_commits,\n+\t\t\t\t    (uintmax_t)g->num_commits);\n \t\t\tnum_commits += g->num_commits;\n \t\t\tg = g->base_graph;\n \n-- \n2.41.0.347.g7b976b8871f\n\n"},{"id":"479444","messageId":"094aca51c2c23fe74016c60d5ba31c325da038d2.1689205042.git.me@ttaylorr.com","threadId":"59978","inReplyTo":"cover.1689205042.git.me@ttaylorr.com","subject":"[PATCH 18/20] commit-graph.c: prevent overflow in `merge_commit_graph()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-07-12T23:38:13Z","receivedAt":"2023-07-12T23:38:59Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When merging two commit graphs, ensure that we don't attempt to merge\ntwo graphs which, when combined, have more total commits than the 32-bit\nunsigned maximum.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n commit-graph.c | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 8010e0763e..c679d1d633 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -2178,6 +2178,11 @@ static void merge_commit_graph(struct write_commit_graph_context *ctx,\n \tuint32_t i;\n \tuint32_t offset = g->num_commits_in_base;\n \n+\tif (unsigned_add_overflows(ctx->commits.nr, g->num_commits))\n+\t\tdie(_(\"cannot merge graph %s, too many commits: %\"PRIuMAX),\n+\t\t    oid_to_hex(&g->oid),\n+\t\t    (uintmax_t)st_add(ctx->commits.nr, g->num_commits));\n+\n \tALLOC_GROW(ctx->commits.list, ctx->commits.nr + g->num_commits, ctx->commits.alloc);\n \n \tfor (i = 0; i < g->num_commits; i++) {\n-- \n2.41.0.347.g7b976b8871f\n\n"},{"id":"479445","messageId":"9f66e1e6eeb7386010d7387c37766260bf38b338.1689205042.git.me@ttaylorr.com","threadId":"59978","inReplyTo":"cover.1689205042.git.me@ttaylorr.com","subject":"[PATCH 19/20] commit-graph.c: prevent overflow in `write_commit_graph()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-07-12T23:38:16Z","receivedAt":"2023-07-12T23:39:03Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In a similar spirit as previous commits, ensure that we don't overflow\nwhen trying to read an existing OID while writing a new commit-graph.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n commit-graph.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex c679d1d633..20d9296c8b 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -2453,7 +2453,7 @@ int write_commit_graph(struct object_directory *odb,\n \t\tstruct commit_graph *g = ctx->r->objects->commit_graph;\n \t\tfor (i = 0; i < g->num_commits; i++) {\n \t\t\tstruct object_id oid;\n-\t\t\toidread(&oid, g->chunk_oid_lookup + g->hash_len * i);\n+\t\t\toidread(&oid, g->chunk_oid_lookup + st_mult(g->hash_len, i));\n \t\t\toid_array_append(&ctx->oids, &oid);\n \t\t}\n \t}\n-- \n2.41.0.347.g7b976b8871f\n\n"},{"id":"479446","messageId":"55fff3eb84d8a70aa370aedaf1057ad292435a67.1689205042.git.me@ttaylorr.com","threadId":"59978","inReplyTo":"cover.1689205042.git.me@ttaylorr.com","subject":"[PATCH 20/20] commit-graph.c: prevent overflow in `verify_commit_graph()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-07-12T23:38:19Z","receivedAt":"2023-07-12T23:39:06Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In a similar spirit as previous commits, ensure that we don't overflow\nwhen trying to read an OID out of an existing commit-graph during\nverification.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n commit-graph.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 20d9296c8b..f7a3f97401 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -2584,7 +2584,7 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g, int flags)\n \tfor (i = 0; i < g->num_commits; i++) {\n \t\tstruct commit *graph_commit;\n \n-\t\toidread(&cur_oid, g->chunk_oid_lookup + g->hash_len * i);\n+\t\toidread(&cur_oid, g->chunk_oid_lookup + st_mult(g->hash_len, i));\n \n \t\tif (i && oidcmp(&prev_oid, &cur_oid) >= 0)\n \t\t\tgraph_report(_(\"commit-graph has incorrect OID order: %s then %s\"),\n@@ -2632,7 +2632,7 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g, int flags)\n \t\ttimestamp_t generation;\n \n \t\tdisplay_progress(progress, i + 1);\n-\t\toidread(&cur_oid, g->chunk_oid_lookup + g->hash_len * i);\n+\t\toidread(&cur_oid, g->chunk_oid_lookup + st_mult(g->hash_len, i));\n \n \t\tgraph_commit = lookup_commit(r, &cur_oid);\n \t\todb_commit = (struct commit *)create_object(r, &cur_oid, alloc_commit_node(r));\n-- \n2.41.0.347.g7b976b8871f\n"},{"id":"479447","messageId":"4067ff3f1b422734b79591268644a38f053d2f54.1689205042.git.me@ttaylorr.com","threadId":"59978","inReplyTo":"cover.1689205042.git.me@ttaylorr.com","subject":"[PATCH 07/20] midx.c: store `nr`, `alloc` variables as `size_t`'s","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-07-12T23:37:44Z","receivedAt":"2023-07-12T23:43:10Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In the `write_midx_context` structure, we use two `uint32_t`'s to track\nthe length and allocated size of the packs, and one `uint32_t` to track\nthe number of objects in the MIDX.\n\nIn practice, having these be 32-bit unsigned values shouldn't cause any\nproblems since we are unlikely to have that many objects or packs in any\nreal-world repository. But these values should be `size_t`'s, so change\ntheir type to reflect that.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n midx.c | 16 +++++++++-------\n 1 file changed, 9 insertions(+), 7 deletions(-)\n\ndiff --git a/midx.c b/midx.c\nindex a5a4ff4398..b176745df1 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -446,14 +446,14 @@ static int idx_or_pack_name_cmp(const void *_va, const void *_vb)\n \n struct write_midx_context {\n \tstruct pack_info *info;\n-\tuint32_t nr;\n-\tuint32_t alloc;\n+\tsize_t nr;\n+\tsize_t alloc;\n \tstruct multi_pack_index *m;\n \tstruct progress *progress;\n \tunsigned pack_paths_checked;\n \n \tstruct pack_midx_entry *entries;\n-\tuint32_t entries_nr;\n+\tsize_t entries_nr;\n \n \tuint32_t *pack_perm;\n \tuint32_t *pack_order;\n@@ -671,17 +671,18 @@ static void midx_fanout_add_pack_fanout(struct midx_fanout *fanout,\n static struct pack_midx_entry *get_sorted_entries(struct multi_pack_index *m,\n \t\t\t\t\t\t  struct pack_info *info,\n \t\t\t\t\t\t  uint32_t nr_packs,\n-\t\t\t\t\t\t  uint32_t *nr_objects,\n+\t\t\t\t\t\t  size_t *nr_objects,\n \t\t\t\t\t\t  int preferred_pack)\n {\n \tuint32_t cur_fanout, cur_pack, cur_object;\n-\tuint32_t alloc_objects, total_objects = 0;\n+\tsize_t alloc_objects, total_objects = 0;\n \tstruct midx_fanout fanout = { 0 };\n \tstruct pack_midx_entry *deduplicated_entries = NULL;\n \tuint32_t start_pack = m ? m->num_packs : 0;\n \n \tfor (cur_pack = start_pack; cur_pack < nr_packs; cur_pack++)\n-\t\ttotal_objects += info[cur_pack].p->num_objects;\n+\t\ttotal_objects = st_add(total_objects,\n+\t\t\t\t       info[cur_pack].p->num_objects);\n \n \t/*\n \t * As we de-duplicate by fanout value, we expect the fanout\n@@ -724,7 +725,8 @@ static struct pack_midx_entry *get_sorted_entries(struct multi_pack_index *m,\n \t\t\t\t\t\t&fanout.entries[cur_object].oid))\n \t\t\t\tcontinue;\n \n-\t\t\tALLOC_GROW(deduplicated_entries, *nr_objects + 1, alloc_objects);\n+\t\t\tALLOC_GROW(deduplicated_entries, st_add(*nr_objects, 1),\n+\t\t\t\t   alloc_objects);\n \t\t\tmemcpy(&deduplicated_entries[*nr_objects],\n \t\t\t       &fanout.entries[cur_object],\n \t\t\t       sizeof(struct pack_midx_entry));\n-- \n2.41.0.347.g7b976b8871f\n\n"},{"id":"479455","messageId":"5d2cf09f-34c7-9a88-bab2-8bf348dd13bb@gmail.com","threadId":"59978","inReplyTo":"d6902cd9e7f7f2a6b8044c8fb782a28c23e15600.1689205042.git.me@ttaylorr.com","subject":"Re: [PATCH 02/20] packfile.c: prevent overflow in `load_idx()`","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-07-13T08:21:55Z","receivedAt":"2023-07-13T08:31:16Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Taylor\n\nOn 13/07/2023 00:37, Taylor Blau wrote:\n> Prevent an overflow when locating a pack's CRC offset when the number\n> of packed items is greater than 2^32-1/hashsz by guarding the\n> computation with an `st_mult()`.\n> \n> Signed-off-by: Taylor Blau <me@ttaylorr.com>\n> ---\n>   packfile.c | 2 +-\n>   1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/packfile.c b/packfile.c\n> index 89220f0e03..70acf1694b 100644\n> --- a/packfile.c\n> +++ b/packfile.c\n> @@ -186,7 +186,7 @@ int load_idx(const char *path, const unsigned int hashsz, void *idx_map,\n>   \t\t     */\n>   \t\t    (sizeof(off_t) <= 4))\n>   \t\t\treturn error(\"pack too large for current definition of off_t in %s\", path);\n> -\t\tp->crc_offset = 8 + 4 * 256 + nr * hashsz;\n> +\t\tp->crc_offset = st_add(8 + 4 * 256, st_mult(nr, hashsz));\n\np->crc_offset is a uint32_t so we're still prone to truncation here \nunless we change the crc_offset member of struct packed_git to be a \nsize_t. I haven't checked if the other users of crc_offset would need \nadjusting if we change its type.\n\nBest Wishes\n\nPhillip\n\n>   \t}\n>   \n>   \tp->index_version = version;\n"},{"id":"479459","messageId":"ZLAJNbIBFUPHYhlt@nand.local","threadId":"59978","inReplyTo":"5d2cf09f-34c7-9a88-bab2-8bf348dd13bb@gmail.com","subject":"Re: [PATCH 02/20] packfile.c: prevent overflow in `load_idx()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-07-13T14:24:53Z","receivedAt":"2023-07-13T14:24:58Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Jul 13, 2023 at 09:21:55AM +0100, Phillip Wood wrote:\n> > diff --git a/packfile.c b/packfile.c\n> > index 89220f0e03..70acf1694b 100644\n> > --- a/packfile.c\n> > +++ b/packfile.c\n> > @@ -186,7 +186,7 @@ int load_idx(const char *path, const unsigned int hashsz, void *idx_map,\n> >   \t\t     */\n> >   \t\t    (sizeof(off_t) <= 4))\n> >   \t\t\treturn error(\"pack too large for current definition of off_t in %s\", path);\n> > -\t\tp->crc_offset = 8 + 4 * 256 + nr * hashsz;\n> > +\t\tp->crc_offset = st_add(8 + 4 * 256, st_mult(nr, hashsz));\n>\n> p->crc_offset is a uint32_t so we're still prone to truncation here unless\n> we change the crc_offset member of struct packed_git to be a size_t. I\n> haven't checked if the other users of crc_offset would need adjusting if we\n> change its type.\n\nThanks for spotting. Luckily, this should be a straightforward change:\n\n    $ git grep crc_offset\n    builtin/index-pack.c:\tidx1 = (((const uint32_t *)((const uint8_t *)p->index_data + p->crc_offset))\n    object-store-ll.h:\tuint32_t crc_offset;\n    packfile.c:\t\tp->crc_offset = st_add(8 + 4 * 256, st_mult(nr, hashsz));\n\nThe single usage in index-pack is OK, so we only need to change its type\nto a size_t.\n\nI could see an argument that this should be an off_t, since it is an\noffset into a file. But since we memory map the whole thing anyway, I\nthink we are equally OK to treat it as a pointer offset. A similar\nargument is made in f86f769550e (compute pack .idx byte offsets using\nsize_t, 2020-11-13), so I am content to leave this as a size_t.\n\nThanks,\nTaylor\n"},{"id":"479460","messageId":"xmqqttu7rdpu.fsf@gitster.g","threadId":"59978","inReplyTo":"5d2cf09f-34c7-9a88-bab2-8bf348dd13bb@gmail.com","subject":"Re: [PATCH 02/20] packfile.c: prevent overflow in `load_idx()`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-07-13T16:14:05Z","receivedAt":"2023-07-13T16:14:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood <phillip.wood123@gmail.com> writes:\n\n>> -\t\tp->crc_offset = 8 + 4 * 256 + nr * hashsz;\n>> +\t\tp->crc_offset = st_add(8 + 4 * 256, st_mult(nr, hashsz));\n>\n> p->crc_offset is a uint32_t so we're still prone to truncation here\n\nGood eyes.  Thanks.\n"},{"id":"479501","messageId":"ZLCc3mRbdXPllpAp@nand.local","threadId":"59978","inReplyTo":"ZLAJNbIBFUPHYhlt@nand.local","subject":"Re: [PATCH 02/20] packfile.c: prevent overflow in `load_idx()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-07-14T00:54:54Z","receivedAt":"2023-07-14T00:55:00Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Jul 13, 2023 at 10:24:53AM -0400, Taylor Blau wrote:\n> On Thu, Jul 13, 2023 at 09:21:55AM +0100, Phillip Wood wrote:\n> > p->crc_offset is a uint32_t so we're still prone to truncation here unless\n> > we change the crc_offset member of struct packed_git to be a size_t. I\n> > haven't checked if the other users of crc_offset would need adjusting if we\n> > change its type.\n>\n> Thanks for spotting. Luckily, this should be a straightforward change:\n\nHere's a replacement patch which changes the type of `crc_offset`. If\nthere end up being other review comments, I'll fold this into the next\nround.\n\n--- 8< ---\nSubject: [PATCH] packfile.c: prevent overflow in `load_idx()`\n\nPrevent an overflow when locating a pack's CRC offset when the number\nof packed items is greater than 2^32-1/hashsz by guarding the\ncomputation with an `st_mult()`.\n\nNote that to avoid truncating the result, the `crc_offset` member must\nitself become a `size_t`. The only usage of this variable (besides the\nassignment in `load_idx()`) is in `read_v2_anomalous_offsets()` in the\nindex-pack code. There we use the `crc_offset` as a pointer offset, so\nwe are already equipped to handle the type change.\n\nHelped-by: Phillip Wood <phillip.wood@dunelm.org.uk>\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n object-store-ll.h | 2 +-\n packfile.c        | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/object-store-ll.h b/object-store-ll.h\nindex e8f22cdb1b..26a3895c82 100644\n--- a/object-store-ll.h\n+++ b/object-store-ll.h\n@@ -106,7 +106,7 @@ struct packed_git {\n \tconst void *index_data;\n \tsize_t index_size;\n \tuint32_t num_objects;\n-\tuint32_t crc_offset;\n+\tsize_t crc_offset;\n \tstruct oidset bad_objects;\n \tint index_version;\n \ttime_t mtime;\ndiff --git a/packfile.c b/packfile.c\nindex 89220f0e03..70acf1694b 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -186,7 +186,7 @@ int load_idx(const char *path, const unsigned int hashsz, void *idx_map,\n \t\t     */\n \t\t    (sizeof(off_t) <= 4))\n \t\t\treturn error(\"pack too large for current definition of off_t in %s\", path);\n-\t\tp->crc_offset = 8 + 4 * 256 + nr * hashsz;\n+\t\tp->crc_offset = st_add(8 + 4 * 256, st_mult(nr, hashsz));\n \t}\n\n \tp->index_version = version;\n--\n2.41.0.329.g0a1adfae833\n--- >8 ---\n\nThanks,\nTaylor\n"},{"id":"479511","messageId":"7b5d695e-07da-adba-d9ff-5551102881e1@gmail.com","threadId":"59978","inReplyTo":"ZLAJNbIBFUPHYhlt@nand.local","subject":"Re: [PATCH 02/20] packfile.c: prevent overflow in `load_idx()`","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-07-14T09:55:10Z","receivedAt":"2023-07-14T09:55:17Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 13/07/2023 15:24, Taylor Blau wrote:\n> On Thu, Jul 13, 2023 at 09:21:55AM +0100, Phillip Wood wrote:\n>>> diff --git a/packfile.c b/packfile.c\n>>> index 89220f0e03..70acf1694b 100644\n>>> --- a/packfile.c\n>>> +++ b/packfile.c\n>>> @@ -186,7 +186,7 @@ int load_idx(const char *path, const unsigned int hashsz, void *idx_map,\n>>>    \t\t     */\n>>>    \t\t    (sizeof(off_t) <= 4))\n>>>    \t\t\treturn error(\"pack too large for current definition of off_t in %s\", path);\n>>> -\t\tp->crc_offset = 8 + 4 * 256 + nr * hashsz;\n>>> +\t\tp->crc_offset = st_add(8 + 4 * 256, st_mult(nr, hashsz));\n>>\n>> p->crc_offset is a uint32_t so we're still prone to truncation here unless\n>> we change the crc_offset member of struct packed_git to be a size_t. I\n>> haven't checked if the other users of crc_offset would need adjusting if we\n>> change its type.\n> \n> Thanks for spotting. Luckily, this should be a straightforward change:\n> \n>      $ git grep crc_offset\n>      builtin/index-pack.c:\tidx1 = (((const uint32_t *)((const uint8_t *)p->index_data + p->crc_offset))\n>      object-store-ll.h:\tuint32_t crc_offset;\n>      packfile.c:\t\tp->crc_offset = st_add(8 + 4 * 256, st_mult(nr, hashsz));\n> \n> The single usage in index-pack is OK, so we only need to change its type\n> to a size_t.\n\nThat's good, it is nice it is such a simple change\n\n> I could see an argument that this should be an off_t, since it is an\n> offset into a file. But since we memory map the whole thing anyway, I\n> think we are equally OK to treat it as a pointer offset. A similar\n> argument is made in f86f769550e (compute pack .idx byte offsets using\n> size_t, 2020-11-13), so I am content to leave this as a size_t.\n\n\nIf we're already using size_t where one could argue in favor of using \noff_t I think that makes sense.\n\nBest Wishes\n\nPhillip\n\n"},{"id":"479512","messageId":"460ff25f-bc2a-2687-923b-7053668f1ce3@gmail.com","threadId":"59978","inReplyTo":"ZLCc3mRbdXPllpAp@nand.local","subject":"Re: [PATCH 02/20] packfile.c: prevent overflow in `load_idx()`","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-07-14T09:56:54Z","receivedAt":"2023-07-14T09:57:01Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 14/07/2023 01:54, Taylor Blau wrote:\n> On Thu, Jul 13, 2023 at 10:24:53AM -0400, Taylor Blau wrote:\n>> On Thu, Jul 13, 2023 at 09:21:55AM +0100, Phillip Wood wrote:\n>>> p->crc_offset is a uint32_t so we're still prone to truncation here unless\n>>> we change the crc_offset member of struct packed_git to be a size_t. I\n>>> haven't checked if the other users of crc_offset would need adjusting if we\n>>> change its type.\n>>\n>> Thanks for spotting. Luckily, this should be a straightforward change:\n> \n> Here's a replacement patch which changes the type of `crc_offset`. If\n> there end up being other review comments, I'll fold this into the next\n> round.\n> \n> --- 8< ---\n> Subject: [PATCH] packfile.c: prevent overflow in `load_idx()`\n> \n> Prevent an overflow when locating a pack's CRC offset when the number\n> of packed items is greater than 2^32-1/hashsz by guarding the\n> computation with an `st_mult()`.\n> \n> Note that to avoid truncating the result, the `crc_offset` member must\n> itself become a `size_t`. The only usage of this variable (besides the\n> assignment in `load_idx()`) is in `read_v2_anomalous_offsets()` in the\n> index-pack code. There we use the `crc_offset` as a pointer offset, so\n> we are already equipped to handle the type change.\n\nThanks for adding that explanation, this version looks good to me\n\nBest Wishes\n\nPhillip\n\n> Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n> Signed-off-by: Taylor Blau <me@ttaylorr.com>\n> ---\n>   object-store-ll.h | 2 +-\n>   packfile.c        | 2 +-\n>   2 files changed, 2 insertions(+), 2 deletions(-)\n> \n> diff --git a/object-store-ll.h b/object-store-ll.h\n> index e8f22cdb1b..26a3895c82 100644\n> --- a/object-store-ll.h\n> +++ b/object-store-ll.h\n> @@ -106,7 +106,7 @@ struct packed_git {\n>   \tconst void *index_data;\n>   \tsize_t index_size;\n>   \tuint32_t num_objects;\n> -\tuint32_t crc_offset;\n> +\tsize_t crc_offset;\n>   \tstruct oidset bad_objects;\n>   \tint index_version;\n>   \ttime_t mtime;\n> diff --git a/packfile.c b/packfile.c\n> index 89220f0e03..70acf1694b 100644\n> --- a/packfile.c\n> +++ b/packfile.c\n> @@ -186,7 +186,7 @@ int load_idx(const char *path, const unsigned int hashsz, void *idx_map,\n>   \t\t     */\n>   \t\t    (sizeof(off_t) <= 4))\n>   \t\t\treturn error(\"pack too large for current definition of off_t in %s\", path);\n> -\t\tp->crc_offset = 8 + 4 * 256 + nr * hashsz;\n> +\t\tp->crc_offset = st_add(8 + 4 * 256, st_mult(nr, hashsz));\n>   \t}\n> \n>   \tp->index_version = version;\n> --\n> 2.41.0.329.g0a1adfae833\n> --- >8 ---\n> \n> Thanks,\n> Taylor\n\n"},{"id":"479521","messageId":"xmqqfs5qqww9.fsf@gitster.g","threadId":"59978","inReplyTo":"ZLCc3mRbdXPllpAp@nand.local","subject":"Re: [PATCH 02/20] packfile.c: prevent overflow in `load_idx()`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-07-14T16:29:42Z","receivedAt":"2023-07-14T16:29:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau <me@ttaylorr.com> writes:\n\n> On Thu, Jul 13, 2023 at 10:24:53AM -0400, Taylor Blau wrote:\n>> On Thu, Jul 13, 2023 at 09:21:55AM +0100, Phillip Wood wrote:\n>> > p->crc_offset is a uint32_t so we're still prone to truncation here unless\n>> > we change the crc_offset member of struct packed_git to be a size_t. I\n>> > haven't checked if the other users of crc_offset would need adjusting if we\n>> > change its type.\n>>\n>> Thanks for spotting. Luckily, this should be a straightforward change:\n>\n> Here's a replacement patch which changes the type of `crc_offset`. If\n> there end up being other review comments, I'll fold this into the next\n> round.\n\nThe code change to use st_add() and st_mult() is the same as before,\nand the type of .crc_offset member changes, both of which is not\nunexpected.\n\nIn the meantime I will replace the copy of [2/20] I have with this\none.\n\nThanks, both.\n\n> --- 8< ---\n> Subject: [PATCH] packfile.c: prevent overflow in `load_idx()`\n>\n> Prevent an overflow when locating a pack's CRC offset when the number\n> of packed items is greater than 2^32-1/hashsz by guarding the\n> computation with an `st_mult()`.\n>\n> Note that to avoid truncating the result, the `crc_offset` member must\n> itself become a `size_t`. The only usage of this variable (besides the\n> assignment in `load_idx()`) is in `read_v2_anomalous_offsets()` in the\n> index-pack code. There we use the `crc_offset` as a pointer offset, so\n> we are already equipped to handle the type change.\n>\n> Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n> Signed-off-by: Taylor Blau <me@ttaylorr.com>\n> ---\n>  object-store-ll.h | 2 +-\n>  packfile.c        | 2 +-\n>  2 files changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/object-store-ll.h b/object-store-ll.h\n> index e8f22cdb1b..26a3895c82 100644\n> --- a/object-store-ll.h\n> +++ b/object-store-ll.h\n> @@ -106,7 +106,7 @@ struct packed_git {\n>  \tconst void *index_data;\n>  \tsize_t index_size;\n>  \tuint32_t num_objects;\n> -\tuint32_t crc_offset;\n> +\tsize_t crc_offset;\n>  \tstruct oidset bad_objects;\n>  \tint index_version;\n>  \ttime_t mtime;\n> diff --git a/packfile.c b/packfile.c\n> index 89220f0e03..70acf1694b 100644\n> --- a/packfile.c\n> +++ b/packfile.c\n> @@ -186,7 +186,7 @@ int load_idx(const char *path, const unsigned int hashsz, void *idx_map,\n>  \t\t     */\n>  \t\t    (sizeof(off_t) <= 4))\n>  \t\t\treturn error(\"pack too large for current definition of off_t in %s\", path);\n> -\t\tp->crc_offset = 8 + 4 * 256 + nr * hashsz;\n> +\t\tp->crc_offset = st_add(8 + 4 * 256, st_mult(nr, hashsz));\n>  \t}\n>\n>  \tp->index_version = version;\n> --\n> 2.41.0.329.g0a1adfae833\n> --- >8 ---\n>\n> Thanks,\n> Taylor\n"}]}