{"thread":{"id":"55995","subject":"[PATCH 1/4] csum-file: introduce checksum_valid()","startedAt":"2021-06-23T18:39:09Z","lastAt":"2021-11-16T22:13:05Z","messageCount":16,"participants":["Taylor Blau","Bagas Sanjaya","Jeff King","SZEDER Gábor","Derrick Stolee"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"428310","messageId":"cover.1624473543.git.me@ttaylorr.com","threadId":"55995","inReplyTo":null,"subject":"[PATCH 0/4] midx: verify MIDX checksum before reusing","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-06-23T18:39:04Z","receivedAt":"2021-06-23T18:39:09Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Here is a small series that came out of a corruption we noticed at GitHub. The\ncorruption is described in the third patch, but the gist is this:\n\n  - After writing a MIDX, we experience a disk failure that corrupts the\n    contents of the repository's MIDX.\n\n  - We later go to write another MIDX, notice that one is already there, and\n    then blindly reuse its data without checking its validity, resulting in a\n    MIDX that contains corrupt data, but whose trailing checksum matches the\n    preceding contents.\n\nThis series checks the validity of the trailing checksum when reusing a MIDX\nwhile writing a new one, as well as a few other related fix-ups. That should be\nsufficient to avoid propagating corrupt data forward while writing a MIDX.\n\nWe could go further here and run 'git multi-pack-index verify' on the existing\nMIDX while writing a new one, but the point of this reuse is to avoid having to\nre-compute/verify the contents of an existing MIDX. So a more lightweight option\nis pursued here, but alternatives are discussed in 3/4.\n\nThanks in advance for your review.\n\nTaylor Blau (4):\n  csum-file: introduce checksum_valid()\n  commit-graph: rewrite to use checksum_valid()\n  midx: don't reuse corrupt MIDXs when writing\n  midx: report checksum mismatches during 'verify'\n\n commit-graph.c              | 14 ++++++--------\n csum-file.c                 | 16 ++++++++++++++++\n csum-file.h                 |  3 +++\n midx.c                      | 13 +++++++++++++\n pack-check.c                | 11 +----------\n t/t5319-multi-pack-index.sh | 13 +++++++++++++\n 6 files changed, 52 insertions(+), 18 deletions(-)\n\n-- \n2.31.1.163.ga65ce7f831\n"},{"id":"428309","messageId":"3f1fcf348473d7320d46e3df6fe97c482b9888f2.1624473543.git.me@ttaylorr.com","threadId":"55995","inReplyTo":"cover.1624473543.git.me@ttaylorr.com","subject":"[PATCH 1/4] csum-file: introduce checksum_valid()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-06-23T18:39:07Z","receivedAt":"2021-06-23T18:39:10Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Introduce a new function which checks the validity of a file's trailing\nchecksum. This is similar to hashfd_check(), but different since it is\nintended to be used by callers who aren't writing the same data (like\n`git index-pack --verify`), but who instead want to validate the\nintegrity of data that they are reading.\n\nRewrite the first of two callers which could benefit from this new\nfunction in pack-check.c. Subsequent callers will be added in the\nfollowing patches.\n\nHelped-by: Jeff King <peff@peff.net>\nSigned-off-by: Jeff King <peff@peff.net>\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n csum-file.c  | 16 ++++++++++++++++\n csum-file.h  |  3 +++\n pack-check.c | 11 +----------\n 3 files changed, 20 insertions(+), 10 deletions(-)\n\ndiff --git a/csum-file.c b/csum-file.c\nindex 3487d28ed7..c951cf8277 100644\n--- a/csum-file.c\n+++ b/csum-file.c\n@@ -217,3 +217,19 @@ uint32_t crc32_end(struct hashfile *f)\n \tf->do_crc = 0;\n \treturn f->crc32;\n }\n+\n+int hashfile_checksum_valid(const unsigned char *data, size_t total_len)\n+{\n+\tunsigned char got[GIT_MAX_RAWSZ];\n+\tgit_hash_ctx ctx;\n+\tsize_t data_len = total_len - the_hash_algo->rawsz;\n+\n+\tif (total_len < the_hash_algo->rawsz)\n+\t\treturn 0; /* say \"too short\"? */\n+\n+\tthe_hash_algo->init_fn(&ctx);\n+\tthe_hash_algo->update_fn(&ctx, data, data_len);\n+\tthe_hash_algo->final_fn(got, &ctx);\n+\n+\treturn hasheq(got, data + data_len);\n+}\ndiff --git a/csum-file.h b/csum-file.h\nindex 3044bd19ab..291215b34e 100644\n--- a/csum-file.h\n+++ b/csum-file.h\n@@ -44,6 +44,9 @@ void hashflush(struct hashfile *f);\n void crc32_begin(struct hashfile *);\n uint32_t crc32_end(struct hashfile *);\n \n+/* Verify checksum validity while reading. Returns non-zero on success. */\n+int hashfile_checksum_valid(const unsigned char *data, size_t len);\n+\n /*\n  * Returns the total number of bytes fed to the hashfile so far (including ones\n  * that have not been written out to the descriptor yet).\ndiff --git a/pack-check.c b/pack-check.c\nindex 4b089fe8ec..c8e560d71a 100644\n--- a/pack-check.c\n+++ b/pack-check.c\n@@ -164,22 +164,13 @@ static int verify_packfile(struct repository *r,\n \n int verify_pack_index(struct packed_git *p)\n {\n-\tsize_t len;\n-\tconst unsigned char *index_base;\n-\tgit_hash_ctx ctx;\n-\tunsigned char hash[GIT_MAX_RAWSZ];\n \tint err = 0;\n \n \tif (open_pack_index(p))\n \t\treturn error(\"packfile %s index not opened\", p->pack_name);\n-\tindex_base = p->index_data;\n-\tlen = p->index_size - the_hash_algo->rawsz;\n \n \t/* Verify SHA1 sum of the index file */\n-\tthe_hash_algo->init_fn(&ctx);\n-\tthe_hash_algo->update_fn(&ctx, index_base, len);\n-\tthe_hash_algo->final_fn(hash, &ctx);\n-\tif (!hasheq(hash, index_base + len))\n+\tif (!hashfile_checksum_valid(p->index_data, p->index_size))\n \t\terr = error(\"Packfile index for %s hash mismatch\",\n \t\t\t    p->pack_name);\n \treturn err;\n-- \n2.31.1.163.ga65ce7f831\n\n"},{"id":"428311","messageId":"64aa0aecbd977a6915c271b9c3c1da3c5043e01d.1624473543.git.me@ttaylorr.com","threadId":"55995","inReplyTo":"cover.1624473543.git.me@ttaylorr.com","subject":"[PATCH 2/4] commit-graph: rewrite to use checksum_valid()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-06-23T18:39:09Z","receivedAt":"2021-06-23T18:39:13Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Rewrite an existing caller in `git commit-graph verify` to take\nadvantage of checksum_valid().\n\nNote that the replacement isn't a verbatim cut-and-paste, since the new\nfunction avoids using hashfile at all and instead talks to the_hash_algo\ndirectly, but it is functionally equivalent.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n commit-graph.c | 14 ++++++--------\n 1 file changed, 6 insertions(+), 8 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 2bcb4e0f89..1a2602da61 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -2422,14 +2422,16 @@ static void graph_report(const char *fmt, ...)\n #define GENERATION_ZERO_EXISTS 1\n #define GENERATION_NUMBER_EXISTS 2\n \n+static int commit_graph_checksum_valid(struct commit_graph *g)\n+{\n+\treturn hashfile_checksum_valid(g->data, g->data_len);\n+}\n+\n int verify_commit_graph(struct repository *r, struct commit_graph *g, int flags)\n {\n \tuint32_t i, cur_fanout_pos = 0;\n \tstruct object_id prev_oid, cur_oid;\n-\tunsigned char checksum[GIT_MAX_HEXSZ];\n \tint generation_zero = 0;\n-\tstruct hashfile *f;\n-\tint devnull;\n \tstruct progress *progress = NULL;\n \tint local_error = 0;\n \n@@ -2442,11 +2444,7 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g, int flags)\n \tif (verify_commit_graph_error)\n \t\treturn verify_commit_graph_error;\n \n-\tdevnull = open(\"/dev/null\", O_WRONLY);\n-\tf = hashfd(devnull, NULL);\n-\thashwrite(f, g->data, g->data_len - g->hash_len);\n-\tfinalize_hashfile(f, checksum, CSUM_CLOSE);\n-\tif (!hasheq(checksum, g->data + g->data_len - g->hash_len)) {\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 \t}\n-- \n2.31.1.163.ga65ce7f831\n\n"},{"id":"428312","messageId":"a216c0c5a3ed5060ffc4d768fc10a663ac0af96d.1624473543.git.me@ttaylorr.com","threadId":"55995","inReplyTo":"cover.1624473543.git.me@ttaylorr.com","subject":"[PATCH 3/4] midx: don't reuse corrupt MIDXs when writing","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-06-23T18:39:12Z","receivedAt":"2021-06-23T18:39:15Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When writing a new multi-pack index, Git tries to reuse as much of the\ndata from an existing MIDX as possible, like object offsets. This is\ndone to avoid re-opening a bunch of *.idx files unnecessarily, but can\nlead to problems if the data we are reusing is corrupt.\n\nThat's because we'll blindly reuse data from an existing MIDX without\nchecking its trailing checksum for validity. So if there is memory\ncorruption while writing a MIDX, or disk corruption in the intervening\nperiod between writing and reuse, we'll blindly propagate those bad\nvalues forward.\n\nSuppose we experience a memory corruption while writing a MIDX such that\nwe write an incorrect object offset (or alternatively, the disk corrupts\nthe data after being written, but before being reused). Then when we go\nto write a new MIDX, we'll reuse the bad object offset without checking\nits validity. This means that the MIDX we just wrote is broken, but its\ntrailing checksum is in-tact, since we never bothered to look at the\nvalues before writing.\n\nIn the above, a \"git multi-pack-index verify\" would have caught the\nproblem before writing, but writing a new MIDX wouldn't have noticed\nanything wrong, blindly carrying forward the corrupt offset.\n\nIndividual pack indexes check their validity by verifying the crc32\nattached to each entry when carrying data forward during a repack.\nWe could solve this problem for MIDXs in the same way, but individual\ncrc32's don't make much sense, since their entries are so small.\nLikewise, checking the whole file on every read may be prohibitively\nexpensive if a repository has a lot of objects, packs, or both.\n\nBut we can check the trailing checksum when reusing an existing MIDX\nwhen writing a new one. And a corrupt MIDX need not stop us from writing\na new one, since we can just avoid reusing the existing one at all and\npretend as if we are writing a new MIDX from scratch.\n\nSuggested-by: Derrick Stolee <dstolee@microsoft.com>\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n midx.c                      | 10 ++++++++++\n t/t5319-multi-pack-index.sh |  8 ++++++++\n 2 files changed, 18 insertions(+)\n\ndiff --git a/midx.c b/midx.c\nindex 21d6a05e88..a12cbbf928 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -885,6 +885,11 @@ static void write_midx_reverse_index(char *midx_name, unsigned char *midx_hash,\n static void clear_midx_files_ext(struct repository *r, const char *ext,\n \t\t\t\t unsigned char *keep_hash);\n \n+static int midx_checksum_valid(struct multi_pack_index *m)\n+{\n+\treturn hashfile_checksum_valid(m->data, m->data_len);\n+}\n+\n static int write_midx_internal(const char *object_dir, struct multi_pack_index *m,\n \t\t\t       struct string_list *packs_to_drop,\n \t\t\t       const char *preferred_pack_name,\n@@ -911,6 +916,11 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index *\n \telse\n \t\tctx.m = load_multi_pack_index(object_dir, 1);\n \n+\tif (ctx.m && !midx_checksum_valid(ctx.m)) {\n+\t\twarning(_(\"ignoring existing multi-pack-index; checksum mismatch\"));\n+\t\tctx.m = NULL;\n+\t}\n+\n \tctx.nr = 0;\n \tctx.alloc = ctx.m ? ctx.m->num_packs : 16;\n \tctx.info = NULL;\ndiff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\nindex 5641d158df..d582f370c4 100755\n--- a/t/t5319-multi-pack-index.sh\n+++ b/t/t5319-multi-pack-index.sh\n@@ -410,6 +410,14 @@ test_expect_success 'git-fsck incorrect offset' '\n \t\t\"git -c core.multipackindex=true fsck\"\n '\n \n+test_expect_success 'corrupt MIDX is not reused' '\n+\tcorrupt_midx_and_verify $MIDX_BYTE_OFFSET \"\\377\" $objdir \\\n+\t\t\"incorrect object offset\" &&\n+\tgit multi-pack-index write 2>err &&\n+\ttest_i18ngrep checksum.mismatch err &&\n+\tgit multi-pack-index verify\n+'\n+\n test_expect_success 'repack progress off for redirected stderr' '\n \tGIT_PROGRESS_DELAY=0 git multi-pack-index --object-dir=$objdir repack 2>err &&\n \ttest_line_count = 0 err\n-- \n2.31.1.163.ga65ce7f831\n\n"},{"id":"428313","messageId":"94e9de44e3b52513c5ab48aecd74f809dc34cbe3.1624473543.git.me@ttaylorr.com","threadId":"55995","inReplyTo":"cover.1624473543.git.me@ttaylorr.com","subject":"[PATCH 4/4] midx: report checksum mismatches during 'verify'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-06-23T18:39:15Z","receivedAt":"2021-06-23T18:39:19Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"'git multi-pack-index verify' inspects the data in an existing MIDX for\ncorrectness by checking that the recorded object offsets are correct,\nand so on.\n\nBut it does not check that the file's trailing checksum matches the data\nthat it records. So, if an on-disk corruption happened to occur in the\nfinal few bytes (and all other data was recorded correctly), we would:\n\n  - get a clean result from 'git multi-pack-index verify', but\n  - be unable to reuse the existing MIDX when writing a new one (since\n    we now check for checksum mismatches before reusing a MIDX)\n\nTeach the 'verify' sub-command to recognize corruption in the checksum\nby calling midx_checksum_valid().\n\nSuggested-by: Derrick Stolee <dstolee@microsoft.com>\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n midx.c                      | 3 +++\n t/t5319-multi-pack-index.sh | 5 +++++\n 2 files changed, 8 insertions(+)\n\ndiff --git a/midx.c b/midx.c\nindex a12cbbf928..9a35b0255d 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -1228,6 +1228,9 @@ int verify_midx_file(struct repository *r, const char *object_dir, unsigned flag\n \t\treturn result;\n \t}\n \n+\tif (!midx_checksum_valid(m))\n+\t\tmidx_report(_(\"incorrect checksum\"));\n+\n \tif (flags & MIDX_PROGRESS)\n \t\tprogress = start_delayed_progress(_(\"Looking for referenced packfiles\"),\n \t\t\t\t\t  m->num_packs);\ndiff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\nindex d582f370c4..7609f1ea64 100755\n--- a/t/t5319-multi-pack-index.sh\n+++ b/t/t5319-multi-pack-index.sh\n@@ -418,6 +418,11 @@ test_expect_success 'corrupt MIDX is not reused' '\n \tgit multi-pack-index verify\n '\n \n+test_expect_success 'verify incorrect checksum' '\n+\tpos=$(($(wc -c <$objdir/pack/multi-pack-index) - 1)) &&\n+\tcorrupt_midx_and_verify $pos \"\\377\" $objdir \"incorrect checksum\"\n+'\n+\n test_expect_success 'repack progress off for redirected stderr' '\n \tGIT_PROGRESS_DELAY=0 git multi-pack-index --object-dir=$objdir repack 2>err &&\n \ttest_line_count = 0 err\n-- \n2.31.1.163.ga65ce7f831\n"},{"id":"428359","messageId":"dab8eb4a-3e8e-ea3f-20ba-5164655841f2@gmail.com","threadId":"55995","inReplyTo":"94e9de44e3b52513c5ab48aecd74f809dc34cbe3.1624473543.git.me@ttaylorr.com","subject":"Re: [PATCH 4/4] midx: report checksum mismatches during 'verify'","fromName":"Bagas Sanjaya","fromEmail":"bagasdotme@gmail.com","sentAt":"2021-06-24T04:22:47Z","receivedAt":"2021-06-24T04:22:54Z","isPatch":true,"sender":{"key":"bagasdotme@gmail.com","avatar":"https://avatars.githubusercontent.com/u/40219486?v=4"},"body":"On 24/06/21 01.39, Taylor Blau wrote:\n> But it does not check that the file's trailing checksum matches the data\n> that it records. So, if an on-disk corruption happened to occur in the\n> final few bytes (and all other data was recorded correctly), we would:\n> \n>    - get a clean result from 'git multi-pack-index verify', but\n>    - be unable to reuse the existing MIDX when writing a new one (since\n>      we now check for checksum mismatches before reusing a MIDX)\n> \n> Teach the 'verify' sub-command to recognize corruption in the checksum\n> by calling midx_checksum_valid().\n...\n> +\tif (!midx_checksum_valid(m))\n> +\t\tmidx_report(_(\"incorrect checksum\"));\n> +\n...\n> +test_expect_success 'verify incorrect checksum' '\n> +\tpos=$(($(wc -c <$objdir/pack/multi-pack-index) - 1)) &&\n> +\tcorrupt_midx_and_verify $pos \"\\377\" $objdir \"incorrect checksum\"\n> +'\n> +\n\nWe're in the context of checksum mismatches because of on-disk \ncorruption, so why didn't the message say \"incorrect checksum, possibly \ncorrupt\"?\n\n-- \nAn old man doll... just what I always wanted! - Clara\n"},{"id":"428427","messageId":"YNTgCMN3ImaLmzKT@coredump.intra.peff.net","threadId":"55995","inReplyTo":"3f1fcf348473d7320d46e3df6fe97c482b9888f2.1624473543.git.me@ttaylorr.com","subject":"Re: [PATCH 1/4] csum-file: introduce checksum_valid()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-06-24T19:42:00Z","receivedAt":"2021-06-24T19:42:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 23, 2021 at 02:39:07PM -0400, Taylor Blau wrote:\n\n> +int hashfile_checksum_valid(const unsigned char *data, size_t total_len)\n> +{\n> +\tunsigned char got[GIT_MAX_RAWSZ];\n> +\tgit_hash_ctx ctx;\n> +\tsize_t data_len = total_len - the_hash_algo->rawsz;\n> +\n> +\tif (total_len < the_hash_algo->rawsz)\n> +\t\treturn 0; /* say \"too short\"? */\n> +\n> +\tthe_hash_algo->init_fn(&ctx);\n> +\tthe_hash_algo->update_fn(&ctx, data, data_len);\n> +\tthe_hash_algo->final_fn(got, &ctx);\n> +\n> +\treturn hasheq(got, data + data_len);\n> +}\n\nThis \"too short\" comment is a little ugly, but I don't think it's easy\nto do better. This function does not otherwise print anything to stderr,\nso calling error() here would probably be bad. And having multiple\nreturn types would complicate the callers.\n\nI _suspect_ it's hard to trigger anyway, because the callers would\ngenerally have complained about a too-short file in the first place. So\npossibly it could be a BUG(). But I think leaving it as-is is the more\nconservative choice, as we don't know what checks callers will have\ndone.\n\nIn the long run, we might want to teach this function to actually write\nto stderr (or an error buffer), because it may be useful to say:\n\n  hash mismatch: trailer checksum claims 1234abcd, but we computed 5678cdef\n\nor similar. In which case we could then say \"too short\" in the same way.\nI'm content to leave that for later if somebody cares. In every case of\nfile corruption I've seen, things are sufficiently complicated and\nconfusing that I turn to a hex dump of the file anyway. ;)\n\n-Peff\n"},{"id":"428428","messageId":"YNTgK+ekHZnqACzN@coredump.intra.peff.net","threadId":"55995","inReplyTo":"64aa0aecbd977a6915c271b9c3c1da3c5043e01d.1624473543.git.me@ttaylorr.com","subject":"Re: [PATCH 2/4] commit-graph: rewrite to use checksum_valid()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-06-24T19:42:35Z","receivedAt":"2021-06-24T19:42:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 23, 2021 at 02:39:09PM -0400, Taylor Blau wrote:\n\n> Rewrite an existing caller in `git commit-graph verify` to take\n> advantage of checksum_valid().\n> \n> Note that the replacement isn't a verbatim cut-and-paste, since the new\n> function avoids using hashfile at all and instead talks to the_hash_algo\n> directly, but it is functionally equivalent.\n\nYay. IMHO the result is much nicer, as we do not have to wonder about\nopen() returning an error.\n\n-Peff\n"},{"id":"428429","messageId":"YNTkbk4eUu3LYVhs@coredump.intra.peff.net","threadId":"55995","inReplyTo":"a216c0c5a3ed5060ffc4d768fc10a663ac0af96d.1624473543.git.me@ttaylorr.com","subject":"Re: [PATCH 3/4] midx: don't reuse corrupt MIDXs when writing","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-06-24T20:00:46Z","receivedAt":"2021-06-24T20:00:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 23, 2021 at 02:39:12PM -0400, Taylor Blau wrote:\n\n> Suppose we experience a memory corruption while writing a MIDX such that\n> we write an incorrect object offset (or alternatively, the disk corrupts\n> the data after being written, but before being reused). Then when we go\n> to write a new MIDX, we'll reuse the bad object offset without checking\n> its validity. This means that the MIDX we just wrote is broken, but its\n> trailing checksum is in-tact, since we never bothered to look at the\n> values before writing.\n> \n> In the above, a \"git multi-pack-index verify\" would have caught the\n> problem before writing, but writing a new MIDX wouldn't have noticed\n> anything wrong, blindly carrying forward the corrupt offset.\n> \n> Individual pack indexes check their validity by verifying the crc32\n> attached to each entry when carrying data forward during a repack.\n> We could solve this problem for MIDXs in the same way, but individual\n> crc32's don't make much sense, since their entries are so small.\n> Likewise, checking the whole file on every read may be prohibitively\n> expensive if a repository has a lot of objects, packs, or both.\n> \n> But we can check the trailing checksum when reusing an existing MIDX\n> when writing a new one. And a corrupt MIDX need not stop us from writing\n> a new one, since we can just avoid reusing the existing one at all and\n> pretend as if we are writing a new MIDX from scratch.\n\nNicely explained. This is not saving us from any corruption, but it is\nnoticing it sooner and helping us better diagnose the root problem\n(e.g., in our real-world case it is still only a guess that the data was\ncorrupted after write, though there was strong correlating evidence).\n\nI wondered what the performance implications might be.\n\nIn my fully-packed clone of linux.git with no midx, I get (before your\npatch):\n\n  $ time git multi-pack-index write\n  real\t0m1.358s\n  user\t0m1.119s\n  sys\t0m0.185s\n\nRunning again immediately is much faster, because we realize there's\nnothing new to pack:\n\n  $ time git multi-pack-index write\n  real\t0m0.007s\n  user\t0m0.000s\n  sys\t0m0.007s\n\nIf we create a dummy pack, then we do real work, which seems to take\nabout the same amount of time as generating from scratch:\n\n  $ git pack-objects .git/objects/pack/pack </dev/null\n  029d08823bd8a8eab510ad6ac75c823cfd3ed31e\n  Total 0 (delta 0), reused 0 (delta 0), pack-reused 0\n\n  $ time git multi-pack-index write\n  real\t0m1.399s\n  user\t0m1.126s\n  sys\t0m0.209s\n\nwhich makes sense. Even if we are reusing the midx, there is no speedup\nto be gained versus the original single-pack (whereas with many packs,\nit would be an improvement to use the midx rather than going back to\neach pack idx).\n\nRunning the same sequence (after deleting the midx and the dummy pack)\nusing your patch, the initial write is the same (unsurprising, since\nthere is no midx to check). The final case (with the dummy pack)\nbecomes:\n\n  $ time git multi-pack-index write\n  real\t0m1.947s\n  user\t0m1.668s\n  sys\t0m0.216s\n\nSo we really do pay a 500-600ms penalty to to look at the checksum. IMHO\nthat is worth it, given the overall cost of the write operation, but I\nwas surprised how big it was as a relative cost.\n\nThere is one more interesting thing, though. The 7ms \"noop\" version\nwhere we don't write anything also gets more expensive:\n\n  $ time git multi-pack-index write\n  real\t0m0.550s\n  user\t0m0.537s\n  sys\t0m0.013s\n\nI suspect we could avoid that by pushing our checksum validation down a\nbit further in the function, but I'm not sure if that creates further\nproblems (i.e., if we'd already have started trusting some of the midx's\ndata by then).\n\nI'm not sure if this \"noop\" midx write is common enough to worry too\nmuch about. Presumably whatever triggers \"multi-pack-index write\" is\ndoing so because it just created or removed some packs. If it knows that\nwhat it did was a noop, it's probably better off skipping the call to\ngit-multi-pack-index entirely.\n\nAll of which is to say this looks just fine to me. But I think we should\nmake a conscious decision on the performance implications (and seeing\nthe real numbers, it is quite clear that checking the trailer checksum\non normal reads is _way_ too expensive).\n\n-Peff\n"},{"id":"428430","messageId":"YNTmsoOKMsaC+cYV@coredump.intra.peff.net","threadId":"55995","inReplyTo":"94e9de44e3b52513c5ab48aecd74f809dc34cbe3.1624473543.git.me@ttaylorr.com","subject":"Re: [PATCH 4/4] midx: report checksum mismatches during 'verify'","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-06-24T20:10:26Z","receivedAt":"2021-06-24T20:10:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 23, 2021 at 02:39:15PM -0400, Taylor Blau wrote:\n\n> 'git multi-pack-index verify' inspects the data in an existing MIDX for\n> correctness by checking that the recorded object offsets are correct,\n> and so on.\n> \n> But it does not check that the file's trailing checksum matches the data\n> that it records. So, if an on-disk corruption happened to occur in the\n> final few bytes (and all other data was recorded correctly), we would:\n> \n>   - get a clean result from 'git multi-pack-index verify', but\n>   - be unable to reuse the existing MIDX when writing a new one (since\n>     we now check for checksum mismatches before reusing a MIDX)\n> \n> Teach the 'verify' sub-command to recognize corruption in the checksum\n> by calling midx_checksum_valid().\n\nMakes sense. I was a little surprised we didn't do this already, but I\nguess it does not do the same \"regenerate and make sure hashfile\nproduces the same checksum\" trick that the pack idx verifier does (as an\naside, I think what the midx code is doing here is much _better_,\nbecause it is looking at semantic problems in the file, and is more\nrobust against irrelevant changes in the format).\n\n> @@ -1228,6 +1228,9 @@ int verify_midx_file(struct repository *r, const char *object_dir, unsigned flag\n>  \t\treturn result;\n>  \t}\n>  \n> +\tif (!midx_checksum_valid(m))\n> +\t\tmidx_report(_(\"incorrect checksum\"));\n\nThis \"midx_report()\" function doesn't provide much context on stderr. I\nget:\n\n  $ echo foo >>.git/objects/pack/multi-pack-index\n  $ git multi-pack-index verify\n  incorrect checksum\n  Verifying OID order in multi-pack-index: 100% (282/282), done.\n  Sorting objects by packfile: 100% (283/283), done.\n  Verifying object offsets: 100% (283/283), done.\n\nI think we should at least say \"error:\", but something along the lines\nof \"midx file at %s does not match its trailing checksum (possibly\ncorruption?)\". Or something like that.\n\nI think all of the existing calls to midx_report() share this issue,\nthough. We probably want to at least say \"error:\" here, but maybe even\nsomething like:\n\ndiff --git a/midx.c b/midx.c\nindex 9a35b0255d..e464907a7c 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -1172,10 +1172,12 @@ void clear_midx_file(struct repository *r)\n \n static int verify_midx_error;\n \n-static void midx_report(const char *fmt, ...)\n+static void midx_report(struct multi_pack_index *m, const char *fmt, ...)\n {\n \tva_list ap;\n \tverify_midx_error = 1;\n+\t/* do we need to care about the \"next\" pointer here? */\n+\tfprintf(stderr, (\"error: %s/multi-pack-index: \"), m->object_dir);\n \tva_start(ap, fmt);\n \tvfprintf(stderr, fmt, ap);\n \tfprintf(stderr, \"\\n\");\n\nAlso, a side note: we should use __attribute__((format)) on this\nfunction to get compile-time checks of our format strings.\n\n-Peff\n"},{"id":"440881","messageId":"20211110231132.GB5811@szeder.dev","threadId":"55995","inReplyTo":"94e9de44e3b52513c5ab48aecd74f809dc34cbe3.1624473543.git.me@ttaylorr.com","subject":"Re: [PATCH 4/4] midx: report checksum mismatches during 'verify'","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2021-11-10T23:11:32Z","receivedAt":"2021-11-10T23:11:40Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Wed, Jun 23, 2021 at 02:39:15PM -0400, Taylor Blau wrote:\n> 'git multi-pack-index verify' inspects the data in an existing MIDX for\n> correctness by checking that the recorded object offsets are correct,\n> and so on.\n> \n> But it does not check that the file's trailing checksum matches the data\n> that it records. So, if an on-disk corruption happened to occur in the\n> final few bytes (and all other data was recorded correctly), we would:\n> \n>   - get a clean result from 'git multi-pack-index verify', but\n>   - be unable to reuse the existing MIDX when writing a new one (since\n>     we now check for checksum mismatches before reusing a MIDX)\n> \n> Teach the 'verify' sub-command to recognize corruption in the checksum\n> by calling midx_checksum_valid().\n> \n> Suggested-by: Derrick Stolee <dstolee@microsoft.com>\n> Signed-off-by: Taylor Blau <me@ttaylorr.com>\n> ---\n>  midx.c                      | 3 +++\n>  t/t5319-multi-pack-index.sh | 5 +++++\n>  2 files changed, 8 insertions(+)\n> \n> diff --git a/midx.c b/midx.c\n> index a12cbbf928..9a35b0255d 100644\n> --- a/midx.c\n> +++ b/midx.c\n> @@ -1228,6 +1228,9 @@ int verify_midx_file(struct repository *r, const char *object_dir, unsigned flag\n>  \t\treturn result;\n>  \t}\n>  \n> +\tif (!midx_checksum_valid(m))\n> +\t\tmidx_report(_(\"incorrect checksum\"));\n> +\n>  \tif (flags & MIDX_PROGRESS)\n>  \t\tprogress = start_delayed_progress(_(\"Looking for referenced packfiles\"),\n>  \t\t\t\t\t  m->num_packs);\n> diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\n> index d582f370c4..7609f1ea64 100755\n> --- a/t/t5319-multi-pack-index.sh\n> +++ b/t/t5319-multi-pack-index.sh\n> @@ -418,6 +418,11 @@ test_expect_success 'corrupt MIDX is not reused' '\n>  \tgit multi-pack-index verify\n>  '\n>  \n> +test_expect_success 'verify incorrect checksum' '\n> +\tpos=$(($(wc -c <$objdir/pack/multi-pack-index) - 1)) &&\n> +\tcorrupt_midx_and_verify $pos \"\\377\" $objdir \"incorrect checksum\"\n\nThis test is flaky and can fail with:\n\n  ...\n  + printf \\377\n  + dd of=.git/objects/pack/multi-pack-index bs=1 seek=3839 conv=notrunc\n  1+0 records in\n  1+0 records out\n  1 byte copied, 5.0656e-05 s, 19.7 kB/s\n  + test_must_fail git multi-pack-index verify --object-dir=.git/objects\n  test_must_fail: command succeeded: git multi-pack-index verify --object-dir=.git/objects\n  error: last command exited with $?=1\n  + mv midx-backup .git/objects/pack/multi-pack-index\n  not ok 44 - verify incorrect checksum\n\nSo the test corrupts the checksum trailer in the 'multi-pack-index'\nfile by overwriting its last byte with 0xff, but if that byte were\nalready 0xff, then the file would be left as is, and 'git\nmulti-pack-index verify' wouldn't find anything amiss.\n\nSince SHA1 is essentially random, there's a 1:256 chance of that\nhappening, assuming that the file's content is random.  That't not\nreally the case, however, because both the test repository's objects\nand the resulting packfiles are deterministic, and, consequently, the\ncontent of the MIDX is somewhat deterministic.  Only \"somewhat\",\nthough, because several of those objects appear in multiple packfiles,\nand the MIDX selects a copy in the most recently modified packfile, so\nfilesystem mtime resolution and second boundaries become significant,\nand cause some variance in the contents of the OOFF chunk.\n\nRecently a laptop crossed my way that is somehow exceptionally good at\ngenerating MIDX files ending with 0xff:\n\n  # ad-hoc checksum statistics:\n  $ git diff\n  diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\n  index f1ee2ce56d..605713b518 100755\n  --- a/t/t5319-multi-pack-index.sh\n  +++ b/t/t5319-multi-pack-index.sh\n  @@ -482,9 +482,13 @@ test_expect_success 'corrupt MIDX is not reused' '\n   '\n   \n   test_expect_success 'verify incorrect checksum' '\n  +\tskip=$(($(wc -c <$objdir/pack/multi-pack-index) - 20)) &&\n  +\tprintf \"checksum: \" >&5 &&\n  +\tod -x -w20 -j$skip --endian=big -An \"$objdir/pack/multi-pack-index\" >&5 &&\n   \tpos=$(($(wc -c <$objdir/pack/multi-pack-index) - 1)) &&\n   \tcorrupt_midx_and_verify $pos \"\\377\" $objdir \"incorrect checksum\"\n   '\n  +test_done\n   \n   test_expect_success 'repack progress off for redirected stderr' '\n   \tGIT_PROGRESS_DELAY=0 git multi-pack-index --object-dir=$objdir repack 2>err &&\n  $ for i in {1..500} ; do ./t5319-multi-pack-index.sh |sed -n -e 's/^checksum:  //p' ; done |sort |uniq -c\n       31 1a70 3b1c 8ed3 56a6 5101 2a38 057e 698d 6faf fbaa\n      340 5fc0 552f 0ac0 c876 f229 d9e3 ef13 a314 5847 89ff\n      129 ce7d 3710 fd21 ef7b 8316 2b99 4e6c e5d5 5e7c 7b08\n\nThat's almost 70% failure rate, though I haven't been able to trigger\nthis failure on any of the other machines that I have access to.\n\n"},{"id":"440904","messageId":"YYzq0uBr+uoVvkbC@coredump.intra.peff.net","threadId":"55995","inReplyTo":"20211110231132.GB5811@szeder.dev","subject":"Re: [PATCH 4/4] midx: report checksum mismatches during 'verify'","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-11-11T10:05:06Z","receivedAt":"2021-11-11T10:05:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 11, 2021 at 12:11:32AM +0100, SZEDER Gábor wrote:\n\n> So the test corrupts the checksum trailer in the 'multi-pack-index'\n> file by overwriting its last byte with 0xff, but if that byte were\n> already 0xff, then the file would be left as is, and 'git\n> multi-pack-index verify' wouldn't find anything amiss.\n\nGood find. I tried running with the patch you showed:\n\n>   $ for i in {1..500} ; do ./t5319-multi-pack-index.sh |sed -n -e 's/^checksum:  //p' ; done |sort |uniq -c\n>        31 1a70 3b1c 8ed3 56a6 5101 2a38 057e 698d 6faf fbaa\n>       340 5fc0 552f 0ac0 c876 f229 d9e3 ef13 a314 5847 89ff\n>       129 ce7d 3710 fd21 ef7b 8316 2b99 4e6c e5d5 5e7c 7b08\n\nbut I only ever got the last one, even under load. However, it's easy\nenough to tweak the mtimes to stimulate a failure:\n\n  diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\n  index 3f69e43178..b84e96779f 100755\n  --- a/t/t5319-multi-pack-index.sh\n  +++ b/t/t5319-multi-pack-index.sh\n  @@ -481,10 +481,25 @@ test_expect_success 'corrupt MIDX is not reused' '\n   \tgit multi-pack-index verify\n   '\n   \n  +while true; do\n   test_expect_success 'verify incorrect checksum' '\n  +\tt=1234567890 &&\n  +\tfor i in $(ls $objdir/pack/*.pack | shuf)\n  +\tdo\n  +\t\ttouch -d @$t $i &&\n  +\t\tt=$((t+100))\n  +\tdone &&\n  +\trm -f $objdir/pack/multi-pack-index &&\n  +\tgit multi-pack-index write &&\n  +\tskip=$(($(wc -c <$objdir/pack/multi-pack-index) - 20)) &&\n  +\tprintf \"checksum: \" >&5 &&\n  +\tod -x -w20 -j$skip --endian=big -An \"$objdir/pack/multi-pack-index\" >&5 &&\n   \tpos=$(($(wc -c <$objdir/pack/multi-pack-index) - 1)) &&\n   \tcorrupt_midx_and_verify $pos \"\\377\" $objdir \"incorrect checksum\"\n   '\n  +done\n  +\n  +test_done\n   \n   test_expect_success 'repack progress off for redirected stderr' '\n   \tGIT_PROGRESS_DELAY=0 git multi-pack-index --object-dir=$objdir repack 2>err &&\n\nBrute force, but it fails after a few hundred attempts. :)\n\n> Since SHA1 is essentially random, there's a 1:256 chance of that\n> happening, assuming that the file's content is random.\n\nThe most exact fix here would be to read the final byte, increment it\nmod-256, and write it back, which would ensure it was always changed.\nBut that's a bit annoying to do in shell. Perhaps just corrupting more\nbytes is a good solution? The patch below should reduce your changes to\n1 in 2^80.\n\ndiff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\nindex 3f69e43178..a612e44547 100755\n--- a/t/t5319-multi-pack-index.sh\n+++ b/t/t5319-multi-pack-index.sh\n@@ -482,8 +482,10 @@ test_expect_success 'corrupt MIDX is not reused' '\n '\n \n test_expect_success 'verify incorrect checksum' '\n-\tpos=$(($(wc -c <$objdir/pack/multi-pack-index) - 1)) &&\n-\tcorrupt_midx_and_verify $pos \"\\377\" $objdir \"incorrect checksum\"\n+\tpos=$(($(wc -c <$objdir/pack/multi-pack-index) - 10)) &&\n+\tcorrupt_midx_and_verify $pos \\\n+\t\t\"\\377\\377\\377\\377\\377\\377\\377\\377\\377\\377\" \\\n+\t\t$objdir \"incorrect checksum\"\n '\n \n test_expect_success 'repack progress off for redirected stderr' '\n\nThere are other variants, of course. Just appending a single byte to the\nfile is enough to give you a high probability of failing the checksum\n(since it shifts it all by one byte, making it essentially random), but\nthe corrupt_midx() helper doesn't support that.\n\n-Peff\n"},{"id":"441363","messageId":"YZQeMjjtxS2LU2Tk@nand.local","threadId":"55995","inReplyTo":"YYzq0uBr+uoVvkbC@coredump.intra.peff.net","subject":"Re: [PATCH 4/4] midx: report checksum mismatches during 'verify'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-11-16T21:10:10Z","receivedAt":"2021-11-16T21:10:13Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Nov 11, 2021 at 05:05:06AM -0500, Jeff King wrote:\n> > Since SHA1 is essentially random, there's a 1:256 chance of that\n> > happening, assuming that the file's content is random.\n>\n> The most exact fix here would be to read the final byte, increment it\n> mod-256, and write it back, which would ensure it was always changed.\n> But that's a bit annoying to do in shell. Perhaps just corrupting more\n> bytes is a good solution? The patch below should reduce your changes to\n> 1 in 2^80.\n>\n> diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\n> index 3f69e43178..a612e44547 100755\n> --- a/t/t5319-multi-pack-index.sh\n> +++ b/t/t5319-multi-pack-index.sh\n> @@ -482,8 +482,10 @@ test_expect_success 'corrupt MIDX is not reused' '\n>  '\n>\n>  test_expect_success 'verify incorrect checksum' '\n> -\tpos=$(($(wc -c <$objdir/pack/multi-pack-index) - 1)) &&\n> -\tcorrupt_midx_and_verify $pos \"\\377\" $objdir \"incorrect checksum\"\n> +\tpos=$(($(wc -c <$objdir/pack/multi-pack-index) - 10)) &&\n> +\tcorrupt_midx_and_verify $pos \\\n> +\t\t\"\\377\\377\\377\\377\\377\\377\\377\\377\\377\\377\" \\\n> +\t\t$objdir \"incorrect checksum\"\n>  '\n>\n>  test_expect_success 'repack progress off for redirected stderr' '\n>\n> There are other variants, of course. Just appending a single byte to the\n> file is enough to give you a high probability of failing the checksum\n> (since it shifts it all by one byte, making it essentially random), but\n> the corrupt_midx() helper doesn't support that.\n\nThanks SZEDER for the report, and thanks Peff for the fix :).\n\nI agree that it's annoyingly cumbersome to write back the last byte\nincremented mod-256. So I'm content to just make it astronomically\nunlikely to run into a collision in practice. (As a matter of fact, I'm\nsurprised that the current implementation hasn't produced failures for\nus more often).\n\nPeff: do you want me to turn this into a patch or were you planning on\nit?\n\nThanks,\nTaylor\n"},{"id":"441366","messageId":"YZQk6t1veegeAlYh@coredump.intra.peff.net","threadId":"55995","inReplyTo":"YZQeMjjtxS2LU2Tk@nand.local","subject":"[PATCH] t5319: corrupt more bytes of the midx checksum","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-11-16T21:38:50Z","receivedAt":"2021-11-16T21:38:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 16, 2021 at 04:10:10PM -0500, Taylor Blau wrote:\n\n> I agree that it's annoyingly cumbersome to write back the last byte\n> incremented mod-256. So I'm content to just make it astronomically\n> unlikely to run into a collision in practice. (As a matter of fact, I'm\n> surprised that the current implementation hasn't produced failures for\n> us more often).\n\nIt's quite deterministic in practice. The randomness is driven by the\nmtimes of the packfiles, so there are only a handful of combinations\nyou'd see in practice (though I'm still curious why one of Gábor's\nmachines sees it so frequently).\n\n> Peff: do you want me to turn this into a patch or were you planning on\n> it?\n\nI hadn't thought that far ahead. ;)\n\nOne thing I did wonder is whether other corruption tests might have the\nsame problem. But if they're not triggering in practice, I don't think\nit's worth spending a bunch of time looking at them.\n\nSo here's this fix wrapped up with a commit message.\n\n-- >8 --\nSubject: [PATCH] t5319: corrupt more bytes of the midx checksum\n\nOne of the tests in t5319 corrupts the checksum of the midx file by\nwriting a single 0xff over the final byte, and then confirms that we\ndetect the problem. This usually works fine, but would break if the\nactual checksum ended with that same byte already.\n\nIt seems like this should happen in 1 out of 256 test runs, but it turns\nout to be less often in practice. The contents of the midx are mostly\ndeterministic because it's based on the objects, and we remove most\nsources of randomness by setting GIT_COMMITTER_DATE, etc.  However,\nthere's still some randomness: some objects are duplicated between\npacks, and the midx must decide which to use, which can be based on\ntiming.\n\nSo very occasionally we can end up with a real 0xff byte, and the test\nfails. The most robust fix would be to read out the final byte and then\nchange it to something else (e.g., adding 1 mod 256). But that's awkward\nto do in shell. Let's just blindly corrupt 10 bytes instead of 1, which\nreduces our chances of an accidental noop to 1 in 2^80.\n\nReported-by: SZEDER Gábor <szeder.dev@gmail.com>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t5319-multi-pack-index.sh | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\nindex 3f69e43178..a612e44547 100755\n--- a/t/t5319-multi-pack-index.sh\n+++ b/t/t5319-multi-pack-index.sh\n@@ -482,8 +482,10 @@ test_expect_success 'corrupt MIDX is not reused' '\n '\n \n test_expect_success 'verify incorrect checksum' '\n-\tpos=$(($(wc -c <$objdir/pack/multi-pack-index) - 1)) &&\n-\tcorrupt_midx_and_verify $pos \"\\377\" $objdir \"incorrect checksum\"\n+\tpos=$(($(wc -c <$objdir/pack/multi-pack-index) - 10)) &&\n+\tcorrupt_midx_and_verify $pos \\\n+\t\t\"\\377\\377\\377\\377\\377\\377\\377\\377\\377\\377\" \\\n+\t\t$objdir \"incorrect checksum\"\n '\n \n test_expect_success 'repack progress off for redirected stderr' '\n-- \n2.34.0.633.g181affe237\n\n"},{"id":"441367","messageId":"YZQl95aaW5loTFCW@nand.local","threadId":"55995","inReplyTo":"YZQk6t1veegeAlYh@coredump.intra.peff.net","subject":"Re: [PATCH] t5319: corrupt more bytes of the midx checksum","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-11-16T21:43:19Z","receivedAt":"2021-11-16T21:43:25Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Nov 16, 2021 at 04:38:50PM -0500, Jeff King wrote:\n> On Tue, Nov 16, 2021 at 04:10:10PM -0500, Taylor Blau wrote:\n> > Peff: do you want me to turn this into a patch or were you planning on\n> > it?\n>\n> I hadn't thought that far ahead. ;)\n>\n> One thing I did wonder is whether other corruption tests might have the\n> same problem. But if they're not triggering in practice, I don't think\n> it's worth spending a bunch of time looking at them.\n\nDefinitely agreed. We can apply a similar fix if and when any other\nspots become a problem.\n\n> So here's this fix wrapped up with a commit message.\n\nThanks, what you wrote looks good to me.\n\n    Reviewed-by: Taylor Blau <me@ttaylorr.com>\n\nThanks,\nTaylor\n"},{"id":"441369","messageId":"cb995591-d837-ac3d-9935-5eab1c98b4d7@gmail.com","threadId":"55995","inReplyTo":"YZQk6t1veegeAlYh@coredump.intra.peff.net","subject":"Re: [PATCH] t5319: corrupt more bytes of the midx checksum","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-11-16T22:12:59Z","receivedAt":"2021-11-16T22:13:05Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 11/16/2021 4:38 PM, Jeff King wrote:\n> So very occasionally we can end up with a real 0xff byte, and the test\n> fails. The most robust fix would be to read out the final byte and then\n> change it to something else (e.g., adding 1 mod 256). But that's awkward\n> to do in shell. Let's just blindly corrupt 10 bytes instead of 1, which\n> reduces our chances of an accidental noop to 1 in 2^80.\n> \n> Reported-by: SZEDER Gábor <szeder.dev@gmail.com>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  t/t5319-multi-pack-index.sh | 6 ++++--\n>  1 file changed, 4 insertions(+), 2 deletions(-)\n> \n> diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\n> index 3f69e43178..a612e44547 100755\n> --- a/t/t5319-multi-pack-index.sh\n> +++ b/t/t5319-multi-pack-index.sh\n> @@ -482,8 +482,10 @@ test_expect_success 'corrupt MIDX is not reused' '\n>  '\n>  \n>  test_expect_success 'verify incorrect checksum' '\n> -\tpos=$(($(wc -c <$objdir/pack/multi-pack-index) - 1)) &&\n> -\tcorrupt_midx_and_verify $pos \"\\377\" $objdir \"incorrect checksum\"\n> +\tpos=$(($(wc -c <$objdir/pack/multi-pack-index) - 10)) &&\n> +\tcorrupt_midx_and_verify $pos \\\n> +\t\t\"\\377\\377\\377\\377\\377\\377\\377\\377\\377\\377\" \\\n> +\t\t$objdir \"incorrect checksum\"\n>  '\n\nThanks for taking the time to make this a patch. This approach looks\ngood to me.\n\nThanks,\n-Stolee\n"}]}