{"thread":{"id":"60496","subject":"[PATCH 0/9] some more chunk-file bounds-checks fixes","startedAt":"2023-11-09T07:03:12Z","lastAt":"2024-01-18T23:59:17Z","messageCount":39,"participants":["Jeff King","Taylor Blau","Junio C Hamano","Linus Arver"],"isPatch":true,"patchVersion":1,"patchTotal":9},"messages":[{"id":"484604","messageId":"20231109070310.GA2697602@coredump.intra.peff.net","threadId":"60496","inReplyTo":null,"subject":"[PATCH 0/9] some more chunk-file bounds-checks fixes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-11-09T07:03:10Z","receivedAt":"2023-11-09T07:03:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This is a follow-up to the series from:\n\n https://lore.kernel.org/git/20231009205544.GA3281950@coredump.intra.peff.net/\n\nwhich was merged to master as jk/chunk-bounds. There were a few issues\nleft open by that series and its review:\n\n 1. the midx code didn't check fanout ordering\n\n 2. whether we needed to sprinkle some more st_mult() on it\n\n 3. improving some of the error messages (translations, some\n consistency, maybe more details)\n\n 4. possible refactoring with a pair_chunk_expect() API (Taylor posted\n a series in that direction, which is currently in limbo)\n\nThe patches here fix the remaining correctness issues (points 1 and 2,\nalong with a few other small issues I found). There's some improvement\non 3, but I stopped short of adding lots more details. Partially because\nthe series was already getting big, and partially because going further\nmay depend on what we do with 4.\n\nWith regards to the pair_chunk_expect() thing, this series is\nincompatible (textually, but not conceptually) with what Taylor posted\nearlier, just because I'm moving some checks into the chunk-reader\ncallbacks. Because it's fixing user-visible bugs, I think we'd want to\ndo this first, and then (possibly) rebase Taylor's series on top. But I\nalso think some of the things I noticed around overflow (especially\npatches 1 and 6) may inform how we'd want the pair_chunk_expect() API to\nlook.\n\nThis is a continuation of the jk/chunk-bounds topic, which is new in the\nv2.43 cycle. But it should be OK to leave this until after the release.\nNothing here is fixing a regression in the 2.43 release candidates; it's\njust a few bits that were incomplete. That said, I did try to float the\ncorrectness bits to the first two patches just in case. ;)\n\n [1/9]: commit-graph: handle overflow in chunk_size checks\n [2/9]: midx: check consistency of fanout table\n [3/9]: commit-graph: drop redundant call to \"lite\" verification\n [4/9]: commit-graph: clarify missing-chunk error messages\n [5/9]: commit-graph: abort as soon as we see a bogus chunk\n [6/9]: commit-graph: use fanout value for graph size\n [7/9]: commit-graph: check order while reading fanout chunk\n [8/9]: commit-graph: drop verify_commit_graph_lite()\n [9/9]: commit-graph: mark chunk error messages for translation\n\n commit-graph.c | 94 +++++++++++++------------------------\n midx.c | 20 ++++----\n t/t5318-commit-graph.sh | 16 ++++---\n t/t5319-multi-pack-index.sh | 14 ++++++\n 4 files changed, 67 insertions(+), 77 deletions(-)\n\n-Peff\n"},{"id":"484605","messageId":"20231109070948.GA2698043@coredump.intra.peff.net","threadId":"60496","inReplyTo":"20231109070310.GA2697602@coredump.intra.peff.net","subject":"[PATCH 1/9] commit-graph: handle overflow in chunk_size checks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-11-09T07:09:48Z","receivedAt":"2023-11-09T07:09:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We check the size of chunks with fixed records by multiplying the width\nof each record by the number of commits in the file. Like:\n\n if (chunk_size != g->num_commits * GRAPH_DATA_WIDTH)\n\nIf this multiplication overflows, we may not notice a chunk is too small\n(which could later lead to out-of-bound reads).\n\nIn the current code this is only possible for the CDAT chunk, but the\nreasons are quite subtle. We compute g->num_commits by dividing the size\nof the OIDL chunk by the hash length (since it consists of a bunch of\nhashes). So we know that any size_t multiplication that uses a value\nsmaller than the hash length cannot overflow. And the CDAT records are\nthe only ones that are larger (the others are just 4-byte records). So\nit's worth fixing all of these, to make it clear that they're not\nsubject to overflow (without having to reason about seemingly unrelated\ncode).\n\nThe obvious thing to do is add an st_mult(), like:\n\n if (chunk_size != st_mult(g->num_commits, GRAPH_DATA_WIDTH))\n\nAnd that certainly works, but it has one downside: if we detect an\noverflow, we'll immediately die(). But the commit graph is an optional\nfile; if we run into other problems loading it, we'll generally return\nan error and fall back to accessing the full objects. Using st_mult()\nmeans a malformed file will abort the whole process.\n\nSo instead, we can do a division like this:\n\n if (chunk_size / GRAPH_DATA_WIDTH != g->num_commits)\n\nwhere there's no possibility of overflow. We do lose a little bit of\nprecision; due to integer division truncation we'd allow up to an extra\nGRAPH_DATA_WIDTH-1 bytes of data in the chunk. That's OK. Our main goal\nhere is making sure we don't have too _few_ bytes, which would cause an\nout-of-bounds read (we could actually replace our \"!=\" with \"<\", but I\nthink it's worth being a little pedantic, as a large mismatch could be a\nsign of other problems).\n\nI didn't add a test here. We'd need to generate a very large graph file\nin order to get g->num_commits large enough to cause an overflow. And a\nlater patch in this series will use this same division technique in a\nway that is much easier to trigger in the tests.\n\nSigned-off-by: Jeff King \n---\nThere are still several st_mult() calls within commit-graph.c, unrelated\nto my jk/chunk-bounds series. I suspect they're all redundant, as the\nchunk-size checks give a stricter bound. But checking and removing them\nis a separate topic.\n\nI think the midx code has similar st_mult() problems, but it's quite\nhappy to die() on any error already. So I've left auditing that for\nanother day.\n\nAnother alternative to the division is introducing an st_mult() variant\nlike:\n\n if (!st_mult(&out, &a, &b))\n return error(...);\n\nWe may want to go there in the long run as part of lib-ification, but I\ndidn't want to get into it for this series.\n\n commit-graph.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex ee66098e07..5d7d7a89e5 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -344,7 +344,7 @@ 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+\tif (chunk_size / GRAPH_DATA_WIDTH != g->num_commits)\n \t\treturn error(\"commit-graph commit data chunk is wrong size\");\n \tg->chunk_commit_data = chunk_start;\n \treturn 0;\n@@ -354,7 +354,7 @@ 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+\tif (chunk_size / sizeof(uint32_t) != g->num_commits)\n \t\treturn error(\"commit-graph generations chunk is wrong size\");\n \tg->chunk_generation_data = chunk_start;\n \treturn 0;\n@@ -364,7 +364,7 @@ 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+\tif (chunk_size / 4 != g->num_commits) {\n \t\twarning(\"commit-graph changed-path index chunk is too small\");\n \t\treturn -1;\n \t}\n-- \n2.43.0.rc1.572.g273fc7bed6\n\n"},{"id":"484610","messageId":"20231109071207.GB2698043@coredump.intra.peff.net","threadId":"60496","inReplyTo":"20231109070310.GA2697602@coredump.intra.peff.net","subject":"[PATCH 2/9] midx: check consistency of fanout table","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-11-09T07:12:07Z","receivedAt":"2023-11-09T07:12:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The commit-graph, midx, and pack idx on-disk formats all have oid fanout\ntables which are fed to bsearch_hash(). If these tables do not increase\nmonotonically, then the binary search may not only produce bogus values,\nit may cause out of bounds reads.\n\nWe fixed this for commit graphs in 4169d89645 (commit-graph: check\nconsistency of fanout table, 2023-10-09). That commit argued that we did\nnot need to do the same for midx and pack idx files, because they\nalready did this check. However, that is wrong. We _do_ check the fanout\ntable for pack idx files when we load them, but we only do so for midx\nfiles when running \"git multi-pack-index verify\". So it is possible to\nget an out-of-bounds read by running a normal command with a specially\ncrafted midx file.\n\nLet's fix this using the same solution (and roughly the same test) we\ndid for the commit-graph in 4169d89645. This replaces the same check\nfrom \"multi-pack-index verify\", because verify uses the same read\nroutines, we'd bail on reading the midx much sooner now. So let's make\nsure to copy its verbose error message.\n\nSigned-off-by: Jeff King \n---\n midx.c | 20 +++++++++++---------\n t/t5319-multi-pack-index.sh | 14 ++++++++++++++\n 2 files changed, 25 insertions(+), 9 deletions(-)\n\ndiff --git a/midx.c b/midx.c\nindex 2f3863c936..1d14661dad 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -64,13 +64,24 @@ void get_midx_rev_filename(struct strbuf *out, struct multi_pack_index *m)\n static int midx_read_oid_fanout(const unsigned char *chunk_start,\n \t\t\t\tsize_t chunk_size, void *data)\n {\n+\tint i;\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+\tfor (i = 0; i < 255; i++) {\n+\t\tuint32_t oid_fanout1 = ntohl(m->chunk_oid_fanout[i]);\n+\t\tuint32_t oid_fanout2 = ntohl(m->chunk_oid_fanout[i+1]);\n+\n+\t\tif (oid_fanout1 > oid_fanout2) {\n+\t\t\terror(_(\"oid fanout out of order: fanout[%d] = %\"PRIx32\" > %\"PRIx32\" = fanout[%d]\"),\n+\t\t\t i, oid_fanout1, oid_fanout2, i + 1);\n+\t\t\treturn 1;\n+\t\t}\n+\t}\n \tm->num_objects = ntohl(m->chunk_oid_fanout[255]);\n \treturn 0;\n }\n@@ -1782,15 +1793,6 @@ int verify_midx_file(struct repository *r, const char *object_dir, unsigned flag\n \t}\n \tstop_progress(&progress);\n \n-\tfor (i = 0; i < 255; i++) {\n-\t\tuint32_t oid_fanout1 = ntohl(m->chunk_oid_fanout[i]);\n-\t\tuint32_t oid_fanout2 = ntohl(m->chunk_oid_fanout[i + 1]);\n-\n-\t\tif (oid_fanout1 > oid_fanout2)\n-\t\t\tmidx_report(_(\"oid fanout out of order: fanout[%d] = %\"PRIx32\" > %\"PRIx32\" = fanout[%d]\"),\n-\t\t\t\t i, oid_fanout1, oid_fanout2, i + 1);\n-\t}\n-\n \tif (m->num_objects == 0) {\n \t\tmidx_report(_(\"the midx contains no oid\"));\n \t\t/*\ndiff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\nindex c4c6060cee..c20aafe99a 100755\n--- a/t/t5319-multi-pack-index.sh\n+++ b/t/t5319-multi-pack-index.sh\n@@ -1157,4 +1157,18 @@ test_expect_success 'reader notices too-small revindex chunk' '\n \ttest_cmp expect.err err\n '\n \n+test_expect_success 'reader notices out-of-bounds fanout' '\n+\t# This is similar to the out-of-bounds fanout test in t5318. The values\n+\t# in adjacent entries should be large but not identical (they\n+\t# are used as hi/lo starts for a binary search, which would then abort\n+\t# immediately).\n+\tcorrupt_chunk OIDF 0 $(printf \"%02x000000\" $(test_seq 0 254)) &&\n+\ttest_must_fail git log 2>err &&\n+\tcat >expect <<-\\EOF &&\n+\terror: oid fanout out of order: fanout[254] = fe000000 > 5c = fanout[255]\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.43.0.rc1.572.g273fc7bed6\n\n"},{"id":"484611","messageId":"20231109071312.GC2698043@coredump.intra.peff.net","threadId":"60496","inReplyTo":"20231109070310.GA2697602@coredump.intra.peff.net","subject":"[PATCH 3/9] commit-graph: drop redundant call to \"lite\" verification","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-11-09T07:13:12Z","receivedAt":"2023-11-09T07:13:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The idea of verify_commit_graph_lite() is to have cheap verification\nchecks both for everyday use of the graph files (to avoid out of bounds\nreads, etc) as well as for doing a full check via \"commit-graph verify\"\n(which will also check the hash, etc).\n\nBut the expensive verification checks operate on a commit_graph struct,\nwhich we get by using the normal everyday-reader code! So any problem\nwe'd find by calling it would have been found before we even got to the\nverify_one_commit_graph() function.\n\nRemoving it simplifies the code a bit, but also frees us up to move the\n\"lite\" verification steps around within that everyday-reader code.\n\nSigned-off-by: Jeff King \n---\n commit-graph.c | 4 ----\n 1 file changed, 4 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 5d7d7a89e5..d9fc08de86 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -2690,10 +2690,6 @@ static int verify_one_commit_graph(struct repository *r,\n \tstruct commit *seen_gen_zero = NULL;\n \tstruct commit *seen_gen_non_zero = NULL;\n \n-\tverify_commit_graph_error = verify_commit_graph_lite(g);\n-\tif (verify_commit_graph_error)\n-\t\treturn verify_commit_graph_error;\n-\n \tif (!commit_graph_checksum_valid(g)) {\n \t\tgraph_report(_(\"the commit-graph file has incorrect checksum and is likely corrupt\"));\n \t\tverify_commit_graph_error = VERIFY_COMMIT_GRAPH_ERROR_HASH;\n-- \n2.43.0.rc1.572.g273fc7bed6\n\n"},{"id":"484612","messageId":"20231109071434.GD2698043@coredump.intra.peff.net","threadId":"60496","inReplyTo":"20231109070310.GA2697602@coredump.intra.peff.net","subject":"[PATCH 4/9] commit-graph: clarify missing-chunk error messages","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-11-09T07:14:34Z","receivedAt":"2023-11-09T07:14:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When a required commit-graph chunk cannot be loaded, we leave its entry\nin the struct NULL, and then later complain that it is missing. But\nthat's just one reason we might not have loaded it, as we also do some\ndata quality checks.\n\nLet's switch these messages to say \"missing or corrupted\", which is\nexactly what the midx code says for the same cases. Likewise, we'll use\nthe same phrasing and capitalization as those for consistency. And while\nwe're here, we can mark them for translation (just like the midx ones).\n\nSigned-off-by: Jeff King \n---\nI went with \"corrupted\" here for consistency with the others (versus\n\"corrupt\"). If we think there's a reason to prefer one over the other,\nI'm happy to take a patch on top fixing all of them.\n\n commit-graph.c | 6 +++---\n t/t5318-commit-graph.sh | 10 +++++-----\n 2 files changed, 8 insertions(+), 8 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex d9fc08de86..989ebbe816 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -292,15 +292,15 @@ static int verify_commit_graph_lite(struct commit_graph *g)\n \t * itself.\n \t */\n \tif (!g->chunk_oid_fanout) {\n-\t\terror(\"commit-graph is missing the OID Fanout chunk\");\n+\t\terror(_(\"commit-graph required OID fanout chunk missing or corrupted\"));\n \t\treturn 1;\n \t}\n \tif (!g->chunk_oid_lookup) {\n-\t\terror(\"commit-graph is missing the OID Lookup chunk\");\n+\t\terror(_(\"commit-graph required OID lookup chunk missing or corrupted\"));\n \t\treturn 1;\n \t}\n \tif (!g->chunk_commit_data) {\n-\t\terror(\"commit-graph is missing the Commit Data chunk\");\n+\t\terror(_(\"commit-graph required commit data chunk missing or corrupted\"));\n \t\treturn 1;\n \t}\n \ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex d4fc65e078..affb959d64 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -540,17 +540,17 @@ test_expect_success 'detect low chunk count' '\n \n test_expect_success 'detect missing OID fanout chunk' '\n \tcorrupt_graph_and_verify $GRAPH_BYTE_OID_FANOUT_ID \"\\0\" \\\n-\t\t\"missing the OID Fanout chunk\"\n+\t\t\"commit-graph required OID fanout chunk missing or corrupted\"\n '\n \n test_expect_success 'detect missing OID lookup chunk' '\n \tcorrupt_graph_and_verify $GRAPH_BYTE_OID_LOOKUP_ID \"\\0\" \\\n-\t\t\"missing the OID Lookup chunk\"\n+\t\t\"commit-graph required OID lookup chunk missing or corrupted\"\n '\n \n test_expect_success 'detect missing commit data chunk' '\n \tcorrupt_graph_and_verify $GRAPH_BYTE_COMMIT_DATA_ID \"\\0\" \\\n-\t\t\"missing the Commit Data chunk\"\n+\t\t\"commit-graph required commit data chunk missing or corrupted\"\n '\n \n test_expect_success 'detect incorrect fanout' '\n@@ -842,7 +842,7 @@ test_expect_success 'reader notices too-small oid fanout chunk' '\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+\terror: commit-graph required OID fanout chunk missing or corrupted\n \tEOF\n \ttest_cmp expect.err err\n '\n@@ -874,7 +874,7 @@ 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+\terror: commit-graph required commit data chunk missing or corrupted\n \tEOF\n \ttest_cmp expect.err err\n '\n-- \n2.43.0.rc1.572.g273fc7bed6\n\n"},{"id":"484613","messageId":"20231109071711.GE2698043@coredump.intra.peff.net","threadId":"60496","inReplyTo":"20231109070310.GA2697602@coredump.intra.peff.net","subject":"[PATCH 5/9] commit-graph: abort as soon as we see a bogus chunk","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-11-09T07:17:11Z","receivedAt":"2023-11-09T07:17:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The code to read commit-graph files tries to read all of the required\nchunks, but doesn't abort if we can't find one (or if it's corrupted).\nIt's only at the end of reading the file that we then do some sanity\nchecks for NULL entries. But it's preferable to detect the errors and\nbail immediately, for a few reasons:\n\n 1. It's less error-prone. It's easy in the reader functions to flag an\n error but still end up setting some struct fields (an error I in\n fact made while working on this patch series).\n\n 2. It's safer. Since verifying some chunks depends on the values of\n other chunks, we may be depending on not-yet-verified data. I don't\n know offhand of any case where this can cause problems, but it's\n one less subtle thing to worry about in the reader code.\n\n 3. It prevents the user from seeing nonsense errors. If we're missing\n an OIDL chunk, then g->num_commits will be zero. And so we may\n complain that the size of our CDAT chunk (which should have a\n fixed-size record for each commit) is wrong unless it's also zero.\n But that's misleading; the problem is the missing OIDL chunk; the\n CDAT one might be fine!\n\nSo let's just check the return value from read_chunk(). This is exactly\nhow the midx chunk-reading code does it.\n\nSigned-off-by: Jeff King \n---\n commit-graph.c | 29 +++++++++++++----------------\n 1 file changed, 13 insertions(+), 16 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 989ebbe816..374575b484 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -291,19 +291,6 @@ static int verify_commit_graph_lite(struct commit_graph *g)\n \t * over g->num_commits, or runs a checksum on the commit-graph\n \t * itself.\n \t */\n-\tif (!g->chunk_oid_fanout) {\n-\t\terror(_(\"commit-graph required OID fanout chunk missing or corrupted\"));\n-\t\treturn 1;\n-\t}\n-\tif (!g->chunk_oid_lookup) {\n-\t\terror(_(\"commit-graph required OID lookup chunk missing or corrupted\"));\n-\t\treturn 1;\n-\t}\n-\tif (!g->chunk_commit_data) {\n-\t\terror(_(\"commit-graph required commit data chunk missing or corrupted\"));\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@@ -462,9 +449,19 @@ 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-\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 (read_chunk(cf, GRAPH_CHUNKID_OIDFANOUT, graph_read_oid_fanout, graph)) {\n+\t\terror(_(\"commit-graph required OID fanout chunk missing or corrupted\"));\n+\t\tgoto free_and_return;\n+\t}\n+\tif (read_chunk(cf, GRAPH_CHUNKID_OIDLOOKUP, graph_read_oid_lookup, graph)) {\n+\t\terror(_(\"commit-graph required OID lookup chunk missing or corrupted\"));\n+\t\tgoto free_and_return;\n+\t}\n+\tif (read_chunk(cf, GRAPH_CHUNKID_DATA, graph_read_commit_data, graph)) {\n+\t\terror(_(\"commit-graph required commit data chunk missing or corrupted\"));\n+\t\tgoto free_and_return;\n+\t}\n+\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.43.0.rc1.572.g273fc7bed6\n\n"},{"id":"484614","messageId":"20231109072435.GF2698043@coredump.intra.peff.net","threadId":"60496","inReplyTo":"20231109070310.GA2697602@coredump.intra.peff.net","subject":"[PATCH 6/9] commit-graph: use fanout value for graph size","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-11-09T07:24:35Z","receivedAt":"2023-11-09T07:24:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Commit-graph, midx, and pack idx files all have both a lookup table of\noids and an oid fanout table. In midx and pack idx files, we take the\nfinal entry of the fanout table as the source of truth for the number of\nentries, and then verify that the size of the lookup table matches that.\nBut for commit-graph files, we do the opposite: we use the size of the\nlookup table as the source of truth, and then check the final fanout\nentry against it.\n\nAs noted in 4169d89645 (commit-graph: check consistency of fanout\ntable, 2023-10-09), either is correct. But there are a few reasons to\nprefer the fanout table as the source of truth:\n\n 1. The fanout entries are 32-bits on disk, and that defines the\n maximum number of entries we can store. But since the size of the\n lookup table is only bounded by the filesystem, it can be much\n larger. And hence computing it as the commit-graph does means that\n we may truncate the result when storing it in a uint32_t.\n\n 2. We read the fanout first, then the lookup table. If we're verifying\n the chunks as we read them, then we'd want to take the fanout as\n truth (we have nothing yet to check it against) and then we can\n check that the lookup table matches what we already know.\n\n 3. It is pointlessly inconsistent with the midx and pack idx code.\n Since the three have to do similar size and bounds checks, it is\n easier to reason about all three if they use the same approach.\n\nSo this patch moves the assignment of g->num_commits to the fanout\nparser, and then we can check the size of the lookup chunk as soon as we\ntry to load it.\n\nThere's already a test covering this situation, which munges the final\nfanout entry to 2^32-1. In the current code we complain that it does not\nagree with the table size. But now that we treat the munged value as the\nsource of truth, we'll complain that the lookup table is the wrong size\n(again, either is correct). So we'll have to update the message we\nexpect (and likewise for an earlier test which does similar munging).\n\nThere's a similar test for this situation on the midx side, but rather\nthan making a very-large fanout value, it just truncates the lookup\ntable. We could do that here, too, but the very-large fanout value\nactually shows an interesting corner case. On a 32-bit system,\nmultiplying to find the expected table size would cause an integer\noverflow. Using st_mult() would detect that, but cause us to die()\nrather than falling back to the non-graph code path. Checking the size\nusing division (as we do with existing chunk-size checks) avoids the\noverflow entirely, and the test demonstrates this when run on a 32-bit\nsystem.\n\nSigned-off-by: Jeff King \n---\n commit-graph.c | 8 +++-----\n t/t5318-commit-graph.sh | 5 +++--\n 2 files changed, 6 insertions(+), 7 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 374575b484..094814c2ba 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -300,10 +300,6 @@ static int verify_commit_graph_lite(struct commit_graph *g)\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@@ -315,6 +311,7 @@ static int graph_read_oid_fanout(const unsigned char *chunk_start,\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+\tg->num_commits = ntohl(g->chunk_oid_fanout[255]);\n \treturn 0;\n }\n \n@@ -323,7 +320,8 @@ static int graph_read_oid_lookup(const unsigned char *chunk_start,\n {\n \tstruct commit_graph *g = data;\n \tg->chunk_oid_lookup = chunk_start;\n-\tg->num_commits = chunk_size / g->hash_len;\n+\tif (chunk_size / g->hash_len != g->num_commits)\n+\t\treturn error(_(\"commit-graph OID lookup chunk is the wrong size\"));\n \treturn 0;\n }\n \ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex affb959d64..8bd7fcefb5 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\"oid table and fanout disagree on size\"\n+\t\t\"OID lookup chunk is the wrong size\"\n '\n \n test_expect_success 'detect incorrect OID order' '\n@@ -850,7 +850,8 @@ test_expect_success 'reader notices too-small oid fanout chunk' '\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+\terror: commit-graph OID lookup chunk is the wrong size\n+\terror: commit-graph required OID lookup chunk missing or corrupted\n \tEOF\n \ttest_cmp expect.err err\n '\n-- \n2.43.0.rc1.572.g273fc7bed6\n\n"},{"id":"484615","messageId":"20231109072507.GG2698043@coredump.intra.peff.net","threadId":"60496","inReplyTo":"20231109070310.GA2697602@coredump.intra.peff.net","subject":"[PATCH 7/9] commit-graph: check order while reading fanout chunk","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-11-09T07:25:07Z","receivedAt":"2023-11-09T07:25:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We read the fanout chunk, storing a pointer to it, but only confirm that\nthe entries are monotonic in a final \"lite\" verification step. Let's\nmove that into the actual OIDF chunk callback, so that we can report\nproblems immediately (for all the reasons given in the previous\n\"commit-graph: abort as soon as we see a bogus chunk\" commit).\n\nSigned-off-by: Jeff King \n---\n commit-graph.c | 25 +++++++++++++------------\n t/t5318-commit-graph.sh | 1 +\n 2 files changed, 14 insertions(+), 12 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 094814c2ba..4ba523cd15 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -277,8 +277,6 @@ 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@@ -291,27 +289,30 @@ static int verify_commit_graph_lite(struct commit_graph *g)\n \t * over g->num_commits, or runs a checksum on the commit-graph\n \t * itself.\n \t */\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-\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+\tint i;\n+\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 \tg->num_commits = ntohl(g->chunk_oid_fanout[255]);\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+\n \treturn 0;\n }\n \ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex 8bd7fcefb5..7fe7c72a87 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -867,6 +867,7 @@ test_expect_success 'reader notices out-of-bounds fanout' '\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+\terror: commit-graph required OID fanout chunk missing or corrupted\n \tEOF\n \ttest_cmp expect.err err\n '\n-- \n2.43.0.rc1.572.g273fc7bed6\n\n"},{"id":"484616","messageId":"20231109072524.GH2698043@coredump.intra.peff.net","threadId":"60496","inReplyTo":"20231109070310.GA2697602@coredump.intra.peff.net","subject":"[PATCH 8/9] commit-graph: drop verify_commit_graph_lite()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-11-09T07:25:24Z","receivedAt":"2023-11-09T07:25:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"As we've moved all of the checks from this function directly into the\nchunk-reading code used by the caller (and there is only one caller), we\ncan just drop it entirely.\n\nSigned-off-by: Jeff King \n---\n commit-graph.c | 20 --------------------\n 1 file changed, 20 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 4ba523cd15..6fbfe4c68e 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -275,23 +275,6 @@ struct commit_graph *load_commit_graph_one_fd_st(struct repository *r,\n \treturn ret;\n }\n \n-static int verify_commit_graph_lite(struct commit_graph *g)\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-\t * much more expensive verify_commit_graph() used by\n-\t * \"commit-graph verify\".\n-\t *\n-\t * There should only be very basic checks here to ensure that\n-\t * we don't e.g. segfault in fill_commit_in_graph(), but\n-\t * because this is a very hot codepath nothing that e.g. loops\n-\t * over g->num_commits, or runs a checksum on the commit-graph\n-\t * itself.\n-\t */\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@@ -495,9 +478,6 @@ struct commit_graph *parse_commit_graph(struct repo_settings *s,\n \n \toidread(&graph->oid, graph->data + graph->data_len - graph->hash_len);\n \n-\tif (verify_commit_graph_lite(graph))\n-\t\tgoto free_and_return;\n-\n \tfree_chunkfile(cf);\n \treturn graph;\n \n-- \n2.43.0.rc1.572.g273fc7bed6\n\n"},{"id":"484617","messageId":"20231109072628.GI2698043@coredump.intra.peff.net","threadId":"60496","inReplyTo":"20231109070310.GA2697602@coredump.intra.peff.net","subject":"[PATCH 9/9] commit-graph: mark chunk error messages for translation","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-11-09T07:26:28Z","receivedAt":"2023-11-09T07:26:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The patches from f32af12cee (Merge branch 'jk/chunk-bounds', 2023-10-23)\nadded many new untranslated error messages. While it's unlikely for most\nusers to see these messages at all, most of the other commit-graph error\nmessages are translated (and likewise for the matching midx messages).\n\nLet's mark them all for consistency (and to help any poor unfortunate\nuser who does manage to find a broken graph file).\n\nSigned-off-by: Jeff King \n---\nThe \"wrong size\" ones may be dropped eventually if we have a generic\npair_chunk_expect() API, but it seemed easier to just fix them all\nmechanically in one go.\n\n commit-graph.c | 18 +++++++++---------\n 1 file changed, 9 insertions(+), 9 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 6fbfe4c68e..acac9bf6e1 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -282,7 +282,7 @@ static int graph_read_oid_fanout(const unsigned char *chunk_start,\n \tint i;\n \n \tif (chunk_size != 256 * sizeof(uint32_t))\n-\t\treturn error(\"commit-graph oid fanout chunk is wrong size\");\n+\t\treturn error(_(\"commit-graph oid fanout chunk is wrong size\"));\n \tg->chunk_oid_fanout = (const uint32_t *)chunk_start;\n \tg->num_commits = ntohl(g->chunk_oid_fanout[255]);\n \n@@ -291,7 +291,7 @@ static int graph_read_oid_fanout(const unsigned char *chunk_start,\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\terror(_(\"commit-graph fanout values out of order\"));\n \t\t\treturn 1;\n \t\t}\n \t}\n@@ -314,7 +314,7 @@ static int graph_read_commit_data(const unsigned char *chunk_start,\n {\n \tstruct commit_graph *g = data;\n \tif (chunk_size / GRAPH_DATA_WIDTH != g->num_commits)\n-\t\treturn error(\"commit-graph commit data chunk is wrong size\");\n+\t\treturn error(_(\"commit-graph commit data chunk is wrong size\"));\n \tg->chunk_commit_data = chunk_start;\n \treturn 0;\n }\n@@ -324,7 +324,7 @@ static int graph_read_generation_data(const unsigned char *chunk_start,\n {\n \tstruct commit_graph *g = data;\n \tif (chunk_size / sizeof(uint32_t) != g->num_commits)\n-\t\treturn error(\"commit-graph generations chunk is wrong size\");\n+\t\treturn error(_(\"commit-graph generations chunk is wrong size\"));\n \tg->chunk_generation_data = chunk_start;\n \treturn 0;\n }\n@@ -334,7 +334,7 @@ static int graph_read_bloom_index(const unsigned char *chunk_start,\n {\n \tstruct commit_graph *g = data;\n \tif (chunk_size / 4 != g->num_commits) {\n-\t\twarning(\"commit-graph changed-path index chunk is too small\");\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@@ -348,8 +348,8 @@ static int graph_read_bloom_data(const unsigned char *chunk_start,\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\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@@ -605,7 +605,7 @@ int open_commit_graph_chain(const char *chain_file,\n \t\t\t/* treat empty files the same as missing */\n \t\t\terrno = ENOENT;\n \t\t} else {\n-\t\t\twarning(\"commit-graph chain file too small\");\n+\t\t\twarning(_(\"commit-graph chain file too small\"));\n \t\t\terrno = EINVAL;\n \t\t}\n \t\treturn 0;\n@@ -946,7 +946,7 @@ static int fill_commit_in_graph(struct repository *r,\n \tparent_data_pos = edge_value & GRAPH_EDGE_LAST_MASK;\n \tdo {\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\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-- \n2.43.0.rc1.572.g273fc7bed6\n"},{"id":"484662","messageId":"ZU1LbV0/ciDdO1aD@nand.local","threadId":"60496","inReplyTo":"20231109070948.GA2698043@coredump.intra.peff.net","subject":"Re: [PATCH 1/9] commit-graph: handle overflow in chunk_size checks","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-11-09T21:13:17Z","receivedAt":"2023-11-09T21:13:20Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Nov 09, 2023 at 02:09:48AM -0500, Jeff King wrote:\n> So instead, we can do a division like this:\n>\n> if (chunk_size / GRAPH_DATA_WIDTH != g->num_commits)\n>\n> where there's no possibility of overflow. We do lose a little bit of\n> precision; due to integer division truncation we'd allow up to an extra\n> GRAPH_DATA_WIDTH-1 bytes of data in the chunk. That's OK. Our main goal\n> here is making sure we don't have too _few_ bytes, which would cause an\n> out-of-bounds read (we could actually replace our \"!=\" with \"<\", but I\n> think it's worth being a little pedantic, as a large mismatch could be a\n> sign of other problems).\n\nThis is wonderfully explained, and the patch below follows trivially\nfrom what you wrote here.\n\nSo everything in this patch makes sense and looks good to me. It does\nmake me think about the pair_chunk_expect() function that I proposed\nelsewhere. I haven't yet read the rest of the series, so it may be a\ntotally useless direction by the end of this series ;-).\n\nBut I wonder if the interface we want is something like:\n\n int pair_chunk_expect(struct chunkfile *cf, uint32_t chunk_id,\n const unsigned char **p,\n size_t record_size, size_t record_nr);\n\nSo we can then grab the size of the chunk, divide it by \"record_size\"\nand ensure that end up with \"record_nr\" as a result.\n\nAgain, this is perhaps totally useless by the end of your series, but\njust having looked at the first patch I wonder if this is a productive\ndirection to consider...\n\nThanks,\nTaylor\n"},{"id":"484663","messageId":"ZU1L8qdIjz/OIAfR@nand.local","threadId":"60496","inReplyTo":"20231109071711.GE2698043@coredump.intra.peff.net","subject":"Re: [PATCH 5/9] commit-graph: abort as soon as we see a bogus chunk","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-11-09T21:18:24Z","receivedAt":"2023-11-09T21:18:26Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Nov 09, 2023 at 02:17:11AM -0500, Jeff King wrote:\n> The code to read commit-graph files tries to read all of the required\n> chunks, but doesn't abort if we can't find one (or if it's corrupted).\n> It's only at the end of reading the file that we then do some sanity\n> checks for NULL entries. But it's preferable to detect the errors and\n> bail immediately, for a few reasons:\n>\n> 1. It's less error-prone. It's easy in the reader functions to flag an\n> error but still end up setting some struct fields (an error I in\n> fact made while working on this patch series).\n>\n> 2. It's safer. Since verifying some chunks depends on the values of\n> other chunks, we may be depending on not-yet-verified data. I don't\n> know offhand of any case where this can cause problems, but it's\n> one less subtle thing to worry about in the reader code.\n>\n> 3. It prevents the user from seeing nonsense errors. If we're missing\n> an OIDL chunk, then g->num_commits will be zero. And so we may\n> complain that the size of our CDAT chunk (which should have a\n> fixed-size record for each commit) is wrong unless it's also zero.\n> But that's misleading; the problem is the missing OIDL chunk; the\n> CDAT one might be fine!\n>\n> So let's just check the return value from read_chunk(). This is exactly\n> how the midx chunk-reading code does it.\n\nAll very well explained. I hit that same snag as you did when I was\nworking on the few patches I proposed we put on top of your earlier\nchunk-format hardening series.\n\nI'm glad to see this getting cleaned up, and I'm very happy with the\npost-image of this patch.\n\nThanks,\nTaylor\n"},{"id":"484664","messageId":"ZU1NNcP/ARxK6D89@nand.local","threadId":"60496","inReplyTo":"20231109072435.GF2698043@coredump.intra.peff.net","subject":"Re: [PATCH 6/9] commit-graph: use fanout value for graph size","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-11-09T21:20:53Z","receivedAt":"2023-11-09T21:20:55Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Nov 09, 2023 at 02:24:35AM -0500, Jeff King wrote:\n> @@ -323,7 +320,8 @@ static int graph_read_oid_lookup(const unsigned char *chunk_start,\n> {\n> \tstruct commit_graph *g = data;\n> \tg->chunk_oid_lookup = chunk_start;\n> -\tg->num_commits = chunk_size / g->hash_len;\n> +\tif (chunk_size / g->hash_len != g->num_commits)\n> +\t\treturn error(_(\"commit-graph OID lookup chunk is the wrong size\"));\n> \treturn 0;\n> }\n\nMy understanding is that you need this error message to come from\ngraph_read_oid_lookup() in order to pass the \"detect incorrect fanout\nfinal value\" test, but I wish that we didn't have to, since having the\nmore-or-less duplicate error messages in the latter \"reader notices\nfanout/lookup table mismatch\" is somewhat unfortunate.\n\n> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\n> index affb959d64..8bd7fcefb5 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\"oid table and fanout disagree on size\"\n> +\t\t\"OID lookup chunk is the wrong size\"\n> '\n>\n> test_expect_success 'detect incorrect OID order' '\n> @@ -850,7 +850,8 @@ test_expect_success 'reader notices too-small oid fanout chunk' '\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> +\terror: commit-graph OID lookup chunk is the wrong size\n> +\terror: commit-graph required OID lookup chunk missing or corrupted\n> \tEOF\n> \ttest_cmp expect.err err\n> '\n> --\n> 2.43.0.rc1.572.g273fc7bed6\n>\nThanks,\nTaylor\n"},{"id":"484665","messageId":"ZU1NiRs2eN5hDNgI@nand.local","threadId":"60496","inReplyTo":"20231109070310.GA2697602@coredump.intra.peff.net","subject":"Re: [PATCH 0/9] some more chunk-file bounds-checks fixes","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-11-09T21:22:17Z","receivedAt":"2023-11-09T21:22:20Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Nov 09, 2023 at 02:03:10AM -0500, Jeff King wrote:\n> This is a follow-up to the series from:\n>\n> https://lore.kernel.org/git/20231009205544.GA3281950@coredump.intra.peff.net/\n>\n> which was merged to master as jk/chunk-bounds. There were a few issues\n> left open by that series and its review:\n>\n> 1. the midx code didn't check fanout ordering\n>\n> 2. whether we needed to sprinkle some more st_mult() on it\n>\n> 3. improving some of the error messages (translations, some\n> consistency, maybe more details)\n>\n> 4. possible refactoring with a pair_chunk_expect() API (Taylor posted\n> a series in that direction, which is currently in limbo)\n\nI read this series thoroughly and was very pleased with the result.\nThanks for patching these up.\n\nI think that I am still of the mind that it would be useful to have some\nkind of pair_chunk_expect() function, so I'll try and rebase/rewrite my\npatches on top of your new ones here.\n\nIn the meantime, this series LGTM.\n\nThanks,\nTaylor\n"},{"id":"484666","messageId":"20231109212721.GA2728242@coredump.intra.peff.net","threadId":"60496","inReplyTo":"ZU1LbV0/ciDdO1aD@nand.local","subject":"Re: [PATCH 1/9] commit-graph: handle overflow in chunk_size checks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-11-09T21:27:21Z","receivedAt":"2023-11-09T21:27:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 09, 2023 at 04:13:17PM -0500, Taylor Blau wrote:\n\n> So everything in this patch makes sense and looks good to me. It does\n> make me think about the pair_chunk_expect() function that I proposed\n> elsewhere. I haven't yet read the rest of the series, so it may be a\n> totally useless direction by the end of this series ;-).\n\nNope, it's not useless. But I do think it affects what we'd want the\ninterface to look like, and...\n\n> But I wonder if the interface we want is something like:\n> \n> int pair_chunk_expect(struct chunkfile *cf, uint32_t chunk_id,\n> const unsigned char **p,\n> size_t record_size, size_t record_nr);\n> \n> So we can then grab the size of the chunk, divide it by \"record_size\"\n> and ensure that end up with \"record_nr\" as a result.\n\n...this is exactly the direction I was thinking it would go. So if you\ngot to the same place after reading my explanation, then hopefully\nsomething went right. ;)\n\n-Peff\n"},{"id":"484667","messageId":"20231109213813.GB2728242@coredump.intra.peff.net","threadId":"60496","inReplyTo":"ZU1NNcP/ARxK6D89@nand.local","subject":"Re: [PATCH 6/9] commit-graph: use fanout value for graph size","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-11-09T21:38:13Z","receivedAt":"2023-11-09T21:38:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 09, 2023 at 04:20:53PM -0500, Taylor Blau wrote:\n\n> On Thu, Nov 09, 2023 at 02:24:35AM -0500, Jeff King wrote:\n> > @@ -323,7 +320,8 @@ static int graph_read_oid_lookup(const unsigned char *chunk_start,\n> > {\n> > \tstruct commit_graph *g = data;\n> > \tg->chunk_oid_lookup = chunk_start;\n> > -\tg->num_commits = chunk_size / g->hash_len;\n> > +\tif (chunk_size / g->hash_len != g->num_commits)\n> > +\t\treturn error(_(\"commit-graph OID lookup chunk is the wrong size\"));\n> > \treturn 0;\n> > }\n> \n> My understanding is that you need this error message to come from\n> graph_read_oid_lookup() in order to pass the \"detect incorrect fanout\n> final value\" test, but I wish that we didn't have to, since having the\n> more-or-less duplicate error messages in the latter \"reader notices\n> fanout/lookup table mismatch\" is somewhat unfortunate.\n\nI'm not sure what you mean here. We notice the problem in the reading\ncode, which is used in turn by the verify code. So both of these tests:\n\n> > test_expect_success 'detect incorrect fanout final value' '\n> > \tcorrupt_graph_and_verify $GRAPH_BYTE_FANOUT2 \"\\01\" \\\n> > -\t\t\"oid table and fanout disagree on size\"\n> > +\t\t\"OID lookup chunk is the wrong size\"\n> > '\n> >\n> > test_expect_success 'detect incorrect OID order' '\n> > @@ -850,7 +850,8 @@ test_expect_success 'reader notices too-small oid fanout chunk' '\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> > +\terror: commit-graph OID lookup chunk is the wrong size\n> > +\terror: commit-graph required OID lookup chunk missing or corrupted\n> > \tEOF\n> > \ttest_cmp expect.err err\n> > '\n\nwill see a message regardless of where we put it. Or by \"duplicate\" did\nyou mean the two-line:\n\n > > + error: commit-graph OID lookup chunk is the wrong size\n > > + error: commit-graph required OID lookup chunk missing or corrupted\n\noutput. That's because our chunk of the return value from read_chunk()\nis a bit lazy, and does not distinguish \"missing chunk\" (where we should\nwrite a string saying so) from errors produced by the callback (where a\nmore specific error message has already been written). That's true of\nall of the read_chunk() callers, including the midx ones.\n\nThis is one of the ways that pair_chunk_expect() could do better without\nadding a lot of code, because it can check the return value from\nread_chunk(). It doesn't help the other cases (like OIDF) that still\nhave to call read_chunk() themselves, though. Possibly read_chunk()\nshould just take a flag to indicate that it should complain when the\nchunk is missing. And then callers could just do:\n\n if (read_chunk(cf, id, our_callback, CHUNK_REQUIRED)\n\treturn -1; /* no need to complain; either our_callback() did, or\n\t read_chunk() itself */\n\n-Peff\n"},{"id":"484668","messageId":"ZU1Z6WUrEhcxLBTO@nand.local","threadId":"60496","inReplyTo":"20231109213813.GB2728242@coredump.intra.peff.net","subject":"Re: [PATCH 6/9] commit-graph: use fanout value for graph size","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-11-09T22:15:05Z","receivedAt":"2023-11-09T22:15:08Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Nov 09, 2023 at 04:38:13PM -0500, Jeff King wrote:\n> Or by \"duplicate\" did you mean the two-line:\n>\n> > > + error: commit-graph OID lookup chunk is the wrong size\n> > > + error: commit-graph required OID lookup chunk missing or corrupted\n>\n> output. That's because our chunk of the return value from read_chunk()\n> is a bit lazy, and does not distinguish \"missing chunk\" (where we should\n> write a string saying so) from errors produced by the callback (where a\n> more specific error message has already been written). That's true of\n> all of the read_chunk() callers, including the midx ones.\n\nYeah, I meant duplicate in the sense above.\n\n> This is one of the ways that pair_chunk_expect() could do better without\n> adding a lot of code, because it can check the return value from\n> read_chunk(). It doesn't help the other cases (like OIDF) that still\n> have to call read_chunk() themselves, though. Possibly read_chunk()\n> should just take a flag to indicate that it should complain when the\n> chunk is missing. And then callers could just do:\n>\n> if (read_chunk(cf, id, our_callback, CHUNK_REQUIRED)\n> \treturn -1; /* no need to complain; either our_callback() did, or\n> \t read_chunk() itself */\n\nWe do return CHUNK_NOT_FOUND when we have a missing chunk, which we\ncould check for individually. But TBH, I don't find the first error all\nthat useful. In this instance, there's really only one way for the OIDL\nchunk to be corrupt, which is that it has the wrong size.\n\nAnd short of making the error much more robust, e.g.:\n\n error: commit-graph OID lookup chunk is the wrong size (got: $X, wanted: $Y)\n\nand dropping the generic \"missing or corrupt\" error, I think that just\nthe generic error is fine here.\n\n> -Peff\n\nThanks,\nTaylor\n"},{"id":"484669","messageId":"cover.1699569246.git.me@ttaylorr.com","threadId":"60496","inReplyTo":"20231109070310.GA2697602@coredump.intra.peff.net","subject":"[PATCH 0/7] chunk-format: introduce `pair_chunk_expect()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-11-09T22:34:09Z","receivedAt":"2023-11-09T22:34:11Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"This series is based on the latest patches in what I am assuming will\nbecome \"jk/chunk-bounds\", found here [1].\n\nThis is a rework of my earlier round of patches in [2], which introduces\na new `pair_chunk_expect()` to replace some of the trivial callbacks\ngiven to `read_chunk()` that assert on the size of the expected chunk.\n\nLet me know what you think, and thanks in advance for your review!\n\n[1]: https://lore.kernel.org/git/20231109070310.GA2697602@coredump.intra.peff.net/\n[2]: https://lore.kernel.org/git/cover.1697225110.git.me@ttaylorr.com/\n\nTaylor Blau (7):\n chunk-format: introduce `pair_chunk_expect()` helper\n commit-graph: read `OIDL` 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 `OIDL` chunk with `pair_chunk_expect()`\n midx: read `OOFF` chunk with `pair_chunk_expect()`\n\n chunk-format.c | 29 ++++++++++++++++\n chunk-format.h | 13 ++++++-\n commit-graph.c | 67 ++++++++++---------------------------\n midx.c | 33 +++---------------\n t/t5318-commit-graph.sh | 6 ++--\n t/t5319-multi-pack-index.sh | 2 --\n 6 files changed, 65 insertions(+), 85 deletions(-)\n\n\nbase-commit: 8be77c5de65442b331a28d63802c7a3b94a06c5a\nprerequisite-patch-id: 507f1dfd74fae351883612048d334ed750db8b8c\nprerequisite-patch-id: c5eef290abc1d28950b3ee8729ea86d2e1773027\nprerequisite-patch-id: 0853baab4862833faee8ade3b1b63ee3aa406224\nprerequisite-patch-id: 6dd32f90fd87aa92f7d0661414cdaab257bd14b7\nprerequisite-patch-id: b0e1617c501a011c703605e59dd5eba89f8a3573\nprerequisite-patch-id: 906426f565c470dc86d7e7379d25ecfbd2bc30ce\nprerequisite-patch-id: 970cd79d1911bde36b88c340f001266d7e8d843b\nprerequisite-patch-id: 083a24abc73a83345bd9e4ca714577990dd9b08b\nprerequisite-patch-id: 210371a338dfe9b6f905d8821501aa9c235c8722\n-- \n2.43.0.rc0.39.g44bd344727\n"},{"id":"484670","messageId":"af5fe3b7237caeba8f970e967933db96c83a230e.1699569246.git.me@ttaylorr.com","threadId":"60496","inReplyTo":"cover.1699569246.git.me@ttaylorr.com","subject":"[PATCH 1/7] chunk-format: introduce `pair_chunk_expect()` helper","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-11-09T22:34:11Z","receivedAt":"2023-11-09T22:34:14Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In 570b8b8836 (chunk-format: note that pair_chunk() is unsafe,\n2023-10-09), the pair_chunk() interface grew a required \"size\" pointer,\nso that the caller is forced to at least have a handle on the actual\nsize 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_lookup(const unsigned char *chunk_start,\n size_t chunk_size, void *data)\n {\n struct commit_graph *g = data;\n g->chunk_oid_lookup = chunk_start;\n if (chunk_size / g->hash_len != g->num_commits)\n return error(_(\"commit-graph OID lookup chunk is the wrong size\"));\n return 0;\n }\n\n, leaving the caller to use read_chunk(), like so:\n\n read_chunk(cf, GRAPH_CHUNKID_OIDLOOKUP, graph_read_oid_lookup, graph);\n\nThe callback to read_chunk() (in the above, `graph_read_oid_lookup()`)\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 (most\noften because they are made up of a known number of fixed-size records),\nwe can 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 pair of \"size_t\"s (corresponding to the record size and count),\ninstead of a \"size_t *\", and validates that the given chunk matches the\nexpected 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_OIDLOOKUP,\n &graph->chunk_oid_lookup,\n graph->hash_len, graph->num_commits))\n error(_(\"commit-graph oid lookup chunk is wrong size\"));\n\nWe will perform those transformations in the following commits.\n\nHelped-by: Jeff King \nSigned-off-by: Taylor Blau \n---\n chunk-format.c | 29 +++++++++++++++++++++++++++++\n chunk-format.h | 13 ++++++++++++-\n 2 files changed, 41 insertions(+), 1 deletion(-)\n\ndiff --git a/chunk-format.c b/chunk-format.c\nindex cdc7f39b70..be078dcca8 100644\n--- a/chunk-format.c\n+++ b/chunk-format.c\n@@ -163,6 +163,10 @@ int read_table_of_contents(struct chunkfile *cf,\n struct pair_chunk_data {\n \tconst unsigned char **p;\n \tsize_t *size;\n+\n+\t/* for pair_chunk_expect() only */\n+\tsize_t record_size;\n+\tsize_t record_nr;\n };\n \n static int pair_chunk_fn(const unsigned char *chunk_start,\n@@ -175,6 +179,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 (chunk_size / pcd->record_size != pcd->record_nr)\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 +199,20 @@ 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 record_size,\n+\t\t size_t record_nr)\n+{\n+\tstruct pair_chunk_data pcd = {\n+\t\t.p = p,\n+\t\t.record_size = record_size,\n+\t\t.record_nr = record_nr,\n+\t};\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..10806d7a9a 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,16 @@ 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 are reading a chunk\n+ * with a known number of fixed-width records.\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 record_size,\n+\t\t size_t record_nr);\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.43.0.rc0.39.g44bd344727\n\n"},{"id":"484671","messageId":"5b3c0b99f8052733bb714122582ab229556c94ef.1699569246.git.me@ttaylorr.com","threadId":"60496","inReplyTo":"cover.1699569246.git.me@ttaylorr.com","subject":"[PATCH 2/7] commit-graph: read `OIDL` chunk with `pair_chunk_expect()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-11-09T22:34:14Z","receivedAt":"2023-11-09T22:34:16Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"The `OIDL` chunk can benefit from the new chunk-format API function\ndescribed in the previous commit. Convert it to `pair_chunk_expect()`\naccordingly.\n\nWhile here, clean up some of the duplicate error messages to simplify\nthe output when we are missing or have a corrupt OIDL chunk.\n\nSigned-off-by: Taylor Blau \n---\n commit-graph.c | 16 ++++------------\n t/t5318-commit-graph.sh | 3 +--\n 2 files changed, 5 insertions(+), 14 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 93cf690565..6072c2a17f 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -299,16 +299,6 @@ static int graph_read_oid_fanout(const unsigned char *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-\tstruct commit_graph *g = data;\n-\tg->chunk_oid_lookup = chunk_start;\n-\tif (chunk_size / g->hash_len != g->num_commits)\n-\t\treturn error(_(\"commit-graph OID lookup chunk is the wrong size\"));\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@@ -435,8 +425,10 @@ struct commit_graph *parse_commit_graph(struct repo_settings *s,\n \t\terror(_(\"commit-graph required OID fanout chunk missing or corrupted\"));\n \t\tgoto free_and_return;\n \t}\n-\tif (read_chunk(cf, GRAPH_CHUNKID_OIDLOOKUP, graph_read_oid_lookup, graph)) {\n-\t\terror(_(\"commit-graph required OID lookup chunk missing or corrupted\"));\n+\tif (pair_chunk_expect(cf, GRAPH_CHUNKID_OIDLOOKUP,\n+\t\t\t &graph->chunk_oid_lookup, graph->hash_len,\n+\t\t\t graph->num_commits)) {\n+\t\terror(_(\"commit-graph OID lookup chunk is the wrong size\"));\n \t\tgoto free_and_return;\n \t}\n \tif (read_chunk(cf, GRAPH_CHUNKID_DATA, graph_read_commit_data, graph)) {\ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex b0d436a6f0..b3e8af018d 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -545,7 +545,7 @@ test_expect_success 'detect missing OID fanout chunk' '\n \n test_expect_success 'detect missing OID lookup chunk' '\n \tcorrupt_graph_and_verify $GRAPH_BYTE_OID_LOOKUP_ID \"\\0\" \\\n-\t\t\"commit-graph required OID lookup chunk missing or corrupted\"\n+\t\t\"commit-graph OID lookup chunk is the wrong size\"\n '\n \n test_expect_success 'detect missing commit data chunk' '\n@@ -851,7 +851,6 @@ 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 lookup chunk is the wrong size\n-\terror: commit-graph required OID lookup chunk missing or corrupted\n \tEOF\n \ttest_cmp expect.err err\n '\n-- \n2.43.0.rc0.39.g44bd344727\n\n"},{"id":"484672","messageId":"400a2ee7f464df0b659ee9ac2f5e112d65e4ff9a.1699569246.git.me@ttaylorr.com","threadId":"60496","inReplyTo":"cover.1699569246.git.me@ttaylorr.com","subject":"[PATCH 3/7] commit-graph: read `CDAT` chunk with `pair_chunk_expect()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-11-09T22:34:18Z","receivedAt":"2023-11-09T22:34:21Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"The `CDAT` chunk can benefit from the new chunk-format API function\ndescribed in the previous commit. Convert it to `pair_chunk_expect()`\naccordingly.\n\nSigned-off-by: Taylor Blau \n---\n commit-graph.c | 15 +++------------\n t/t5318-commit-graph.sh | 3 +--\n 2 files changed, 4 insertions(+), 14 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 6072c2a17f..67ab0f2448 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -299,16 +299,6 @@ static int graph_read_oid_fanout(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 / GRAPH_DATA_WIDTH != g->num_commits)\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@@ -431,8 +421,9 @@ struct commit_graph *parse_commit_graph(struct repo_settings *s,\n \t\terror(_(\"commit-graph OID lookup chunk is the wrong size\"));\n \t\tgoto free_and_return;\n \t}\n-\tif (read_chunk(cf, GRAPH_CHUNKID_DATA, graph_read_commit_data, graph)) {\n-\t\terror(_(\"commit-graph required commit data chunk missing or corrupted\"));\n+\tif (pair_chunk_expect(cf, GRAPH_CHUNKID_DATA, &graph->chunk_commit_data,\n+\t\t\t GRAPH_DATA_WIDTH, graph->num_commits)) {\n+\t\terror(_(\"commit-graph commit data chunk is wrong size\"));\n \t\tgoto free_and_return;\n \t}\n \ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex b3e8af018d..fd5165bd07 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -550,7 +550,7 @@ test_expect_success 'detect missing OID lookup chunk' '\n \n test_expect_success 'detect missing commit data chunk' '\n \tcorrupt_graph_and_verify $GRAPH_BYTE_COMMIT_DATA_ID \"\\0\" \\\n-\t\t\"commit-graph required commit data chunk missing or corrupted\"\n+\t\t\"commit-graph commit data chunk is wrong size\"\n '\n \n test_expect_success 'detect incorrect fanout' '\n@@ -875,7 +875,6 @@ 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 required commit data chunk missing or corrupted\n \tEOF\n \ttest_cmp expect.err err\n '\n-- \n2.43.0.rc0.39.g44bd344727\n\n"},{"id":"484673","messageId":"dcde17455ceb135b5298e86e8256e62cf05ed124.1699569246.git.me@ttaylorr.com","threadId":"60496","inReplyTo":"cover.1699569246.git.me@ttaylorr.com","subject":"[PATCH 4/7] commit-graph: read `GDAT` chunk with `pair_chunk_expect()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-11-09T22:34:21Z","receivedAt":"2023-11-09T22:34:24Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"The `GDAT` chunk can benefit from the new chunk-format API function\ndescribed in the previous commit. Convert it to `pair_chunk_expect()`\naccordingly.\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 67ab0f2448..3b456d199f 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -299,16 +299,6 @@ static int graph_read_oid_fanout(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 / sizeof(uint32_t) != g->num_commits)\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@@ -433,8 +423,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 sizeof(uint32_t),\n+\t\t\t\t graph->num_commits))\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.43.0.rc0.39.g44bd344727\n\n"},{"id":"484674","messageId":"1f556f5b02d69bed688cc290d22ed8b7e646c4a7.1699569246.git.me@ttaylorr.com","threadId":"60496","inReplyTo":"cover.1699569246.git.me@ttaylorr.com","subject":"[PATCH 5/7] commit-graph: read `BIDX` chunk with `pair_chunk_expect()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-11-09T22:34:24Z","receivedAt":"2023-11-09T22:34:26Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"The `BIDX` chunk can benefit from the new chunk-format API function\ndescribed in the previous commit. Convert it to `pair_chunk_expect()`\naccordingly.\n\nSigned-off-by: Taylor Blau \n---\n commit-graph.c | 19 +++++--------------\n 1 file changed, 5 insertions(+), 14 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 3b456d199f..76d220508b 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -299,18 +299,6 @@ static int graph_read_oid_fanout(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 / 4 != g->num_commits) {\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@@ -437,8 +425,11 @@ 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\tint res = pair_chunk_expect(cf, GRAPH_CHUNKID_BLOOMINDEXES,\n+\t\t\t\t &graph->chunk_bloom_indexes,\n+\t\t\t\t sizeof(uint32_t), graph->num_commits);\n+\t\tif (res && res != CHUNK_NOT_FOUND)\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.43.0.rc0.39.g44bd344727\n\n"},{"id":"484675","messageId":"c2b0e54241658af55d4ad3d3d600c7059f9bd33c.1699569246.git.me@ttaylorr.com","threadId":"60496","inReplyTo":"cover.1699569246.git.me@ttaylorr.com","subject":"[PATCH 6/7] midx: read `OIDL` chunk with `pair_chunk_expect()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-11-09T22:34:26Z","receivedAt":"2023-11-09T22:34:29Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"The `OIDL` chunk can benefit from the new chunk-format API function\ndescribed in the previous commit. Convert it to `pair_chunk_expect()`\naccordingly.\n\nSigned-off-by: Taylor Blau \n---\n midx.c | 16 ++--------------\n t/t5319-multi-pack-index.sh | 1 -\n 2 files changed, 2 insertions(+), 15 deletions(-)\n\ndiff --git a/midx.c b/midx.c\nindex 1d14661dad..66f34ed1a3 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -86,19 +86,6 @@ static int midx_read_oid_fanout(const unsigned char *chunk_start,\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 static int midx_read_object_offsets(const unsigned char *chunk_start,\n \t\t\t\t size_t chunk_size, void *data)\n {\n@@ -186,7 +173,8 @@ 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 (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 m->hash_len, m->num_objects))\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 \t\tdie(_(\"multi-pack-index required object offsets chunk missing or corrupted\"));\ndiff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\nindex 313496c0cf..2d68616c59 100755\n--- a/t/t5319-multi-pack-index.sh\n+++ b/t/t5319-multi-pack-index.sh\n@@ -1077,7 +1077,6 @@ 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-- \n2.43.0.rc0.39.g44bd344727\n\n"},{"id":"484676","messageId":"44bd344727622b86a00424f37dbaa0c35d85189a.1699569246.git.me@ttaylorr.com","threadId":"60496","inReplyTo":"cover.1699569246.git.me@ttaylorr.com","subject":"[PATCH 7/7] midx: read `OOFF` chunk with `pair_chunk_expect()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-11-09T22:34:29Z","receivedAt":"2023-11-09T22:34:31Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"The `OOFF` chunk can benefit from the new chunk-format API function\ndescribed in the previous commit. Convert it to `pair_chunk_expect()`\naccordingly.\n\nSigned-off-by: Taylor Blau \n---\n midx.c | 17 +++--------------\n t/t5319-multi-pack-index.sh | 1 -\n 2 files changed, 3 insertions(+), 15 deletions(-)\n\ndiff --git a/midx.c b/midx.c\nindex 66f34ed1a3..ca41748b74 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -86,19 +86,6 @@ static int midx_read_oid_fanout(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@@ -176,7 +163,9 @@ struct multi_pack_index *load_multi_pack_index(const char *object_dir, int local\n \tif (pair_chunk_expect(cf, MIDX_CHUNKID_OIDLOOKUP, &m->chunk_oid_lookup,\n \t\t\t m->hash_len, m->num_objects))\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+\tif (pair_chunk_expect(cf, MIDX_CHUNKID_OBJECTOFFSETS,\n+\t\t\t &m->chunk_object_offsets, MIDX_CHUNK_OFFSET_WIDTH,\n+\t\t\t m->num_objects))\n \t\tdie(_(\"multi-pack-index required object offsets chunk missing or corrupted\"));\n \n \tpair_chunk(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 2d68616c59..f1f6764efe 100755\n--- a/t/t5319-multi-pack-index.sh\n+++ b/t/t5319-multi-pack-index.sh\n@@ -1111,7 +1111,6 @@ 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-- \n2.43.0.rc0.39.g44bd344727\n"},{"id":"484688","messageId":"xmqqedgyw6jv.fsf@gitster.g","threadId":"60496","inReplyTo":"af5fe3b7237caeba8f970e967933db96c83a230e.1699569246.git.me@ttaylorr.com","subject":"Re: [PATCH 1/7] chunk-format: introduce `pair_chunk_expect()` helper","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-11-10T04:55:48Z","receivedAt":"2023-11-10T06:13:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\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 (chunk_size / pcd->record_size != pcd->record_nr)\n> +\t\treturn -1;\n> +\t*pcd->p = chunk_start;\n> +\treturn 0;\n> +}\n\nI know one of the original places did the \"divide the whole by\nper-record size and see if it matches the number of records\", the\nsame as we see above, but the check in the above could also be\n\n\tif (chunk_size != st_mult(pcd->record_size, pcd->record_nr))\n\t\treturn -1;\n\nwhich would also catch the case where chunk_size is not a multiple\nof the record size. Your conversion of OOFF in midx.c loses this\nprotection as the original uses the multiplication-and-compare, but\nthe rewrite to call pair_chunk_expect would call the above and\nchecks with the truncating-divide-and-compare.\n\nDoes the distinction matter? I dunno. If the record/chunk\nalignment is asserted elsewhere, then the distinction should not\nmatter, but even if it were, seeing a truncating division used in\nany validation makes my skin tingle.\n\nOther than that, the series was a pleasant read.\n\nThanks.\n\n\n"},{"id":"484710","messageId":"ZU5Z/Z4PcdNP5U1r@nand.local","threadId":"60496","inReplyTo":"xmqqedgyw6jv.fsf@gitster.g","subject":"Re: [PATCH 1/7] chunk-format: introduce `pair_chunk_expect()` helper","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-11-10T16:27:41Z","receivedAt":"2023-11-10T16:27:44Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Nov 10, 2023 at 01:55:48PM +0900, Junio C Hamano wrote:\n> Taylor Blau writes:\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 (chunk_size / pcd->record_size != pcd->record_nr)\n> > +\t\treturn -1;\n> > +\t*pcd->p = chunk_start;\n> > +\treturn 0;\n> > +}\n>\n> I know one of the original places did the \"divide the whole by\n> per-record size and see if it matches the number of records\", the\n> same as we see above, but the check in the above could also be\n>\n> \tif (chunk_size != st_mult(pcd->record_size, pcd->record_nr))\n> \t\treturn -1;\n>\n> which would also catch the case where chunk_size is not a multiple\n> of the record size. Your conversion of OOFF in midx.c loses this\n> protection as the original uses the multiplication-and-compare, but\n> the rewrite to call pair_chunk_expect would call the above and\n> checks with the truncating-divide-and-compare.\n\nHmm. I was thinking of Peff's \"commit-graph: handle overflow in\nchunk_size checks\", but I think that I was overly eager in applying the\nsame reasoning to the MIDX code.\n\nThe important piece of the rationale in that patch is as follows:\n\n In the current code this is only possible for the CDAT chunk, but\n the reasons are quite subtle. We compute g->num_commits by dividing\n the size of the OIDL chunk by the hash length (since it consists of\n a bunch of hashes). So we know that any size_t multiplication that\n uses a value smaller than the hash length cannot overflow. And the\n CDAT records are the only ones that are larger (the others are just\n 4-byte records). So it's worth fixing all of these, to make it clear\n that they're not subject to overflow (without having to reason about\n seemingly unrelated code).\n\nIn particular, that g->num_commits is computed by dividing the length of\nthe OIDL chunk by the hash length, thus any size_t multiplication of\ng->num_commits with a value smaller than that hash length cannot\noverflow.\n\nBut I don't think we enjoy the same benefits in the MIDX scenario. In\nthis case, the num_objects field is just:\n\n m->num_objects = ntohl(m->chunk_oid_fanout[255])\n\nso I don't think we can make the same guarantees about what is and isn't\nsave to compute under size_t arithmetic.\n\nI'd be curious what Peff has to say, but if he agrees with me, I'd\nrecommend taking the first five patches, and dropping the two\nMIDX-related ones.\n\nThanks,\nTaylor\n"},{"id":"484719","messageId":"20231110215255.GF2758295@coredump.intra.peff.net","threadId":"60496","inReplyTo":"ZU1Z6WUrEhcxLBTO@nand.local","subject":"Re: [PATCH 6/9] commit-graph: use fanout value for graph size","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-11-10T21:52:55Z","receivedAt":"2023-11-10T21:52:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 09, 2023 at 05:15:05PM -0500, Taylor Blau wrote:\n\n> > This is one of the ways that pair_chunk_expect() could do better without\n> > adding a lot of code, because it can check the return value from\n> > read_chunk(). It doesn't help the other cases (like OIDF) that still\n> > have to call read_chunk() themselves, though. Possibly read_chunk()\n> > should just take a flag to indicate that it should complain when the\n> > chunk is missing. And then callers could just do:\n> >\n> > if (read_chunk(cf, id, our_callback, CHUNK_REQUIRED)\n> > \treturn -1; /* no need to complain; either our_callback() did, or\n> > \t read_chunk() itself */\n> \n> We do return CHUNK_NOT_FOUND when we have a missing chunk, which we\n> could check for individually. But TBH, I don't find the first error all\n> that useful. In this instance, there's really only one way for the OIDL\n> chunk to be corrupt, which is that it has the wrong size.\n\nBut aren't there two ways it can go wrong? It can be the wrong size, or\nit can be missing. In the first we say:\n\n error: wrong size\n error: missing or corrupted\n\nand in the second we say:\n\n error: missing or corrupted\n\nWhich is why I think issuing a message from the callback has value. Of\ncourse the ideal would be:\n\n error: wrong size\n\nand:\n\n error: missing\n\nbut I didn't think it was worth making the code much longer for an error\ncondition we don't really expect anybody to see in practice.\n\nAnd also...\n\n> And short of making the error much more robust, e.g.:\n> \n> error: commit-graph OID lookup chunk is the wrong size (got: $X, wanted: $Y)\n\n...yes, I think that would be worth doing, especially if you are\ncentralizing the error messages in pair_chunk_expect(). But your series\ngoes the opposite direction.\n\n> and dropping the generic \"missing or corrupt\" error, I think that just\n> the generic error is fine here.\n\nIf you drop that error, then who will warn about the missing-chunk case?\nThe user would see nothing at all.\n\n-Peff\n"},{"id":"484720","messageId":"20231110215747.GG2758295@coredump.intra.peff.net","threadId":"60496","inReplyTo":"xmqqedgyw6jv.fsf@gitster.g","subject":"Re: [PATCH 1/7] chunk-format: introduce `pair_chunk_expect()` helper","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-11-10T21:57:47Z","receivedAt":"2023-11-10T21:57:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 10, 2023 at 01:55:48PM +0900, Junio C Hamano wrote:\n\n> Taylor Blau writes:\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 (chunk_size / pcd->record_size != pcd->record_nr)\n> > +\t\treturn -1;\n> > +\t*pcd->p = chunk_start;\n> > +\treturn 0;\n> > +}\n> \n> I know one of the original places did the \"divide the whole by\n> per-record size and see if it matches the number of records\", the\n> same as we see above, but the check in the above could also be\n> \n> \tif (chunk_size != st_mult(pcd->record_size, pcd->record_nr))\n> \t\treturn -1;\n> \n> which would also catch the case where chunk_size is not a multiple\n> of the record size. Your conversion of OOFF in midx.c loses this\n> protection as the original uses the multiplication-and-compare, but\n> the rewrite to call pair_chunk_expect would call the above and\n> checks with the truncating-divide-and-compare.\n> \n> Does the distinction matter? I dunno. If the record/chunk\n> alignment is asserted elsewhere, then the distinction should not\n> matter, but even if it were, seeing a truncating division used in\n> any validation makes my skin tingle.\n\nYes, the distinction does matter. If pair_chunk_expect_fn() used\nst_mult(), then it would die on overflow, rather than returning an\nerror. For commit-graph files this is propagated up, and we continue the\noperation by falling back to the non-graph code. There's a test in t5318\nthat will fail in this case. See patches 1 and 6 in my series for more\ndiscussion.\n\nOne of those patches calls out the truncating division issue, but to\nsummarize: IMHO this is OK, as what we really want to know is \"is it big\nenough that we can always ask for NR records of size ELEM\", which\ndivision gives us. If we do want to be more precise, but also avoid\ndie(), we'd need a variant of st_mult() that returns a boolean. I didn't\nthink it was worth it for this case (but arguably it is something that\nwould be useful to have in general).\n\n-Peff\n"},{"id":"484721","messageId":"20231110220142.GH2758295@coredump.intra.peff.net","threadId":"60496","inReplyTo":"ZU5Z/Z4PcdNP5U1r@nand.local","subject":"Re: [PATCH 1/7] chunk-format: introduce `pair_chunk_expect()` helper","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-11-10T22:01:42Z","receivedAt":"2023-11-10T22:01:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 10, 2023 at 11:27:41AM -0500, Taylor Blau wrote:\n\n> Hmm. I was thinking of Peff's \"commit-graph: handle overflow in\n> chunk_size checks\", but I think that I was overly eager in applying the\n> same reasoning to the MIDX code.\n> \n> The important piece of the rationale in that patch is as follows:\n> \n> In the current code this is only possible for the CDAT chunk, but\n> the reasons are quite subtle. We compute g->num_commits by dividing\n> the size of the OIDL chunk by the hash length (since it consists of\n> a bunch of hashes). So we know that any size_t multiplication that\n> uses a value smaller than the hash length cannot overflow. And the\n> CDAT records are the only ones that are larger (the others are just\n> 4-byte records). So it's worth fixing all of these, to make it clear\n> that they're not subject to overflow (without having to reason about\n> seemingly unrelated code).\n> \n> In particular, that g->num_commits is computed by dividing the length of\n> the OIDL chunk by the hash length, thus any size_t multiplication of\n> g->num_commits with a value smaller than that hash length cannot\n> overflow.\n> \n> But I don't think we enjoy the same benefits in the MIDX scenario. In\n> this case, the num_objects field is just:\n> \n> m->num_objects = ntohl(m->chunk_oid_fanout[255])\n> \n> so I don't think we can make the same guarantees about what is and isn't\n> save to compute under size_t arithmetic.\n\nYes, the logic does not apply to the midx code (just like the graph code\nafter we switch to using the fanout value later in my series). But that\nparagraph was just explaining why only the CDAT chunk was _currently_\nvulnerable.\n\nThe more interesting question is the paragraphs after that, about\nwhether it is OK to die() or not when we see overflow (and IMHO it is\nnot for commit-graphs).\n\nThe situation _is_ different there for midx's, because we immediately\ndie() if we see a bogus midx anyway. But I think that is wrong, and\nsomething we may want to change in the long run. Both because it's the\nwrong thing for lib-ification, but also because we can easily keep going\nif the midx is broken; we can still use the individual pack idx files.\n\n> I'd be curious what Peff has to say, but if he agrees with me, I'd\n> recommend taking the first five patches, and dropping the two\n> MIDX-related ones.\n\nI think dropping those is a bad direction. The point of adding\npair_chunk_expect() is that we could use it consistently.\n\n-Peff\n"},{"id":"484722","messageId":"20231110220828.GI2758295@coredump.intra.peff.net","threadId":"60496","inReplyTo":"af5fe3b7237caeba8f970e967933db96c83a230e.1699569246.git.me@ttaylorr.com","subject":"Re: [PATCH 1/7] chunk-format: introduce `pair_chunk_expect()` helper","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-11-10T22:08:28Z","receivedAt":"2023-11-10T22:08:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 09, 2023 at 05:34:11PM -0500, Taylor Blau wrote:\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 (chunk_size / pcd->record_size != pcd->record_nr)\n> +\t\treturn -1;\n> +\t*pcd->p = chunk_start;\n> +\treturn 0;\n> +}\n\nI think this is taking us backwards in terms of the output the user sees\n(see the message I just sent elsewhere in the thread). The user won't be\nable to tell the difference between a missing chunk and one with the\nwrong size.\n\nAnd we miss the opportunity to give more details about the expected and\ndetected sizes of the chunks.\n\nIf the two-line error message really bothers you, then...\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 record_size,\n> +\t\t size_t record_nr)\n> +{\n> +\tstruct pair_chunk_data pcd = {\n> +\t\t.p = p,\n> +\t\t.record_size = record_size,\n> +\t\t.record_nr = record_nr,\n> +\t};\n> +\treturn read_chunk(cf, chunk_id, pair_chunk_expect_fn, &pcd);\n> +}\n\n...this is where we could take a CHUNK_REQUIRED flag, and so:\n\n ret = read_chunk(...);\n /* pair_chunk_expect_fn() wrote an error already for other cases */\n if (ret == CHUNK_MISSING)\n\terror(\"chunk not found\");\n return ret;\n\nAnd then the callers become a very nice:\n\n if (pair_chunk_expect(cf, id, &ptr, size, nr, CHUNK_REQUIRED))\n\treturn -1;\n\nYou probably would need to pass some kind of string giving more context\nfor the error messages, though. We can show the chunk id, but the\ncontext of \"commit-graph\" vs \"midx\" is important.\n\n-Peff\n"},{"id":"484723","messageId":"20231110220943.GJ2758295@coredump.intra.peff.net","threadId":"60496","inReplyTo":"20231110215747.GG2758295@coredump.intra.peff.net","subject":"Re: [PATCH 1/7] chunk-format: introduce `pair_chunk_expect()` helper","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-11-10T22:09:43Z","receivedAt":"2023-11-10T22:09:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 10, 2023 at 04:57:47PM -0500, Jeff King wrote:\n\n> One of those patches calls out the truncating division issue, but to\n> summarize: IMHO this is OK, as what we really want to know is \"is it big\n> enough that we can always ask for NR records of size ELEM\", which\n> division gives us. If we do want to be more precise, but also avoid\n> die(), we'd need a variant of st_mult() that returns a boolean. I didn't\n> think it was worth it for this case (but arguably it is something that\n> would be useful to have in general).\n\nOh, and obviously there is another option here if we want to be more\ncareful but don't want to introduce an st_mult() variant: we can use \"%\"\nto check for divisibility ourselves.\n\nI don't think it's worth doing that in every individual size-check, but\nmaybe it would be in a central pair_chunk_expect().\n\n-Peff\n"},{"id":"484724","messageId":"20231110221041.GK2758295@coredump.intra.peff.net","threadId":"60496","inReplyTo":"5b3c0b99f8052733bb714122582ab229556c94ef.1699569246.git.me@ttaylorr.com","subject":"Re: [PATCH 2/7] commit-graph: read `OIDL` chunk with `pair_chunk_expect()`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-11-10T22:10:41Z","receivedAt":"2023-11-10T22:10:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 09, 2023 at 05:34:14PM -0500, Taylor Blau wrote:\n\n> @@ -435,8 +425,10 @@ struct commit_graph *parse_commit_graph(struct repo_settings *s,\n> \t\terror(_(\"commit-graph required OID fanout chunk missing or corrupted\"));\n> \t\tgoto free_and_return;\n> \t}\n> -\tif (read_chunk(cf, GRAPH_CHUNKID_OIDLOOKUP, graph_read_oid_lookup, graph)) {\n> -\t\terror(_(\"commit-graph required OID lookup chunk missing or corrupted\"));\n> +\tif (pair_chunk_expect(cf, GRAPH_CHUNKID_OIDLOOKUP,\n> +\t\t\t &graph->chunk_oid_lookup, graph->hash_len,\n> +\t\t\t graph->num_commits)) {\n> +\t\terror(_(\"commit-graph OID lookup chunk is the wrong size\"));\n> \t\tgoto free_and_return;\n\nI know the original message was vague, but I think the new one is\nactively misleading in the case of a missing chunk. We'll say \"wrong\nsize\", but it was not present at all!\n\n-Peff\n"},{"id":"484728","messageId":"xmqqa5rluqk1.fsf@gitster.g","threadId":"60496","inReplyTo":"ZU5Z/Z4PcdNP5U1r@nand.local","subject":"Re: [PATCH 1/7] chunk-format: introduce `pair_chunk_expect()` helper","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-11-10T23:38:54Z","receivedAt":"2023-11-10T23:39:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> But I don't think we enjoy the same benefits in the MIDX scenario. In\n> this case, the num_objects field is just:\n>\n> m->num_objects = ntohl(m->chunk_oid_fanout[255])\n>\n> so I don't think we can make the same guarantees about what is and isn't\n> save to compute under size_t arithmetic.\n\nSo ..., from the correctness's point of view, if we do not mind\nst_mult() dying, the \"multiply-and-compare\" should give us much more\nrobust results. If we do mind st_mult() dying, we could pair the\n\"truncating-division-and-compare\" you wrote with \"ensure that chunk\nsize is a multiple of record size\" to achieve the same, I would\nthink. I.e.,\n\n\tif (chunk_size % pcd->record_size ||\n\t chunk_size / pcd->record_size != pcd->record_nr)\n\t\treturn -1;\n\nor something like that.\n"},{"id":"484729","messageId":"xmqq5y29uqi9.fsf@gitster.g","threadId":"60496","inReplyTo":"20231110220142.GH2758295@coredump.intra.peff.net","subject":"Re: [PATCH 1/7] chunk-format: introduce `pair_chunk_expect()` helper","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-11-10T23:39:58Z","receivedAt":"2023-11-10T23:40:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I think dropping those is a bad direction. The point of adding\n> pair_chunk_expect() is that we could use it consistently.\n\nI think so, too. If we are adding a helper to avoid common\nmistakes, and if it cannot be used in some situations, then the\nhelper needs to be improved.\n"},{"id":"486818","messageId":"owly8r4qi5zj.fsf@fine.c.googlers.com","threadId":"60496","inReplyTo":"af5fe3b7237caeba8f970e967933db96c83a230e.1699569246.git.me@ttaylorr.com","subject":"Re: [PATCH 1/7] chunk-format: introduce `pair_chunk_expect()` helper","fromName":"Linus Arver","fromEmail":"linusa@google.com","sentAt":"2024-01-15T22:31:12Z","receivedAt":"2024-01-15T22:31:14Z","isPatch":true,"sender":{"key":"linus@ucla.edu","avatar":null},"body":"\nTaylor Blau writes:\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 (chunk_size / pcd->record_size != pcd->record_nr)\n> +\t\treturn -1;\n> +\t*pcd->p = chunk_start;\n> +\treturn 0;\n> +}\n> +\n\nI don't think this function should assume anything about the inputs\n(chunk_size, record_size, nor record_nr). If we are asking the helper to\nbe the common tool for multiple locations, it should assume even less\nabout what these inputs could look like.\n\nFor example, if record_size is 0 this will lead to a\ndivide-by-zero. Likewise, if record_nr is zero it doesn't make\nsense to check if chunk_size fits into record_size * record_nr.\n\nAnd even if there are no (unexpected) zero-values involved, shouldn't we\nalso check for nonsensical comparisons, such as if chunk_size is smaller\nthan record_size?\n\nI think there are several possibilities:\n\nCHUNK_MISSING (chunk_size == 0)\nCHUNK_TOO_SMALL (chunk_size < record_size)\nCHUNK_OK (chunk_size == record_nr * record_size)\nCHUNK_TOO_BIG (chunk_size > record_size, record_nr * record_size < chunk_size)\n\nAnd for the CHUNK_TOO_BIG case there are two possibilities --- the\nleftover parts of chunk_size that are not accounted by \"record_nr *\nrecord_size\" are (or are not) \"aligned\" such that increasing the\nrecord_size by some positive increment could exactly match the\nchunk_size. For example, chunk_size = 128, record_size = 8, record_nr =\n8. In this case the records account for 64 bytes so we have 128 - 64 =\n64 bytes left over, and simply increasing record_nr to 16 could account\nfor every bytes in chunk_size. If chunk_size in this example was 120 or\n130, there would be bytes left over that cannot be accounted for by\nadjusting record_size. This divisibility-of-leftover-bytes check could\nbe done with '%' as mentioned already by others.\n\nSo in summary there appear to be the following possibilities:\n\nCHUNK_MISSING\nCHUNK_TOO_SMALL\nCHUNK_OK\nCHUNK_TOO_BIG_ALIGNED\nCHUNK_TOO_BIG_MISALIGNED\n\n(on top of the cases where record_* inputs are zero).\n\nAnd it seems prudent to treat each of the not-OK cases differently\n(including how we report error messages) once we know which category we\nfall into.\n"},{"id":"486819","messageId":"owly4jfei4xk.fsf@fine.c.googlers.com","threadId":"60496","inReplyTo":"owly8r4qi5zj.fsf@fine.c.googlers.com","subject":"Re: [PATCH 1/7] chunk-format: introduce `pair_chunk_expect()` helper","fromName":"Linus Arver","fromEmail":"linusa@google.com","sentAt":"2024-01-15T22:53:59Z","receivedAt":"2024-01-15T22:54:01Z","isPatch":true,"sender":{"key":"linus@ucla.edu","avatar":null},"body":"Linus Arver writes:\n\n> So in summary there appear to be the following possibilities:\n>\n> CHUNK_MISSING\n> CHUNK_TOO_SMALL\n> CHUNK_OK\n> CHUNK_TOO_BIG_ALIGNED\n> CHUNK_TOO_BIG_MISALIGNED\n\nOn second thought, it appears that CHUNK_TOO_SMALL has three cases:\n\n(1) chunk_size < record_size\n(2) chunk_size >= record_size, but chunk_size < record_size * record_nr\n and decreasing record_nr can make chunk_size \"fit\"\n(3) chunk_size >= record_size, but chunk_size < record_size * record_nr\n and decreasing record_nr cannot make chunk_size \"fit\"\n\nwhere (2) and (3) are just like the *_(MIS)ALIGNED cases above when\nchunk_size is too big.\n\nMy default position is that these additional cases might need to be\ntreated differently, although maybe it's overkill also.\n\nThanks.\n"},{"id":"486842","messageId":"20240116151029.GC2119690@coredump.intra.peff.net","threadId":"60496","inReplyTo":"owly8r4qi5zj.fsf@fine.c.googlers.com","subject":"Re: [PATCH 1/7] chunk-format: introduce `pair_chunk_expect()` helper","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-01-16T15:10:29Z","receivedAt":"2024-01-16T15:10:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jan 15, 2024 at 02:31:12PM -0800, Linus Arver wrote:\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 (chunk_size / pcd->record_size != pcd->record_nr)\n> > +\t\treturn -1;\n> > +\t*pcd->p = chunk_start;\n> > +\treturn 0;\n> > +}\n> > +\n> \n> I don't think this function should assume anything about the inputs\n> (chunk_size, record_size, nor record_nr). If we are asking the helper to\n> be the common tool for multiple locations, it should assume even less\n> about what these inputs could look like.\n>\n> For example, if record_size is 0 this will lead to a\n> divide-by-zero. Likewise, if record_nr is zero it doesn't make\n> sense to check if chunk_size fits into record_size * record_nr.\n\nI'm not sure that divide-by-zero is a big deal, because 0-length\nfixed-size records do not make any sense to ask about. And even if\nsomebody accidentally passed 0, even though it won't be caught by the\ncompiler, it would barf on any input, so even rudimentary testing would\ncatch it.\n\nA zero record_nr is a perfectly reasonable thing to ask about. If you\nhave a chunk file with zero entries for some entity, then we are\nchecking that the chunk is the expected zero length as well.\n\n> And even if there are no (unexpected) zero-values involved, shouldn't we\n> also check for nonsensical comparisons, such as if chunk_size is smaller\n> than record_size?\n\nAren't we checking that already? If chunk_size is less than record_size,\nthen the division will result in \"0\". If the expected number of records\nis not also 0, then that's a bogus file.\n\nWhat we really care about here is that we won't walk off the end of the\nbuffer while looking at N fixed-size records (in that sense, the \"too\nbig\" test is overly careful, but it's easy to include as a sanity\ncheck).\n\n> So in summary there appear to be the following possibilities:\n> \n> CHUNK_MISSING\n> CHUNK_TOO_SMALL\n> CHUNK_OK\n> CHUNK_TOO_BIG_ALIGNED\n> CHUNK_TOO_BIG_MISALIGNED\n\nSo yes, I agree all these cases exist. We detect them all except the\n\"misaligned\" case (which I think was discussed earlier in the thread as\nnot worth caring too much about).\n\nBut...\n\n> (on top of the cases where record_* inputs are zero).\n> \n> And it seems prudent to treat each of the not-OK cases differently\n> (including how we report error messages) once we know which category we\n> fall into.\n\nI'm not sure it is worth caring too much about the different cases. From\nthe caller's perspective the end result is always to avoid using the\nchunk/file. From the user's perspective, any error of the form \"we\nexpected N bytes and got X\" is plenty. Especially since record_nr\ntypically comes from another part of the file, we cannot tell if our\nchunk is too big/small, or if another chunk gave us a bogus record_nr.\n\n-Peff\n"},{"id":"487017","messageId":"owlyy1cmgpm4.fsf@fine.c.googlers.com","threadId":"60496","inReplyTo":"20240116151029.GC2119690@coredump.intra.peff.net","subject":"Re: [PATCH 1/7] chunk-format: introduce `pair_chunk_expect()` helper","fromName":"Linus Arver","fromEmail":"linusa@google.com","sentAt":"2024-01-18T23:59:15Z","receivedAt":"2024-01-18T23:59:17Z","isPatch":true,"sender":{"key":"linus@ucla.edu","avatar":null},"body":"Jeff King writes:\n\n> On Mon, Jan 15, 2024 at 02:31:12PM -0800, Linus Arver wrote:\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 (chunk_size / pcd->record_size != pcd->record_nr)\n>> > +\t\treturn -1;\n>> > +\t*pcd->p = chunk_start;\n>> > +\treturn 0;\n>> > +}\n>> > +\n>> \n>> I don't think this function should assume anything about the inputs\n>> (chunk_size, record_size, nor record_nr). If we are asking the helper to\n>> be the common tool for multiple locations, it should assume even less\n>> about what these inputs could look like.\n>>\n>> For example, if record_size is 0 this will lead to a\n>> divide-by-zero. Likewise, if record_nr is zero it doesn't make\n>> sense to check if chunk_size fits into record_size * record_nr.\n>\n> I'm not sure that divide-by-zero is a big deal, because 0-length\n> fixed-size records do not make any sense to ask about.\n\nSo why not make this function check for this? While it may be true that\n0-length fixed-size records are impossible currently, nothing guarantees\nthey will always be that way all the time in the future.\n\n> And even if\n> somebody accidentally passed 0, even though it won't be caught by the\n> compiler, it would barf on any input, so even rudimentary testing would\n> catch it.\n\nIf somebody is accidentally passing an invalid value to a function, then\nit feels right for that function to be able to handle it instead of\ncrashing (or doing any other undefined behavior) with division-by-zero.\n\nTaking a step back though, maybe I am being overly defensive about\npossible failure modes. I don't know the surrounding area well so I\nmight be overreacting.\n\n> A zero record_nr is a perfectly reasonable thing to ask about. If you\n> have a chunk file with zero entries for some entity, then we are\n> checking that the chunk is the expected zero length as well.\n\nRight.\n\n>> And even if there are no (unexpected) zero-values involved, shouldn't we\n>> also check for nonsensical comparisons, such as if chunk_size is smaller\n>> than record_size?\n>\n> Aren't we checking that already? If chunk_size is less than record_size,\n> then the division will result in \"0\". If the expected number of records\n> is not also 0, then that's a bogus file.\n\nI was thinking of an early return like\n\n if (chunk_size < record_size)\n return CHUNK_TOO_SMALL\n\nbefore evaluating (chunk_size / pcd->record_size != pcd->record_nr).\nYou're correct that the division will result in \"0\" if chunk_size is\nless than record_size. But I didn't like having the extra mental load\nfor reading and understanding the correctness of \"if (chunk_size /\npcd->record_size != pcd->record_nr)\" for that case. IOW, the more early\nreturns we have for known-bad cases, by the time we get to \"if\n(chunk_size / pcd->record_size != pcd->record_nr)\" it would be that much\neasier to understand that code.\n \n> What we really care about here is that we won't walk off the end of the\n> buffer while looking at N fixed-size records ...\n\nAh, I see. This sort of insight would be great to have as a comment in\nthe code.\n\n> ... (in that sense, the \"too\n> big\" test is overly careful, but it's easy to include as a sanity\n> check).\n\nOK.\n\n>> So in summary there appear to be the following possibilities:\n>> \n>> CHUNK_MISSING\n>> CHUNK_TOO_SMALL\n>> CHUNK_OK\n>> CHUNK_TOO_BIG_ALIGNED\n>> CHUNK_TOO_BIG_MISALIGNED\n>\n> So yes, I agree all these cases exist. We detect them all except the\n> \"misaligned\" case (which I think was discussed earlier in the thread as\n> not worth caring too much about).\n\nOK.\n\n> But...\n>\n>> (on top of the cases where record_* inputs are zero).\n>> \n>> And it seems prudent to treat each of the not-OK cases differently\n>> (including how we report error messages) once we know which category we\n>> fall into.\n>\n> I'm not sure it is worth caring too much about the different cases. From\n> the caller's perspective the end result is always to avoid using the\n> chunk/file.\n\nAh OK. Then yes, it does seem like caring about the different cases is\ntoo much from the callers' perspective.\n\nBut I do think that checking the different cases with early returns will\nat least help readability (and as a bonus assure future Git developers\nthat divide-by-zero errors are impossible).\n"}]}