{"thread":{"id":"60334","subject":"[PATCH 0/20] bounds-checks for chunk-based files","startedAt":"2023-10-09T20:55:49Z","lastAt":"2023-10-20T10:31:19Z","messageCount":67,"participants":["Jeff King","Taylor Blau","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":20},"messages":[{"id":"482903","messageId":"20231009205544.GA3281950@coredump.intra.peff.net","threadId":"60334","inReplyTo":null,"subject":"[PATCH 0/20] bounds-checks for chunk-based files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-10-09T20:55:44Z","receivedAt":"2023-10-09T20:55:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"As part of my -Wunused-parameter series, I noticed that a few callbacks\nused with the chunk-format API ignored the \"chunk_size\" parameter. I had\ninitially just annotated these with UNUSED under the usual \"well, not\nall callbacks need all parameters\" logic.\n\nBut if you think about it, a chunk callback that does not look at the\nchunk size _must_ be buggy, as there is no way it could ensure that it\ndoes not read past the end of the chunk. In a well-formed file this\nisn't a problem, since most chunks have expected sizes (e.g., a\ncommit-graph has a fixed-size commit-data record for every commit\nmentioned in its index chunk). But it is very easy to get out-of-bounds\nreads for files that are not well-formed.\n\nI think the security implications here are pretty minor. The only files\nwhich use the chunk format are multi-pack-index and commit-graph,\nneither of which we'd expect to receive over the network (so you'd need\nto access an untrusted tarball, etc, to even see a malicious file). And\nwe'd never try to write to this memory (they're read-only mmaps of the\nfiles). So your worst case is probably an unexpected out-of-bounds read\nand segfault, on a file that is hard to put in front of the victim.\n\nBut I think it's still worth fixing. The extra checks aren't very\nexpensive, and let us handle bugs or corruption more gracefully, as\nwell.\n\nIt's a lot of patches because there are a lot of chunk types. ;) But\neach one is hopefully pretty straightforward in isolation. I tried to\ngroup similar chunks together (e.g., commit-graph and midx both have\nOIDF and OIDL chunks), but otherwise just fixed the midx chunks in the\norder they appear in the code, followed by the same for commit-graph.\n\n [01/20]: chunk-format: note that pair_chunk() is unsafe\n [02/20]: t: add library for munging chunk-format files\n [03/20]: midx: stop ignoring malformed oid fanout chunk\n [04/20]: commit-graph: check size of oid fanout chunk\n [05/20]: midx: check size of oid lookup chunk\n [06/20]: commit-graph: check consistency of fanout table\n [07/20]: midx: check size of pack names chunk\n [08/20]: midx: enforce chunk alignment on reading\n [09/20]: midx: check size of object offset chunk\n [10/20]: midx: bounds-check large offset chunk\n [11/20]: midx: check size of revindex chunk\n [12/20]: commit-graph: check size of commit data chunk\n [13/20]: commit-graph: detect out-of-bounds extra-edges pointers\n [14/20]: commit-graph: bounds-check base graphs chunk\n [15/20]: commit-graph: check size of generations chunk\n [16/20]: commit-graph: bounds-check generation overflow chunk\n [17/20]: commit-graph: check bounds when accessing BDAT chunk\n [18/20]: commit-graph: check bounds when accessing BIDX chunk\n [19/20]: commit-graph: detect out-of-order BIDX offsets\n [20/20]: chunk-format: drop pair_chunk_unsafe()\n\n bloom.c | 34 +++++++++\n chunk-format.c | 24 ++++--\n chunk-format.h | 9 ++-\n commit-graph.c | 119 ++++++++++++++++++++++++-----\n commit-graph.h | 4 +\n midx.c | 68 +++++++++++++----\n midx.h | 3 +\n pack-revindex.c | 13 +++-\n t/lib-chunk.sh | 17 +++++\n t/lib-chunk/corrupt-chunk-file.pl | 66 ++++++++++++++++\n t/t4216-log-bloom.sh | 50 ++++++++++++\n t/t5318-commit-graph.sh | 76 +++++++++++++++++-\n t/t5319-multi-pack-index.sh | 102 ++++++++++++++++++++++++-\n t/t5324-split-commit-graph.sh | 20 ++++-\n t/t5328-commit-graph-64bit-time.sh | 10 +++\n 15 files changed, 568 insertions(+), 47 deletions(-)\n create mode 100644 t/lib-chunk.sh\n create mode 100644 t/lib-chunk/corrupt-chunk-file.pl\n\n-Peff\n"},{"id":"482904","messageId":"20231009205823.GA3282181@coredump.intra.peff.net","threadId":"60334","inReplyTo":"20231009205544.GA3281950@coredump.intra.peff.net","subject":"[PATCH 01/20] chunk-format: note that pair_chunk() is unsafe","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-10-09T20:58:23Z","receivedAt":"2023-10-09T20:58:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The pair_chunk() function is provided as an easy helper for parsing\nchunks that just want a pointer to a set of bytes. But every caller has\na hidden bug: because we return only the pointer without the matching\nchunk size, the callers have no clue how many bytes they are allowed to\nlook at. And as a result, they may read off the end of the mmap'd data\nwhen the on-disk file does not match their expectations.\n\nSince chunk files are typically used for local-repository data like\ncommit-graph files and midx's, the security implications here are pretty\nmild. The worst that can happen is that you hand somebody a corrupted\nrepository tarball, and running Git on it does an out-of-bounds read and\ncrashes. So it's worth being more defensive, but we don't need to drop\neverything and fix every caller immediately.\n\nI noticed the problem because the pair_chunk_fn() callback does not look\nat its chunk_size argument, and wanted to annotate it to silence\n-Wunused-parameter. We could do that now, but we'd lose the hint that\nthis code should be audited and fixed.\n\nSo instead, let's set ourselves up for going down that path:\n\n 1. Provide a pair_chunk() function that does return the size, which\n prepares us for fixing these cases.\n\n 2. Rename the existing function to pair_chunk_unsafe(). That gives us\n an easy way to grep for cases which still need to be fixed, and the\n name should cause anybody adding new calls to think twice before\n using it.\n\nThere are no callers of the \"safe\" version yet, but we'll add some in\nsubsequent patches.\n\nSigned-off-by: Jeff King \n---\nI originally wasn't sure if I'd convert every case, or if we'd have to\nleave some longer-term. But as you'll see at the end, we can get rid of\npair_chunk_unsafe() in the final patch (but we need both forms to\nco-exist during the series, since the intermediate state is that we have\nsome safe and some unsafe calls).\n\n chunk-format.c | 24 ++++++++++++++++++++----\n chunk-format.h | 19 +++++++++++++++++--\n commit-graph.c | 14 +++++++-------\n midx.c | 10 +++++-----\n 4 files changed, 49 insertions(+), 18 deletions(-)\n\ndiff --git a/chunk-format.c b/chunk-format.c\nindex 140dfa0dcc..8d910e23f6 100644\n--- a/chunk-format.c\n+++ b/chunk-format.c\n@@ -154,20 +154,36 @@ int read_table_of_contents(struct chunkfile *cf,\n \treturn 0;\n }\n \n+struct pair_chunk_data {\n+\tconst unsigned char **p;\n+\tsize_t *size;\n+};\n+\n static int pair_chunk_fn(const unsigned char *chunk_start,\n \t\t\t size_t chunk_size,\n \t\t\t void *data)\n {\n-\tconst unsigned char **p = data;\n-\t*p = chunk_start;\n+\tstruct pair_chunk_data *pcd = data;\n+\t*pcd->p = chunk_start;\n+\t*pcd->size = chunk_size;\n \treturn 0;\n }\n \n int pair_chunk(struct chunkfile *cf,\n \t uint32_t chunk_id,\n-\t const unsigned char **p)\n+\t const unsigned char **p,\n+\t size_t *size)\n+{\n+\tstruct pair_chunk_data pcd = { .p = p, .size = size };\n+\treturn read_chunk(cf, chunk_id, pair_chunk_fn, &pcd);\n+}\n+\n+int pair_chunk_unsafe(struct chunkfile *cf,\n+\t\t uint32_t chunk_id,\n+\t\t const unsigned char **p)\n {\n-\treturn read_chunk(cf, chunk_id, pair_chunk_fn, p);\n+\tsize_t dummy;\n+\treturn pair_chunk(cf, chunk_id, p, &dummy);\n }\n \n int read_chunk(struct chunkfile *cf,\ndiff --git a/chunk-format.h b/chunk-format.h\nindex c7794e84ad..8dce7967f4 100644\n--- a/chunk-format.h\n+++ b/chunk-format.h\n@@ -43,13 +43,28 @@ int read_table_of_contents(struct chunkfile *cf,\n /*\n * Find 'chunk_id' in the given chunkfile and assign the\n * given pointer to the position in the mmap'd file where\n- * that chunk begins.\n+ * that chunk begins. Likewise the \"size\" parameter is filled\n+ * with the size of the chunk.\n *\n * Returns CHUNK_NOT_FOUND if the chunk does not exist.\n */\n int pair_chunk(struct chunkfile *cf,\n \t uint32_t chunk_id,\n-\t const unsigned char **p);\n+\t const unsigned char **p,\n+\t size_t *size);\n+\n+/*\n+ * Unsafe version of pair_chunk; it does not return the size,\n+ * meaning that the caller cannot possibly be careful about\n+ * reading out of bounds from the mapped memory.\n+ *\n+ * No new callers should use this function, and old callers should\n+ * be audited and migrated over to using the regular pair_chunk()\n+ * function.\n+ */\n+int pair_chunk_unsafe(struct chunkfile *cf,\n+\t\t uint32_t chunk_id,\n+\t\t const unsigned char **p);\n \n typedef int (*chunk_read_fn)(const unsigned char *chunk_start,\n \t\t\t size_t chunk_size, void *data);\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 1a56efcf69..a689a55b79 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -394,25 +394,25 @@ struct commit_graph *parse_commit_graph(struct repo_settings *s,\n \t\t\t\t GRAPH_HEADER_SIZE, graph->num_chunks))\n \t\tgoto free_and_return;\n \n-\tpair_chunk(cf, GRAPH_CHUNKID_OIDFANOUT,\n+\tpair_chunk_unsafe(cf, GRAPH_CHUNKID_OIDFANOUT,\n \t\t (const unsigned char **)&graph->chunk_oid_fanout);\n \tread_chunk(cf, GRAPH_CHUNKID_OIDLOOKUP, graph_read_oid_lookup, graph);\n-\tpair_chunk(cf, GRAPH_CHUNKID_DATA, &graph->chunk_commit_data);\n-\tpair_chunk(cf, GRAPH_CHUNKID_EXTRAEDGES, &graph->chunk_extra_edges);\n-\tpair_chunk(cf, GRAPH_CHUNKID_BASE, &graph->chunk_base_graphs);\n+\tpair_chunk_unsafe(cf, GRAPH_CHUNKID_DATA, &graph->chunk_commit_data);\n+\tpair_chunk_unsafe(cf, GRAPH_CHUNKID_EXTRAEDGES, &graph->chunk_extra_edges);\n+\tpair_chunk_unsafe(cf, GRAPH_CHUNKID_BASE, &graph->chunk_base_graphs);\n \n \tif (s->commit_graph_generation_version >= 2) {\n-\t\tpair_chunk(cf, GRAPH_CHUNKID_GENERATION_DATA,\n+\t\tpair_chunk_unsafe(cf, GRAPH_CHUNKID_GENERATION_DATA,\n \t\t\t&graph->chunk_generation_data);\n-\t\tpair_chunk(cf, GRAPH_CHUNKID_GENERATION_DATA_OVERFLOW,\n+\t\tpair_chunk_unsafe(cf, GRAPH_CHUNKID_GENERATION_DATA_OVERFLOW,\n \t\t\t&graph->chunk_generation_data_overflow);\n \n \t\tif (graph->chunk_generation_data)\n \t\t\tgraph->read_generation_data = 1;\n \t}\n \n \tif (s->commit_graph_read_changed_paths) {\n-\t\tpair_chunk(cf, GRAPH_CHUNKID_BLOOMINDEXES,\n+\t\tpair_chunk_unsafe(cf, GRAPH_CHUNKID_BLOOMINDEXES,\n \t\t\t &graph->chunk_bloom_indexes);\n \t\tread_chunk(cf, GRAPH_CHUNKID_BLOOMDATA,\n \t\t\t graph_read_bloom_data, graph);\ndiff --git a/midx.c b/midx.c\nindex 931f557350..3165218ab5 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -143,19 +143,19 @@ struct multi_pack_index *load_multi_pack_index(const char *object_dir, int local\n \t\t\t\t MIDX_HEADER_SIZE, m->num_chunks))\n \t\tgoto cleanup_fail;\n \n-\tif (pair_chunk(cf, MIDX_CHUNKID_PACKNAMES, &m->chunk_pack_names) == CHUNK_NOT_FOUND)\n+\tif (pair_chunk_unsafe(cf, MIDX_CHUNKID_PACKNAMES, &m->chunk_pack_names) == CHUNK_NOT_FOUND)\n \t\tdie(_(\"multi-pack-index missing required pack-name chunk\"));\n \tif (read_chunk(cf, MIDX_CHUNKID_OIDFANOUT, midx_read_oid_fanout, m) == CHUNK_NOT_FOUND)\n \t\tdie(_(\"multi-pack-index missing required OID fanout chunk\"));\n-\tif (pair_chunk(cf, MIDX_CHUNKID_OIDLOOKUP, &m->chunk_oid_lookup) == CHUNK_NOT_FOUND)\n+\tif (pair_chunk_unsafe(cf, MIDX_CHUNKID_OIDLOOKUP, &m->chunk_oid_lookup) == CHUNK_NOT_FOUND)\n \t\tdie(_(\"multi-pack-index missing required OID lookup chunk\"));\n-\tif (pair_chunk(cf, MIDX_CHUNKID_OBJECTOFFSETS, &m->chunk_object_offsets) == CHUNK_NOT_FOUND)\n+\tif (pair_chunk_unsafe(cf, MIDX_CHUNKID_OBJECTOFFSETS, &m->chunk_object_offsets) == CHUNK_NOT_FOUND)\n \t\tdie(_(\"multi-pack-index missing required object offsets chunk\"));\n \n-\tpair_chunk(cf, MIDX_CHUNKID_LARGEOFFSETS, &m->chunk_large_offsets);\n+\tpair_chunk_unsafe(cf, MIDX_CHUNKID_LARGEOFFSETS, &m->chunk_large_offsets);\n \n \tif (git_env_bool(\"GIT_TEST_MIDX_READ_RIDX\", 1))\n-\t\tpair_chunk(cf, MIDX_CHUNKID_REVINDEX, &m->chunk_revindex);\n+\t\tpair_chunk_unsafe(cf, MIDX_CHUNKID_REVINDEX, &m->chunk_revindex);\n \n \tm->num_objects = ntohl(m->chunk_oid_fanout[255]);\n \n-- \n2.42.0.884.g35e1fe1a6a\n\n"},{"id":"482905","messageId":"20231009205838.GB3282181@coredump.intra.peff.net","threadId":"60334","inReplyTo":"20231009205544.GA3281950@coredump.intra.peff.net","subject":"[PATCH 02/20] t: add library for munging chunk-format files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-10-09T20:58:38Z","receivedAt":"2023-10-09T20:58:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When testing corruption of files using the chunk format (like\ncommit-graphs and midx files), it's helpful to be able to modify bytes\nin specific chunks. This requires being able both to read the\ntable-of-contents (to find the chunk to modify) but also to adjust it\n(to account for size changes in the offsets of subsequent chunks).\n\nWe have some tests already which corrupt chunk files, but they have some\ndownsides:\n\n 1. They are very brittle, as they manually compute the expected size\n of a particular instance of the file (e.g., see the definitions\n starting with NUM_OBJECTS in t5319).\n\n 2. Because they rely on manual offsets and don't read the\n table-of-contents, they're limited to overwriting bytes. But there\n are many interesting corruptions that involve changing the sizes of\n chunks (especially smaller-than-expected ones).\n\nThis patch adds a perl script which makes such corruptions easy. We'll\nuse it in subsequent patches.\n\nNote that we could get by with just a big \"perl -e\" inside the helper\nfunction. I chose to put it in a separate script for two reasons. One,\nso we don't have to worry about the extra layer of shell quoting. And\ntwo, the script is kind of big, and running the tests with \"-x\" would\nrepeatedly dump it into the log output.\n\nSigned-off-by: Jeff King \n---\n t/lib-chunk.sh | 17 ++++++++\n t/lib-chunk/corrupt-chunk-file.pl | 66 +++++++++++++++++++++++++++++++\n 2 files changed, 83 insertions(+)\n create mode 100644 t/lib-chunk.sh\n create mode 100644 t/lib-chunk/corrupt-chunk-file.pl\n\ndiff --git a/t/lib-chunk.sh b/t/lib-chunk.sh\nnew file mode 100644\nindex 0000000000..a7cd9c3c6d\n--- /dev/null\n+++ b/t/lib-chunk.sh\n@@ -0,0 +1,17 @@\n+# Shell library for working with \"chunk\" files (commit-graph, midx, etc).\n+\n+# corrupt_chunk_file \n+#\n+# Corrupt a chunk-based file (like a commit-graph) by overwriting the bytes\n+# found in the chunk specified by the 4-byte identifier. If is\n+# \"clear\", replace the chunk entirely. Otherwise, overwrite data bytes\n+# into the chunk.\n+#\n+# The are interpreted as pairs of hex digits (so \"000000FE\" would be\n+# big-endian 254).\n+corrupt_chunk_file () {\n+\tfn=$1; shift\n+\tperl \"$TEST_DIRECTORY\"/lib-chunk/corrupt-chunk-file.pl \\\n+\t\t\"$@\" <\"$fn\" >\"$fn.tmp\" &&\n+\tmv \"$fn.tmp\" \"$fn\"\n+}\ndiff --git a/t/lib-chunk/corrupt-chunk-file.pl b/t/lib-chunk/corrupt-chunk-file.pl\nnew file mode 100644\nindex 0000000000..cd6d386fef\n--- /dev/null\n+++ b/t/lib-chunk/corrupt-chunk-file.pl\n@@ -0,0 +1,66 @@\n+#!/usr/bin/perl\n+\n+my ($chunk, $seek, $bytes) = @ARGV;\n+$bytes =~ s/../chr(hex($&))/ge;\n+\n+binmode STDIN;\n+binmode STDOUT;\n+\n+# A few helpers to read bytes, or read and copy them to the\n+# output.\n+sub get {\n+\tmy $n = shift;\n+\treturn unless $n;\n+\tread(STDIN, my $buf, $n)\n+\t\tor die \"read error or eof: $!\\n\";\n+\treturn $buf;\n+}\n+sub copy {\n+\tmy $buf = get(@_);\n+\tprint $buf;\n+\treturn $buf;\n+}\n+\n+# read until we find table-of-contents entry for chunk;\n+# note that we cheat a bit by assuming 4-byte alignment and\n+# that no ToC entry will accidentally look like a header.\n+#\n+# If we don't find the entry, copy() will hit EOF and exit\n+# (which should cause the caller to fail the test).\n+while (copy(4) ne $chunk) { }\n+my $offset = unpack(\"Q>\", copy(8));\n+\n+# In clear mode, our length will change. So figure out\n+# the length by comparing to the offset of the next chunk, and\n+# then adjust that offset (and all subsequent) ones.\n+my $len;\n+if ($seek eq \"clear\") {\n+\tmy $id;\n+\tdo {\n+\t\t$id = copy(4);\n+\t\tmy $next = unpack(\"Q>\", get(8));\n+\t\tif (!defined $len) {\n+\t\t\t$len = $next - $offset;\n+\t\t}\n+\t\tprint pack(\"Q>\", $next - $len + length($bytes));\n+\t} while (unpack(\"N\", $id));\n+}\n+\n+# and now copy up to our existing chunk data\n+copy($offset - tell(STDIN));\n+if ($seek eq \"clear\") {\n+\t# if clearing, skip past existing data\n+\tget($len);\n+} else {\n+\t# otherwise, copy up to the requested offset,\n+\t# and skip past the overwritten bytes\n+\tcopy($seek);\n+\tget(length($bytes));\n+}\n+\n+# now write out the requested bytes, along\n+# with any other remaining data\n+print $bytes;\n+while (read(STDIN, my $buf, 4096)) {\n+\tprint $buf;\n+}\n-- \n2.42.0.884.g35e1fe1a6a\n\n"},{"id":"482906","messageId":"20231009205919.GC3282181@coredump.intra.peff.net","threadId":"60334","inReplyTo":"20231009205544.GA3281950@coredump.intra.peff.net","subject":"[PATCH 03/20] midx: stop ignoring malformed oid fanout chunk","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-10-09T20:59:19Z","receivedAt":"2023-10-09T20:59:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we load the oid-fanout chunk, our callback checks that its size is\nreasonable and returns an error if not. However, the caller only checks\nour return value against CHUNK_NOT_FOUND, so we end up ignoring the\nerror completely! Using a too-small fanout table means we end up\naccessing random memory for the fanout and segfault.\n\nWe can fix this by checking for any non-zero return value, rather than\njust CHUNK_NOT_FOUND, and adjusting our error message to cover both\ncases. We could handle each error code individually, but there's not\nmuch point for such a rare case. The extra message produced in the\ncallback makes it clear what is going on.\n\nThe same pattern is used in the adjacent code. Those cases are actually\nOK for now because they do not use a custom callback, so the only error\nthey can get is CHUNK_NOT_FOUND. But let's convert them, as this is an\naccident waiting to happen (especially as we convert some of them away\nfrom pair_chunk). The error messages are more verbose, but it should be\nrare for a user to see these anyway.\n\nSigned-off-by: Jeff King \n---\n midx.c | 16 ++++++++--------\n t/t5319-multi-pack-index.sh | 20 +++++++++++++++++++-\n 2 files changed, 27 insertions(+), 9 deletions(-)\n\ndiff --git a/midx.c b/midx.c\nindex 3165218ab5..21d7dd15ef 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -143,14 +143,14 @@ struct multi_pack_index *load_multi_pack_index(const char *object_dir, int local\n \t\t\t\t MIDX_HEADER_SIZE, m->num_chunks))\n \t\tgoto cleanup_fail;\n \n-\tif (pair_chunk_unsafe(cf, MIDX_CHUNKID_PACKNAMES, &m->chunk_pack_names) == CHUNK_NOT_FOUND)\n-\t\tdie(_(\"multi-pack-index missing required pack-name chunk\"));\n-\tif (read_chunk(cf, MIDX_CHUNKID_OIDFANOUT, midx_read_oid_fanout, m) == CHUNK_NOT_FOUND)\n-\t\tdie(_(\"multi-pack-index missing required OID fanout chunk\"));\n-\tif (pair_chunk_unsafe(cf, MIDX_CHUNKID_OIDLOOKUP, &m->chunk_oid_lookup) == CHUNK_NOT_FOUND)\n-\t\tdie(_(\"multi-pack-index missing required OID lookup chunk\"));\n-\tif (pair_chunk_unsafe(cf, MIDX_CHUNKID_OBJECTOFFSETS, &m->chunk_object_offsets) == CHUNK_NOT_FOUND)\n-\t\tdie(_(\"multi-pack-index missing required object offsets chunk\"));\n+\tif (pair_chunk_unsafe(cf, MIDX_CHUNKID_PACKNAMES, &m->chunk_pack_names))\n+\t\tdie(_(\"multi-pack-index required pack-name chunk missing or corrupted\"));\n+\tif (read_chunk(cf, MIDX_CHUNKID_OIDFANOUT, midx_read_oid_fanout, m))\n+\t\tdie(_(\"multi-pack-index required OID fanout chunk missing or corrupted\"));\n+\tif (pair_chunk_unsafe(cf, MIDX_CHUNKID_OIDLOOKUP, &m->chunk_oid_lookup))\n+\t\tdie(_(\"multi-pack-index required OID lookup chunk missing or corrupted\"));\n+\tif (pair_chunk_unsafe(cf, MIDX_CHUNKID_OBJECTOFFSETS, &m->chunk_object_offsets))\n+\t\tdie(_(\"multi-pack-index required object offsets chunk missing or corrupted\"));\n \n \tpair_chunk_unsafe(cf, MIDX_CHUNKID_LARGEOFFSETS, &m->chunk_large_offsets);\n \ndiff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\nindex 1bcc02004d..b8fe85aeba 100755\n--- a/t/t5319-multi-pack-index.sh\n+++ b/t/t5319-multi-pack-index.sh\n@@ -2,6 +2,7 @@\n \n test_description='multi-pack-indexes'\n . ./test-lib.sh\n+. \"$TEST_DIRECTORY\"/lib-chunk.sh\n \n GIT_TEST_MULTI_PACK_INDEX=0\n objdir=.git/objects\n@@ -438,7 +439,7 @@ test_expect_success 'verify extended chunk count' '\n \n test_expect_success 'verify missing required chunk' '\n \tcorrupt_midx_and_verify $MIDX_BYTE_CHUNK_ID \"\\01\" $objdir \\\n-\t\t\"missing required\"\n+\t\t\"required pack-name chunk missing\"\n '\n \n test_expect_success 'verify invalid chunk offset' '\n@@ -1055,4 +1056,21 @@ test_expect_success 'repack with delta islands' '\n \t)\n '\n \n+corrupt_chunk () {\n+\tmidx=.git/objects/pack/multi-pack-index &&\n+\ttest_when_finished \"rm -rf $midx\" &&\n+\tgit repack -ad --write-midx &&\n+\tcorrupt_chunk_file $midx \"$@\"\n+}\n+\n+test_expect_success 'reader notices too-small oid fanout chunk' '\n+\tcorrupt_chunk OIDF clear 00000000 &&\n+\ttest_must_fail git log 2>err &&\n+\tcat >expect <<-\\EOF &&\n+\terror: multi-pack-index OID fanout is of the wrong size\n+\tfatal: multi-pack-index required OID fanout chunk missing or corrupted\n+\tEOF\n+\ttest_cmp expect err\n+'\n+\n test_done\n-- \n2.42.0.884.g35e1fe1a6a\n\n"},{"id":"482908","messageId":"20231009205951.GD3282181@coredump.intra.peff.net","threadId":"60334","inReplyTo":"20231009205544.GA3281950@coredump.intra.peff.net","subject":"[PATCH 04/20] commit-graph: check size of oid fanout chunk","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-10-09T20:59:51Z","receivedAt":"2023-10-09T20:59:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We load the oid fanout chunk with pair_chunk(), which means we never see\nthe size of the chunk. We just assume the on-disk file uses the\nappropriate size, and if it's too small we'll access random memory.\n\nIt's easy to check this up-front; the fanout always consists of 256\nuint32's, since it is a fanout of the first byte of the hash pointing\ninto the oid index. These parameters can't be changed without\nintroducing a new chunk type.\n\nThis matches the similar check in the midx OIDF chunk (but note that\nrather than checking for the error immediately, the graph code just\nleaves parts of the struct NULL and checks for required fields later).\n\nSigned-off-by: Jeff King \n---\n commit-graph.c | 13 +++++++++++--\n t/t5318-commit-graph.sh | 26 ++++++++++++++++++++++++++\n 2 files changed, 37 insertions(+), 2 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex a689a55b79..9b3b01da61 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -305,6 +305,16 @@ static int verify_commit_graph_lite(struct commit_graph *g)\n \treturn 0;\n }\n \n+static int graph_read_oid_fanout(const unsigned char *chunk_start,\n+\t\t\t\t size_t chunk_size, void *data)\n+{\n+\tstruct commit_graph *g = data;\n+\tif (chunk_size != 256 * sizeof(uint32_t))\n+\t\treturn error(\"commit-graph oid fanout chunk is wrong size\");\n+\tg->chunk_oid_fanout = (const uint32_t *)chunk_start;\n+\treturn 0;\n+}\n+\n static int graph_read_oid_lookup(const unsigned char *chunk_start,\n \t\t\t\t size_t chunk_size, void *data)\n {\n@@ -394,8 +404,7 @@ struct commit_graph *parse_commit_graph(struct repo_settings *s,\n \t\t\t\t GRAPH_HEADER_SIZE, graph->num_chunks))\n \t\tgoto free_and_return;\n \n-\tpair_chunk_unsafe(cf, GRAPH_CHUNKID_OIDFANOUT,\n-\t\t (const unsigned char **)&graph->chunk_oid_fanout);\n+\tread_chunk(cf, GRAPH_CHUNKID_OIDFANOUT, graph_read_oid_fanout, graph);\n \tread_chunk(cf, GRAPH_CHUNKID_OIDLOOKUP, graph_read_oid_lookup, graph);\n \tpair_chunk_unsafe(cf, GRAPH_CHUNKID_DATA, &graph->chunk_commit_data);\n \tpair_chunk_unsafe(cf, GRAPH_CHUNKID_EXTRAEDGES, &graph->chunk_extra_edges);\ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex ba65f17dd9..d25bea3ec5 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -2,6 +2,7 @@\n \n test_description='commit graph'\n . ./test-lib.sh\n+. \"$TEST_DIRECTORY\"/lib-chunk.sh\n \n GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS=0\n \n@@ -821,4 +822,29 @@ test_expect_success 'overflow during generation version upgrade' '\n \t)\n '\n \n+corrupt_chunk () {\n+\tgraph=full/.git/objects/info/commit-graph &&\n+\ttest_when_finished \"rm -rf $graph\" &&\n+\tgit -C full commit-graph write --reachable &&\n+\tcorrupt_chunk_file $graph \"$@\"\n+}\n+\n+check_corrupt_chunk () {\n+\tcorrupt_chunk \"$@\" &&\n+\tgit -C full -c core.commitGraph=false log >expect.out &&\n+\tgit -C full -c core.commitGraph=true log >out 2>err &&\n+\ttest_cmp expect.out out\n+}\n+\n+test_expect_success 'reader notices too-small oid fanout chunk' '\n+\t# make it big enough that the graph file is plausible,\n+\t# otherwise we hit an earlier check\n+\tcheck_corrupt_chunk OIDF clear $(printf \"000000%02x\" $(test_seq 250)) &&\n+\tcat >expect.err <<-\\EOF &&\n+\terror: commit-graph oid fanout chunk is wrong size\n+\terror: commit-graph is missing the OID Fanout chunk\n+\tEOF\n+\ttest_cmp expect.err err\n+'\n+\n test_done\n-- \n2.42.0.884.g35e1fe1a6a\n\n"},{"id":"482909","messageId":"20231009210203.GE3282181@coredump.intra.peff.net","threadId":"60334","inReplyTo":"20231009205544.GA3281950@coredump.intra.peff.net","subject":"[PATCH 05/20] midx: check size of oid lookup chunk","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-10-09T21:02:03Z","receivedAt":"2023-10-09T21:02:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When reading an on-disk multi-pack-index, we take the number of objects\nin the midx from the final value of the fanout table. But we just\nblindly assume that the chunk containing the actual oid entries is the\ncorrect size. This can lead to us reading out-of-bounds memory if the\nlookup chunk is too small (or if the fanout is corrupted; when they\ndon't agree we cannot tell which one is wrong).\n\nNote that we bump the assignment of m->num_objects into the fanout\nparser callback, so that it's set when we parse the lookup table\n(otherwise we'd have to manually record the lookup table size and check\nit later).\n\nSigned-off-by: Jeff King \n---\nAs an aside, the first hunk of the diff annoyingly slides the \"return 0\"\ninto the wrong spot. I thought this was our heuristics gone wrong, but I\nsuspect it's actually the shortest diff because of the one-line change\nin midx_read_oid_fanout(), which would otherwise require extra context\nto split.\n\n midx.c | 18 +++++++++++++++---\n t/t5319-multi-pack-index.sh | 10 ++++++++++\n 2 files changed, 25 insertions(+), 3 deletions(-)\n\ndiff --git a/midx.c b/midx.c\nindex 21d7dd15ef..62e4c03e79 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -71,6 +71,20 @@ static int midx_read_oid_fanout(const unsigned char *chunk_start,\n \t\terror(_(\"multi-pack-index OID fanout is of the wrong size\"));\n \t\treturn 1;\n \t}\n+\tm->num_objects = ntohl(m->chunk_oid_fanout[255]);\n+\treturn 0;\n+}\n+\n+static int midx_read_oid_lookup(const unsigned char *chunk_start,\n+\t\t\t\tsize_t chunk_size, void *data)\n+{\n+\tstruct multi_pack_index *m = data;\n+\tm->chunk_oid_lookup = chunk_start;\n+\n+\tif (chunk_size != st_mult(m->hash_len, m->num_objects)) {\n+\t\terror(_(\"multi-pack-index OID lookup chunk is the wrong size\"));\n+\t\treturn 1;\n+\t}\n \treturn 0;\n }\n \n@@ -147,7 +161,7 @@ struct multi_pack_index *load_multi_pack_index(const char *object_dir, int local\n \t\tdie(_(\"multi-pack-index required pack-name chunk missing or corrupted\"));\n \tif (read_chunk(cf, MIDX_CHUNKID_OIDFANOUT, midx_read_oid_fanout, m))\n \t\tdie(_(\"multi-pack-index required OID fanout chunk missing or corrupted\"));\n-\tif (pair_chunk_unsafe(cf, MIDX_CHUNKID_OIDLOOKUP, &m->chunk_oid_lookup))\n+\tif (read_chunk(cf, MIDX_CHUNKID_OIDLOOKUP, midx_read_oid_lookup, m))\n \t\tdie(_(\"multi-pack-index required OID lookup chunk missing or corrupted\"));\n \tif (pair_chunk_unsafe(cf, MIDX_CHUNKID_OBJECTOFFSETS, &m->chunk_object_offsets))\n \t\tdie(_(\"multi-pack-index required object offsets chunk missing or corrupted\"));\n@@ -157,8 +171,6 @@ struct multi_pack_index *load_multi_pack_index(const char *object_dir, int local\n \tif (git_env_bool(\"GIT_TEST_MIDX_READ_RIDX\", 1))\n \t\tpair_chunk_unsafe(cf, MIDX_CHUNKID_REVINDEX, &m->chunk_revindex);\n \n-\tm->num_objects = ntohl(m->chunk_oid_fanout[255]);\n-\n \tCALLOC_ARRAY(m->pack_names, m->num_packs);\n \tCALLOC_ARRAY(m->packs, m->num_packs);\n \ndiff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\nindex b8fe85aeba..2722e495b2 100755\n--- a/t/t5319-multi-pack-index.sh\n+++ b/t/t5319-multi-pack-index.sh\n@@ -1073,4 +1073,14 @@ test_expect_success 'reader notices too-small oid fanout chunk' '\n \ttest_cmp expect err\n '\n \n+test_expect_success 'reader notices too-small oid lookup chunk' '\n+\tcorrupt_chunk OIDL clear 00000000 &&\n+\ttest_must_fail git log 2>err &&\n+\tcat >expect <<-\\EOF &&\n+\terror: multi-pack-index OID lookup chunk is the wrong size\n+\tfatal: multi-pack-index required OID lookup chunk missing or corrupted\n+\tEOF\n+\ttest_cmp expect err\n+'\n+\n test_done\n-- \n2.42.0.884.g35e1fe1a6a\n\n"},{"id":"482910","messageId":"20231009210458.GF3282181@coredump.intra.peff.net","threadId":"60334","inReplyTo":"20231009205544.GA3281950@coredump.intra.peff.net","subject":"[PATCH 06/20] commit-graph: check consistency of fanout table","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-10-09T21:04:58Z","receivedAt":"2023-10-09T21:05:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We use bsearch_hash() to look up items in the oid index of a\ncommit-graph. It also has a fanout table to reduce the initial range in\nwhich we'll search. But since the fanout comes from the on-disk file, a\ncorrupted or malicious file can cause us to look outside of the\nallocated index memory.\n\nOne solution here would be to pass the total table size to\nbsearch_hash(), which could then bounds check the values it reads from\nthe fanout. But there's an inexpensive up-front check we can do, and\nit's the same one used by the midx and pack idx code (both of which\nlikewise have fanout tables and use bsearch_hash(), but are not affected\nby this bug):\n\n 1. We can check the value of the final fanout entry against the size\n of the table we got from the index chunk. These must always match,\n since the fanout is just slicing up the index.\n\n As a side note, the midx and pack idx code compute it the other\n way around: they use the final fanout value as the object count, and\n check the index size against it. Either is valid; if they\n disagree we cannot know which is wrong (a corrupted fanout value,\n or a too-small table of oids).\n\n 2. We can quickly scan the fanout table to make sure it is\n monotonically increasing. If it is, then we know that every value\n is less than or equal to the final value, and therefore less than\n or equal to the table size.\n\n It would also be sufficient to just check that each fanout value is\n smaller than the final one, but the midx and pack idx code both do\n a full monotonicity check. It's the same cost, and it catches some\n other corruptions (though not all; the checks done by \"commit-graph\n verify\" are more complete but more expensive, and our goal here is\n to be fast and memory-safe).\n\nThere are two new tests. One just checks the final fanout value (this is\nthe mirror image of the \"too small oid lookup\" case added for the midx\nin the previous commit; it's flipped here because commit-graph considers\nthe oid lookup chunk to be the source of truth).\n\nThe other actually creates a fanout with many out-of-bounds entries, and\nprior to this patch, it does cause the segfault you'd expect. But note\nthat the error is not \"your fanout entry is out-of-bounds\", but rather\n\"fanout value out of order\". That's because we leave the final fanout\nvalue in place (to get past the table size check), making the index\nnon-monotonic (the second-to-last entry is big, but the last one must\nremain small to match the actual table).\n\nWe need adjustments to a few existing tests, as well:\n\n - an earlier test in t5318 corrupts the fanout and runs \"commit-graph\n verify\". Its message is now changed, since we catch the problem\n earlier (during the load step, rather than the careful validation\n step).\n\n - in t5324, we test that \"commit-graph verify --shallow\" does not do\n expensive verification on the base file of the chain. But the\n corruption it uses (munging a byte at offset 1000) happens to be in\n the middle of the fanout table. And now we detect that problem in\n the cheaper checks that are performed for every part of the graph.\n We'll push this back to offset 1500, which is only caught by the\n more expensive checksum validation.\n\n Likewise, there's a later test in t5324 which munges an offset 100\n bytes into a file (also in the fanout table) that is referenced by\n an alternates file. So we now find that corruption during the load\n step, rather than the verification step. At the very least we need\n to change the error message (like the case above in t5318). But it\n is probably good to make sure we handle all parts of the\n verification even for alternate graph files. So let's likewise\n corrupt byte 1500 and make sure we found the invalid checksum.\n\nSigned-off-by: Jeff King \n---\nSo I actually implemented the bsearch_hash() bounds checks and wrote\ntests for midx and idx files before realizing how they handle this. ;)\nWhich makes sense, because the usual outcome for a corrupted idx file is\nfor it to say \"non-monotonic index\", which I have seen lead to user\nconfusion. Arguably we should have it say something about \"hey, your idx\nfile seems to be corrupted, because...\". But that can be its own topic.\n\n commit-graph.c | 16 ++++++++++++++++\n t/t5318-commit-graph.sh | 25 ++++++++++++++++++++++++-\n t/t5324-split-commit-graph.sh | 6 +++---\n 3 files changed, 43 insertions(+), 4 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 9b3b01da61..b217e19194 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -277,6 +277,8 @@ struct commit_graph *load_commit_graph_one_fd_st(struct repository *r,\n \n static int verify_commit_graph_lite(struct commit_graph *g)\n {\n+\tint i;\n+\n \t/*\n \t * Basic validation shared between parse_commit_graph()\n \t * which'll be called every time the graph is used, and the\n@@ -302,6 +304,20 @@ static int verify_commit_graph_lite(struct commit_graph *g)\n \t\treturn 1;\n \t}\n \n+\tfor (i = 0; i < 255; i++) {\n+\t\tuint32_t oid_fanout1 = ntohl(g->chunk_oid_fanout[i]);\n+\t\tuint32_t oid_fanout2 = ntohl(g->chunk_oid_fanout[i + 1]);\n+\n+\t\tif (oid_fanout1 > oid_fanout2) {\n+\t\t\terror(\"commit-graph fanout values out of order\");\n+\t\t\treturn 1;\n+\t\t}\n+\t}\n+\tif (ntohl(g->chunk_oid_fanout[255]) != g->num_commits) {\n+\t\terror(\"commit-graph oid table and fanout disagree on size\");\n+\t\treturn 1;\n+\t}\n+\n \treturn 0;\n }\n \ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex d25bea3ec5..d10658de9e 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -560,7 +560,7 @@ test_expect_success 'detect incorrect fanout' '\n \n test_expect_success 'detect incorrect fanout final value' '\n \tcorrupt_graph_and_verify $GRAPH_BYTE_FANOUT2 \"\\01\" \\\n-\t\t\"fanout value\"\n+\t\t\"oid table and fanout disagree on size\"\n '\n \n test_expect_success 'detect incorrect OID order' '\n@@ -847,4 +847,27 @@ test_expect_success 'reader notices too-small oid fanout chunk' '\n \ttest_cmp expect.err err\n '\n \n+test_expect_success 'reader notices fanout/lookup table mismatch' '\n+\tcheck_corrupt_chunk OIDF 1020 \"FFFFFFFF\" &&\n+\tcat >expect.err <<-\\EOF &&\n+\terror: commit-graph oid table and fanout disagree on size\n+\tEOF\n+\ttest_cmp expect.err err\n+'\n+\n+test_expect_success 'reader notices out-of-bounds fanout' '\n+\t# Rather than try to corrupt a specific hash, we will just\n+\t# wreck them all. But we cannot just set them all to 0xFFFFFFFF or\n+\t# similar, as they are used for hi/lo starts in a binary search (so if\n+\t# they are identical, that indicates that the search should abort\n+\t# immediately). Instead, we will give them high values that differ by\n+\t# 2^24, ensuring that any that are used would cause an out-of-bounds\n+\t# read.\n+\tcheck_corrupt_chunk OIDF 0 $(printf \"%02x000000\" $(test_seq 0 254)) &&\n+\tcat >expect.err <<-\\EOF &&\n+\terror: commit-graph fanout values out of order\n+\tEOF\n+\ttest_cmp expect.err err\n+'\n+\n test_done\ndiff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh\nindex 06bb897f02..55b5765e2d 100755\n--- a/t/t5324-split-commit-graph.sh\n+++ b/t/t5324-split-commit-graph.sh\n@@ -317,7 +317,7 @@ test_expect_success 'verify --shallow does not check base contents' '\n \t\tcd verify-shallow &&\n \t\tgit commit-graph verify &&\n \t\tbase_file=$graphdir/graph-$(head -n 1 $graphdir/commit-graph-chain).graph &&\n-\t\tcorrupt_file \"$base_file\" 1000 \"\\01\" &&\n+\t\tcorrupt_file \"$base_file\" 1500 \"\\01\" &&\n \t\tgit commit-graph verify --shallow &&\n \t\ttest_must_fail git commit-graph verify 2>test_err &&\n \t\tgrep -v \"^+\" test_err >err &&\n@@ -391,10 +391,10 @@ test_expect_success 'verify across alternates' '\n \t\ttest_commit extra &&\n \t\tgit commit-graph write --reachable --split &&\n \t\ttip_file=$graphdir/graph-$(tail -n 1 $graphdir/commit-graph-chain).graph &&\n-\t\tcorrupt_file \"$tip_file\" 100 \"\\01\" &&\n+\t\tcorrupt_file \"$tip_file\" 1500 \"\\01\" &&\n \t\ttest_must_fail git commit-graph verify --shallow 2>test_err &&\n \t\tgrep -v \"^+\" test_err >err &&\n-\t\ttest_i18ngrep \"commit-graph has incorrect fanout value\" err\n+\t\ttest_i18ngrep \"incorrect checksum\" err\n \t)\n '\n \n-- \n2.42.0.884.g35e1fe1a6a\n\n"},{"id":"482911","messageId":"20231009210514.GG3282181@coredump.intra.peff.net","threadId":"60334","inReplyTo":"20231009205544.GA3281950@coredump.intra.peff.net","subject":"[PATCH 07/20] midx: check size of pack names chunk","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-10-09T21:05:14Z","receivedAt":"2023-10-09T21:05:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We parse the pack-name chunk as a series of NUL-terminated strings. But\nsince we don't look at the chunk size, there's nothing to guarantee that\nwe don't parse off the end of the chunk (or even off the end of the\nmapped file).\n\nWe can record the length, and then as we parse make sure that we never\nwalk past it.\n\nThe new test exercises the case, though note that it does not actually\nsegfault before this patch. It hits a NUL byte somewhere in one of the\nother chunks, and comes up with a garbage pack name. You could construct\none that reads out-of-bounds (e.g., a PNAM chunk at the end of file),\nbut this case is simple and sufficient to check that we detect the\nproblem.\n\nSigned-off-by: Jeff King \n---\n midx.c | 11 +++++++++--\n midx.h | 1 +\n t/t5319-multi-pack-index.sh | 11 +++++++++++\n 3 files changed, 21 insertions(+), 2 deletions(-)\n\ndiff --git a/midx.c b/midx.c\nindex 62e4c03e79..ec585cae1b 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -157,7 +157,7 @@ struct multi_pack_index *load_multi_pack_index(const char *object_dir, int local\n \t\t\t\t MIDX_HEADER_SIZE, m->num_chunks))\n \t\tgoto cleanup_fail;\n \n-\tif (pair_chunk_unsafe(cf, MIDX_CHUNKID_PACKNAMES, &m->chunk_pack_names))\n+\tif (pair_chunk(cf, MIDX_CHUNKID_PACKNAMES, &m->chunk_pack_names, &m->chunk_pack_names_len))\n \t\tdie(_(\"multi-pack-index required pack-name chunk missing or corrupted\"));\n \tif (read_chunk(cf, MIDX_CHUNKID_OIDFANOUT, midx_read_oid_fanout, m))\n \t\tdie(_(\"multi-pack-index required OID fanout chunk missing or corrupted\"));\n@@ -176,9 +176,16 @@ struct multi_pack_index *load_multi_pack_index(const char *object_dir, int local\n \n \tcur_pack_name = (const char *)m->chunk_pack_names;\n \tfor (i = 0; i < m->num_packs; i++) {\n+\t\tconst char *end;\n+\t\tsize_t avail = m->chunk_pack_names_len -\n+\t\t\t\t(cur_pack_name - (const char *)m->chunk_pack_names);\n+\n \t\tm->pack_names[i] = cur_pack_name;\n \n-\t\tcur_pack_name += strlen(cur_pack_name) + 1;\n+\t\tend = memchr(cur_pack_name, '\\0', avail);\n+\t\tif (!end)\n+\t\t\tdie(_(\"multi-pack-index pack-name chunk is too short\"));\n+\t\tcur_pack_name = end + 1;\n \n \t\tif (i && strcmp(m->pack_names[i], m->pack_names[i - 1]) <= 0)\n \t\t\tdie(_(\"multi-pack-index pack names out of order: '%s' before '%s'\"),\ndiff --git a/midx.h b/midx.h\nindex 5578cd7b83..5b2a7da043 100644\n--- a/midx.h\n+++ b/midx.h\n@@ -32,6 +32,7 @@ struct multi_pack_index {\n \tint local;\n \n \tconst unsigned char *chunk_pack_names;\n+\tsize_t chunk_pack_names_len;\n \tconst uint32_t *chunk_oid_fanout;\n \tconst unsigned char *chunk_oid_lookup;\n \tconst unsigned char *chunk_object_offsets;\ndiff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\nindex 2722e495b2..0a0ccec8a4 100755\n--- a/t/t5319-multi-pack-index.sh\n+++ b/t/t5319-multi-pack-index.sh\n@@ -1083,4 +1083,15 @@ test_expect_success 'reader notices too-small oid lookup chunk' '\n \ttest_cmp expect err\n '\n \n+test_expect_success 'reader notices too-small pack names chunk' '\n+\t# There is no NUL to terminate the name here, so the\n+\t# chunk is too short.\n+\tcorrupt_chunk PNAM clear 70656666 &&\n+\ttest_must_fail git log 2>err &&\n+\tcat >expect <<-\\EOF &&\n+\tfatal: multi-pack-index pack-name chunk is too short\n+\tEOF\n+\ttest_cmp expect err\n+'\n+\n test_done\n-- \n2.42.0.884.g35e1fe1a6a\n\n"},{"id":"482912","messageId":"20231009210523.GH3282181@coredump.intra.peff.net","threadId":"60334","inReplyTo":"20231009205544.GA3281950@coredump.intra.peff.net","subject":"[PATCH 08/20] midx: enforce chunk alignment on reading","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-10-09T21:05:23Z","receivedAt":"2023-10-09T21:05:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The midx reader assumes chunks are aligned to a 4-byte boundary: we\ntreat the fanout chunk as an array of uint32_t, indexing it to feed the\nresults to ntohl(). Without aligning the chunks, we may violate the\nCPU's alignment constraints. Though many platforms allow this, some do\nnot. And certanily UBSan will complain, since it is undefined behavior.\n\nEven though most chunks are naturally 4-byte-aligned (because they are\nstoring uint32_t or larger types), PNAM is not. It stores NUL-terminated\npack names, so you can have a valid chunk with any length. The writing\nside handles this by 4-byte-aligning the chunk, introducing a few extra\nNULs as necessary. But since we don't check this on the reading side, we\nmay end up with a misaligned fanout and trigger the undefined behavior.\n\nWe have two options here:\n\n 1. Swap out ntohl(fanout[i]) for get_be32(fanout+i) everywhere. The\n latter handles alignment itself. It's possible that it's slightly\n slower (though in practice I'm not sure how true that is,\n especially for these code paths which then go on to do a binary\n search).\n\n 2. Enforce the alignment when reading the chunks. This is easy to do,\n since the table-of-contents reader can check it in one spot.\n\nI went with the second option here, just because it places less burden\non maintenance going forward (it is OK to continue using ntohl), and we\nknow it can't have any performance impact on the actual reads.\n\nThe commit-graph code uses the same chunk API. It's usually also 4-byte\naligned, but some chunks are not (like Bloom filter BDAT chunks). So\nwe'll pass \"1\" here to allow any alignment. It doesn't suffer from the\nsame problem as midx with its fanout because the fanout chunk is always\nthe first (and the rest of the format dictates that the first chunk will\nstart aligned).\n\nThe new test shows the effect on a midx with a misaligned PNAM chunk.\nNote that the midx-reading code treats chunk-toc errors as soft, falling\nback to the non-midx path rather than calling die(), as we do for other\nparsing errors. Arguably we should make all of these behave the same,\nbut that's out of scope for this patch. For now the test just expects\nthe fallback behavior.\n\nSigned-off-by: Jeff King \n---\n chunk-format.c | 8 +++++++-\n chunk-format.h | 3 ++-\n commit-graph.c | 2 +-\n midx.c | 3 ++-\n t/t5319-multi-pack-index.sh | 14 ++++++++++++++\n 5 files changed, 26 insertions(+), 4 deletions(-)\n\ndiff --git a/chunk-format.c b/chunk-format.c\nindex 8d910e23f6..09ef86afa6 100644\n--- a/chunk-format.c\n+++ b/chunk-format.c\n@@ -102,7 +102,8 @@ int read_table_of_contents(struct chunkfile *cf,\n \t\t\t const unsigned char *mfile,\n \t\t\t size_t mfile_size,\n \t\t\t uint64_t toc_offset,\n-\t\t\t int toc_length)\n+\t\t\t int toc_length,\n+\t\t\t unsigned expected_alignment)\n {\n \tint i;\n \tuint32_t chunk_id;\n@@ -120,6 +121,11 @@ int read_table_of_contents(struct chunkfile *cf,\n \t\t\terror(_(\"terminating chunk id appears earlier than expected\"));\n \t\t\treturn 1;\n \t\t}\n+\t\tif (chunk_offset % expected_alignment != 0) {\n+\t\t\terror(_(\"chunk id %\"PRIx32\" not %d-byte aligned\"),\n+\t\t\t chunk_id, expected_alignment);\n+\t\t\treturn 1;\n+\t\t}\n \n \t\ttable_of_contents += CHUNK_TOC_ENTRY_SIZE;\n \t\tnext_chunk_offset = get_be64(table_of_contents + 4);\ndiff --git a/chunk-format.h b/chunk-format.h\nindex 8dce7967f4..d608b8135c 100644\n--- a/chunk-format.h\n+++ b/chunk-format.h\n@@ -36,7 +36,8 @@ int read_table_of_contents(struct chunkfile *cf,\n \t\t\t const unsigned char *mfile,\n \t\t\t size_t mfile_size,\n \t\t\t uint64_t toc_offset,\n-\t\t\t int toc_length);\n+\t\t\t int toc_length,\n+\t\t\t unsigned expected_alignment);\n \n #define CHUNK_NOT_FOUND (-2)\n \ndiff --git a/commit-graph.c b/commit-graph.c\nindex b217e19194..472332f603 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -417,7 +417,7 @@ struct commit_graph *parse_commit_graph(struct repo_settings *s,\n \tcf = init_chunkfile(NULL);\n \n \tif (read_table_of_contents(cf, graph->data, graph_size,\n-\t\t\t\t GRAPH_HEADER_SIZE, graph->num_chunks))\n+\t\t\t\t GRAPH_HEADER_SIZE, graph->num_chunks, 1))\n \t\tgoto free_and_return;\n \n \tread_chunk(cf, GRAPH_CHUNKID_OIDFANOUT, graph_read_oid_fanout, graph);\ndiff --git a/midx.c b/midx.c\nindex ec585cae1b..98f84fbe61 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -154,7 +154,8 @@ struct multi_pack_index *load_multi_pack_index(const char *object_dir, int local\n \tcf = init_chunkfile(NULL);\n \n \tif (read_table_of_contents(cf, m->data, midx_size,\n-\t\t\t\t MIDX_HEADER_SIZE, m->num_chunks))\n+\t\t\t\t MIDX_HEADER_SIZE, m->num_chunks,\n+\t\t\t\t MIDX_CHUNK_ALIGNMENT))\n \t\tgoto cleanup_fail;\n \n \tif (pair_chunk(cf, MIDX_CHUNKID_PACKNAMES, &m->chunk_pack_names, &m->chunk_pack_names_len))\ndiff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\nindex 0a0ccec8a4..34f5944142 100755\n--- a/t/t5319-multi-pack-index.sh\n+++ b/t/t5319-multi-pack-index.sh\n@@ -1094,4 +1094,18 @@ test_expect_success 'reader notices too-small pack names chunk' '\n \ttest_cmp expect err\n '\n \n+test_expect_success 'reader handles unaligned chunks' '\n+\t# A 9-byte PNAM means all of the subsequent chunks\n+\t# will no longer be 4-byte aligned, but it is still\n+\t# a valid one-pack chunk on its own (it is \"foo.pack\\0\").\n+\tcorrupt_chunk PNAM clear 666f6f2e7061636b00 &&\n+\tgit -c core.multipackindex=false log >expect.out &&\n+\tgit -c core.multipackindex=true log >out 2>err &&\n+\ttest_cmp expect.out out &&\n+\tcat >expect.err <<-\\EOF &&\n+\terror: chunk id 4f494446 not 4-byte aligned\n+\tEOF\n+\ttest_cmp expect.err err\n+'\n+\n test_done\n-- \n2.42.0.884.g35e1fe1a6a\n\n"},{"id":"482913","messageId":"20231009210527.GI3282181@coredump.intra.peff.net","threadId":"60334","inReplyTo":"20231009205544.GA3281950@coredump.intra.peff.net","subject":"[PATCH 09/20] midx: check size of object offset chunk","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-10-09T21:05:27Z","receivedAt":"2023-10-09T21:05:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The object offset chunk has one fixed-size entry for each object in the\nmidx. But since we don't check its size, we may access out-of-bounds\nmemory if we see a corrupt or malicious midx file.\n\nSine the entries are fixed-size, the total length can be known up-front,\nand we can just check it while parsing the chunk (this is similar to\nwhat we do when opening pack idx files, which contain a similar offset\ntable).\n\nSigned-off-by: Jeff King \n---\n midx.c | 15 ++++++++++++++-\n t/t5319-multi-pack-index.sh | 10 ++++++++++\n 2 files changed, 24 insertions(+), 1 deletion(-)\n\ndiff --git a/midx.c b/midx.c\nindex 98f84fbe61..7b1b45f381 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -88,6 +88,19 @@ static int midx_read_oid_lookup(const unsigned char *chunk_start,\n \treturn 0;\n }\n \n+static int midx_read_object_offsets(const unsigned char *chunk_start,\n+\t\t\t\t size_t chunk_size, void *data)\n+{\n+\tstruct multi_pack_index *m = data;\n+\tm->chunk_object_offsets = chunk_start;\n+\n+\tif (chunk_size != st_mult(m->num_objects, MIDX_CHUNK_OFFSET_WIDTH)) {\n+\t\terror(_(\"multi-pack-index object offset chunk is the wrong size\"));\n+\t\treturn 1;\n+\t}\n+\treturn 0;\n+}\n+\n struct multi_pack_index *load_multi_pack_index(const char *object_dir, int local)\n {\n \tstruct multi_pack_index *m = NULL;\n@@ -164,7 +177,7 @@ struct multi_pack_index *load_multi_pack_index(const char *object_dir, int local\n \t\tdie(_(\"multi-pack-index required OID fanout chunk missing or corrupted\"));\n \tif (read_chunk(cf, MIDX_CHUNKID_OIDLOOKUP, midx_read_oid_lookup, m))\n \t\tdie(_(\"multi-pack-index required OID lookup chunk missing or corrupted\"));\n-\tif (pair_chunk_unsafe(cf, MIDX_CHUNKID_OBJECTOFFSETS, &m->chunk_object_offsets))\n+\tif (read_chunk(cf, MIDX_CHUNKID_OBJECTOFFSETS, midx_read_object_offsets, m))\n \t\tdie(_(\"multi-pack-index required object offsets chunk missing or corrupted\"));\n \n \tpair_chunk_unsafe(cf, MIDX_CHUNKID_LARGEOFFSETS, &m->chunk_large_offsets);\ndiff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\nindex 34f5944142..30687d5452 100755\n--- a/t/t5319-multi-pack-index.sh\n+++ b/t/t5319-multi-pack-index.sh\n@@ -1108,4 +1108,14 @@ test_expect_success 'reader handles unaligned chunks' '\n \ttest_cmp expect.err err\n '\n \n+test_expect_success 'reader notices too-small object offset chunk' '\n+\tcorrupt_chunk OOFF clear 00000000 &&\n+\ttest_must_fail git log 2>err &&\n+\tcat >expect <<-\\EOF &&\n+\terror: multi-pack-index object offset chunk is the wrong size\n+\tfatal: multi-pack-index required object offsets chunk missing or corrupted\n+\tEOF\n+\ttest_cmp expect err\n+'\n+\n test_done\n-- \n2.42.0.884.g35e1fe1a6a\n\n"},{"id":"482914","messageId":"20231009210530.GJ3282181@coredump.intra.peff.net","threadId":"60334","inReplyTo":"20231009205544.GA3281950@coredump.intra.peff.net","subject":"[PATCH 10/20] midx: bounds-check large offset chunk","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-10-09T21:05:30Z","receivedAt":"2023-10-09T21:05:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we see a large offset bit in the regular midx offset table, we\nuse the entry as an index into a separate large offset table (just like\na pack idx does). But we don't bounds-check the access to that large\noffset table (nor even record its size when we parse the chunk!).\n\nThe equivalent code for a regular pack idx is in check_pack_index_ptr().\nBut things are a bit simpler here because of the chunked format: we can\njust check our array index directly.\n\nAs a bonus, we can get rid of the st_mult() here. If our array\nbounds-check is successful, then we know that the result will fit in a\nsize_t (and the bounds check uses a division to avoid overflow\nentirely).\n\nSigned-off-by: Jeff King \n---\n midx.c | 8 +++++---\n midx.h | 1 +\n t/t5319-multi-pack-index.sh | 20 ++++++++++++++++++++\n 3 files changed, 26 insertions(+), 3 deletions(-)\n\ndiff --git a/midx.c b/midx.c\nindex 7b1b45f381..3e768d0df0 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -180,7 +180,8 @@ struct multi_pack_index *load_multi_pack_index(const char *object_dir, int local\n \tif (read_chunk(cf, MIDX_CHUNKID_OBJECTOFFSETS, midx_read_object_offsets, m))\n \t\tdie(_(\"multi-pack-index required object offsets chunk missing or corrupted\"));\n \n-\tpair_chunk_unsafe(cf, MIDX_CHUNKID_LARGEOFFSETS, &m->chunk_large_offsets);\n+\tpair_chunk(cf, MIDX_CHUNKID_LARGEOFFSETS, &m->chunk_large_offsets,\n+\t\t &m->chunk_large_offsets_len);\n \n \tif (git_env_bool(\"GIT_TEST_MIDX_READ_RIDX\", 1))\n \t\tpair_chunk_unsafe(cf, MIDX_CHUNKID_REVINDEX, &m->chunk_revindex);\n@@ -303,8 +304,9 @@ 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 +\n-\t\t\t\tst_mult(sizeof(uint64_t), offset32));\n+\t\tif (offset32 >= m->chunk_large_offsets_len / sizeof(uint64_t))\n+\t\t\tdie(_(\"multi-pack-index large offset out of bounds\"));\n+\t\treturn get_be64(m->chunk_large_offsets + sizeof(uint64_t) * offset32);\n \t}\n \n \treturn offset32;\ndiff --git a/midx.h b/midx.h\nindex 5b2a7da043..e8e8884d16 100644\n--- a/midx.h\n+++ b/midx.h\n@@ -37,6 +37,7 @@ struct multi_pack_index {\n \tconst unsigned char *chunk_oid_lookup;\n \tconst unsigned char *chunk_object_offsets;\n \tconst unsigned char *chunk_large_offsets;\n+\tsize_t chunk_large_offsets_len;\n \tconst unsigned char *chunk_revindex;\n \n \tconst char **pack_names;\ndiff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\nindex 30687d5452..16050f39d9 100755\n--- a/t/t5319-multi-pack-index.sh\n+++ b/t/t5319-multi-pack-index.sh\n@@ -1118,4 +1118,24 @@ test_expect_success 'reader notices too-small object offset chunk' '\n \ttest_cmp expect err\n '\n \n+test_expect_success 'reader bounds-checks large offset table' '\n+\t# re-use the objects64 dir here to cheaply get access to a midx\n+\t# with large offsets.\n+\tgit init repo &&\n+\ttest_when_finished \"rm -rf repo\" &&\n+\t(\n+\t\tcd repo &&\n+\t\t(cd ../objects64 && pwd) >.git/objects/info/alternates &&\n+\t\tgit multi-pack-index --object-dir=../objects64 write &&\n+\t\tmidx=../objects64/pack/multi-pack-index &&\n+\t\tcorrupt_chunk_file $midx LOFF clear &&\n+\t\ttest_must_fail git cat-file \\\n+\t\t\t--batch-check --batch-all-objects 2>err &&\n+\t\tcat >expect <<-\\EOF &&\n+\t\tfatal: multi-pack-index large offset out of bounds\n+\t\tEOF\n+\t\ttest_cmp expect err\n+\t)\n+'\n+\n test_done\n-- \n2.42.0.884.g35e1fe1a6a\n\n"},{"id":"482915","messageId":"20231009210533.GK3282181@coredump.intra.peff.net","threadId":"60334","inReplyTo":"20231009205544.GA3281950@coredump.intra.peff.net","subject":"[PATCH 11/20] midx: check size of revindex chunk","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-10-09T21:05:33Z","receivedAt":"2023-10-09T21:05:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we load a revindex from disk, we check the size of the file\ncompared to the number of objects we expect it to have. But when we use\na RIDX chunk stored directly in the midx, we just access the memory\ndirectly. This can lead to out-of-bounds memory access for a corrupted\nor malicious multi-pack-index file.\n\nWe can catch this by recording the RIDX chunk size, and then checking it\nagainst the expected size when we \"load\" the revindex. Note that this\ncheck is much simpler than the one that load_revindex_from_disk() does,\nbecause we just have the data array with no header (so we do not need\nto account for the header size, and nor do we need to bother validating\nthe header values).\n\nThe test confirms both that we catch this case, and that we continue the\nprocess (the revindex is required to use the midx bitmaps, but we\nfallback to a non-bitmap traversal).\n\nSigned-off-by: Jeff King \n---\n midx.c | 3 ++-\n midx.h | 1 +\n pack-revindex.c | 13 ++++++++++++-\n t/t5319-multi-pack-index.sh | 17 +++++++++++++++++\n 4 files changed, 32 insertions(+), 2 deletions(-)\n\ndiff --git a/midx.c b/midx.c\nindex 3e768d0df0..2f3863c936 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -184,7 +184,8 @@ struct multi_pack_index *load_multi_pack_index(const char *object_dir, int local\n \t\t &m->chunk_large_offsets_len);\n \n \tif (git_env_bool(\"GIT_TEST_MIDX_READ_RIDX\", 1))\n-\t\tpair_chunk_unsafe(cf, MIDX_CHUNKID_REVINDEX, &m->chunk_revindex);\n+\t\tpair_chunk(cf, MIDX_CHUNKID_REVINDEX, &m->chunk_revindex,\n+\t\t\t &m->chunk_revindex_len);\n \n \tCALLOC_ARRAY(m->pack_names, m->num_packs);\n \tCALLOC_ARRAY(m->packs, m->num_packs);\ndiff --git a/midx.h b/midx.h\nindex e8e8884d16..a5d98919c8 100644\n--- a/midx.h\n+++ b/midx.h\n@@ -39,6 +39,7 @@ struct multi_pack_index {\n \tconst unsigned char *chunk_large_offsets;\n \tsize_t chunk_large_offsets_len;\n \tconst unsigned char *chunk_revindex;\n+\tsize_t chunk_revindex_len;\n \n \tconst char **pack_names;\n \tstruct packed_git **packs;\ndiff --git a/pack-revindex.c b/pack-revindex.c\nindex 7fffcad912..6d8fd3645a 100644\n--- a/pack-revindex.c\n+++ b/pack-revindex.c\n@@ -343,6 +343,17 @@ int verify_pack_revindex(struct packed_git *p)\n \treturn res;\n }\n \n+static int can_use_midx_ridx_chunk(struct multi_pack_index *m)\n+{\n+\tif (!m->chunk_revindex)\n+\t\treturn 0;\n+\tif (m->chunk_revindex_len != st_mult(sizeof(uint32_t), m->num_objects)) {\n+\t\terror(_(\"multi-pack-index reverse-index chunk is the wrong size\"));\n+\t\treturn 0;\n+\t}\n+\treturn 1;\n+}\n+\n int load_midx_revindex(struct multi_pack_index *m)\n {\n \tstruct strbuf revindex_name = STRBUF_INIT;\n@@ -351,7 +362,7 @@ int load_midx_revindex(struct multi_pack_index *m)\n \tif (m->revindex_data)\n \t\treturn 0;\n \n-\tif (m->chunk_revindex) {\n+\tif (can_use_midx_ridx_chunk(m)) {\n \t\t/*\n \t\t * If the MIDX `m` has a `RIDX` chunk, then use its contents for\n \t\t * the reverse index instead of trying to load a separate `.rev`\ndiff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\nindex 16050f39d9..2a11dd1af6 100755\n--- a/t/t5319-multi-pack-index.sh\n+++ b/t/t5319-multi-pack-index.sh\n@@ -1138,4 +1138,21 @@ test_expect_success 'reader bounds-checks large offset table' '\n \t)\n '\n \n+test_expect_success 'reader notices too-small revindex chunk' '\n+\t# We only get a revindex with bitmaps (and likewise only\n+\t# load it when they are asked for).\n+\ttest_config repack.writeBitmaps true &&\n+\tcorrupt_chunk RIDX clear 00000000 &&\n+\tgit -c core.multipackIndex=false rev-list \\\n+\t\t--all --use-bitmap-index >expect.out &&\n+\tgit -c core.multipackIndex=true rev-list \\\n+\t\t--all --use-bitmap-index >out 2>err &&\n+\ttest_cmp expect.out out &&\n+\tcat >expect.err <<-\\EOF &&\n+\terror: multi-pack-index reverse-index chunk is the wrong size\n+\twarning: multi-pack bitmap is missing required reverse index\n+\tEOF\n+\ttest_cmp expect.err err\n+'\n+\n test_done\n-- \n2.42.0.884.g35e1fe1a6a\n\n"},{"id":"482916","messageId":"20231009210536.GL3282181@coredump.intra.peff.net","threadId":"60334","inReplyTo":"20231009205544.GA3281950@coredump.intra.peff.net","subject":"[PATCH 12/20] commit-graph: check size of commit data chunk","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-10-09T21:05:36Z","receivedAt":"2023-10-09T21:05:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We expect a commit-graph file to have a fixed-size data record for each\ncommit in the file (and we know the number of commits to expct from the\nsize of the lookup table). If we encounter a file where this is too\nsmall, we'll look past the end of the chunk (and possibly even off the\nmapped memory).\n\nWe can fix this by checking the size up front when we record the\npointer.\n\nThe included test doesn't segfault, since it ends up reading bytes\nfrom another chunk. But it produces nonsense results, since the values\nit reads are garbage. Our test notices this by comparing the output to a\nnon-corrupted run of the same command (and of course we also check that\nthe expected error is printed to stderr).\n\nSigned-off-by: Jeff King \n---\n commit-graph.c | 12 +++++++++++-\n t/t5318-commit-graph.sh | 9 +++++++++\n 2 files changed, 20 insertions(+), 1 deletion(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 472332f603..9b80bbd75b 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -340,6 +340,16 @@ static int graph_read_oid_lookup(const unsigned char *chunk_start,\n \treturn 0;\n }\n \n+static int graph_read_commit_data(const unsigned char *chunk_start,\n+\t\t\t\t size_t chunk_size, void *data)\n+{\n+\tstruct commit_graph *g = data;\n+\tif (chunk_size != g->num_commits * GRAPH_DATA_WIDTH)\n+\t\treturn error(\"commit-graph commit data chunk is wrong size\");\n+\tg->chunk_commit_data = chunk_start;\n+\treturn 0;\n+}\n+\n static int graph_read_bloom_data(const unsigned char *chunk_start,\n \t\t\t\t size_t chunk_size, void *data)\n {\n@@ -422,7 +432,7 @@ struct commit_graph *parse_commit_graph(struct repo_settings *s,\n \n \tread_chunk(cf, GRAPH_CHUNKID_OIDFANOUT, graph_read_oid_fanout, graph);\n \tread_chunk(cf, GRAPH_CHUNKID_OIDLOOKUP, graph_read_oid_lookup, graph);\n-\tpair_chunk_unsafe(cf, GRAPH_CHUNKID_DATA, &graph->chunk_commit_data);\n+\tread_chunk(cf, GRAPH_CHUNKID_DATA, graph_read_commit_data, graph);\n \tpair_chunk_unsafe(cf, GRAPH_CHUNKID_EXTRAEDGES, &graph->chunk_extra_edges);\n \tpair_chunk_unsafe(cf, GRAPH_CHUNKID_BASE, &graph->chunk_base_graphs);\n \ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex d10658de9e..492460157d 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -870,4 +870,13 @@ test_expect_success 'reader notices out-of-bounds fanout' '\n \ttest_cmp expect.err err\n '\n \n+test_expect_success 'reader notices too-small commit data chunk' '\n+\tcheck_corrupt_chunk CDAT clear 00000000 &&\n+\tcat >expect.err <<-\\EOF &&\n+\terror: commit-graph commit data chunk is wrong size\n+\terror: commit-graph is missing the Commit Data chunk\n+\tEOF\n+\ttest_cmp expect.err err\n+'\n+\n test_done\n-- \n2.42.0.884.g35e1fe1a6a\n\n"},{"id":"482917","messageId":"20231009210538.GM3282181@coredump.intra.peff.net","threadId":"60334","inReplyTo":"20231009205544.GA3281950@coredump.intra.peff.net","subject":"[PATCH 13/20] commit-graph: detect out-of-bounds extra-edges pointers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-10-09T21:05:38Z","receivedAt":"2023-10-09T21:05:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If an entry in a commit-graph file has more than 2 parents, the\nfixed-size parent fields instead point to an offset within an \"extra\nedges\" chunk. We blindly follow these, assuming that the chunk is\npresent and sufficiently large; this can lead to an out-of-bounds read\nfor a corrupt or malicious file.\n\nWe can fix this by recording the size of the chunk and adding a\nbounds-check in fill_commit_in_graph(). There are a few tricky bits:\n\n 1. We'll switch from working with a pointer to an offset. This makes\n some corner cases just fall out naturally:\n\n a. If we did not find an EDGE chunk at all, our size will\n correctly be zero (so everything is \"out of bounds\").\n\n b. Comparing \"size / 4\" lets us make sure we have at least 4 bytes\n to read, and we never compute a pointer more than one element\n past the end of the array (computing a larger pointer is\n probably OK in practice, but is technically undefined\n behavior).\n\n c. The current code casts to \"uint32_t *\". Replacing it with an\n offset avoids any comparison between different types of pointer\n (since the chunk is stored as \"unsigned char *\").\n\n 2. This is the first case in which fill_commit_in_graph() may return\n anything but success. We need to make sure to roll back the\n \"parsed\" flag (and any parents we might have added before running\n out of buffer) so that the caller can cleanly fall back to\n loading the commit object itself.\n\n It's a little non-trivial to do this, and we might benefit from\n factoring it out. But we can wait on that until we actually see a\n second case where we return an error.\n\nAs a bonus, this lets us drop the st_mult() call. Since we've already\ndone a bounds check, we know there won't be any integer overflow (it\nwould imply our buffer is larger than a size_t can hold).\n\nThe included test does not actually segfault before this patch (though\nyou could construct a case where it does). Instead, it reads garbage\nfrom the next chunk which results in it complaining about a bogus parent\nid. This is sufficient for our needs, though (we care that the fallback\nsucceeds, and that stderr mentions the out-of-bounds read).\n\nSigned-off-by: Jeff King \n---\n commit-graph.c | 20 ++++++++++++++------\n commit-graph.h | 1 +\n t/t5318-commit-graph.sh | 8 ++++++++\n 3 files changed, 23 insertions(+), 6 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 9b80bbd75b..e4860841fc 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -433,7 +433,8 @@ struct commit_graph *parse_commit_graph(struct repo_settings *s,\n \tread_chunk(cf, GRAPH_CHUNKID_OIDFANOUT, graph_read_oid_fanout, graph);\n \tread_chunk(cf, GRAPH_CHUNKID_OIDLOOKUP, graph_read_oid_lookup, graph);\n \tread_chunk(cf, GRAPH_CHUNKID_DATA, graph_read_commit_data, graph);\n-\tpair_chunk_unsafe(cf, GRAPH_CHUNKID_EXTRAEDGES, &graph->chunk_extra_edges);\n+\tpair_chunk(cf, GRAPH_CHUNKID_EXTRAEDGES, &graph->chunk_extra_edges,\n+\t\t &graph->chunk_extra_edges_size);\n \tpair_chunk_unsafe(cf, GRAPH_CHUNKID_BASE, &graph->chunk_base_graphs);\n \n \tif (s->commit_graph_generation_version >= 2) {\n@@ -899,7 +900,7 @@ static int fill_commit_in_graph(struct repository *r,\n \t\t\t\tstruct commit_graph *g, uint32_t pos)\n {\n \tuint32_t edge_value;\n-\tuint32_t *parent_data_ptr;\n+\tuint32_t parent_data_pos;\n \tstruct commit_list **pptr;\n \tconst unsigned char *commit_data;\n \tuint32_t lex_index;\n@@ -931,14 +932,21 @@ static int fill_commit_in_graph(struct repository *r,\n \t\treturn 1;\n \t}\n \n-\tparent_data_ptr = (uint32_t*)(g->chunk_extra_edges +\n-\t\t\t st_mult(4, edge_value & GRAPH_EDGE_LAST_MASK));\n+\tparent_data_pos = edge_value & GRAPH_EDGE_LAST_MASK;\n \tdo {\n-\t\tedge_value = get_be32(parent_data_ptr);\n+\t\tif (g->chunk_extra_edges_size / sizeof(uint32_t) <= parent_data_pos) {\n+\t\t\terror(\"commit-graph extra-edges pointer out of bounds\");\n+\t\t\tfree_commit_list(item->parents);\n+\t\t\titem->parents = NULL;\n+\t\t\titem->object.parsed = 0;\n+\t\t\treturn 0;\n+\t\t}\n+\t\tedge_value = get_be32(g->chunk_extra_edges +\n+\t\t\t\t sizeof(uint32_t) * parent_data_pos);\n \t\tpptr = insert_parent_or_die(r, g,\n \t\t\t\t\t edge_value & GRAPH_EDGE_LAST_MASK,\n \t\t\t\t\t pptr);\n-\t\tparent_data_ptr++;\n+\t\tparent_data_pos++;\n \t} while (!(edge_value & GRAPH_LAST_EDGE));\n \n \treturn 1;\ndiff --git a/commit-graph.h b/commit-graph.h\nindex 20ada7e891..1f8a9de4fb 100644\n--- a/commit-graph.h\n+++ b/commit-graph.h\n@@ -95,6 +95,7 @@ struct commit_graph {\n \tconst unsigned char *chunk_generation_data;\n \tconst unsigned char *chunk_generation_data_overflow;\n \tconst unsigned char *chunk_extra_edges;\n+\tsize_t chunk_extra_edges_size;\n \tconst unsigned char *chunk_base_graphs;\n \tconst unsigned char *chunk_bloom_indexes;\n \tconst unsigned char *chunk_bloom_data;\ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex 492460157d..05bafcfe5f 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -879,4 +879,12 @@ test_expect_success 'reader notices too-small commit data chunk' '\n \ttest_cmp expect.err err\n '\n \n+test_expect_success 'reader notices out-of-bounds extra edge' '\n+\tcheck_corrupt_chunk EDGE clear &&\n+\tcat >expect.err <<-\\EOF &&\n+\terror: commit-graph extra-edges pointer out of bounds\n+\tEOF\n+\ttest_cmp expect.err err\n+'\n+\n test_done\n-- \n2.42.0.884.g35e1fe1a6a\n\n"},{"id":"482918","messageId":"20231009210541.GN3282181@coredump.intra.peff.net","threadId":"60334","inReplyTo":"20231009205544.GA3281950@coredump.intra.peff.net","subject":"[PATCH 14/20] commit-graph: bounds-check base graphs chunk","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-10-09T21:05:41Z","receivedAt":"2023-10-09T21:05:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we are loading a commit-graph chain, we check that each slice of the\nchain points to the appropriate set of base graphs via its BASE chunk.\nBut since we don't record the size of the chunk, we may access\nout-of-bounds memory if the file is corrupted.\n\nSince we know the number of entries we expect to find (based on the\nposition within the commit-graph-chain file), we can just check the size\nup front.\n\nIn theory this would also let us drop the st_mult() call a few lines\nlater when we actually access the memory, since we know that the\ncomputed offset will fit in a size_t. But because the operands\n\"g->hash_len\" and \"n\" have types \"unsigned char\" and \"int\", we'd have to\ncast to size_t first. Leaving the st_mult() does that cast, and makes it\nmore obvious that we don't have an overflow problem.\n\nNote that the test does not actually segfault before this patch, since\nit just reads garbage from the chunk after BASE (and indeed, it even\nrejects the file because that garbage does not have the expected hash\nvalue). You could construct a file with BASE at the end that did\nsegfault, but corrupting the existing one is easy, and we can check\nstderr for the expected message.\n\nSigned-off-by: Jeff King \n---\n commit-graph.c | 8 +++++++-\n commit-graph.h | 1 +\n t/t5324-split-commit-graph.sh | 14 ++++++++++++++\n 3 files changed, 22 insertions(+), 1 deletion(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex e4860841fc..4377b547c8 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -435,7 +435,8 @@ struct commit_graph *parse_commit_graph(struct repo_settings *s,\n \tread_chunk(cf, GRAPH_CHUNKID_DATA, graph_read_commit_data, graph);\n \tpair_chunk(cf, GRAPH_CHUNKID_EXTRAEDGES, &graph->chunk_extra_edges,\n \t\t &graph->chunk_extra_edges_size);\n-\tpair_chunk_unsafe(cf, GRAPH_CHUNKID_BASE, &graph->chunk_base_graphs);\n+\tpair_chunk(cf, GRAPH_CHUNKID_BASE, &graph->chunk_base_graphs,\n+\t\t &graph->chunk_base_graphs_size);\n \n \tif (s->commit_graph_generation_version >= 2) {\n \t\tpair_chunk_unsafe(cf, GRAPH_CHUNKID_GENERATION_DATA,\n@@ -546,6 +547,11 @@ static int add_graph_to_chain(struct commit_graph *g,\n \t\treturn 0;\n \t}\n \n+\tif (g->chunk_base_graphs_size / g->hash_len < n) {\n+\t\twarning(_(\"commit-graph base graphs chunk is too small\"));\n+\t\treturn 0;\n+\t}\n+\n \twhile (n) {\n \t\tn--;\n \ndiff --git a/commit-graph.h b/commit-graph.h\nindex 1f8a9de4fb..e4248ea05d 100644\n--- a/commit-graph.h\n+++ b/commit-graph.h\n@@ -97,6 +97,7 @@ struct commit_graph {\n \tconst unsigned char *chunk_extra_edges;\n \tsize_t chunk_extra_edges_size;\n \tconst unsigned char *chunk_base_graphs;\n+\tsize_t chunk_base_graphs_size;\n \tconst unsigned char *chunk_bloom_indexes;\n \tconst unsigned char *chunk_bloom_data;\n \ndiff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh\nindex 55b5765e2d..3c8482d073 100755\n--- a/t/t5324-split-commit-graph.sh\n+++ b/t/t5324-split-commit-graph.sh\n@@ -2,6 +2,7 @@\n \n test_description='split commit graph'\n . ./test-lib.sh\n+. \"$TEST_DIRECTORY\"/lib-chunk.sh\n \n GIT_TEST_COMMIT_GRAPH=0\n GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS=0\n@@ -398,6 +399,19 @@ test_expect_success 'verify across alternates' '\n \t)\n '\n \n+test_expect_success 'reader bounds-checks base-graph chunk' '\n+\tgit clone --no-hardlinks . corrupt-base-chunk &&\n+\t(\n+\t\tcd corrupt-base-chunk &&\n+\t\ttip_file=$graphdir/graph-$(tail -n 1 $graphdir/commit-graph-chain).graph &&\n+\t\tcorrupt_chunk_file \"$tip_file\" BASE clear 01020304 &&\n+\t\tgit -c core.commitGraph=false log >expect.out &&\n+\t\tgit -c core.commitGraph=true log >out 2>err &&\n+\t\ttest_cmp expect.out out &&\n+\t\tgrep \"commit-graph base graphs chunk is too small\" err\n+\t)\n+'\n+\n test_expect_success 'add octopus merge' '\n \tgit reset --hard commits/10 &&\n \tgit merge commits/3 commits/4 &&\n-- \n2.42.0.884.g35e1fe1a6a\n\n"},{"id":"482919","messageId":"20231009210544.GO3282181@coredump.intra.peff.net","threadId":"60334","inReplyTo":"20231009205544.GA3281950@coredump.intra.peff.net","subject":"[PATCH 15/20] commit-graph: check size of generations chunk","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-10-09T21:05:44Z","receivedAt":"2023-10-09T21:06:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We neither check nor record the size of the generations chunk we parse\nfrom a commit-graph file. This should have one uint32_t for each commit\nin the file; if it is smaller (due to corruption, etc), we may read\noutside the mapped memory.\n\nThe included test segfaults without this patch, as it shrinks the size\nconsiderably (and the chunk is near the end of the file, so we read off\nthe end of the array rather than accidentally reading another chunk).\n\nWe can fix this by checking the size up front (like we do for other\nfixed-size chunks, like CDAT).\n\nSigned-off-by: Jeff King \n---\n commit-graph.c | 14 ++++++++++++--\n t/t5318-commit-graph.sh | 8 ++++++++\n 2 files changed, 20 insertions(+), 2 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 4377b547c8..ca26870d1b 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -350,6 +350,16 @@ static int graph_read_commit_data(const unsigned char *chunk_start,\n \treturn 0;\n }\n \n+static int graph_read_generation_data(const unsigned char *chunk_start,\n+\t\t\t\t size_t chunk_size, void *data)\n+{\n+\tstruct commit_graph *g = data;\n+\tif (chunk_size != g->num_commits * sizeof(uint32_t))\n+\t\treturn error(\"commit-graph generations chunk is wrong size\");\n+\tg->chunk_generation_data = chunk_start;\n+\treturn 0;\n+}\n+\n static int graph_read_bloom_data(const unsigned char *chunk_start,\n \t\t\t\t size_t chunk_size, void *data)\n {\n@@ -439,8 +449,8 @@ struct commit_graph *parse_commit_graph(struct repo_settings *s,\n \t\t &graph->chunk_base_graphs_size);\n \n \tif (s->commit_graph_generation_version >= 2) {\n-\t\tpair_chunk_unsafe(cf, GRAPH_CHUNKID_GENERATION_DATA,\n-\t\t\t&graph->chunk_generation_data);\n+\t\tread_chunk(cf, GRAPH_CHUNKID_GENERATION_DATA,\n+\t\t\t graph_read_generation_data, graph);\n \t\tpair_chunk_unsafe(cf, GRAPH_CHUNKID_GENERATION_DATA_OVERFLOW,\n \t\t\t&graph->chunk_generation_data_overflow);\n \ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex 05bafcfe5f..6505ff595a 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -887,4 +887,12 @@ test_expect_success 'reader notices out-of-bounds extra edge' '\n \ttest_cmp expect.err err\n '\n \n+test_expect_success 'reader notices too-small generations chunk' '\n+\tcheck_corrupt_chunk GDA2 clear 00000000 &&\n+\tcat >expect.err <<-\\EOF &&\n+\terror: commit-graph generations chunk is wrong size\n+\tEOF\n+\ttest_cmp expect.err err\n+'\n+\n test_done\n-- \n2.42.0.884.g35e1fe1a6a\n\n"},{"id":"482920","messageId":"20231009210547.GP3282181@coredump.intra.peff.net","threadId":"60334","inReplyTo":"20231009205544.GA3281950@coredump.intra.peff.net","subject":"[PATCH 16/20] commit-graph: bounds-check generation overflow chunk","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-10-09T21:05:47Z","receivedAt":"2023-10-09T21:06:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If the generation entry in a commit-graph doesn't fit, we instead insert\nan offset into a generation overflow chunk. But since we don't record\nthe size of the chunk, we may read outside the chunk if the offset we\nfind on disk is malicious or corrupted.\n\nWe can't check the size of the chunk up-front; it will vary based on how\nmany entries need overflow. So instead, we'll do a bounds-check before\naccessing the chunk memory. Unfortunately there is no error-return from\nthis function, so we'll just have to die(), which is what it does for\nother forms of corruption.\n\nAs with other cases, we can drop the st_mult() call, since we know our\nbounds-checked value will fit within a size_t.\n\nBefore this patch, the test here actually \"works\" because we read\ngarbage data from the next chunk. And since that garbage data happens\nnot to provide a generation number which changes the output, it appears\nto work. We could construct a case that actually segfaults or produces\nwrong output, but it would be a bit tricky. For our purposes its\nsufficient to check that we've detected the bounds error.\n\nSigned-off-by: Jeff King \n---\n commit-graph.c | 10 +++++++---\n commit-graph.h | 1 +\n t/t5328-commit-graph-64bit-time.sh | 10 ++++++++++\n 3 files changed, 18 insertions(+), 3 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex ca26870d1b..f446e76c28 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -451,8 +451,9 @@ struct commit_graph *parse_commit_graph(struct repo_settings *s,\n \tif (s->commit_graph_generation_version >= 2) {\n \t\tread_chunk(cf, GRAPH_CHUNKID_GENERATION_DATA,\n \t\t\t graph_read_generation_data, graph);\n-\t\tpair_chunk_unsafe(cf, GRAPH_CHUNKID_GENERATION_DATA_OVERFLOW,\n-\t\t\t&graph->chunk_generation_data_overflow);\n+\t\tpair_chunk(cf, GRAPH_CHUNKID_GENERATION_DATA_OVERFLOW,\n+\t\t\t &graph->chunk_generation_data_overflow,\n+\t\t\t &graph->chunk_generation_data_overflow_size);\n \n \t\tif (graph->chunk_generation_data)\n \t\t\tgraph->read_generation_data = 1;\n@@ -896,7 +897,10 @@ static void fill_commit_graph_info(struct commit *item, struct commit_graph *g,\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 + st_mult(8, offset_pos));\n+\t\t\tif (g->chunk_generation_data_overflow_size / sizeof(uint64_t) <= offset_pos)\n+\t\t\t\tdie(_(\"commit-graph overflow generation data is too small\"));\n+\t\t\tgraph_data->generation = item->date +\n+\t\t\t\tget_be64(g->chunk_generation_data_overflow + sizeof(uint64_t) * offset_pos);\n \t\t} else\n \t\t\tgraph_data->generation = item->date + offset;\n \t} else\ndiff --git a/commit-graph.h b/commit-graph.h\nindex e4248ea05d..b373f15802 100644\n--- a/commit-graph.h\n+++ b/commit-graph.h\n@@ -94,6 +94,7 @@ struct commit_graph {\n \tconst unsigned char *chunk_commit_data;\n \tconst unsigned char *chunk_generation_data;\n \tconst unsigned char *chunk_generation_data_overflow;\n+\tsize_t chunk_generation_data_overflow_size;\n \tconst unsigned char *chunk_extra_edges;\n \tsize_t chunk_extra_edges_size;\n \tconst unsigned char *chunk_base_graphs;\ndiff --git a/t/t5328-commit-graph-64bit-time.sh b/t/t5328-commit-graph-64bit-time.sh\nindex e9c521c061..e5ff3e07ad 100755\n--- a/t/t5328-commit-graph-64bit-time.sh\n+++ b/t/t5328-commit-graph-64bit-time.sh\n@@ -10,6 +10,7 @@ then\n fi\n \n . \"$TEST_DIRECTORY\"/lib-commit-graph.sh\n+. \"$TEST_DIRECTORY/lib-chunk.sh\"\n \n UNIX_EPOCH_ZERO=\"@0 +0000\"\n FUTURE_DATE=\"@4147483646 +0000\"\n@@ -72,4 +73,13 @@ test_expect_success 'single commit with generation data exceeding UINT32_MAX' '\n \tgit -C repo-uint32-max commit-graph verify\n '\n \n+test_expect_success 'reader notices out-of-bounds generation overflow' '\n+\tgraph=.git/objects/info/commit-graph &&\n+\ttest_when_finished \"rm -rf $graph\" &&\n+\tgit commit-graph write --reachable &&\n+\tcorrupt_chunk_file $graph GDO2 clear &&\n+\ttest_must_fail git log 2>err &&\n+\tgrep \"commit-graph overflow generation data is too small\" err\n+'\n+\n test_done\n-- \n2.42.0.884.g35e1fe1a6a\n\n"},{"id":"482921","messageId":"20231009210550.GQ3282181@coredump.intra.peff.net","threadId":"60334","inReplyTo":"20231009205544.GA3281950@coredump.intra.peff.net","subject":"[PATCH 17/20] commit-graph: check bounds when accessing BDAT chunk","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-10-09T21:05:50Z","receivedAt":"2023-10-09T21:06:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When loading Bloom filters from a commit-graph file, we use the offset\nvalues in the BIDX chunk to index into the memory mapped for the BDAT\nchunk. But since we don't record how big the BDAT chunk is, we just\ntrust that the BIDX offsets won't cause us to read outside of the chunk\nmemory. A corrupted or malicious commit-graph file will cause us to\nsegfault (in practice this isn't a very interesting attack, since\ncommit-graph files are local-only, and the worst case is an\nout-of-bounds read).\n\nWe can't fix this by checking the chunk size during parsing, since the\ndata in the BDAT chunk doesn't have a fixed size (that's why we need the\nBIDX in the first place). So we'll fix it in two parts:\n\n 1. Record the BDAT chunk size during parsing, and then later check\n that the BIDX offsets we look up are within bounds.\n\n 2. Because the offsets are relative to the end of the BDAT header, we\n must also make sure that the BDAT chunk is at least as large as the\n expected header size. Otherwise, we overflow when trying to move\n past the header, even for an offset of \"0\". We can check this\n early, during the parsing stage.\n\nThe error messages are rather verbose, but since this is not something\nyou'd expect to see outside of severe bugs or corruption, it makes sense\nto err on the side of too many details. Sadly we can't mention the\nfilename during the chunk-parsing stage, as we haven't set g->filename\nat this point, nor passed it down through the stack.\n\nSigned-off-by: Jeff King \n---\n bloom.c | 24 ++++++++++++++++++++++++\n commit-graph.c | 10 ++++++++++\n commit-graph.h | 1 +\n t/t4216-log-bloom.sh | 28 ++++++++++++++++++++++++++++\n 4 files changed, 63 insertions(+)\n\ndiff --git a/bloom.c b/bloom.c\nindex aef6b5fea2..61abad7f8c 100644\n--- a/bloom.c\n+++ b/bloom.c\n@@ -29,6 +29,26 @@ static inline unsigned char get_bitmask(uint32_t pos)\n \treturn ((unsigned char)1) << (pos & (BITS_PER_WORD - 1));\n }\n \n+static int check_bloom_offset(struct commit_graph *g, uint32_t pos,\n+\t\t\t uint32_t offset)\n+{\n+\t/*\n+\t * Note that we allow offsets equal to the data size, which would set\n+\t * our pointers at one past the end of the chunk memory. This is\n+\t * necessary because the on-disk index points to the end of the\n+\t * entries (so we can compute size by comparing adjacent ones). And\n+\t * naturally the final entry's end is one-past-the-end of the chunk.\n+\t */\n+\tif (offset <= g->chunk_bloom_data_size - BLOOMDATA_CHUNK_HEADER_SIZE)\n+\t\treturn 0;\n+\n+\twarning(\"ignoring out-of-range offset (%\"PRIuMAX\") for changed-path\"\n+\t\t\" filter at pos %\"PRIuMAX\" of %s (chunk size: %\"PRIuMAX\")\",\n+\t\t(uintmax_t)offset, (uintmax_t)pos,\n+\t\tg->filename, (uintmax_t)g->chunk_bloom_data_size);\n+\treturn -1;\n+}\n+\n static int load_bloom_filter_from_graph(struct commit_graph *g,\n \t\t\t\t\tstruct bloom_filter *filter,\n \t\t\t\t\tuint32_t graph_pos)\n@@ -51,6 +71,10 @@ static int load_bloom_filter_from_graph(struct commit_graph *g,\n \telse\n \t\tstart_index = 0;\n \n+\tif (check_bloom_offset(g, lex_pos, end_index) < 0 ||\n+\t check_bloom_offset(g, lex_pos - 1, start_index) < 0)\n+\t\treturn 0;\n+\n \tfilter->len = end_index - start_index;\n \tfilter->data = (unsigned char *)(g->chunk_bloom_data +\n \t\t\t\t\tsizeof(unsigned char) * start_index +\ndiff --git a/commit-graph.c b/commit-graph.c\nindex f446e76c28..f7a42be6d0 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -365,7 +365,17 @@ static int graph_read_bloom_data(const unsigned char *chunk_start,\n {\n \tstruct commit_graph *g = data;\n \tuint32_t hash_version;\n+\n+\tif (chunk_size < BLOOMDATA_CHUNK_HEADER_SIZE) {\n+\t\twarning(\"ignoring too-small changed-path chunk\"\n+\t\t\t\" (%\"PRIuMAX\" < %\"PRIuMAX\") in commit-graph file\",\n+\t\t\t(uintmax_t)chunk_size,\n+\t\t\t(uintmax_t)BLOOMDATA_CHUNK_HEADER_SIZE);\n+\t\treturn -1;\n+\t}\n+\n \tg->chunk_bloom_data = chunk_start;\n+\tg->chunk_bloom_data_size = chunk_size;\n \thash_version = get_be32(chunk_start);\n \n \tif (hash_version != 1)\ndiff --git a/commit-graph.h b/commit-graph.h\nindex b373f15802..c6870274c5 100644\n--- a/commit-graph.h\n+++ b/commit-graph.h\n@@ -101,6 +101,7 @@ struct commit_graph {\n \tsize_t chunk_base_graphs_size;\n \tconst unsigned char *chunk_bloom_indexes;\n \tconst unsigned char *chunk_bloom_data;\n+\tsize_t chunk_bloom_data_size;\n \n \tstruct topo_level_slab *topo_levels;\n \tstruct bloom_filter_settings *bloom_filter_settings;\ndiff --git a/t/t4216-log-bloom.sh b/t/t4216-log-bloom.sh\nindex fa9d32facf..7a727bcddd 100755\n--- a/t/t4216-log-bloom.sh\n+++ b/t/t4216-log-bloom.sh\n@@ -5,6 +5,7 @@ GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n . ./test-lib.sh\n+. \"$TEST_DIRECTORY\"/lib-chunk.sh\n \n GIT_TEST_COMMIT_GRAPH=0\n GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS=0\n@@ -404,4 +405,31 @@ test_expect_success 'Bloom generation backfills empty commits' '\n \t)\n '\n \n+corrupt_graph () {\n+\tgraph=.git/objects/info/commit-graph &&\n+\ttest_when_finished \"rm -rf $graph\" &&\n+\tgit commit-graph write --reachable --changed-paths &&\n+\tcorrupt_chunk_file $graph \"$@\"\n+}\n+\n+check_corrupt_graph () {\n+\tcorrupt_graph \"$@\" &&\n+\tgit -c core.commitGraph=false log -- A/B/file2 >expect.out &&\n+\tgit -c core.commitGraph=true log -- A/B/file2 >out 2>err &&\n+\ttest_cmp expect.out out\n+}\n+\n+test_expect_success 'Bloom reader notices too-small data chunk' '\n+\tcheck_corrupt_graph BDAT clear 00000000 &&\n+\techo \"warning: ignoring too-small changed-path chunk\" \\\n+\t\t\"(4 < 12) in commit-graph file\" >expect.err &&\n+\ttest_cmp expect.err err\n+'\n+\n+test_expect_success 'Bloom reader notices out-of-bounds filter offsets' '\n+\tcheck_corrupt_graph BIDX 12 FFFFFFFF &&\n+\t# use grep to avoid depending on exact chunk size\n+\tgrep \"warning: ignoring out-of-range offset (4294967295) for changed-path filter at pos 3 of .git/objects/info/commit-graph\" err\n+'\n+\n test_done\n-- \n2.42.0.884.g35e1fe1a6a\n\n"},{"id":"482922","messageId":"20231009210553.GR3282181@coredump.intra.peff.net","threadId":"60334","inReplyTo":"20231009205544.GA3281950@coredump.intra.peff.net","subject":"[PATCH 18/20] commit-graph: check bounds when accessing BIDX chunk","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-10-09T21:05:53Z","receivedAt":"2023-10-09T21:06:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We load the bloom_filter_indexes chunk using pair_chunk(), so we have no\nidea how big it is. This can lead to out-of-bounds reads if it is\nsmaller than expected, since we index it based on the number of commits\nfound elsewhere in the graph file.\n\nWe can check the chunk size up front, like we do for CDAT and other\nchunks with one fixed-size record per commit.\n\nThe test case demonstrates the problem. It actually won't segfault,\nbecause we end up reading random data from the follow-on chunk (BDAT in\nthis case), and the bounds checks added in the previous patch complain.\nBut this is by no means assured, and you can craft a commit-graph file\nwith BIDX at the end (or a smaller BDAT) that does segfault.\n\nSigned-off-by: Jeff King \n---\n commit-graph.c | 16 ++++++++++++++--\n t/t4216-log-bloom.sh | 9 +++++++++\n 2 files changed, 23 insertions(+), 2 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex f7a42be6d0..1f334987b5 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -360,6 +360,18 @@ static int graph_read_generation_data(const unsigned char *chunk_start,\n \treturn 0;\n }\n \n+static int graph_read_bloom_index(const unsigned char *chunk_start,\n+\t\t\t\t size_t chunk_size, void *data)\n+{\n+\tstruct commit_graph *g = data;\n+\tif (chunk_size != g->num_commits * 4) {\n+\t\twarning(\"commit-graph changed-path index chunk is too small\");\n+\t\treturn -1;\n+\t}\n+\tg->chunk_bloom_indexes = chunk_start;\n+\treturn 0;\n+}\n+\n static int graph_read_bloom_data(const unsigned char *chunk_start,\n \t\t\t\t size_t chunk_size, void *data)\n {\n@@ -470,8 +482,8 @@ struct commit_graph *parse_commit_graph(struct repo_settings *s,\n \t}\n \n \tif (s->commit_graph_read_changed_paths) {\n-\t\tpair_chunk_unsafe(cf, GRAPH_CHUNKID_BLOOMINDEXES,\n-\t\t\t &graph->chunk_bloom_indexes);\n+\t\tread_chunk(cf, GRAPH_CHUNKID_BLOOMINDEXES,\n+\t\t\t graph_read_bloom_index, graph);\n \t\tread_chunk(cf, GRAPH_CHUNKID_BLOOMDATA,\n \t\t\t graph_read_bloom_data, graph);\n \t}\ndiff --git a/t/t4216-log-bloom.sh b/t/t4216-log-bloom.sh\nindex 7a727bcddd..f6054cbb27 100755\n--- a/t/t4216-log-bloom.sh\n+++ b/t/t4216-log-bloom.sh\n@@ -432,4 +432,13 @@ test_expect_success 'Bloom reader notices out-of-bounds filter offsets' '\n \tgrep \"warning: ignoring out-of-range offset (4294967295) for changed-path filter at pos 3 of .git/objects/info/commit-graph\" err\n '\n \n+test_expect_success 'Bloom reader notices too-small index chunk' '\n+\t# replace the index with a single entry, making most\n+\t# lookups out-of-bounds\n+\tcheck_corrupt_graph BIDX clear 00000000 &&\n+\techo \"warning: commit-graph changed-path index chunk\" \\\n+\t\t\"is too small\" >expect.err &&\n+\ttest_cmp expect.err err\n+'\n+\n test_done\n-- \n2.42.0.884.g35e1fe1a6a\n\n"},{"id":"482923","messageId":"20231009210556.GS3282181@coredump.intra.peff.net","threadId":"60334","inReplyTo":"20231009205544.GA3281950@coredump.intra.peff.net","subject":"[PATCH 19/20] commit-graph: detect out-of-order BIDX offsets","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-10-09T21:05:56Z","receivedAt":"2023-10-09T21:06:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The BIDX chunk tells us the offsets at which each commit's Bloom filters\ncan be found in the BDAT chunk. We compute the length of each filter by\nchecking the offsets of neighbors and subtracting them.\n\nIf the offsets are out of order, then we'll get a negative length, which\nwe then store as a very large unsigned value. This can cause us to read\nout-of-bounds memory, as we access the hash data modulo \"filter->len *\nBITS_PER_WORD\".\n\nWe can easily detect this case when loading the individual filters.\n\nSigned-off-by: Jeff King \n---\n bloom.c | 10 ++++++++++\n t/t4216-log-bloom.sh | 13 +++++++++++++\n 2 files changed, 23 insertions(+)\n\ndiff --git a/bloom.c b/bloom.c\nindex 61abad7f8c..1474aa19fa 100644\n--- a/bloom.c\n+++ b/bloom.c\n@@ -75,6 +75,16 @@ static int load_bloom_filter_from_graph(struct commit_graph *g,\n \t check_bloom_offset(g, lex_pos - 1, start_index) < 0)\n \t\treturn 0;\n \n+\tif (end_index < start_index) {\n+\t\twarning(\"ignoring decreasing changed-path index offsets\"\n+\t\t\t\" (%\"PRIuMAX\" > %\"PRIuMAX\") for positions\"\n+\t\t\t\" %\"PRIuMAX\" and %\"PRIuMAX\" of %s\",\n+\t\t\t(uintmax_t)start_index, (uintmax_t)end_index,\n+\t\t\t(uintmax_t)(lex_pos-1), (uintmax_t)lex_pos,\n+\t\t\tg->filename);\n+\t\treturn 0;\n+\t}\n+\n \tfilter->len = end_index - start_index;\n \tfilter->data = (unsigned char *)(g->chunk_bloom_data +\n \t\t\t\t\tsizeof(unsigned char) * start_index +\ndiff --git a/t/t4216-log-bloom.sh b/t/t4216-log-bloom.sh\nindex f6054cbb27..2ba0324a69 100755\n--- a/t/t4216-log-bloom.sh\n+++ b/t/t4216-log-bloom.sh\n@@ -441,4 +441,17 @@ test_expect_success 'Bloom reader notices too-small index chunk' '\n \ttest_cmp expect.err err\n '\n \n+test_expect_success 'Bloom reader notices out-of-order index offsets' '\n+\t# we do not know any real offsets, but we can pick\n+\t# something plausible; we should not get to the point of\n+\t# actually reading from the bogus offsets anyway.\n+\tcorrupt_graph BIDX 4 0000000c00000005 &&\n+\techo \"warning: ignoring decreasing changed-path index offsets\" \\\n+\t\t\"(12 > 5) for positions 1 and 2 of .git/objects/info/commit-graph\" >expect.err &&\n+\tgit -c core.commitGraph=false log -- A/B/file2 >expect.out &&\n+\tgit -c core.commitGraph=true log -- A/B/file2 >out 2>err &&\n+\ttest_cmp expect.out out &&\n+\ttest_cmp expect.err err\n+'\n+\n test_done\n-- \n2.42.0.884.g35e1fe1a6a\n\n"},{"id":"482924","messageId":"20231009210601.GT3282181@coredump.intra.peff.net","threadId":"60334","inReplyTo":"20231009205544.GA3281950@coredump.intra.peff.net","subject":"[PATCH 20/20] chunk-format: drop pair_chunk_unsafe()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-10-09T21:06:01Z","receivedAt":"2023-10-09T21:06:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"There are no callers left, and we don't want anybody to add new ones (they\nshould use the not-unsafe version instead). So let's drop the function.\n\nSigned-off-by: Jeff King \n---\n chunk-format.c | 8 --------\n chunk-format.h | 13 -------------\n 2 files changed, 21 deletions(-)\n\ndiff --git a/chunk-format.c b/chunk-format.c\nindex 09ef86afa6..cdc7f39b70 100644\n--- a/chunk-format.c\n+++ b/chunk-format.c\n@@ -184,14 +184,6 @@ int pair_chunk(struct chunkfile *cf,\n \treturn read_chunk(cf, chunk_id, pair_chunk_fn, &pcd);\n }\n \n-int pair_chunk_unsafe(struct chunkfile *cf,\n-\t\t uint32_t chunk_id,\n-\t\t const unsigned char **p)\n-{\n-\tsize_t dummy;\n-\treturn pair_chunk(cf, chunk_id, p, &dummy);\n-}\n-\n int read_chunk(struct chunkfile *cf,\n \t uint32_t chunk_id,\n \t chunk_read_fn fn,\ndiff --git a/chunk-format.h b/chunk-format.h\nindex d608b8135c..14b76180ef 100644\n--- a/chunk-format.h\n+++ b/chunk-format.h\n@@ -54,19 +54,6 @@ int pair_chunk(struct chunkfile *cf,\n \t const unsigned char **p,\n \t size_t *size);\n \n-/*\n- * Unsafe version of pair_chunk; it does not return the size,\n- * meaning that the caller cannot possibly be careful about\n- * reading out of bounds from the mapped memory.\n- *\n- * No new callers should use this function, and old callers should\n- * be audited and migrated over to using the regular pair_chunk()\n- * function.\n- */\n-int pair_chunk_unsafe(struct chunkfile *cf,\n-\t\t uint32_t chunk_id,\n-\t\t const unsigned char **p);\n-\n typedef int (*chunk_read_fn)(const unsigned char *chunk_start,\n \t\t\t size_t chunk_size, void *data);\n /*\n-- \n2.42.0.884.g35e1fe1a6a\n"},{"id":"483027","messageId":"ZSXiJZFnmpzAmuwx@nand.local","threadId":"60334","inReplyTo":"20231009205823.GA3282181@coredump.intra.peff.net","subject":"Re: [PATCH 01/20] chunk-format: note that pair_chunk() is unsafe","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-10-10T23:45:41Z","receivedAt":"2023-10-10T23:45:54Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Oct 09, 2023 at 04:58:23PM -0400, Jeff King wrote:\n> There are no callers of the \"safe\" version yet, but we'll add some in\n> subsequent patches.\n\nMakes sense.\n\n> +int pair_chunk_unsafe(struct chunkfile *cf,\n> +\t\t uint32_t chunk_id,\n> +\t\t const unsigned char **p)\n> {\n> -\treturn read_chunk(cf, chunk_id, pair_chunk_fn, p);\n> +\tsize_t dummy;\n> +\treturn pair_chunk(cf, chunk_id, p, &dummy);\n\nI have always disliked functions that require you to pass a non-NULL\npointer to some value that you may or may not want to have that function\nfill out. So I was going to suggest something along the lines of\n\"pair_chunk() should tolerate a NULL fourth argument instead of filling\nout the size unconditionally\".\n\nBut that's (a) the whole point of the series ;-), and (b) unnecessary,\nsince this function is going to go away entirely by the end of the\nseries, too.\n\nSo I think that the call you made here was the right one. The rest of\nthe patch all looks good to me, let's read on...\n\nThanks,\nTaylor\n"},{"id":"483028","messageId":"ZSXijWNVCyO7FelP@nand.local","threadId":"60334","inReplyTo":"20231009205838.GB3282181@coredump.intra.peff.net","subject":"Re: [PATCH 02/20] t: add library for munging chunk-format files","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-10-10T23:47:25Z","receivedAt":"2023-10-10T23:47:35Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Oct 09, 2023 at 04:58:38PM -0400, Jeff King wrote:\n> ---\n> t/lib-chunk.sh | 17 ++++++++\n> t/lib-chunk/corrupt-chunk-file.pl | 66 +++++++++++++++++++++++++++++++\n> 2 files changed, 83 insertions(+)\n> create mode 100644 t/lib-chunk.sh\n> create mode 100644 t/lib-chunk/corrupt-chunk-file.pl\n\nI can't claim to be a competent reviewer for the Perl portions of this\npatch. But I agree with the goal and am glad to see us getting away from\nthe brittle implementation that we have been living with in the test\nsuite.\n\nThanks,\nTaylor\n"},{"id":"483029","messageId":"ZSXjR6BjvaokNwPe@nand.local","threadId":"60334","inReplyTo":"20231009205919.GC3282181@coredump.intra.peff.net","subject":"Re: [PATCH 03/20] midx: stop ignoring malformed oid fanout chunk","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-10-10T23:50:31Z","receivedAt":"2023-10-10T23:50:37Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Oct 09, 2023 at 04:59:19PM -0400, Jeff King wrote:\n> When we load the oid-fanout chunk, our callback checks that its size is\n> reasonable and returns an error if not. However, the caller only checks\n> our return value against CHUNK_NOT_FOUND, so we end up ignoring the\n> error completely! Using a too-small fanout table means we end up\n> accessing random memory for the fanout and segfault.\n>\n> We can fix this by checking for any non-zero return value, rather than\n> just CHUNK_NOT_FOUND, and adjusting our error message to cover both\n> cases. We could handle each error code individually, but there's not\n> much point for such a rare case. The extra message produced in the\n> callback makes it clear what is going on.\n>\n> The same pattern is used in the adjacent code. Those cases are actually\n> OK for now because they do not use a custom callback, so the only error\n> they can get is CHUNK_NOT_FOUND. But let's convert them, as this is an\n> accident waiting to happen (especially as we convert some of them away\n> from pair_chunk). The error messages are more verbose, but it should be\n> rare for a user to see these anyway.\n>\n> Signed-off-by: Jeff King \n> ---\n> midx.c | 16 ++++++++--------\n> t/t5319-multi-pack-index.sh | 20 +++++++++++++++++++-\n> 2 files changed, 27 insertions(+), 9 deletions(-)\n>\n> diff --git a/midx.c b/midx.c\n> index 3165218ab5..21d7dd15ef 100644\n> --- a/midx.c\n> +++ b/midx.c\n> @@ -143,14 +143,14 @@ struct multi_pack_index *load_multi_pack_index(const char *object_dir, int local\n> \t\t\t\t MIDX_HEADER_SIZE, m->num_chunks))\n> \t\tgoto cleanup_fail;\n>\n> -\tif (pair_chunk_unsafe(cf, MIDX_CHUNKID_PACKNAMES, &m->chunk_pack_names) == CHUNK_NOT_FOUND)\n> -\t\tdie(_(\"multi-pack-index missing required pack-name chunk\"));\n> -\tif (read_chunk(cf, MIDX_CHUNKID_OIDFANOUT, midx_read_oid_fanout, m) == CHUNK_NOT_FOUND)\n> -\t\tdie(_(\"multi-pack-index missing required OID fanout chunk\"));\n> -\tif (pair_chunk_unsafe(cf, MIDX_CHUNKID_OIDLOOKUP, &m->chunk_oid_lookup) == CHUNK_NOT_FOUND)\n> -\t\tdie(_(\"multi-pack-index missing required OID lookup chunk\"));\n> -\tif (pair_chunk_unsafe(cf, MIDX_CHUNKID_OBJECTOFFSETS, &m->chunk_object_offsets) == CHUNK_NOT_FOUND)\n> -\t\tdie(_(\"multi-pack-index missing required object offsets chunk\"));\n> +\tif (pair_chunk_unsafe(cf, MIDX_CHUNKID_PACKNAMES, &m->chunk_pack_names))\n> +\t\tdie(_(\"multi-pack-index required pack-name chunk missing or corrupted\"));\n> +\tif (read_chunk(cf, MIDX_CHUNKID_OIDFANOUT, midx_read_oid_fanout, m))\n> +\t\tdie(_(\"multi-pack-index required OID fanout chunk missing or corrupted\"));\n> +\tif (pair_chunk_unsafe(cf, MIDX_CHUNKID_OIDLOOKUP, &m->chunk_oid_lookup))\n> +\t\tdie(_(\"multi-pack-index required OID lookup chunk missing or corrupted\"));\n> +\tif (pair_chunk_unsafe(cf, MIDX_CHUNKID_OBJECTOFFSETS, &m->chunk_object_offsets))\n> +\t\tdie(_(\"multi-pack-index required object offsets chunk missing or corrupted\"));\n\nAll makes sense. I have a mild preference for \"missing or corrupt\" over\n\"missing or corrupted\", but it's mild enough that I wouldn't be sad if\nyou kept it as-is ;-).\n\nI do wonder if translators would be happy with:\n\n die(_(\"multi-pack-index required %s chunk missing or corrupt\"),\n \"OID fanout\");\n\nor if that is assuming too much about the languages that we translate\ninto.\n\n> diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\n> index 1bcc02004d..b8fe85aeba 100755\n> --- a/t/t5319-multi-pack-index.sh\n> +++ b/t/t5319-multi-pack-index.sh\n\nThe rest of the patch is looking good...\n\nThanks,\nTaylor\n"},{"id":"483030","messageId":"ZSXnZXglgbfK3VYd@nand.local","threadId":"60334","inReplyTo":"20231009205951.GD3282181@coredump.intra.peff.net","subject":"Re: [PATCH 04/20] commit-graph: check size of oid fanout chunk","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-10-11T00:08:05Z","receivedAt":"2023-10-11T00:08:11Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Oct 09, 2023 at 04:59:51PM -0400, Jeff King wrote:\n> We load the oid fanout chunk with pair_chunk(), which means we never see\n> the size of the chunk. We just assume the on-disk file uses the\n> appropriate size, and if it's too small we'll access random memory.\n>\n> It's easy to check this up-front; the fanout always consists of 256\n> uint32's, since it is a fanout of the first byte of the hash pointing\n> into the oid index. These parameters can't be changed without\n> introducing a new chunk type.\n\nCool, this is the first patch that should start reducing our usage of\nthe new pair_chunk_unsafe() and hardening these reads. Let's take a\nlook...\n\n> This matches the similar check in the midx OIDF chunk (but note that\n> rather than checking for the error immediately, the graph code just\n> leaves parts of the struct NULL and checks for required fields later).\n>\n> Signed-off-by: Jeff King \n> ---\n> commit-graph.c | 13 +++++++++++--\n> t/t5318-commit-graph.sh | 26 ++++++++++++++++++++++++++\n> 2 files changed, 37 insertions(+), 2 deletions(-)\n>\n> diff --git a/commit-graph.c b/commit-graph.c\n> index a689a55b79..9b3b01da61 100644\n> --- a/commit-graph.c\n> +++ b/commit-graph.c\n> @@ -305,6 +305,16 @@ static int verify_commit_graph_lite(struct commit_graph *g)\n> \treturn 0;\n> }\n>\n> +static int graph_read_oid_fanout(const unsigned char *chunk_start,\n> +\t\t\t\t size_t chunk_size, void *data)\n> +{\n> +\tstruct commit_graph *g = data;\n> +\tif (chunk_size != 256 * sizeof(uint32_t))\n> +\t\treturn error(\"commit-graph oid fanout chunk is wrong size\");\n\nShould we mark this string for translation?\n\n> +\tg->chunk_oid_fanout = (const uint32_t *)chunk_start;\n> +\treturn 0;\n> +}\n> +\n\nNice. This makes sense and seems like an obvious improvement over the\nexisting code.\n\nI wonder how common this pattern is. We have read_chunk() which is for\nhandling more complex scenarios than this. But the safe version of\npair_chunk() really just wants to check that the size of the chunk is as\nexpected and assign the location in the mmap to some pointer.\n\nDo you think it would be worth changing pair_chunk() to take an expected\nsize_t and handle this generically? I.e. have a version of\nchunk-format::pair_chunk_fn() that looks something like:\n\n static int pair_chunk_fn(const unsigned char *chunk_start,\n size_t chunk_size, void *data)\n {\n const unsigned char **p = data;\n if (chunk_size != data->size)\n return -1;\n *p = chunk_start;\n return 0;\n }\n\nand then our call here would be:\n\n if (pair_chunk(cf, GRAPH_CHUNKID_OIDFANOUT,\n (const unsigned char **)&graph->chunk_oid_fanout,\n 256 * sizeof(uint32_t)) < 0)\n return error(\"commit-graph oid fanout chunk is wrong size\");\n\nI dunno. It's hard to have a more concrete recomendation without having\nread the rest of the series. So it's possible that this is just complete\nnonsense ;-). But my hunch is that there are a number of callers that\nwould benefit from having this built in.\n\nThanks,\nTaylor\n"},{"id":"483031","messageId":"ZSX5V4VkpNbY5izU@nand.local","threadId":"60334","inReplyTo":"ZSXnZXglgbfK3VYd@nand.local","subject":"Re: [PATCH 04/20] commit-graph: check size of oid fanout chunk","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-10-11T01:24:39Z","receivedAt":"2023-10-11T01:24:48Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Oct 10, 2023 at 08:08:05PM -0400, Taylor Blau wrote:\n> Do you think it would be worth changing pair_chunk() to take an expected\n> size_t and handle this generically? I.e. have a version of\n> chunk-format::pair_chunk_fn() that looks something like:\n>\n> static int pair_chunk_fn(const unsigned char *chunk_start,\n> size_t chunk_size, void *data)\n> {\n> const unsigned char **p = data;\n> if (chunk_size != data->size)\n> return -1;\n> *p = chunk_start;\n> return 0;\n> }\n>\n> and then our call here would be:\n>\n> if (pair_chunk(cf, GRAPH_CHUNKID_OIDFANOUT,\n> (const unsigned char **)&graph->chunk_oid_fanout,\n> 256 * sizeof(uint32_t)) < 0)\n> return error(\"commit-graph oid fanout chunk is wrong size\");\n\nI took a brief stab at implementing this tonight and came up with this,\nwhich applies on top of this patch. Looking through the rest of the\nseries briefly[^1], I think having something like this would be useful:\n\n--- 8< ---\ndiff --git a/chunk-format.c b/chunk-format.c\nindex 8d910e23f6..38b0f847be 100644\n--- a/chunk-format.c\n+++ b/chunk-format.c\n@@ -157,6 +157,8 @@ int read_table_of_contents(struct chunkfile *cf,\n struct pair_chunk_data {\n \tconst unsigned char **p;\n \tsize_t *size;\n+\n+\tsize_t expected_size;\n };\n\n static int pair_chunk_fn(const unsigned char *chunk_start,\n@@ -169,6 +171,20 @@ static int pair_chunk_fn(const unsigned char *chunk_start,\n \treturn 0;\n }\n\n+static int pair_chunk_expect_fn(const unsigned char *chunk_start,\n+\t\t\t\tsize_t chunk_size,\n+\t\t\t\tvoid *data)\n+{\n+\tstruct pair_chunk_data *pcd = data;\n+\tif (pcd->expected_size != chunk_size)\n+\t\treturn error(_(\"mismatched chunk size, got: %\"PRIuMAX\", wanted:\"\n+\t\t\t \" %\"PRIuMAX),\n+\t\t\t (uintmax_t)chunk_size,\n+\t\t\t (uintmax_t)pcd->expected_size);\n+\t*pcd->p = chunk_start;\n+\treturn 0;\n+}\n+\n int pair_chunk(struct chunkfile *cf,\n \t uint32_t chunk_id,\n \t const unsigned char **p,\n@@ -178,6 +194,14 @@ int pair_chunk(struct chunkfile *cf,\n \treturn read_chunk(cf, chunk_id, pair_chunk_fn, &pcd);\n }\n\n+int pair_chunk_expect(struct chunkfile *cf,\n+\t\t uint32_t chunk_id, const unsigned char **p,\n+\t\t size_t expected_size)\n+{\n+\tstruct pair_chunk_data pcd = { .p = p, .expected_size = expected_size };\n+\treturn read_chunk(cf, chunk_id, pair_chunk_expect_fn, &pcd);\n+}\n+\n int pair_chunk_unsafe(struct chunkfile *cf,\n \t\t uint32_t chunk_id,\n \t\t const unsigned char **p)\ndiff --git a/chunk-format.h b/chunk-format.h\nindex 8dce7967f4..778f81f555 100644\n--- a/chunk-format.h\n+++ b/chunk-format.h\n@@ -53,6 +53,10 @@ int pair_chunk(struct chunkfile *cf,\n \t const unsigned char **p,\n \t size_t *size);\n\n+int pair_chunk_expect(struct chunkfile *cf,\n+\t\t uint32_t chunk_id, const unsigned char **p,\n+\t\t size_t expected_size);\n+\n /*\n * Unsafe version of pair_chunk; it does not return the size,\n * meaning that the caller cannot possibly be careful about\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 9b3b01da61..ed85161fdb 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -305,16 +305,6 @@ static int verify_commit_graph_lite(struct commit_graph *g)\n \treturn 0;\n }\n\n-static int graph_read_oid_fanout(const unsigned char *chunk_start,\n-\t\t\t\t size_t chunk_size, void *data)\n-{\n-\tstruct commit_graph *g = data;\n-\tif (chunk_size != 256 * sizeof(uint32_t))\n-\t\treturn error(\"commit-graph oid fanout chunk is wrong size\");\n-\tg->chunk_oid_fanout = (const uint32_t *)chunk_start;\n-\treturn 0;\n-}\n-\n static int graph_read_oid_lookup(const unsigned char *chunk_start,\n \t\t\t\t size_t chunk_size, void *data)\n {\n@@ -404,7 +394,10 @@ struct commit_graph *parse_commit_graph(struct repo_settings *s,\n \t\t\t\t GRAPH_HEADER_SIZE, graph->num_chunks))\n \t\tgoto free_and_return;\n\n-\tread_chunk(cf, GRAPH_CHUNKID_OIDFANOUT, graph_read_oid_fanout, graph);\n+\tif (pair_chunk_expect(cf, GRAPH_CHUNKID_OIDFANOUT,\n+\t\t\t (const unsigned char **)&graph->chunk_oid_fanout,\n+\t\t\t 256 * sizeof(uint32_t)) < 0)\n+\t\terror(_(\"commit-graph oid fanout chunk is wrong size\"));\n \tread_chunk(cf, GRAPH_CHUNKID_OIDLOOKUP, graph_read_oid_lookup, graph);\n \tpair_chunk_unsafe(cf, GRAPH_CHUNKID_DATA, &graph->chunk_commit_data);\n \tpair_chunk_unsafe(cf, GRAPH_CHUNKID_EXTRAEDGES, &graph->chunk_extra_edges);\ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex d25bea3ec5..467b5f5e9c 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -841,6 +841,7 @@ test_expect_success 'reader notices too-small oid fanout chunk' '\n \t# otherwise we hit an earlier check\n \tcheck_corrupt_chunk OIDF clear $(printf \"000000%02x\" $(test_seq 250)) &&\n \tcat >expect.err <<-\\EOF &&\n+\terror: mismatched chunk size, got: 1000, wanted: 1024\n \terror: commit-graph oid fanout chunk is wrong size\n \terror: commit-graph is missing the OID Fanout chunk\n \tEOF\n--- >8 ---\n\nOr, quite honestly, having the \"expected_size\" parameter be required in\nthe safe version of `pair_chunk()`.\n\nThanks,\nTaylor\n\n[^1]: A non-brief review is on my to-do list for tomorrow.\n"},{"id":"483046","messageId":"ZSa1F//QwawkTxvm@nand.local","threadId":"60334","inReplyTo":"20231009210458.GF3282181@coredump.intra.peff.net","subject":"Re: [PATCH 06/20] commit-graph: check consistency of fanout table","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-10-11T14:45:43Z","receivedAt":"2023-10-11T14:45:51Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Oct 09, 2023 at 05:04:58PM -0400, Jeff King wrote:\n> We use bsearch_hash() to look up items in the oid index of a\n> commit-graph. It also has a fanout table to reduce the initial range in\n> which we'll search. But since the fanout comes from the on-disk file, a\n> corrupted or malicious file can cause us to look outside of the\n> allocated index memory.\n\nThis is all very well written and explained. The patch LGTM.\n\n> ---\n> So I actually implemented the bsearch_hash() bounds checks and wrote\n> tests for midx and idx files before realizing how they handle this. ;)\n> Which makes sense, because the usual outcome for a corrupted idx file is\n> for it to say \"non-monotonic index\", which I have seen lead to user\n> confusion. Arguably we should have it say something about \"hey, your idx\n> file seems to be corrupted, because...\". But that can be its own topic.\n\nYeah, I definitely agree that that is out of scope here, and can be left\nas #leftoverbits.\n\nThanks,\nTaylor\n"},{"id":"483048","messageId":"ZSa2nbVDTXvFZvSx@nand.local","threadId":"60334","inReplyTo":"20231009210514.GG3282181@coredump.intra.peff.net","subject":"Re: [PATCH 07/20] midx: check size of pack names chunk","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-10-11T14:52:13Z","receivedAt":"2023-10-11T14:52:22Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Oct 09, 2023 at 05:05:14PM -0400, Jeff King wrote:\n> @@ -176,9 +176,16 @@ struct multi_pack_index *load_multi_pack_index(const char *object_dir, int local\n>\n> \tcur_pack_name = (const char *)m->chunk_pack_names;\n> \tfor (i = 0; i < m->num_packs; i++) {\n> +\t\tconst char *end;\n> +\t\tsize_t avail = m->chunk_pack_names_len -\n> +\t\t\t\t(cur_pack_name - (const char *)m->chunk_pack_names);\n> +\n\nThis patch all looks good to me, but reading this hunk gave me a little\nbit of pause. I was wondering what might happen if chunk_pack_names_len\nwas zero, and subtracting some other non-zero size_t from it might cause\nus to wrap around.\n\nBut I think that's a non-issue here, since we'd set cur_pack_name to the\nbeginning of the chunk, and compute avail as 0 - (m->chunk_pack_names -\nm->chunk_pack_names), and get 0 back, causing our memchr() lookup below\nto fail, and for us to report this chunk is garbage.\n\nAnd since cur_pack_name monotonically increases, I think that there is\nan inductive argument to be made that this subtraction is always safe to\ndo. But it couldn't hurt to do something like:\n\n size_t read = cur_pack_name - (const char *)m->chunk_pack_names;\n size_t avail = m->chunk_pack_names_len;\n\n if (read > avail)\n die(\"...\");\n avail -= read;\n\nto make absolutely sure that we would never underflow here.\n\nThanks,\nTaylor\n"},{"id":"483049","messageId":"ZSa3m7yyf/BVNKxb@nand.local","threadId":"60334","inReplyTo":"20231009210523.GH3282181@coredump.intra.peff.net","subject":"Re: [PATCH 08/20] midx: enforce chunk alignment on reading","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-10-11T14:56:27Z","receivedAt":"2023-10-11T14:56:47Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Oct 09, 2023 at 05:05:23PM -0400, Jeff King wrote:\n> ---\n> chunk-format.c | 8 +++++++-\n> chunk-format.h | 3 ++-\n> commit-graph.c | 2 +-\n> midx.c | 3 ++-\n> t/t5319-multi-pack-index.sh | 14 ++++++++++++++\n> 5 files changed, 26 insertions(+), 4 deletions(-)\n\nVery nicely written and explained. I agree that the choice you made here\n(to validate the alignment in the chunk-format API itself when reading\nthe table of contents) feels more sensible and places less burden on the\ncallers.\n\nLGTM, let's keep reading...\n\nThanks,\nTaylor\n"},{"id":"483050","messageId":"ZSa4tfG2NyM1s9Jz@nand.local","threadId":"60334","inReplyTo":"20231009210523.GH3282181@coredump.intra.peff.net","subject":"Re: [PATCH 08/20] midx: enforce chunk alignment on reading","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-10-11T15:01:09Z","receivedAt":"2023-10-11T15:01:21Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Oct 09, 2023 at 05:05:23PM -0400, Jeff King wrote:\n> @@ -120,6 +121,11 @@ int read_table_of_contents(struct chunkfile *cf,\n> \t\t\terror(_(\"terminating chunk id appears earlier than expected\"));\n> \t\t\treturn 1;\n> \t\t}\n> +\t\tif (chunk_offset % expected_alignment != 0) {\n\nOops, I missed this in my first read. I'm definitely nit-picking here,\nbut this should probably be:\n\n if (chunk_offset % expected_alignment)\n\nwithout the trailing \"!= 0\".\n\nI don't have a strong preference here, since we are doing a comparison\nof an arithmetic operation against an (un-)expected value. But I think\nthe CodingGuidelines would technically call this out of style...\n\n> +\t\t\terror(_(\"chunk id %\"PRIx32\" not %d-byte aligned\"),\n> +\t\t\t chunk_id, expected_alignment);\n> +\t\t\treturn 1;\n> +\t\t}\n>\n> \t\ttable_of_contents += CHUNK_TOC_ENTRY_SIZE;\n> \t\tnext_chunk_offset = get_be64(table_of_contents + 4);\n\nThanks,\nTaylor\n"},{"id":"483068","messageId":"ZSbqFcoxUJUqC6qz@nand.local","threadId":"60334","inReplyTo":"20231009210527.GI3282181@coredump.intra.peff.net","subject":"Re: [PATCH 09/20] midx: check size of object offset chunk","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-10-11T18:31:49Z","receivedAt":"2023-10-11T18:31:57Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Oct 09, 2023 at 05:05:27PM -0400, Jeff King wrote:\n> @@ -88,6 +88,19 @@ static int midx_read_oid_lookup(const unsigned char *chunk_start,\n> \treturn 0;\n> }\n>\n> +static int midx_read_object_offsets(const unsigned char *chunk_start,\n> +\t\t\t\t size_t chunk_size, void *data)\n> +{\n> +\tstruct multi_pack_index *m = data;\n> +\tm->chunk_object_offsets = chunk_start;\n> +\n> +\tif (chunk_size != st_mult(m->num_objects, MIDX_CHUNK_OFFSET_WIDTH)) {\n> +\t\terror(_(\"multi-pack-index object offset chunk is the wrong size\"));\n> +\t\treturn 1;\n> +\t}\n> +\treturn 0;\n> +}\n\nMakes sense, and the (elided) test below looks good, too. I think that\nthis is another case that would benefit from having the chunk-format API\ntake in an \"expected size\" and validate that the requested chunk is\nactually that size before assigning its address to some pointer.\n\nThanks,\nTaylor\n"},{"id":"483069","messageId":"ZSbrj3hmm8H7ce2l@nand.local","threadId":"60334","inReplyTo":"20231009210530.GJ3282181@coredump.intra.peff.net","subject":"Re: [PATCH 10/20] midx: bounds-check large offset chunk","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-10-11T18:38:07Z","receivedAt":"2023-10-11T18:38:13Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Oct 09, 2023 at 05:05:30PM -0400, Jeff King wrote:\n> When we see a large offset bit in the regular midx offset table, we\n> use the entry as an index into a separate large offset table (just like\n> a pack idx does). But we don't bounds-check the access to that large\n> offset table (nor even record its size when we parse the chunk!).\n>\n> The equivalent code for a regular pack idx is in check_pack_index_ptr().\n> But things are a bit simpler here because of the chunked format: we can\n> just check our array index directly.\n>\n> As a bonus, we can get rid of the st_mult() here. If our array\n> bounds-check is successful, then we know that the result will fit in a\n> size_t (and the bounds check uses a division to avoid overflow\n> entirely).\n>\n> Signed-off-by: Jeff King \n> ---\n> midx.c | 8 +++++---\n> midx.h | 1 +\n> t/t5319-multi-pack-index.sh | 20 ++++++++++++++++++++\n> 3 files changed, 26 insertions(+), 3 deletions(-)\n>\n> diff --git a/midx.c b/midx.c\n> index 7b1b45f381..3e768d0df0 100644\n> --- a/midx.c\n> +++ b/midx.c\n> @@ -180,7 +180,8 @@ struct multi_pack_index *load_multi_pack_index(const char *object_dir, int local\n> \tif (read_chunk(cf, MIDX_CHUNKID_OBJECTOFFSETS, midx_read_object_offsets, m))\n> \t\tdie(_(\"multi-pack-index required object offsets chunk missing or corrupted\"));\n>\n> -\tpair_chunk_unsafe(cf, MIDX_CHUNKID_LARGEOFFSETS, &m->chunk_large_offsets);\n> +\tpair_chunk(cf, MIDX_CHUNKID_LARGEOFFSETS, &m->chunk_large_offsets,\n> +\t\t &m->chunk_large_offsets_len);\n>\n> \tif (git_env_bool(\"GIT_TEST_MIDX_READ_RIDX\", 1))\n> \t\tpair_chunk_unsafe(cf, MIDX_CHUNKID_REVINDEX, &m->chunk_revindex);\n> @@ -303,8 +304,9 @@ 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 +\n> -\t\t\t\tst_mult(sizeof(uint64_t), offset32));\n> +\t\tif (offset32 >= m->chunk_large_offsets_len / sizeof(uint64_t))\n> +\t\t\tdie(_(\"multi-pack-index large offset out of bounds\"));\n> +\t\treturn get_be64(m->chunk_large_offsets + sizeof(uint64_t) * offset32);\n\nMakes sense, and this seems very reasonable. I had a couple of thoughts\non this hunk, one nitpick, and one non-nitpick ;-).\n\nThe nitpick is the easy one, which is that I typically think of scaling\nsome index to produce an offset into some chunk, instead of dividing and\ngoing the other way.\n\nSo I probably would have written something like:\n\n if (st_mult(offset32, sizeof(uint64_t)) >= m->chunk_large_offsets_len)\n die(_(\"multi-pack-index large offset out of bounds\"));\n\nBut that is definitely a subjective/minor point, and I would not at all\nbe sad if you felt differently about it. That said, I do wish for a\nlittle more information in the die() message, perhaps:\n\n if (st_mult(offset32, sizeof(uint64_t)) >= m->chunk_large_offsets_len)\n die(_(\"multi-pack-index large offset for %s out of bounds \"\n \"(%\"PRIuMAX\" is beyond %\"PRIuMAX\")\"),\n (uintmax_t)(offset32 * sizeof(uint64_t)), /* checked earlier */\n (uintmax_t)m->chunk_large_offsets_len);\n\nI can imagine that for debugging corrupt MIDXs in the future, having\nsome extra information like the above would give us a better starting\npoint than popping into a debugger to get the same information.\n\nThanks,\nTaylor\n"},{"id":"483070","messageId":"ZSbsUQyA71AGMkhl@nand.local","threadId":"60334","inReplyTo":"20231009210533.GK3282181@coredump.intra.peff.net","subject":"Re: [PATCH 11/20] midx: check size of revindex chunk","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-10-11T18:41:21Z","receivedAt":"2023-10-11T18:41:29Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Oct 09, 2023 at 05:05:33PM -0400, Jeff King wrote:\n> +static int can_use_midx_ridx_chunk(struct multi_pack_index *m)\n> +{\n> +\tif (!m->chunk_revindex)\n> +\t\treturn 0;\n> +\tif (m->chunk_revindex_len != st_mult(sizeof(uint32_t), m->num_objects)) {\n> +\t\terror(_(\"multi-pack-index reverse-index chunk is the wrong size\"));\n> +\t\treturn 0;\n> +\t}\n> +\treturn 1;\n> +}\n\nThis all looks good to me. I was going to suggest that it might be worth\njust NULL-ing out the chunk_revindex field here altogether. I think that\nwe have some prior art for doing that in the commit-graph code (though\nI'd have to look to be sure...), but that's all within the same\ncompilation unit.\n\nHaving the pack-revindex machinery NULL out a field of the MIDX\nstructure feels icky to me, so I think that having a \"can we use this\nfunction?\" instead makes much more sense.\n\nThanks,\nTaylor\n"},{"id":"483071","messageId":"ZSbthK5919QdrEPx@nand.local","threadId":"60334","inReplyTo":"20231009210536.GL3282181@coredump.intra.peff.net","subject":"Re: [PATCH 12/20] commit-graph: check size of commit data chunk","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-10-11T18:46:28Z","receivedAt":"2023-10-11T18:46:34Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Oct 09, 2023 at 05:05:36PM -0400, Jeff King wrote:\n> We expect a commit-graph file to have a fixed-size data record for each\n> commit in the file (and we know the number of commits to expct from the\n> size of the lookup table). If we encounter a file where this is too\n> small, we'll look past the end of the chunk (and possibly even off the\n> mapped memory).\n>\n> We can fix this by checking the size up front when we record the\n> pointer.\n>\n> The included test doesn't segfault, since it ends up reading bytes\n> from another chunk. But it produces nonsense results, since the values\n> it reads are garbage. Our test notices this by comparing the output to a\n> non-corrupted run of the same command (and of course we also check that\n> the expected error is printed to stderr).\n>\n> Signed-off-by: Jeff King \n> ---\n> commit-graph.c | 12 +++++++++++-\n> t/t5318-commit-graph.sh | 9 +++++++++\n> 2 files changed, 20 insertions(+), 1 deletion(-)\n>\n> diff --git a/commit-graph.c b/commit-graph.c\n> index 472332f603..9b80bbd75b 100644\n> --- a/commit-graph.c\n> +++ b/commit-graph.c\n> @@ -340,6 +340,16 @@ static int graph_read_oid_lookup(const unsigned char *chunk_start,\n> \treturn 0;\n> }\n>\n> +static int graph_read_commit_data(const unsigned char *chunk_start,\n> +\t\t\t\t size_t chunk_size, void *data)\n> +{\n> +\tstruct commit_graph *g = data;\n> +\tif (chunk_size != g->num_commits * GRAPH_DATA_WIDTH)\n\nShould this be guarded with an st_mult? I think that GRAPH_DATA_WIDTH is\ndefined as (the_hash_algo->rawsz + 16), so I *think* that the expression\nin the parenthesis would get done as a size_t, and then g->num_commits\nwould be widened to a size_t for the purposes of evaluating this\nexpression.\n\nSo I think that this is all OK in the sense that we'd never underflow\nthe 64-bit space, and having more than 2^64-1/36 (some eighteen\nquintillion) commits is... unlikely ;-).\n\nBut it may be worth wrapping this computation in an st_mult() anyway to\navoid future readers having to think about this.\n\n> +\t\treturn error(\"commit-graph commit data chunk is wrong size\");\n> +\tg->chunk_commit_data = chunk_start;\n> +\treturn 0;\n> +}\n> +\n> static int graph_read_bloom_data(const unsigned char *chunk_start,\n> \t\t\t\t size_t chunk_size, void *data)\n> {\n> @@ -422,7 +432,7 @@ struct commit_graph *parse_commit_graph(struct repo_settings *s,\n>\n> \tread_chunk(cf, GRAPH_CHUNKID_OIDFANOUT, graph_read_oid_fanout, graph);\n> \tread_chunk(cf, GRAPH_CHUNKID_OIDLOOKUP, graph_read_oid_lookup, graph);\n> -\tpair_chunk_unsafe(cf, GRAPH_CHUNKID_DATA, &graph->chunk_commit_data);\n> +\tread_chunk(cf, GRAPH_CHUNKID_DATA, graph_read_commit_data, graph);\n\nHere again would be a good use-case for a `pair_chunk_expect()`\nfunction, but I don't want to beat a dead horse ;-).\n\nThanks,\nTaylor\n"},{"id":"483072","messageId":"ZSbxM1XEy/n8/8lY@nand.local","threadId":"60334","inReplyTo":"20231009210538.GM3282181@coredump.intra.peff.net","subject":"Re: [PATCH 13/20] commit-graph: detect out-of-bounds extra-edges pointers","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-10-11T19:02:11Z","receivedAt":"2023-10-11T19:02:17Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Oct 09, 2023 at 05:05:38PM -0400, Jeff King wrote:\n> ---\n> commit-graph.c | 20 ++++++++++++++------\n> commit-graph.h | 1 +\n> t/t5318-commit-graph.sh | 8 ++++++++\n> 3 files changed, 23 insertions(+), 6 deletions(-)\n\nAll looks good here, and the proposed log message is very clear and\nwell-written. One minor note below...\n> @@ -931,14 +932,21 @@ static int fill_commit_in_graph(struct repository *r,\n> \t\treturn 1;\n> \t}\n>\n> -\tparent_data_ptr = (uint32_t*)(g->chunk_extra_edges +\n> -\t\t\t st_mult(4, edge_value & GRAPH_EDGE_LAST_MASK));\n> +\tparent_data_pos = edge_value & GRAPH_EDGE_LAST_MASK;\n> \tdo {\n> -\t\tedge_value = get_be32(parent_data_ptr);\n> +\t\tif (g->chunk_extra_edges_size / sizeof(uint32_t) <= parent_data_pos) {\n> +\t\t\terror(\"commit-graph extra-edges pointer out of bounds\");\n\nIt might be nice to add some extra data here, too, like which commit OID\nwe were trying to load, the offset we were supposed to read at, and the\nsize of the extra edges chunk itself.\n\nWe should probably also mark this string for translation, but both are\nminor points.\n\nThanks,\nTaylor\n"},{"id":"483073","messageId":"ZSbx6ZqgEJQ9ikPJ@nand.local","threadId":"60334","inReplyTo":"20231009210541.GN3282181@coredump.intra.peff.net","subject":"Re: [PATCH 14/20] commit-graph: bounds-check base graphs chunk","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-10-11T19:05:13Z","receivedAt":"2023-10-11T19:05:18Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Oct 09, 2023 at 05:05:41PM -0400, Jeff King wrote:\n> When we are loading a commit-graph chain, we check that each slice of the\n> chain points to the appropriate set of base graphs via its BASE chunk.\n> But since we don't record the size of the chunk, we may access\n> out-of-bounds memory if the file is corrupted.\n>\n> Since we know the number of entries we expect to find (based on the\n> position within the commit-graph-chain file), we can just check the size\n> up front.\n>\n> In theory this would also let us drop the st_mult() call a few lines\n> later when we actually access the memory, since we know that the\n> computed offset will fit in a size_t. But because the operands\n> \"g->hash_len\" and \"n\" have types \"unsigned char\" and \"int\", we'd have to\n> cast to size_t first. Leaving the st_mult() does that cast, and makes it\n> more obvious that we don't have an overflow problem.\n>\n> Note that the test does not actually segfault before this patch, since\n> it just reads garbage from the chunk after BASE (and indeed, it even\n> rejects the file because that garbage does not have the expected hash\n> value). You could construct a file with BASE at the end that did\n> segfault, but corrupting the existing one is easy, and we can check\n> stderr for the expected message.\n>\n> Signed-off-by: Jeff King \n> ---\n> commit-graph.c | 8 +++++++-\n> commit-graph.h | 1 +\n> t/t5324-split-commit-graph.sh | 14 ++++++++++++++\n> 3 files changed, 22 insertions(+), 1 deletion(-)\n>\n> diff --git a/commit-graph.c b/commit-graph.c\n> index e4860841fc..4377b547c8 100644\n> --- a/commit-graph.c\n> +++ b/commit-graph.c\n> @@ -435,7 +435,8 @@ struct commit_graph *parse_commit_graph(struct repo_settings *s,\n> \tread_chunk(cf, GRAPH_CHUNKID_DATA, graph_read_commit_data, graph);\n> \tpair_chunk(cf, GRAPH_CHUNKID_EXTRAEDGES, &graph->chunk_extra_edges,\n> \t\t &graph->chunk_extra_edges_size);\n> -\tpair_chunk_unsafe(cf, GRAPH_CHUNKID_BASE, &graph->chunk_base_graphs);\n> +\tpair_chunk(cf, GRAPH_CHUNKID_BASE, &graph->chunk_base_graphs,\n> +\t\t &graph->chunk_base_graphs_size);\n>\n> \tif (s->commit_graph_generation_version >= 2) {\n> \t\tpair_chunk_unsafe(cf, GRAPH_CHUNKID_GENERATION_DATA,\n> @@ -546,6 +547,11 @@ static int add_graph_to_chain(struct commit_graph *g,\n> \t\treturn 0;\n> \t}\n>\n> +\tif (g->chunk_base_graphs_size / g->hash_len < n) {\n> +\t\twarning(_(\"commit-graph base graphs chunk is too small\"));\n> +\t\treturn 0;\n> +\t}\n> +\n\nNice. Here's a spot where we would not benefit from a function like\n`pair_chunk_expect()`, since we don't know about the chain when we are\nparsing an individual layer of it. So storing the length off to the side\nand checking it within `add_graph_to_chain()` makes sense.\n\nThanks,\nTaylor\n"},{"id":"483075","messageId":"ZSbzf0o7r5GWAZFF@nand.local","threadId":"60334","inReplyTo":"20231009210550.GQ3282181@coredump.intra.peff.net","subject":"Re: [PATCH 17/20] commit-graph: check bounds when accessing BDAT chunk","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-10-11T19:11:59Z","receivedAt":"2023-10-11T19:12:06Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Oct 09, 2023 at 05:05:50PM -0400, Jeff King wrote:\n> ---\n> bloom.c | 24 ++++++++++++++++++++++++\n> commit-graph.c | 10 ++++++++++\n> commit-graph.h | 1 +\n> t/t4216-log-bloom.sh | 28 ++++++++++++++++++++++++++++\n> 4 files changed, 63 insertions(+)\n>\n> diff --git a/bloom.c b/bloom.c\n> index aef6b5fea2..61abad7f8c 100644\n> --- a/bloom.c\n> +++ b/bloom.c\n> @@ -29,6 +29,26 @@ static inline unsigned char get_bitmask(uint32_t pos)\n> \treturn ((unsigned char)1) << (pos & (BITS_PER_WORD - 1));\n> }\n>\n> +static int check_bloom_offset(struct commit_graph *g, uint32_t pos,\n> +\t\t\t uint32_t offset)\n> +{\n> +\t/*\n> +\t * Note that we allow offsets equal to the data size, which would set\n> +\t * our pointers at one past the end of the chunk memory. This is\n> +\t * necessary because the on-disk index points to the end of the\n> +\t * entries (so we can compute size by comparing adjacent ones). And\n> +\t * naturally the final entry's end is one-past-the-end of the chunk.\n> +\t */\n> +\tif (offset <= g->chunk_bloom_data_size - BLOOMDATA_CHUNK_HEADER_SIZE)\n> +\t\treturn 0;\n> +\n> +\twarning(\"ignoring out-of-range offset (%\"PRIuMAX\") for changed-path\"\n> +\t\t\" filter at pos %\"PRIuMAX\" of %s (chunk size: %\"PRIuMAX\")\",\n> +\t\t(uintmax_t)offset, (uintmax_t)pos,\n> +\t\tg->filename, (uintmax_t)g->chunk_bloom_data_size);\n\nShould this be marked for translation?\n\n> +\treturn -1;\n> +}\n> +\n> static int load_bloom_filter_from_graph(struct commit_graph *g,\n> \t\t\t\t\tstruct bloom_filter *filter,\n> \t\t\t\t\tuint32_t graph_pos)\n> @@ -51,6 +71,10 @@ static int load_bloom_filter_from_graph(struct commit_graph *g,\n> \telse\n> \t\tstart_index = 0;\n>\n> +\tif (check_bloom_offset(g, lex_pos, end_index) < 0 ||\n> +\t check_bloom_offset(g, lex_pos - 1, start_index) < 0)\n\nCan lex_pos ever be zero? I can't think of any reason that it couldn't,\nand indeed the (elided) diff context shows that immediately preceding\nthis if-statement is another that checks \"if (lex_pos > 0)\".\n\nSo I think we'd want to avoid checking lex_pos - 1 if we know that\nlex_pos is zero. Not that any of this really matters, since the only\nthing we use the computed value for is showing the graph position in the\nwarning() message. So seeing a bogus value there isn't the end of the\nworld.\n\nBut avoiding this check when lex_pos == 0 is straightforward, so is\nprobably worth doing.\n\n(As an aside, we should be able to simplify that if statement to just\n\"(if lex_pos)\", since lex_pos will never be negative).\n\n> +\t\treturn 0;\n> +\n> \tfilter->len = end_index - start_index;\n> \tfilter->data = (unsigned char *)(g->chunk_bloom_data +\n> \t\t\t\t\tsizeof(unsigned char) * start_index +\n> diff --git a/commit-graph.c b/commit-graph.c\n> index f446e76c28..f7a42be6d0 100644\n> --- a/commit-graph.c\n> +++ b/commit-graph.c\n> @@ -365,7 +365,17 @@ static int graph_read_bloom_data(const unsigned char *chunk_start,\n> {\n> \tstruct commit_graph *g = data;\n> \tuint32_t hash_version;\n> +\n> +\tif (chunk_size < BLOOMDATA_CHUNK_HEADER_SIZE) {\n> +\t\twarning(\"ignoring too-small changed-path chunk\"\n> +\t\t\t\" (%\"PRIuMAX\" < %\"PRIuMAX\") in commit-graph file\",\n> +\t\t\t(uintmax_t)chunk_size,\n> +\t\t\t(uintmax_t)BLOOMDATA_CHUNK_HEADER_SIZE);\n\nDitto on the translation suggestion from above.\n\nThanks,\nTaylor\n"},{"id":"483077","messageId":"ZSb0ZzvW9sYr3/kX@nand.local","threadId":"60334","inReplyTo":"20231009210553.GR3282181@coredump.intra.peff.net","subject":"Re: [PATCH 18/20] commit-graph: check bounds when accessing BIDX chunk","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-10-11T19:15:51Z","receivedAt":"2023-10-11T19:15:59Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Oct 09, 2023 at 05:05:53PM -0400, Jeff King wrote:\n> We load the bloom_filter_indexes chunk using pair_chunk(), so we have no\n> idea how big it is. This can lead to out-of-bounds reads if it is\n> smaller than expected, since we index it based on the number of commits\n> found elsewhere in the graph file.\n>\n> We can check the chunk size up front, like we do for CDAT and other\n> chunks with one fixed-size record per commit.\n>\n> The test case demonstrates the problem. It actually won't segfault,\n> because we end up reading random data from the follow-on chunk (BDAT in\n> this case), and the bounds checks added in the previous patch complain.\n> But this is by no means assured, and you can craft a commit-graph file\n> with BIDX at the end (or a smaller BDAT) that does segfault.\n>\n> Signed-off-by: Jeff King \n> ---\n> commit-graph.c | 16 ++++++++++++++--\n> t/t4216-log-bloom.sh | 9 +++++++++\n> 2 files changed, 23 insertions(+), 2 deletions(-)\n>\n> diff --git a/commit-graph.c b/commit-graph.c\n> index f7a42be6d0..1f334987b5 100644\n> --- a/commit-graph.c\n> +++ b/commit-graph.c\n> @@ -360,6 +360,18 @@ static int graph_read_generation_data(const unsigned char *chunk_start,\n> \treturn 0;\n> }\n>\n> +static int graph_read_bloom_index(const unsigned char *chunk_start,\n> +\t\t\t\t size_t chunk_size, void *data)\n> +{\n> +\tstruct commit_graph *g = data;\n> +\tif (chunk_size != g->num_commits * 4) {\n\nHere we probably should wrap `g->num_commits * 4` in an st_mult() call.\nHaving 4B commits is probably pretty unlikely in practice, but we have\ndefinitely seen issues in the wild at GitHub where we have more than\n2^32-1/20 commits in a single network.git.\n\n> +\t\twarning(\"commit-graph changed-path index chunk is too small\");\n\nShould this be marked for translation?\n\n> +\t\treturn -1;\n> +\t}\n> +\tg->chunk_bloom_indexes = chunk_start;\n> +\treturn 0;\n> +}\n> +\n> static int graph_read_bloom_data(const unsigned char *chunk_start,\n> \t\t\t\t size_t chunk_size, void *data)\n> {\n> @@ -470,8 +482,8 @@ struct commit_graph *parse_commit_graph(struct repo_settings *s,\n> \t}\n>\n> \tif (s->commit_graph_read_changed_paths) {\n> -\t\tpair_chunk_unsafe(cf, GRAPH_CHUNKID_BLOOMINDEXES,\n> -\t\t\t &graph->chunk_bloom_indexes);\n> +\t\tread_chunk(cf, GRAPH_CHUNKID_BLOOMINDEXES,\n> +\t\t\t graph_read_bloom_index, graph);\n\nSince we know g->num_commits by this point, this would be another spot\nthat would benefit from a `pair_chunk_expect()` function. But, again,\nnot trying to beat a dead horse here or anything ;-).\n\nThanks,\nTaylor\n"},{"id":"483078","messageId":"ZSb0iCFxYL3nymvJ@nand.local","threadId":"60334","inReplyTo":"20231009210556.GS3282181@coredump.intra.peff.net","subject":"Re: [PATCH 19/20] commit-graph: detect out-of-order BIDX offsets","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-10-11T19:16:24Z","receivedAt":"2023-10-11T19:16:29Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Oct 09, 2023 at 05:05:56PM -0400, Jeff King wrote:\n> diff --git a/bloom.c b/bloom.c\n> index 61abad7f8c..1474aa19fa 100644\n> --- a/bloom.c\n> +++ b/bloom.c\n> @@ -75,6 +75,16 @@ static int load_bloom_filter_from_graph(struct commit_graph *g,\n> \t check_bloom_offset(g, lex_pos - 1, start_index) < 0)\n> \t\treturn 0;\n>\n> +\tif (end_index < start_index) {\n> +\t\twarning(\"ignoring decreasing changed-path index offsets\"\n> +\t\t\t\" (%\"PRIuMAX\" > %\"PRIuMAX\") for positions\"\n> +\t\t\t\" %\"PRIuMAX\" and %\"PRIuMAX\" of %s\",\n\nShould this be marked for translation?\n\nOtherwise this LGTM.\n\nThanks,\nTaylor\n"},{"id":"483079","messageId":"ZSb1QAaLX+xcZK4a@nand.local","threadId":"60334","inReplyTo":"20231009205544.GA3281950@coredump.intra.peff.net","subject":"Re: [PATCH 0/20] bounds-checks for chunk-based files","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-10-11T19:19:28Z","receivedAt":"2023-10-11T19:19:34Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Oct 09, 2023 at 04:55:44PM -0400, Jeff King wrote:\n> bloom.c | 34 +++++++++\n> chunk-format.c | 24 ++++--\n> chunk-format.h | 9 ++-\n> commit-graph.c | 119 ++++++++++++++++++++++++-----\n> commit-graph.h | 4 +\n> midx.c | 68 +++++++++++++----\n> midx.h | 3 +\n> pack-revindex.c | 13 +++-\n> t/lib-chunk.sh | 17 +++++\n> t/lib-chunk/corrupt-chunk-file.pl | 66 ++++++++++++++++\n> t/t4216-log-bloom.sh | 50 ++++++++++++\n> t/t5318-commit-graph.sh | 76 +++++++++++++++++-\n> t/t5319-multi-pack-index.sh | 102 ++++++++++++++++++++++++-\n> t/t5324-split-commit-graph.sh | 20 ++++-\n> t/t5328-commit-graph-64bit-time.sh | 10 +++\n> 15 files changed, 568 insertions(+), 47 deletions(-)\n> create mode 100644 t/lib-chunk.sh\n> create mode 100644 t/lib-chunk/corrupt-chunk-file.pl\n\nI reviewed this carefully (well, except for the new Perl script, for\nobvious[^1] reasons ;-)).\n\nEverything mostly looks good to me, though I\nhad a handful of review comments throughout. Many of them are trivial\n(e.g. a number of warning() and error() strings should be marked for\ntranslation, etc.), but a couple of them I think are worth looking at.\n\nMost notably, I think that by the end of the series, I was convinced\nthat having some kind of 'pair_chunk_expectsz()' or similar would be\nuseful and eliminate a good chunk of the boilerplate you have to write\nto check the chunk size against an expected value when using\nread_chunk().\n\nOtherwise, this looks great. I appreciate the care you took in finding\nand fixing these issues, as well as thoroughly documenting the process\n(and the security implications, or lack thereof). Thanks for working on\nthis!\n\nThanks,\nTaylor\n\n[^1]: That I may be the world's least competent Perl programmer.\n"},{"id":"483107","messageId":"20231011224914.GF518221@coredump.intra.peff.net","threadId":"60334","inReplyTo":"ZSXiJZFnmpzAmuwx@nand.local","subject":"Re: [PATCH 01/20] chunk-format: note that pair_chunk() is unsafe","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-10-11T22:49:14Z","receivedAt":"2023-10-11T22:49:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 10, 2023 at 07:45:41PM -0400, Taylor Blau wrote:\n\n> On Mon, Oct 09, 2023 at 04:58:23PM -0400, Jeff King wrote:\n> > There are no callers of the \"safe\" version yet, but we'll add some in\n> > subsequent patches.\n> \n> Makes sense.\n> \n> > +int pair_chunk_unsafe(struct chunkfile *cf,\n> > +\t\t uint32_t chunk_id,\n> > +\t\t const unsigned char **p)\n> > {\n> > -\treturn read_chunk(cf, chunk_id, pair_chunk_fn, p);\n> > +\tsize_t dummy;\n> > +\treturn pair_chunk(cf, chunk_id, p, &dummy);\n> \n> I have always disliked functions that require you to pass a non-NULL\n> pointer to some value that you may or may not want to have that function\n> fill out. So I was going to suggest something along the lines of\n> \"pair_chunk() should tolerate a NULL fourth argument instead of filling\n> out the size unconditionally\".\n> \n> But that's (a) the whole point of the series ;-), and (b) unnecessary,\n> since this function is going to go away entirely by the end of the\n> series, too.\n\nYeah, for the record, I think a dummy variable like this is usually a\ncode smell. And it truly is a \"problem\" here, because we are\nintentionally doing the unsafe and stupid thing. :)\n\n-Peff\n"},{"id":"483109","messageId":"20231011225243.GG518221@coredump.intra.peff.net","threadId":"60334","inReplyTo":"ZSXjR6BjvaokNwPe@nand.local","subject":"Re: [PATCH 03/20] midx: stop ignoring malformed oid fanout chunk","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-10-11T22:52:43Z","receivedAt":"2023-10-11T22:52:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 10, 2023 at 07:50:31PM -0400, Taylor Blau wrote:\n\n> All makes sense. I have a mild preference for \"missing or corrupt\" over\n> \"missing or corrupted\", but it's mild enough that I wouldn't be sad if\n> you kept it as-is ;-).\n\nHmm, now I wonder which is grammatically more appropriate.\n\nAccording to the internet (which is never wrong), the distinction is\ngenerally one of intrinsic corrupt state versus something that has\nbecome corrupted. So I guess it depends on whether the file was created\ncorrupt by a bug or corrupted by a hard disk failure. ;)\n\nI do not have a strong preference myself. The commits went into next\nalready, but I think you also pointed out that many of these could be\nmarked for translation (though this one is already).\n\nSo we could do one or two patches to adjust the error messages on top.\n\n-Peff\n"},{"id":"483110","messageId":"20231011230106.GH518221@coredump.intra.peff.net","threadId":"60334","inReplyTo":"ZSXnZXglgbfK3VYd@nand.local","subject":"Re: [PATCH 04/20] commit-graph: check size of oid fanout chunk","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-10-11T23:01:06Z","receivedAt":"2023-10-11T23:01:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 10, 2023 at 08:08:05PM -0400, Taylor Blau wrote:\n\n> Nice. This makes sense and seems like an obvious improvement over the\n> existing code.\n> \n> I wonder how common this pattern is. We have read_chunk() which is for\n> handling more complex scenarios than this. But the safe version of\n> pair_chunk() really just wants to check that the size of the chunk is as\n> expected and assign the location in the mmap to some pointer.\n\nSometimes yes, sometimes no. For fixed-size ones like this, that's\nsufficient. For others we have to record the size and use it for later\nbounds-checking. IIRC it's about 50/50 between the two.\n\n> Do you think it would be worth changing pair_chunk() to take an expected\n> size_t and handle this generically? I.e. have a version of\n> chunk-format::pair_chunk_fn() that looks something like:\n> \n> static int pair_chunk_fn(const unsigned char *chunk_start,\n> size_t chunk_size, void *data)\n> {\n> const unsigned char **p = data;\n> if (chunk_size != data->size)\n> return -1;\n> *p = chunk_start;\n> return 0;\n> }\n> \n> and then our call here would be:\n> \n> if (pair_chunk(cf, GRAPH_CHUNKID_OIDFANOUT,\n> (const unsigned char **)&graph->chunk_oid_fanout,\n> 256 * sizeof(uint32_t)) < 0)\n> return error(\"commit-graph oid fanout chunk is wrong size\");\n> \n> I dunno. It's hard to have a more concrete recomendation without having\n> read the rest of the series. So it's possible that this is just complete\n> nonsense ;-). But my hunch is that there are a number of callers that\n> would benefit from having this built in.\n\nI don't think it's nonsense, and I do think other callers would benefit.\nOn the other hand, I kind of like the notion that there is a complete\nvalidation callback for each of these chunks. Even though it just checks\nthe size for now, it could handle other things. In the case of OIDF, for\nexample, we can check whether the entries are monotonic. It's just that\nwe happen to do those checks elsewhere.\n\nHmm, actually, looking at that again, I think I may have missed a case\nin patch 6. For pack .idx files, we check the fanout table when they are\nloaded. And patch 6 adds the same for commit-graph files. I thought midx\nfiles were handled the same .idx, but looking at it again, I only see\nthe monotonicity check in the \"multi-pack-index verify\" code paths. So\nit might need the same treatment.\n\nI'm not sure how I missed that (I started by making a corrupted midx\nfirst and couldn't get it to fail, which is when I discovered the\nexisting checks, but maybe I am mixing up .idx and midx in my memory).\n\n-Peff\n"},{"id":"483112","messageId":"20231011230641.GI518221@coredump.intra.peff.net","threadId":"60334","inReplyTo":"ZSa2nbVDTXvFZvSx@nand.local","subject":"Re: [PATCH 07/20] midx: check size of pack names chunk","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-10-11T23:06:41Z","receivedAt":"2023-10-11T23:06:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 11, 2023 at 10:52:13AM -0400, Taylor Blau wrote:\n\n> On Mon, Oct 09, 2023 at 05:05:14PM -0400, Jeff King wrote:\n> > @@ -176,9 +176,16 @@ struct multi_pack_index *load_multi_pack_index(const char *object_dir, int local\n> >\n> > \tcur_pack_name = (const char *)m->chunk_pack_names;\n> > \tfor (i = 0; i < m->num_packs; i++) {\n> > +\t\tconst char *end;\n> > +\t\tsize_t avail = m->chunk_pack_names_len -\n> > +\t\t\t\t(cur_pack_name - (const char *)m->chunk_pack_names);\n> > +\n> \n> This patch all looks good to me, but reading this hunk gave me a little\n> bit of pause. I was wondering what might happen if chunk_pack_names_len\n> was zero, and subtracting some other non-zero size_t from it might cause\n> us to wrap around.\n> \n> But I think that's a non-issue here, since we'd set cur_pack_name to the\n> beginning of the chunk, and compute avail as 0 - (m->chunk_pack_names -\n> m->chunk_pack_names), and get 0 back, causing our memchr() lookup below\n> to fail, and for us to report this chunk is garbage.\n\nRight. If it is 0, then we should have 0 avail here in the first loop.\n\nI was more worried while writing this that:\n\n cur_pack_name = end + 1;\n\nlater in the loop could get us an off-by-one. But we know we are always\npointing to one past an available NUL there, so at most our subtraction\nwill equal m->chunk_pack_names_len.\n\n> And since cur_pack_name monotonically increases, I think that there is\n> an inductive argument to be made that this subtraction is always safe to\n> do. But it couldn't hurt to do something like:\n> \n> size_t read = cur_pack_name - (const char *)m->chunk_pack_names;\n> size_t avail = m->chunk_pack_names_len;\n> \n> if (read > avail)\n> die(\"...\");\n> avail -= read;\n> \n> to make absolutely sure that we would never underflow here.\n\nYou end up with two die() calls, then. One for \"hey, we somehow read too\nfar\", and one for \"hey, I ran out of data while reading the entry\". And\nthe first one cannot be triggered.\n\nIOW, I think your die() here is a BUG().\n\n-Peff\n"},{"id":"483113","messageId":"20231011230927.GJ518221@coredump.intra.peff.net","threadId":"60334","inReplyTo":"ZSa4tfG2NyM1s9Jz@nand.local","subject":"Re: [PATCH 08/20] midx: enforce chunk alignment on reading","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-10-11T23:09:27Z","receivedAt":"2023-10-11T23:09:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 11, 2023 at 11:01:09AM -0400, Taylor Blau wrote:\n\n> On Mon, Oct 09, 2023 at 05:05:23PM -0400, Jeff King wrote:\n> > @@ -120,6 +121,11 @@ int read_table_of_contents(struct chunkfile *cf,\n> > \t\t\terror(_(\"terminating chunk id appears earlier than expected\"));\n> > \t\t\treturn 1;\n> > \t\t}\n> > +\t\tif (chunk_offset % expected_alignment != 0) {\n> \n> Oops, I missed this in my first read. I'm definitely nit-picking here,\n> but this should probably be:\n> \n> if (chunk_offset % expected_alignment)\n> \n> without the trailing \"!= 0\".\n> \n> I don't have a strong preference here, since we are doing a comparison\n> of an arithmetic operation against an (un-)expected value. But I think\n> the CodingGuidelines would technically call this out of style...\n\nI suppose so, but somehow I consider the subtlety of \"%\" to merit the\nmore explicit comparison (versus something like \"if (foo)\"). Grepping\nfor \"if (.* %)' seems to show some of both.\n\n-Peff\n"},{"id":"483114","messageId":"20231011231840.GK518221@coredump.intra.peff.net","threadId":"60334","inReplyTo":"ZSbrj3hmm8H7ce2l@nand.local","subject":"Re: [PATCH 10/20] midx: bounds-check large offset chunk","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-10-11T23:18:40Z","receivedAt":"2023-10-11T23:18:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 11, 2023 at 02:38:07PM -0400, Taylor Blau wrote:\n\n> > \t\toffset32 ^= MIDX_LARGE_OFFSET_NEEDED;\n> > -\t\treturn get_be64(m->chunk_large_offsets +\n> > -\t\t\t\tst_mult(sizeof(uint64_t), offset32));\n> > +\t\tif (offset32 >= m->chunk_large_offsets_len / sizeof(uint64_t))\n> > +\t\t\tdie(_(\"multi-pack-index large offset out of bounds\"));\n> > +\t\treturn get_be64(m->chunk_large_offsets + sizeof(uint64_t) * offset32);\n> \n> Makes sense, and this seems very reasonable. I had a couple of thoughts\n> on this hunk, one nitpick, and one non-nitpick ;-).\n> \n> The nitpick is the easy one, which is that I typically think of scaling\n> some index to produce an offset into some chunk, instead of dividing and\n> going the other way.\n> \n> So I probably would have written something like:\n> \n> if (st_mult(offset32, sizeof(uint64_t)) >= m->chunk_large_offsets_len)\n> die(_(\"multi-pack-index large offset out of bounds\"));\n\nYeah, I admit that my inclination is to think of it that way, too, and\nthe division is a bit of an inversion. But I guess I am hard-wired to do\nbounds checks with division, because I know it avoids overflow issues.\nAnd the behavior is actually different, since st_mult() dies (with a\nsomewhat vague message) rather than returning with an error.\n\nI was actually tempted to write a small inline helper to do\nbounds-checks (since this pattern appears a few times in this series).\nBut of course it suffers from the same issues (it dies with a vague\nmessage, or it returns a result code and then is awkward to call/use\nbecause you have to stick the output in an out-parameter).\n\n> But that is definitely a subjective/minor point, and I would not at all\n> be sad if you felt differently about it. That said, I do wish for a\n> little more information in the die() message, perhaps:\n> \n> if (st_mult(offset32, sizeof(uint64_t)) >= m->chunk_large_offsets_len)\n> die(_(\"multi-pack-index large offset for %s out of bounds \"\n> \"(%\"PRIuMAX\" is beyond %\"PRIuMAX\")\"),\n> (uintmax_t)(offset32 * sizeof(uint64_t)), /* checked earlier */\n> (uintmax_t)m->chunk_large_offsets_len);\n> \n> I can imagine that for debugging corrupt MIDXs in the future, having\n> some extra information like the above would give us a better starting\n> point than popping into a debugger to get the same information.\n\nAs you'll see when you get to the BDAT/BIDX messages, I put in a load of\ndetail. That's because I did those ones first, and then after seeing the\nterse \"eh, it's the wrong size\" messages in the rest of the commit-graph\nand midx code, I just followed suit.\n\nWe can circle back and improve the detail in the messages. One slightly\nannoying thing is dealing with it in the tests. We'd have to do one of:\n\n - make the tests more brittle by hard-coding positions and offsets we\n don't necessarily care about\n\n - loosen the tests by just matching with \"grep\", which can sometimes\n miss other unexpected output\n\n - do some post-processing on the output to massage out the details we\n don't care about; this in theory is the best of both worlds, but\n reliably post-processing is non-trivial.\n\nSo of the three I'd probably just loosen to use \"grep\" in most spots.\n\n-Peff\n"},{"id":"483115","messageId":"20231011232217.GL518221@coredump.intra.peff.net","threadId":"60334","inReplyTo":"ZSbthK5919QdrEPx@nand.local","subject":"Re: [PATCH 12/20] commit-graph: check size of commit data chunk","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-10-11T23:22:17Z","receivedAt":"2023-10-11T23:22:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 11, 2023 at 02:46:28PM -0400, Taylor Blau wrote:\n\n> > +static int graph_read_commit_data(const unsigned char *chunk_start,\n> > +\t\t\t\t size_t chunk_size, void *data)\n> > +{\n> > +\tstruct commit_graph *g = data;\n> > +\tif (chunk_size != g->num_commits * GRAPH_DATA_WIDTH)\n> \n> Should this be guarded with an st_mult? I think that GRAPH_DATA_WIDTH is\n> defined as (the_hash_algo->rawsz + 16), so I *think* that the expression\n> in the parenthesis would get done as a size_t, and then g->num_commits\n> would be widened to a size_t for the purposes of evaluating this\n> expression.\n> \n> So I think that this is all OK in the sense that we'd never underflow\n> the 64-bit space, and having more than 2^64-1/36 (some eighteen\n> quintillion) commits is... unlikely ;-).\n\nHmm, yeah, I think you are right, but I agree it's awfully subtle. There\nis no reason somebody couldn't later change \"rawsz\" to a smaller type\n(after all, we know it's going to be tiny), and it would be quite\nsurprising if that introduces an overflow in far-away code. We should\nprotect ourselves here.\n\n-Peff\n"},{"id":"483117","messageId":"20231011232747.GM518221@coredump.intra.peff.net","threadId":"60334","inReplyTo":"ZSbzf0o7r5GWAZFF@nand.local","subject":"Re: [PATCH 17/20] commit-graph: check bounds when accessing BDAT chunk","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-10-11T23:27:47Z","receivedAt":"2023-10-11T23:27:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 11, 2023 at 03:11:59PM -0400, Taylor Blau wrote:\n\n> > +\tif (check_bloom_offset(g, lex_pos, end_index) < 0 ||\n> > +\t check_bloom_offset(g, lex_pos - 1, start_index) < 0)\n> \n> Can lex_pos ever be zero? I can't think of any reason that it couldn't,\n> and indeed the (elided) diff context shows that immediately preceding\n> this if-statement is another that checks \"if (lex_pos > 0)\".\n> \n> So I think we'd want to avoid checking lex_pos - 1 if we know that\n> lex_pos is zero. Not that any of this really matters, since the only\n> thing we use the computed value for is showing the graph position in the\n> warning() message. So seeing a bogus value there isn't the end of the\n> world.\n\nIf lex_pos is 0, then we set start_index to 0 manually, which we know to\nbe valid. So we can't trigger a bogus warning(). My thinking was that it\nwas OK to just let this fall out naturally as it does here, since it\nmakes the code shorter. But if we want to cover that case separately,\nI think you'd want to split the checks like:\n\ndiff --git a/bloom.c b/bloom.c\nindex 1474aa19fa..d9ad69ad82 100644\n--- a/bloom.c\n+++ b/bloom.c\n@@ -65,16 +65,16 @@ static int load_bloom_filter_from_graph(struct commit_graph *g,\n \tlex_pos = graph_pos - g->num_commits_in_base;\n \n \tend_index = get_be32(g->chunk_bloom_indexes + 4 * lex_pos);\n+\tif (check_bloom_offset(g, lex_pos, end_index) < 0)\n+\t\treturn 0;\n \n-\tif (lex_pos > 0)\n+\tif (lex_pos > 0) {\n \t\tstart_index = get_be32(g->chunk_bloom_indexes + 4 * (lex_pos - 1));\n-\telse\n+\t\tif (check_bloom_offset(g, lex_pos - 1, start_index) < 0)\n+\t\t\treturn 0;\n+\t} else\n \t\tstart_index = 0;\n \n-\tif (check_bloom_offset(g, lex_pos, end_index) < 0 ||\n-\t check_bloom_offset(g, lex_pos - 1, start_index) < 0)\n-\t\treturn 0;\n-\n \tif (end_index < start_index) {\n \t\twarning(\"ignoring decreasing changed-path index offsets\"\n \t\t\t\" (%\"PRIuMAX\" > %\"PRIuMAX\") for positions\"\n\n-Peff\n"},{"id":"483119","messageId":"20231011233121.GN518221@coredump.intra.peff.net","threadId":"60334","inReplyTo":"ZSb1QAaLX+xcZK4a@nand.local","subject":"Re: [PATCH 0/20] bounds-checks for chunk-based files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-10-11T23:31:21Z","receivedAt":"2023-10-11T23:31:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 11, 2023 at 03:19:28PM -0400, Taylor Blau wrote:\n\n> I reviewed this carefully (well, except for the new Perl script, for\n> obvious[^1] reasons ;-)).\n> \n> Everything mostly looks good to me, though I\n> had a handful of review comments throughout. Many of them are trivial\n> (e.g. a number of warning() and error() strings should be marked for\n> translation, etc.), but a couple of them I think are worth looking at.\n\nThanks for taking a look. I think it may make sense to come back on top\nand adjust a few of the commit messages, along with adding a few\nst_mult() overflow checks that you suggest.\n\n> Most notably, I think that by the end of the series, I was convinced\n> that having some kind of 'pair_chunk_expectsz()' or similar would be\n> useful and eliminate a good chunk of the boilerplate you have to write\n> to check the chunk size against an expected value when using\n> read_chunk().\n\nThis I'm less convinced by. In fact, I _almost_ just dropped\npair_chunk() entirely. Adding an out-parameter for the size at least\nforces the caller to consider what to do with the size. But really, I\nthink the right mindset is \"we should be sanity-checking this chunk as\nwe load it\". And having a callback, even if it is a little bit of\nboilerplate, helps set that frame of mind.\n\nI dunno. Maybe that is all just programmer pseudo-psychology. But I also\ndon't like that about half the calls to pair_chunk() can't do a size\ncheck (so we need two functions, or to make the \"expect\" parameter\noptional).\n\n-Peff\n"},{"id":"483217","messageId":"cover.1697225110.git.me@ttaylorr.com","threadId":"60334","inReplyTo":"20231009205544.GA3281950@coredump.intra.peff.net","subject":"[PATCH 0/8] chunk-format: introduce `pair_chunk_expect()` convenience API","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-10-13T19:25:12Z","receivedAt":"2023-10-13T19:25:18Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"While reviewing this series, I noticed a couple of spots that would be\nhelped by having a convenience function that stores the start of a\nchunk in a designated location *after* checking that the chunk has the\nexpected size.\n\nI called this function `pair_chunk_expect()` and touched up seven spots\nthat I think could benefit from having a convenience function like this.\n\nThis applies directly on top of 'jk/chunk-bounds'. Thanks in advance for\nyour review!\n\nTaylor Blau (8):\n chunk-format: introduce `pair_chunk_expect()` helper\n commit-graph: read `OIDF` chunk with `pair_chunk_expect()`\n commit-graph: read `CDAT` chunk with `pair_chunk_expect()`\n commit-graph: read `GDAT` chunk with `pair_chunk_expect()`\n commit-graph: read `BIDX` chunk with `pair_chunk_expect()`\n midx: read `OIDF` chunk with `pair_chunk_expect()`\n midx: read `OIDL` chunk with `pair_chunk_expect()`\n midx: read `OOFF` chunk with `pair_chunk_expect()`\n\n chunk-format.c | 22 +++++++++++++++++\n chunk-format.h | 12 +++++++++-\n commit-graph.c | 65 +++++++++++++-------------------------------------\n midx.c | 58 ++++++++++++--------------------------------\n 4 files changed, 65 insertions(+), 92 deletions(-)\n\n-- \n2.42.0.352.gd9c5062ff7.dirty\n"},{"id":"483218","messageId":"6fa20034b2246599b3e1cd49accfc421b0fba4fb.1697225110.git.me@ttaylorr.com","threadId":"60334","inReplyTo":"cover.1697225110.git.me@ttaylorr.com","subject":"[PATCH 1/8] chunk-format: introduce `pair_chunk_expect()` helper","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-10-13T19:25:18Z","receivedAt":"2023-10-13T19:25:22Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In previous commits, the pair_chunk() interface grew a required \"size\"\npointer, so that the caller is forced to at least have a handle on the\nactual size of the given chunk.\n\nMany callers were converted to the new interface. A handful were instead\nconverted from the unsafe version of pair_chunk() to read_chunk() so\nthat they could check their expected size.\n\nThis led to a lot of code like:\n\n static int graph_read_oid_fanout(const unsigned char *chunk_start,\n size_t chunk_size, void *data)\n {\n struct commit_graph *g = data;\n if (chunk_size != 256 * sizeof(uint32_t))\n return error(\"commit-graph oid fanout chunk is wrong size\");\n g->chunk_oid_fanout = (const uint32_t *)chunk_start;\n return 0;\n }\n\n, leaving the caller to use read_chunk(), like so:\n\n read_chunk(cf, GRAPH_CHUNKID_OIDFANOUT, graph_read_oid_fanout, graph);\n\nThe callback to read_chunk() (in the above, `graph_read_oid_fanout()`)\ndoes nothing more than (a) assign a pointer to the location of the start\nof the chunk in the mmap'd file, and (b) assert that it has the correct\nsize.\n\nFor callers that know the expected size of their chunk(s) up-front, we\ncan simplify this by teaching the chunk-format API itself to validate\nthe expected size for us.\n\nThis is wrapped in a new function, called `pair_chunk_expect()` which\ntakes a \"size_t\" instead of a \"size_t *\", and validates that the given\nchunk matches the expected size as given.\n\nThis will allow us to reduce the number of lines of code it takes to\nperform these basic read_chunk() operations, by taking the above and\nreplacing it with something like:\n\n if (pair_chunk_expect(cf, GRAPH_CHUNKID_OIDFANOUT,\n (const unsigned char **)&graph->chunk_oid_fanout,\n 256 * sizeof(uint32_t)))\n error(_(\"commit-graph oid fanout chunk is wrong size\"));\n\nWe will perform those transformations in the following commits.\n\nSigned-off-by: Taylor Blau \n---\n chunk-format.c | 22 ++++++++++++++++++++++\n chunk-format.h | 12 +++++++++++-\n 2 files changed, 33 insertions(+), 1 deletion(-)\n\ndiff --git a/chunk-format.c b/chunk-format.c\nindex cdc7f39b70..9550f15699 100644\n--- a/chunk-format.c\n+++ b/chunk-format.c\n@@ -163,6 +163,8 @@ int read_table_of_contents(struct chunkfile *cf,\n struct pair_chunk_data {\n \tconst unsigned char **p;\n \tsize_t *size;\n+\n+\tsize_t expected_size;\n };\n \n static int pair_chunk_fn(const unsigned char *chunk_start,\n@@ -175,6 +177,17 @@ static int pair_chunk_fn(const unsigned char *chunk_start,\n \treturn 0;\n }\n \n+static int pair_chunk_expect_fn(const unsigned char *chunk_start,\n+\t\t\t\tsize_t chunk_size,\n+\t\t\t\tvoid *data)\n+{\n+\tstruct pair_chunk_data *pcd = data;\n+\tif (pcd->expected_size != chunk_size)\n+\t\treturn -1;\n+\t*pcd->p = chunk_start;\n+\treturn 0;\n+}\n+\n int pair_chunk(struct chunkfile *cf,\n \t uint32_t chunk_id,\n \t const unsigned char **p,\n@@ -184,6 +197,15 @@ int pair_chunk(struct chunkfile *cf,\n \treturn read_chunk(cf, chunk_id, pair_chunk_fn, &pcd);\n }\n \n+int pair_chunk_expect(struct chunkfile *cf,\n+\t\t uint32_t chunk_id,\n+\t\t const unsigned char **p,\n+\t\t size_t expected_size)\n+{\n+\tstruct pair_chunk_data pcd = { .p = p, .expected_size = expected_size };\n+\treturn read_chunk(cf, chunk_id, pair_chunk_expect_fn, &pcd);\n+}\n+\n int read_chunk(struct chunkfile *cf,\n \t uint32_t chunk_id,\n \t chunk_read_fn fn,\ndiff --git a/chunk-format.h b/chunk-format.h\nindex 14b76180ef..92c529d7ab 100644\n--- a/chunk-format.h\n+++ b/chunk-format.h\n@@ -17,7 +17,8 @@ struct chunkfile;\n *\n * If reading a file, use a NULL 'struct hashfile *' and then call\n * read_table_of_contents(). Supply the memory-mapped data to the\n- * pair_chunk() or read_chunk() methods, as appropriate.\n+ * pair_chunk(), pair_chunk_expect(), or read_chunk() methods, as\n+ * appropriate.\n *\n * DO NOT MIX THESE MODES. Use different 'struct chunkfile' instances\n * for reading and writing.\n@@ -54,6 +55,15 @@ int pair_chunk(struct chunkfile *cf,\n \t const unsigned char **p,\n \t size_t *size);\n \n+/*\n+ * Similar to 'pair_chunk', but used for callers who have an expected\n+ * size for the given 'chunk_id' in advance.\n+ */\n+int pair_chunk_expect(struct chunkfile *cf,\n+\t\t uint32_t chunk_id,\n+\t\t const unsigned char **p,\n+\t\t size_t expected_size);\n+\n typedef int (*chunk_read_fn)(const unsigned char *chunk_start,\n \t\t\t size_t chunk_size, void *data);\n /*\n-- \n2.42.0.352.gd9c5062ff7.dirty\n\n"},{"id":"483219","messageId":"3e0969d794a775f9167d7a20fe6aeb17edc60d8a.1697225110.git.me@ttaylorr.com","threadId":"60334","inReplyTo":"cover.1697225110.git.me@ttaylorr.com","subject":"[PATCH 2/8] commit-graph: read `OIDF` chunk with `pair_chunk_expect()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-10-13T19:25:21Z","receivedAt":"2023-10-13T19:25:24Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"The `OIDF` chunk can benefit from the new chunk-format API function\ndescribed in the previous commit. Convert it to `pair_chunk_expect()`\naccordingly.\n\nWhile we're at it, let's mark the error() string for translation.\n\nSigned-off-by: Taylor Blau \n---\n commit-graph.c | 15 ++++-----------\n 1 file changed, 4 insertions(+), 11 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 1f334987b5..cdefd7f926 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -321,16 +321,6 @@ static int verify_commit_graph_lite(struct commit_graph *g)\n \treturn 0;\n }\n \n-static int graph_read_oid_fanout(const unsigned char *chunk_start,\n-\t\t\t\t size_t chunk_size, void *data)\n-{\n-\tstruct commit_graph *g = data;\n-\tif (chunk_size != 256 * sizeof(uint32_t))\n-\t\treturn error(\"commit-graph oid fanout chunk is wrong size\");\n-\tg->chunk_oid_fanout = (const uint32_t *)chunk_start;\n-\treturn 0;\n-}\n-\n static int graph_read_oid_lookup(const unsigned char *chunk_start,\n \t\t\t\t size_t chunk_size, void *data)\n {\n@@ -462,7 +452,10 @@ struct commit_graph *parse_commit_graph(struct repo_settings *s,\n \t\t\t\t GRAPH_HEADER_SIZE, graph->num_chunks, 1))\n \t\tgoto free_and_return;\n \n-\tread_chunk(cf, GRAPH_CHUNKID_OIDFANOUT, graph_read_oid_fanout, graph);\n+\tif (pair_chunk_expect(cf, GRAPH_CHUNKID_OIDFANOUT,\n+\t\t\t (const unsigned char **)&graph->chunk_oid_fanout,\n+\t\t\t 256 * sizeof(uint32_t)))\n+\t\terror(_(\"commit-graph oid fanout chunk is wrong size\"));\n \tread_chunk(cf, GRAPH_CHUNKID_OIDLOOKUP, graph_read_oid_lookup, graph);\n \tread_chunk(cf, GRAPH_CHUNKID_DATA, graph_read_commit_data, graph);\n \tpair_chunk(cf, GRAPH_CHUNKID_EXTRAEDGES, &graph->chunk_extra_edges,\n-- \n2.42.0.352.gd9c5062ff7.dirty\n\n"},{"id":"483220","messageId":"141ab00a1eec89666f3fb23e9bb428667f774e14.1697225110.git.me@ttaylorr.com","threadId":"60334","inReplyTo":"cover.1697225110.git.me@ttaylorr.com","subject":"[PATCH 3/8] commit-graph: read `CDAT` chunk with `pair_chunk_expect()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-10-13T19:25:24Z","receivedAt":"2023-10-13T19:25:27Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Perform a similar conversion as in the previous commit read the CDAT\nbits.\n\nWhile we're here, mark the error() string for translation, and guard\nagainst overflow when computing the expected size by wrapping it in an\nst_mult() call.\n\nNote that the pre-image of this patch was already sufficiently guarded\nagainst overflow, since GRAPH_DATA_WIDTH is defined as\n(the_hash_algo->rawsz + 16), so the expression in the parenthesis would\nget performed as a size_t, and then g->num_commits would be promoted to\nthe width of size_t for the purposes of evaluating this expression.\n\nBut let's make it explicitly clear that this computation is safe by\nwrapping it in an st_mult() call.\n\nSigned-off-by: Taylor Blau \n---\n commit-graph.c | 15 ++++-----------\n 1 file changed, 4 insertions(+), 11 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex cdefd7f926..97d4824673 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -330,16 +330,6 @@ static int graph_read_oid_lookup(const unsigned char *chunk_start,\n \treturn 0;\n }\n \n-static int graph_read_commit_data(const unsigned char *chunk_start,\n-\t\t\t\t size_t chunk_size, void *data)\n-{\n-\tstruct commit_graph *g = data;\n-\tif (chunk_size != g->num_commits * GRAPH_DATA_WIDTH)\n-\t\treturn error(\"commit-graph commit data chunk is wrong size\");\n-\tg->chunk_commit_data = chunk_start;\n-\treturn 0;\n-}\n-\n static int graph_read_generation_data(const unsigned char *chunk_start,\n \t\t\t\t size_t chunk_size, void *data)\n {\n@@ -457,7 +447,10 @@ struct commit_graph *parse_commit_graph(struct repo_settings *s,\n \t\t\t 256 * sizeof(uint32_t)))\n \t\terror(_(\"commit-graph oid fanout chunk is wrong size\"));\n \tread_chunk(cf, GRAPH_CHUNKID_OIDLOOKUP, graph_read_oid_lookup, graph);\n-\tread_chunk(cf, GRAPH_CHUNKID_DATA, graph_read_commit_data, graph);\n+\tif (pair_chunk_expect(cf, GRAPH_CHUNKID_DATA,\n+\t\t\t &graph->chunk_commit_data,\n+\t\t\t st_mult(graph->num_commits, GRAPH_DATA_WIDTH)))\n+\t\terror(_(\"commit-graph commit data chunk is wrong size\"));\n \tpair_chunk(cf, GRAPH_CHUNKID_EXTRAEDGES, &graph->chunk_extra_edges,\n \t\t &graph->chunk_extra_edges_size);\n \tpair_chunk(cf, GRAPH_CHUNKID_BASE, &graph->chunk_base_graphs,\n-- \n2.42.0.352.gd9c5062ff7.dirty\n\n"},{"id":"483221","messageId":"898b5bdb033518f2022d41b3cb5f03f71cc90070.1697225110.git.me@ttaylorr.com","threadId":"60334","inReplyTo":"cover.1697225110.git.me@ttaylorr.com","subject":"[PATCH 4/8] commit-graph: read `GDAT` chunk with `pair_chunk_expect()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-10-13T19:25:27Z","receivedAt":"2023-10-13T19:25:30Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Perform an identical conversion as in previous commits to read the GDAT\nchunk.\n\nSigned-off-by: Taylor Blau \n---\n commit-graph.c | 17 +++++------------\n 1 file changed, 5 insertions(+), 12 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 97d4824673..0fab99f5dd 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -330,16 +330,6 @@ static int graph_read_oid_lookup(const unsigned char *chunk_start,\n \treturn 0;\n }\n \n-static int graph_read_generation_data(const unsigned char *chunk_start,\n-\t\t\t\t size_t chunk_size, void *data)\n-{\n-\tstruct commit_graph *g = data;\n-\tif (chunk_size != g->num_commits * sizeof(uint32_t))\n-\t\treturn error(\"commit-graph generations chunk is wrong size\");\n-\tg->chunk_generation_data = chunk_start;\n-\treturn 0;\n-}\n-\n static int graph_read_bloom_index(const unsigned char *chunk_start,\n \t\t\t\t size_t chunk_size, void *data)\n {\n@@ -457,8 +447,11 @@ struct commit_graph *parse_commit_graph(struct repo_settings *s,\n \t\t &graph->chunk_base_graphs_size);\n \n \tif (s->commit_graph_generation_version >= 2) {\n-\t\tread_chunk(cf, GRAPH_CHUNKID_GENERATION_DATA,\n-\t\t\t graph_read_generation_data, graph);\n+\t\tif (pair_chunk_expect(cf, GRAPH_CHUNKID_GENERATION_DATA,\n+\t\t\t\t &graph->chunk_generation_data,\n+\t\t\t\t st_mult(graph->num_commits,\n+\t\t\t\t\t sizeof(uint32_t))))\n+\t\t\terror(_(\"commit-graph generations chunk is wrong size\"));\n \t\tpair_chunk(cf, GRAPH_CHUNKID_GENERATION_DATA_OVERFLOW,\n \t\t\t &graph->chunk_generation_data_overflow,\n \t\t\t &graph->chunk_generation_data_overflow_size);\n-- \n2.42.0.352.gd9c5062ff7.dirty\n\n"},{"id":"483222","messageId":"45cac29403e63483951f7766c6da3c022c68d9f0.1697225110.git.me@ttaylorr.com","threadId":"60334","inReplyTo":"cover.1697225110.git.me@ttaylorr.com","subject":"[PATCH 5/8] commit-graph: read `BIDX` chunk with `pair_chunk_expect()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-10-13T19:25:30Z","receivedAt":"2023-10-13T19:25:33Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Perform an identical conversion as in previous commits to read the BIDX\nchunk.\n\nSigned-off-by: Taylor Blau \n---\n commit-graph.c | 18 ++++--------------\n 1 file changed, 4 insertions(+), 14 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 0fab99f5dd..ad98f6334d 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -330,18 +330,6 @@ static int graph_read_oid_lookup(const unsigned char *chunk_start,\n \treturn 0;\n }\n \n-static int graph_read_bloom_index(const unsigned char *chunk_start,\n-\t\t\t\t size_t chunk_size, void *data)\n-{\n-\tstruct commit_graph *g = data;\n-\tif (chunk_size != g->num_commits * 4) {\n-\t\twarning(\"commit-graph changed-path index chunk is too small\");\n-\t\treturn -1;\n-\t}\n-\tg->chunk_bloom_indexes = chunk_start;\n-\treturn 0;\n-}\n-\n static int graph_read_bloom_data(const unsigned char *chunk_start,\n \t\t\t\t size_t chunk_size, void *data)\n {\n@@ -461,8 +449,10 @@ struct commit_graph *parse_commit_graph(struct repo_settings *s,\n \t}\n \n \tif (s->commit_graph_read_changed_paths) {\n-\t\tread_chunk(cf, GRAPH_CHUNKID_BLOOMINDEXES,\n-\t\t\t graph_read_bloom_index, graph);\n+\t\tif (pair_chunk_expect(cf, GRAPH_CHUNKID_BLOOMINDEXES,\n+\t\t\t\t &graph->chunk_bloom_indexes,\n+\t\t\t\t st_mult(graph->num_commits, 4)) == -1)\n+\t\t\twarning(_(\"commit-graph changed-path index chunk is too small (%d)\"), graph->num_commits * 4);\n \t\tread_chunk(cf, GRAPH_CHUNKID_BLOOMDATA,\n \t\t\t graph_read_bloom_data, graph);\n \t}\n-- \n2.42.0.352.gd9c5062ff7.dirty\n\n"},{"id":"483223","messageId":"b39203b32c24772d5d8a257c4f647b7d9bd92d53.1697225110.git.me@ttaylorr.com","threadId":"60334","inReplyTo":"cover.1697225110.git.me@ttaylorr.com","subject":"[PATCH 6/8] midx: read `OIDF` chunk with `pair_chunk_expect()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-10-13T19:25:32Z","receivedAt":"2023-10-13T19:25:36Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Perform an identical conversion as in previous commits to read the\nOIDF chunk in the MIDX machinery.\n\nSigned-off-by: Taylor Blau \n---\n midx.c | 21 ++++++---------------\n 1 file changed, 6 insertions(+), 15 deletions(-)\n\ndiff --git a/midx.c b/midx.c\nindex 2f3863c936..38bf816cce 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -61,20 +61,6 @@ void get_midx_rev_filename(struct strbuf *out, struct multi_pack_index *m)\n \tstrbuf_addf(out, \"-%s.rev\", hash_to_hex(get_midx_checksum(m)));\n }\n \n-static int midx_read_oid_fanout(const unsigned char *chunk_start,\n-\t\t\t\tsize_t chunk_size, void *data)\n-{\n-\tstruct multi_pack_index *m = data;\n-\tm->chunk_oid_fanout = (uint32_t *)chunk_start;\n-\n-\tif (chunk_size != 4 * 256) {\n-\t\terror(_(\"multi-pack-index OID fanout is of the wrong size\"));\n-\t\treturn 1;\n-\t}\n-\tm->num_objects = ntohl(m->chunk_oid_fanout[255]);\n-\treturn 0;\n-}\n-\n static int midx_read_oid_lookup(const unsigned char *chunk_start,\n \t\t\t\tsize_t chunk_size, void *data)\n {\n@@ -173,8 +159,13 @@ struct multi_pack_index *load_multi_pack_index(const char *object_dir, int local\n \n \tif (pair_chunk(cf, MIDX_CHUNKID_PACKNAMES, &m->chunk_pack_names, &m->chunk_pack_names_len))\n \t\tdie(_(\"multi-pack-index required pack-name chunk missing or corrupted\"));\n-\tif (read_chunk(cf, MIDX_CHUNKID_OIDFANOUT, midx_read_oid_fanout, m))\n+\tif (pair_chunk_expect(cf, MIDX_CHUNKID_OIDFANOUT,\n+\t\t\t (const unsigned char **)&m->chunk_oid_fanout,\n+\t\t\t 256 * sizeof(uint32_t))) {\n+\t\terror(_(\"multi-pack-index OID fanout is of the wrong size\"));\n \t\tdie(_(\"multi-pack-index required OID fanout chunk missing or corrupted\"));\n+\t}\n+\tm->num_objects = ntohl(m->chunk_oid_fanout[255]);\n \tif (read_chunk(cf, MIDX_CHUNKID_OIDLOOKUP, midx_read_oid_lookup, m))\n \t\tdie(_(\"multi-pack-index required OID lookup chunk missing or corrupted\"));\n \tif (read_chunk(cf, MIDX_CHUNKID_OBJECTOFFSETS, midx_read_object_offsets, m))\n-- \n2.42.0.352.gd9c5062ff7.dirty\n\n"},{"id":"483224","messageId":"5844dd9383076b158eba20b9e43019280b74dd76.1697225110.git.me@ttaylorr.com","threadId":"60334","inReplyTo":"cover.1697225110.git.me@ttaylorr.com","subject":"[PATCH 7/8] midx: read `OIDL` chunk with `pair_chunk_expect()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-10-13T19:25:35Z","receivedAt":"2023-10-13T19:25:39Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Perform an identical conversion as in previous commits to read the\nOIDL chunk in the MIDX machinery.\n\nSigned-off-by: Taylor Blau \n---\n midx.c | 18 ++++--------------\n 1 file changed, 4 insertions(+), 14 deletions(-)\n\ndiff --git a/midx.c b/midx.c\nindex 38bf816cce..74167b8fdb 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -61,19 +61,6 @@ void get_midx_rev_filename(struct strbuf *out, struct multi_pack_index *m)\n \tstrbuf_addf(out, \"-%s.rev\", hash_to_hex(get_midx_checksum(m)));\n }\n \n-static int midx_read_oid_lookup(const unsigned char *chunk_start,\n-\t\t\t\tsize_t chunk_size, void *data)\n-{\n-\tstruct multi_pack_index *m = data;\n-\tm->chunk_oid_lookup = chunk_start;\n-\n-\tif (chunk_size != st_mult(m->hash_len, m->num_objects)) {\n-\t\terror(_(\"multi-pack-index OID lookup chunk is the wrong size\"));\n-\t\treturn 1;\n-\t}\n-\treturn 0;\n-}\n-\n static int midx_read_object_offsets(const unsigned char *chunk_start,\n \t\t\t\t size_t chunk_size, void *data)\n {\n@@ -166,8 +153,11 @@ struct multi_pack_index *load_multi_pack_index(const char *object_dir, int local\n \t\tdie(_(\"multi-pack-index required OID fanout chunk missing or corrupted\"));\n \t}\n \tm->num_objects = ntohl(m->chunk_oid_fanout[255]);\n-\tif (read_chunk(cf, MIDX_CHUNKID_OIDLOOKUP, midx_read_oid_lookup, m))\n+\tif (pair_chunk_expect(cf, MIDX_CHUNKID_OIDLOOKUP, &m->chunk_oid_lookup,\n+\t\t\t st_mult(m->hash_len, m->num_objects))) {\n+\t\terror(_(\"multi-pack-index OID lookup chunk is the wrong size\"));\n \t\tdie(_(\"multi-pack-index required OID lookup chunk missing or corrupted\"));\n+\t}\n \tif (read_chunk(cf, MIDX_CHUNKID_OBJECTOFFSETS, midx_read_object_offsets, m))\n \t\tdie(_(\"multi-pack-index required object offsets chunk missing or corrupted\"));\n \n-- \n2.42.0.352.gd9c5062ff7.dirty\n\n"},{"id":"483225","messageId":"a920f426ed9442b7e2ddc4de5d827e393a20c2f3.1697225110.git.me@ttaylorr.com","threadId":"60334","inReplyTo":"cover.1697225110.git.me@ttaylorr.com","subject":"[PATCH 8/8] midx: read `OOFF` chunk with `pair_chunk_expect()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-10-13T19:25:38Z","receivedAt":"2023-10-13T19:25:45Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Perform an identical conversion as in previous commits to read the\nOOFF chunk in the MIDX machinery.\n\nSigned-off-by: Taylor Blau \n---\n midx.c | 19 +++++--------------\n 1 file changed, 5 insertions(+), 14 deletions(-)\n\ndiff --git a/midx.c b/midx.c\nindex 74167b8fdb..4aeadd5e00 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -61,19 +61,6 @@ void get_midx_rev_filename(struct strbuf *out, struct multi_pack_index *m)\n \tstrbuf_addf(out, \"-%s.rev\", hash_to_hex(get_midx_checksum(m)));\n }\n \n-static int midx_read_object_offsets(const unsigned char *chunk_start,\n-\t\t\t\t size_t chunk_size, void *data)\n-{\n-\tstruct multi_pack_index *m = data;\n-\tm->chunk_object_offsets = chunk_start;\n-\n-\tif (chunk_size != st_mult(m->num_objects, MIDX_CHUNK_OFFSET_WIDTH)) {\n-\t\terror(_(\"multi-pack-index object offset chunk is the wrong size\"));\n-\t\treturn 1;\n-\t}\n-\treturn 0;\n-}\n-\n struct multi_pack_index *load_multi_pack_index(const char *object_dir, int local)\n {\n \tstruct multi_pack_index *m = NULL;\n@@ -158,8 +145,12 @@ struct multi_pack_index *load_multi_pack_index(const char *object_dir, int local\n \t\terror(_(\"multi-pack-index OID lookup chunk is the wrong size\"));\n \t\tdie(_(\"multi-pack-index required OID lookup chunk missing or corrupted\"));\n \t}\n-\tif (read_chunk(cf, MIDX_CHUNKID_OBJECTOFFSETS, midx_read_object_offsets, m))\n+\tif (pair_chunk_expect(cf, MIDX_CHUNKID_OBJECTOFFSETS,\n+\t\t\t &m->chunk_object_offsets,\n+\t\t\t st_mult(m->num_objects, MIDX_CHUNK_OFFSET_WIDTH))) {\n+\t\terror(_(\"multi-pack-index object offset chunk is the wrong size\"));\n \t\tdie(_(\"multi-pack-index required object offsets chunk missing or corrupted\"));\n+\t}\n \n \tpair_chunk(cf, MIDX_CHUNKID_LARGEOFFSETS, &m->chunk_large_offsets,\n \t\t &m->chunk_large_offsets_len);\n-- \n2.42.0.352.gd9c5062ff7.dirty\n"},{"id":"483226","messageId":"ZSmfYtkOjqwqtbzz@nand.local","threadId":"60334","inReplyTo":"45cac29403e63483951f7766c6da3c022c68d9f0.1697225110.git.me@ttaylorr.com","subject":"Re: [PATCH 5/8] commit-graph: read `BIDX` chunk with `pair_chunk_expect()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-10-13T19:49:54Z","receivedAt":"2023-10-13T19:49:58Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Oct 13, 2023 at 03:25:30PM -0400, Taylor Blau wrote:\n> Perform an identical conversion as in previous commits to read the BIDX\n> chunk.\n>\n> Signed-off-by: Taylor Blau \n> ---\n> commit-graph.c | 18 ++++--------------\n> 1 file changed, 4 insertions(+), 14 deletions(-)\n\nOops. This fails t4216 because it changes the warning() message, which I\nthought I excluded from this patch :-<.\n\nHere is a replacement that passes the test. I can reroll the entire\n\"series\" if we decide that this is a worthwhile direction to go in:\n\n--- 8< ---\n\nSubject: [PATCH] commit-graph: read `BIDX` chunk with `pair_chunk_expect()`\n\nPerform an identical conversion as in previous commits to read the BIDX\nchunk.\n\nSigned-off-by: Taylor Blau \n---\n commit-graph.c | 18 ++++--------------\n 1 file changed, 4 insertions(+), 14 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 0fab99f5dd..66c2e628d8 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -330,18 +330,6 @@ static int graph_read_oid_lookup(const unsigned char *chunk_start,\n \treturn 0;\n }\n\n-static int graph_read_bloom_index(const unsigned char *chunk_start,\n-\t\t\t\t size_t chunk_size, void *data)\n-{\n-\tstruct commit_graph *g = data;\n-\tif (chunk_size != g->num_commits * 4) {\n-\t\twarning(\"commit-graph changed-path index chunk is too small\");\n-\t\treturn -1;\n-\t}\n-\tg->chunk_bloom_indexes = chunk_start;\n-\treturn 0;\n-}\n-\n static int graph_read_bloom_data(const unsigned char *chunk_start,\n \t\t\t\t size_t chunk_size, void *data)\n {\n@@ -461,8 +449,10 @@ struct commit_graph *parse_commit_graph(struct repo_settings *s,\n \t}\n\n \tif (s->commit_graph_read_changed_paths) {\n-\t\tread_chunk(cf, GRAPH_CHUNKID_BLOOMINDEXES,\n-\t\t\t graph_read_bloom_index, graph);\n+\t\tif (pair_chunk_expect(cf, GRAPH_CHUNKID_BLOOMINDEXES,\n+\t\t\t\t &graph->chunk_bloom_indexes,\n+\t\t\t\t st_mult(graph->num_commits, 4)) == -1)\n+\t\t\twarning(_(\"commit-graph changed-path index chunk is too small\"));\n \t\tread_chunk(cf, GRAPH_CHUNKID_BLOOMDATA,\n \t\t\t graph_read_bloom_data, graph);\n \t}\n--\n2.38.0.16.g393fd4c6db\n\n"},{"id":"483235","messageId":"xmqqmswmxmhp.fsf@gitster.g","threadId":"60334","inReplyTo":"b39203b32c24772d5d8a257c4f647b7d9bd92d53.1697225110.git.me@ttaylorr.com","subject":"Re: [PATCH 6/8] midx: read `OIDF` chunk with `pair_chunk_expect()`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-13T21:04:34Z","receivedAt":"2023-10-13T21:04:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> +\tif (pair_chunk_expect(cf, MIDX_CHUNKID_OIDFANOUT,\n> +\t\t\t (const unsigned char **)&m->chunk_oid_fanout,\n> +\t\t\t 256 * sizeof(uint32_t))) {\n> +\t\terror(_(\"multi-pack-index OID fanout is of the wrong size\"));\n> \t\tdie(_(\"multi-pack-index required OID fanout chunk missing or corrupted\"));\n> +\t}\n\nThis is not a new problem, but when laid out this way, the doubled\nmessages look a bit suboptimal.\n\nTogether with reporting the actual and expected byte counts and\ndoing so consistenly I alluded to in a separate message, cleaning\nthese up should probably be left outside of the topic, I suspect.\n\n> +\tm->num_objects = ntohl(m->chunk_oid_fanout[255]);\n> \tif (read_chunk(cf, MIDX_CHUNKID_OIDLOOKUP, midx_read_oid_lookup, m))\n> \t\tdie(_(\"multi-pack-index required OID lookup chunk missing or corrupted\"));\n> \tif (read_chunk(cf, MIDX_CHUNKID_OBJECTOFFSETS, midx_read_object_offsets, m))\n"},{"id":"483238","messageId":"20231014004348.GA43880@coredump.intra.peff.net","threadId":"60334","inReplyTo":"20231009205544.GA3281950@coredump.intra.peff.net","subject":"[PATCH 21/20] t5319: make corrupted large-offset test more robust","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-10-14T00:43:48Z","receivedAt":"2023-10-14T00:43:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 09, 2023 at 04:55:44PM -0400, Jeff King wrote:\n\n> [10/20]: midx: bounds-check large offset chunk\n\nHere's one more patch on top that fixes a flaky test. Sorry not to have\ncaught it before sending out the original series. I did see a flaky CI\nrun before then, but I tweaked the test with what I thought was the\nsolution, and then it didn't re-occur until I ran CI again today.\n\nI'm pretty sure this should fix it for good.\n\n(I also saw Taylor posted some further patches; I haven't looked closely\nyet, this should be orthogonal to those).\n\n-- >8 --\nSubject: [PATCH] t5319: make corrupted large-offset test more robust\n\nThe test t5319.88 (\"reader bounds-checks large offset table\") can fail\nintermittently. The failure mode looks like this:\n\n 1. An earlier test sets up \"objects64\", a directory that can be used\n to produce a midx with a corrupted large-offsets table. To get the\n large offsets, it corrupts the normal \".idx\" file to have a fake\n large offset, and then builds a midx from that.\n\n That midx now has a large offset table, which is what we want. But\n we also have a .idx on disk that has a corrupted entry. We'll call\n the object with the corrupted large-offset \"X\".\n\n 2. In t5319.88, we further corrupt the midx by reducing the size of\n the large-offset chunk (because our goal is to make sure we do not\n do an out-of-bounds read on it).\n\n 3. We then enumerate all of the objects with \"cat-file --batch-check\n --batch-all-objects\", expecting to see a complaint when we try to\n show object X. We use --batch-all-objects because our objects64\n repo doesn't actually have any refs (but if we check them all, one\n of them will be the failing one). The default batch-check format\n includes %(objecttype) and %(objectsize), both of which require us\n to access the actual pack data (and thus requires looking at the\n offset).\n\n 4a. Usually, this succeeds. We try to output object X, do a lookup via\n the midx for the type/size lookup, and run into the corrupt\n large-offset table.\n\n 4b. But sometimes we hit a different error. If another object points\n to X as a delta base, then trying to find the type of that object\n requires walking the delta chain to the base entry (since only the\n base has the concrete type; deltas themselves are either OFS_DELTA\n or REF_DELTA).\n\n Normally this would not require separate offset lookups at all, as\n deltas are usually stored as OFS_DELTA, specifying the relative\n offset to the base. But the corrupt idx created in step 1 is done\n directly with \"git pack-objects\" and does not pass the\n --delta-base-offset option, meaning we have REF_DELTA entries!\n Those do have to consult an index to find the location of the base\n object, and they use the pack .idx to do this. The same pack .idx\n that we know is corrupted from step 1!\n\n Git does notice the error, but it does so by seeing the corrupt\n .idx file, not the corrupt midx file, and the error it reports is\n different, causing the test to fail.\n\nThe set of objects created in the test is deterministic. But the delta\nselection seems not to be (which is not too surprising, as it is\nmulti-threaded). I have seen the failure in Windows CI but haven't\nreproduced it locally (not even with --stress). Re-running a failed\nWindows CI job tends to work. But when I download and examine the trash\ndirectory from a failed run, it shows a different set of deltas than I\nget locally. But the exact source of non-determinism isn't that\nimportant; our test should be robust against any order.\n\nThere are a few options to fix this:\n\n a. It would be OK for the \"objects64\" setup to \"unbreak\" the .idx file\n after generating the midx. But then it would be hard for subsequent\n tests to reuse it, since it is the corrupted idx that forces the\n midx to have a large offset table.\n\n b. The \"objects64\" setup could use --delta-base-offset. This would fix\n our problem, but earlier tests have many hard-coded offsets. Using\n OFS_DELTA would change the locations of objects in the pack (this\n might even be OK because I think most of the offsets are within the\n .idx file, but it seems brittle and I'm afraid to touch it).\n\n c. Our cat-file output is in oid order by default. Since we store\n bases before deltas, if we went in pack order (using the\n \"--unordered\" flag), we'd always see our corrupt X before any delta\n which depends on it. But using \"--unordered\" means we skip the midx\n entirely. That makes sense, since it is just enumerating all of\n the packs, using the offsets found in their .idx files directly.\n So it doesn't work for our test.\n\n d. We could ask directly about object X, rather than enumerating all\n of them. But that requires further hard-coding of the oid (both\n sha1 and sha256) of object X. I'd prefer not to introduce more\n brittleness.\n\n e. We can use a --batch-check format that looks at the pack data, but\n doesn't have to chase deltas. The problem in this case is\n %(objecttype), which has to walk to the base. But %(objectsize)\n does not; we can get the value directly from the delta itself.\n Another option would be %(deltabase), where we report the REF_DELTA\n name but don't look at its data.\n\nI've gone with option (e) here. It's kind of subtle, but it's simple and\nhas no side effects.\n\nSigned-off-by: Jeff King \n---\n t/t5319-multi-pack-index.sh | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\nindex 2a11dd1af6..d3c9e97feb 100755\n--- a/t/t5319-multi-pack-index.sh\n+++ b/t/t5319-multi-pack-index.sh\n@@ -1129,8 +1129,10 @@ test_expect_success 'reader bounds-checks large offset table' '\n \t\tgit multi-pack-index --object-dir=../objects64 write &&\n \t\tmidx=../objects64/pack/multi-pack-index &&\n \t\tcorrupt_chunk_file $midx LOFF clear &&\n-\t\ttest_must_fail git cat-file \\\n-\t\t\t--batch-check --batch-all-objects 2>err &&\n+\t\t# using only %(objectsize) is important here; see the commit\n+\t\t# message for more details\n+\t\ttest_must_fail git cat-file --batch-all-objects \\\n+\t\t\t--batch-check=\"%(objectsize)\" 2>err &&\n \t\tcat >expect <<-\\EOF &&\n \t\tfatal: multi-pack-index large offset out of bounds\n \t\tEOF\n-- \n2.42.0.937.g4d9eb42d36\n\n"},{"id":"483247","messageId":"xmqqcyxhxk0h.fsf@gitster.g","threadId":"60334","inReplyTo":"45cac29403e63483951f7766c6da3c022c68d9f0.1697225110.git.me@ttaylorr.com","subject":"Re: [PATCH 5/8] commit-graph: read `BIDX` chunk with `pair_chunk_expect()`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-14T16:10:22Z","receivedAt":"2023-10-14T16:10:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> -static int graph_read_bloom_index(const unsigned char *chunk_start,\n> -\t\t\t\t size_t chunk_size, void *data)\n> -{\n> -\tstruct commit_graph *g = data;\n> -\tif (chunk_size != g->num_commits * 4) {\n> -\t\twarning(\"commit-graph changed-path index chunk is too small\");\n> -\t\treturn -1;\n> -\t}\n> ...\n> @@ -461,8 +449,10 @@ struct commit_graph *parse_commit_graph(struct repo_settings *s,\n> \t}\n> \n> \tif (s->commit_graph_read_changed_paths) {\n> +\t\tif (pair_chunk_expect(cf, GRAPH_CHUNKID_BLOOMINDEXES,\n> +\t\t\t\t &graph->chunk_bloom_indexes,\n> +\t\t\t\t st_mult(graph->num_commits, 4)) == -1)\n> +\t\t\twarning(_(\"commit-graph changed-path index chunk is too small (%d)\"), graph->num_commits * 4);\n> \t\tread_chunk(cf, GRAPH_CHUNKID_BLOOMDATA,\n> \t\t\t graph_read_bloom_data, graph);\n> \t}\n\nOverall the series looked sane, but the need for each caller to\nsupply error messages, when the helper perfectly well knows how many\nbytes the caller expected and how many bytes there are avaiable, was\na bit disturbing, as the level of detail given per each caller will\ninevitably become uneven. Even now, some give an error() while\nothers give a warning(), even though I suspect all of them should be\ndata errors.\n\nI wonder if it makes sense to stuff the message template in the\npair_chunk_data structure and do\n\nstatic int pair_chunk_expect_fn(const unsigned char *chunk_start,\n\t\t\t\tsize_t chunk_size,\n\t\t\t\tvoid *data)\n{\n\tstruct pair_chunk_data *pcd = data;\n\tif (pcd->expected_size != chunk_size)\n\t\treturn error(_(pcd->message), pcd->expected_size, chunk_size);\n\t*pcd->p = chunk_start;\n\treturn 0;\n}\n\n"},{"id":"483259","messageId":"xmqq4jitt2ie.fsf@gitster.g","threadId":"60334","inReplyTo":"20231014004348.GA43880@coredump.intra.peff.net","subject":"Re: [PATCH 21/20] t5319: make corrupted large-offset test more robust","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-14T19:42:01Z","receivedAt":"2023-10-14T19:42:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> 4b. But sometimes we hit a different error. If another object points\n> to X as a delta base, then trying to find the type of that object\n> requires walking the delta chain to the base entry (since only the\n> base has the concrete type; deltas themselves are either OFS_DELTA\n> or REF_DELTA).\n>\n> Normally this would not require separate offset lookups at all, as\n> deltas are usually stored as OFS_DELTA, specifying the relative\n> offset to the base. But the corrupt idx created in step 1 is done\n> directly with \"git pack-objects\" and does not pass the\n> --delta-base-offset option, meaning we have REF_DELTA entries!\n> Those do have to consult an index to find the location of the base\n> object, and they use the pack .idx to do this. The same pack .idx\n> that we know is corrupted from step 1!\n\nTricky.\n\n> The set of objects created in the test is deterministic. But the delta\n> selection seems not to be (which is not too surprising, as it is\n> multi-threaded).\n\nSo, the offsets of the objects are also not deterministic?\n\n> I have seen the failure in Windows CI but haven't\n> reproduced it locally (not even with --stress). Re-running a failed\n> Windows CI job tends to work. But when I download and examine the trash\n> directory from a failed run, it shows a different set of deltas than I\n> get locally. But the exact source of non-determinism isn't that\n> important; our test should be robust against any order.\n\nYeah, thanks for digging this tricky situation through.\n\n> b. The \"objects64\" setup could use --delta-base-offset. This would fix\n> our problem, but earlier tests have many hard-coded offsets. Using\n> OFS_DELTA would change the locations of objects in the pack (this\n> might even be OK because I think most of the offsets are within the\n> .idx file, but it seems brittle and I'm afraid to touch it).\n\nI am not sure I follow, as it does not sound a solution to anything\nif the offsets are not deterministic (and \"earlier tests\" that have\nhard coded offsets are broken no matter what, which is not a problem\nthis patch introduces). Puzzled, but not curious enough to think\nabout it further, as you have already rejected this approach ;-)\n\n> d. We could ask directly about object X, rather than enumerating all\n> of them. But that requires further hard-coding of the oid (both\n> sha1 and sha256) of object X. I'd prefer not to introduce more\n> brittleness.\n\nRight.\n\n> e. We can use a --batch-check format that looks at the pack data, but\n> doesn't have to chase deltas. The problem in this case is\n> %(objecttype), which has to walk to the base. But %(objectsize)\n> does not; we can get the value directly from the delta itself.\n> Another option would be %(deltabase), where we report the REF_DELTA\n> name but don't look at its data.\n>\n> I've gone with option (e) here. It's kind of subtle, but it's simple and\n> has no side effects.\n\nNice.\n\n> @@ -1129,8 +1129,10 @@ test_expect_success 'reader bounds-checks large offset table' '\n> \t\tgit multi-pack-index --object-dir=../objects64 write &&\n> \t\tmidx=../objects64/pack/multi-pack-index &&\n> \t\tcorrupt_chunk_file $midx LOFF clear &&\n> -\t\ttest_must_fail git cat-file \\\n> -\t\t\t--batch-check --batch-all-objects 2>err &&\n> +\t\t# using only %(objectsize) is important here; see the commit\n> +\t\t# message for more details\n> +\t\ttest_must_fail git cat-file --batch-all-objects \\\n> +\t\t\t--batch-check=\"%(objectsize)\" 2>err &&\n\nA rather unfriendly message to readers, as it is unclear which\ncommit you are talking about, and a fun thing is that you cannot\nsay which one.\n\nWill queue. Thanks.\n\n"},{"id":"483276","messageId":"20231015031732.GB554702@coredump.intra.peff.net","threadId":"60334","inReplyTo":"xmqq4jitt2ie.fsf@gitster.g","subject":"Re: [PATCH 21/20] t5319: make corrupted large-offset test more robust","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-10-15T03:17:32Z","receivedAt":"2023-10-15T03:17:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Oct 14, 2023 at 12:42:01PM -0700, Junio C Hamano wrote:\n\n> > The set of objects created in the test is deterministic. But the delta\n> > selection seems not to be (which is not too surprising, as it is\n> > multi-threaded).\n> \n> So, the offsets of the objects are also not deterministic?\n\nHmm, yeah, you're right. The pack clearly is not deterministic, and so\nneither will its offsets be.\n\nAnd thus...\n\n> > b. The \"objects64\" setup could use --delta-base-offset. This would fix\n> > our problem, but earlier tests have many hard-coded offsets. Using\n> > OFS_DELTA would change the locations of objects in the pack (this\n> > might even be OK because I think most of the offsets are within the\n> > .idx file, but it seems brittle and I'm afraid to touch it).\n> \n> I am not sure I follow, as it does not sound a solution to anything\n> if the offsets are not deterministic (and \"earlier tests\" that have\n> hard coded offsets are broken no matter what, which is not a problem\n> this patch introduces). Puzzled, but not curious enough to think\n> about it further, as you have already rejected this approach ;-)\n\n...yes, my \"this might even be OK\" is true. So it would work to solve\nthe problem by using --delta-base-offset. Those earlier tests are\nactually OK (they munge only the idx and the midx, not the pack). And if\nwe have delta base offsets, then walking delta chains never requires\nlooking at the pack idx.\n\nI'm not sure if that is any less subtle than the solution I did come up\nwith, though.\n\nThe most unsubtle thing is cleaning up the broken .idx immediately\nafter generating the midx. I didn't want to do that because it disrupts\nthe state of the objects64 directory. But we could always turn its setup\ninto a function or something. I dunno. It is probably not worth spending\ntoo many brain cycles on. My hope is that nobody ever has to touch this\ntest ever again. ;)\n\n> > @@ -1129,8 +1129,10 @@ test_expect_success 'reader bounds-checks large offset table' '\n> > \t\tgit multi-pack-index --object-dir=../objects64 write &&\n> > \t\tmidx=../objects64/pack/multi-pack-index &&\n> > \t\tcorrupt_chunk_file $midx LOFF clear &&\n> > -\t\ttest_must_fail git cat-file \\\n> > -\t\t\t--batch-check --batch-all-objects 2>err &&\n> > +\t\t# using only %(objectsize) is important here; see the commit\n> > +\t\t# message for more details\n> > +\t\ttest_must_fail git cat-file --batch-all-objects \\\n> > +\t\t\t--batch-check=\"%(objectsize)\" 2>err &&\n> \n> A rather unfriendly message to readers, as it is unclear which\n> commit you are talking about, and a fun thing is that you cannot\n> say which one.\n\nYeah, I had a similar thought process. I sort of assume anybody working\non git.git is capable of turning to \"git blame\" in a situation like\nthis. But maybe:\n\n # using only %(objectsize) is important here; run \"git blame\"\n # on these lines for more details\n\nwould spell it out more clearly.\n\n-Peff\n"},{"id":"483288","messageId":"xmqqil77stph.fsf@gitster.g","threadId":"60334","inReplyTo":"20231015031732.GB554702@coredump.intra.peff.net","subject":"Re: [PATCH 21/20] t5319: make corrupted large-offset test more robust","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-15T17:04:26Z","receivedAt":"2023-10-15T17:04:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Yeah, I had a similar thought process. I sort of assume anybody working\n> on git.git is capable of turning to \"git blame\" in a situation like\n> this. But maybe:\n>\n> # using only %(objectsize) is important here; run \"git blame\"\n> # on these lines for more details\n>\n> would spell it out more clearly.\n\nThe comment was about \"the commit\" being not so clear which commit.\n\"see the message of the commit that added this comment\" would have\nbeen perfectly fine and they are not required to use \"blame\" if they\ndon't like it.\n\n"},{"id":"483559","messageId":"20231020102307.GB2673857@coredump.intra.peff.net","threadId":"60334","inReplyTo":"cover.1697225110.git.me@ttaylorr.com","subject":"Re: [PATCH 0/8] chunk-format: introduce `pair_chunk_expect()` convenience API","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-10-20T10:23:07Z","receivedAt":"2023-10-20T10:23:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 13, 2023 at 03:25:12PM -0400, Taylor Blau wrote:\n\n> While reviewing this series, I noticed a couple of spots that would be\n> helped by having a convenience function that stores the start of a\n> chunk in a designated location *after* checking that the chunk has the\n> expected size.\n> \n> I called this function `pair_chunk_expect()` and touched up seven spots\n> that I think could benefit from having a convenience function like this.\n> \n> This applies directly on top of 'jk/chunk-bounds'. Thanks in advance for\n> your review!\n\nI'm still a little skeptical of this approach, just because I think it\nit discourages adding further checks. And in particular, I was planning\nto add monotonicity checks to the midx OIDF chunk based on the\ndiscussion in the earlier thread. And likewise, I think I probably\nshould have put the commit-graph checks into its OIDF reader, rather\nthan saving them for verify_commit_graph_lite(). That would match the\nway we check the validity of the other chunks.\n\nI haven't actually started writing those patches, though, so I'm not\nsure of the full details yet.\n\n-Peff\n"},{"id":"483560","messageId":"20231020103116.GC2673857@coredump.intra.peff.net","threadId":"60334","inReplyTo":"xmqqcyxhxk0h.fsf@gitster.g","subject":"Re: [PATCH 5/8] commit-graph: read `BIDX` chunk with `pair_chunk_expect()`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-10-20T10:31:16Z","receivedAt":"2023-10-20T10:31:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Oct 14, 2023 at 09:10:22AM -0700, Junio C Hamano wrote:\n\n> > @@ -461,8 +449,10 @@ struct commit_graph *parse_commit_graph(struct repo_settings *s,\n> > \t}\n> > \n> > \tif (s->commit_graph_read_changed_paths) {\n> > +\t\tif (pair_chunk_expect(cf, GRAPH_CHUNKID_BLOOMINDEXES,\n> > +\t\t\t\t &graph->chunk_bloom_indexes,\n> > +\t\t\t\t st_mult(graph->num_commits, 4)) == -1)\n> > +\t\t\twarning(_(\"commit-graph changed-path index chunk is too small (%d)\"), graph->num_commits * 4);\n> > \t\tread_chunk(cf, GRAPH_CHUNKID_BLOOMDATA,\n> > \t\t\t graph_read_bloom_data, graph);\n> > \t}\n> \n> Overall the series looked sane, but the need for each caller to\n> supply error messages, when the helper perfectly well knows how many\n> bytes the caller expected and how many bytes there are avaiable, was\n> a bit disturbing, as the level of detail given per each caller will\n> inevitably become uneven. Even now, some give an error() while\n> others give a warning(), even though I suspect all of them should be\n> data errors.\n> \n> I wonder if it makes sense to stuff the message template in the\n> pair_chunk_data structure and do\n> \n> static int pair_chunk_expect_fn(const unsigned char *chunk_start,\n> \t\t\t\tsize_t chunk_size,\n> \t\t\t\tvoid *data)\n> {\n> \tstruct pair_chunk_data *pcd = data;\n> \tif (pcd->expected_size != chunk_size)\n> \t\treturn error(_(pcd->message), pcd->expected_size, chunk_size);\n> \t*pcd->p = chunk_start;\n> \treturn 0;\n> }\n\nOne issue with the series as-is is that the \"chunk is too small\"\nmessages can be misleading when the chunk is in fact missing. We do say\n\"missing or corrupt\" in the die message (at least for midx; I did not\nupdate the similar ones for commit-graph), but the explicit \"too small\"\nfor a missing chunk seems to me to cross the line.\n\nThe caller can distinguish the cases by the actual value returned from\npair_chunk_expect(), but doing so makes the code a lot longer and harder\nto read.\n\nYour suggestion above takes care of it naturally (in the same way that\nthe existing code does, which basically is emitting the same message in\neach read_chunk callback).\n\n-Peff\n"}]}