{"thread":{"id":"58147","subject":"[PATCH 0/3] commit-graph: fix corruption during generation v2 upgrade","startedAt":"2022-07-12T23:10:31Z","lastAt":"2022-07-16T00:17:57Z","messageCount":12,"participants":["Taylor Blau","Junio C Hamano","Derrick Stolee"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"458922","messageId":"cover.1657667404.git.me@ttaylorr.com","threadId":"58147","inReplyTo":null,"subject":"[PATCH 0/3] commit-graph: fix corruption during generation v2 upgrade","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-07-12T23:10:18Z","receivedAt":"2022-07-12T23:10:31Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"This brief series resolves a bug where a commit-graph would become\ncorrupt when upgrading from generation number v1 to v2, as originally\nreported in [1].\n\nSome speculation occurred as to what might be causing that bug in the\nthread beginning at [1], until the problem was explained in more detail\nby Will Chandler in [2].\n\nThe crux of the issue, as is described in [2] and [3], is that the\ncommit_graph_data slab is reused for read and write operations involving\nthe commit-graph, leading to situations where data computed in\npreparation of a write is clobbered by a read of existing data.\n\nThe first patch demonstrates the issue, and the second patch prepares to\nfix it by introducing a helper function. The crux of the issue is\ndescribed and fixed in the third patch.\n\n[1]: https://lore.kernel.org/git/YqD5dgalb9EPnz85@coredump.intra.peff.net/\n[2]: https://lore.kernel.org/git/DD88D523-0ECA-4474-9AA5-1D4A431E532A@wfchandler.org/\n[3]: https://lore.kernel.org/git/YsS7H4i5DqUZVQ5i@nand.local/\n\nTaylor Blau (3):\n t5318: demonstrate commit-graph generation v2 corruption\n commit-graph: introduce `repo_find_commit_pos_in_graph()`\n commit-graph: fix corrupt upgrade from generation v1 to v2\n\n bloom.c | 10 +++++-----\n commit-graph.c | 12 +++++++++---\n commit-graph.h | 15 +++++++++++++++\n t/t5318-commit-graph.sh | 27 +++++++++++++++++++++++++++\n 4 files changed, 56 insertions(+), 8 deletions(-)\n\n-- \n2.37.0.1.g1379af2e9d\n"},{"id":"458923","messageId":"0a49c86037bac200bb23e1abf9f67363e99c4b7c.1657667404.git.me@ttaylorr.com","threadId":"58147","inReplyTo":"cover.1657667404.git.me@ttaylorr.com","subject":"[PATCH 1/3] t5318: demonstrate commit-graph generation v2 corruption","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-07-12T23:10:28Z","receivedAt":"2022-07-12T23:10:36Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When upgrading a commit-graph using generation v1 to one using\ngeneration v2, it is possible to force Git into a corrupt state where it\n(incorrectly) believes that a GDO2 chunk is necessary, *after* deciding\nnot to write one.\n\nThis makes subsequent reads using the commit-graph produce the following\nerror message:\n\n fatal: commit-graph requires overflow generation data but has none\n\nDemonstrate this bug by increasing our test coverage to include a\nminimal example of upgrading a commit-graph from generation v1 to v2.\nThe only notable components of this test are:\n\n - The committer date of the commit is chosen carefully so that the\n offset underflows when computed using a v1 generation number, but\n would not overflow when using v2 generation numbers.\n\n - The upgrade to generation number v2 must read in the v1 generation\n numbers, which we can do by passing `--changed-paths`, which will\n force the commit-graph internals to call `fill_commit_graph_info()`.\n\nA future patch will squash this bug.\n\nReported-by: Jeff King \nReproduced-by: Will Chandler \nSigned-off-by: Taylor Blau \n---\n t/t5318-commit-graph.sh | 27 +++++++++++++++++++++++++++\n 1 file changed, 27 insertions(+)\n\ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex fbf0d64578..4d9f62f22d 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -811,4 +811,31 @@ test_expect_success 'set up and verify repo with generation data overflow chunk'\n \n graph_git_behavior 'generation data overflow chunk repo' repo left right\n \n+test_expect_failure 'overflow during generation version upgrade' '\n+\tgit init overflow-v2-upgrade &&\n+\t(\n+\t\tcd overflow-v2-upgrade &&\n+\n+\t\t# This commit will have a date at two seconds past the Epoch,\n+\t\t# and a (v1) generation number of 1, since it is a root commit.\n+\t\t#\n+\t\t# The offset will then be computed as 2-1, which will underflow\n+\t\t# to 2^31, which is greater than the v2 offset small limit of\n+\t\t# 2^31-1.\n+\t\t#\n+\t\t# This is sufficient to need a large offset table for the v2\n+\t\t# generation numbers.\n+\t\ttest_commit --date \"@2 +0000\" base &&\n+\t\tgit repack -d &&\n+\n+\t\t# Test that upgrading from generation v1 to v2 correctly\n+\t\t# produces the overflow table.\n+\t\tgit -c commitGraph.generationVersion=1 commit-graph write &&\n+\t\tgit -c commitGraph.generationVersion=2 commit-graph write \\\n+\t\t\t--changed-paths &&\n+\n+\t\tgit rev-list --all\n+\t)\n+'\n+\n test_done\n-- \n2.37.0.1.g1379af2e9d\n\n"},{"id":"458924","messageId":"e988c974119edbabc06dae6d24810fc0bafbdc94.1657667404.git.me@ttaylorr.com","threadId":"58147","inReplyTo":"cover.1657667404.git.me@ttaylorr.com","subject":"[PATCH 2/3] commit-graph: introduce `repo_find_commit_pos_in_graph()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-07-12T23:10:31Z","receivedAt":"2022-07-12T23:10:42Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Low-level callers in systems that are adjacent to the commit-graph (like\nthe changed-path Bloom filter code) could benefit from being able to\ncall a function like `parse_commit_in_graph()` without modifying the\ncorresponding commit slab data.\n\nThis is useful in contexts where that slab data is being used to prepare\nfor an upcoming commit-graph write, where Git must be careful to avoid\nclobbering any of that data during a read operation.\n\nIntroduce a low-level variant of `parse_commit_in_graph()` which returns\nthe graph position of a given commit only, without modifying any of the\nslab data.\n\nSigned-off-by: Taylor Blau \n---\n commit-graph.c | 12 +++++++++---\n commit-graph.h | 15 +++++++++++++++\n 2 files changed, 24 insertions(+), 3 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 92d4503336..1c34ae1ea4 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -889,6 +889,14 @@ static int find_commit_pos_in_graph(struct commit *item, struct commit_graph *g,\n \t}\n }\n \n+int repo_find_commit_pos_in_graph(struct repository *r, struct commit *c,\n+\t\t\t\t uint32_t *pos)\n+{\n+\tif (!prepare_commit_graph(r))\n+\t\treturn 0;\n+\treturn find_commit_pos_in_graph(c, r->objects->commit_graph, pos);\n+}\n+\n struct commit *lookup_commit_in_graph(struct repository *repo, const struct object_id *id)\n {\n \tstruct commit *commit;\n@@ -946,9 +954,7 @@ int parse_commit_in_graph(struct repository *r, struct commit *item)\n void load_commit_graph_info(struct repository *r, struct commit *item)\n {\n \tuint32_t pos;\n-\tif (!prepare_commit_graph(r))\n-\t\treturn;\n-\tif (find_commit_pos_in_graph(item, r->objects->commit_graph, &pos))\n+\tif (repo_find_commit_pos_in_graph(r, item, &pos))\n \t\tfill_commit_graph_info(item, r->objects->commit_graph, pos);\n }\n \ndiff --git a/commit-graph.h b/commit-graph.h\nindex 2e3ac35237..f23b9e9026 100644\n--- a/commit-graph.h\n+++ b/commit-graph.h\n@@ -40,6 +40,21 @@ int open_commit_graph(const char *graph_file, int *fd, struct stat *st);\n */\n int parse_commit_in_graph(struct repository *r, struct commit *item);\n \n+/*\n+ * Fills `*pos` with the graph position of `c`, and returns 1 if `c` is\n+ * found in the commit-graph belonging to `r`, or 0 otherwise.\n+ * Initializes the commit-graph belonging to `r` if it hasn't been\n+ * already.\n+ *\n+ * Note: this is a low-level helper that does not alter any slab data\n+ * associated with `c`. Useful in circumstances where the slab data is\n+ * already being modified (e.g., writing the commit-graph itself).\n+ *\n+ * In most cases, callers should use `parse_commit_in_graph()` instead.\n+ */\n+int repo_find_commit_pos_in_graph(struct repository *r, struct commit *c,\n+\t\t\t\t uint32_t *pos);\n+\n /*\n * Look up the given commit ID in the commit-graph. This will only return a\n * commit if the ID exists both in the graph and in the object database such\n-- \n2.37.0.1.g1379af2e9d\n\n"},{"id":"458925","messageId":"b39209718226b3e313ef4f610f02038c9faf0990.1657667404.git.me@ttaylorr.com","threadId":"58147","inReplyTo":"cover.1657667404.git.me@ttaylorr.com","subject":"[PATCH 3/3] commit-graph: fix corrupt upgrade from generation v1 to v2","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-07-12T23:10:33Z","receivedAt":"2022-07-12T23:10:47Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"The previous commit demonstrates a bug where a commit-graph using\ngeneration v2 could enter a state where one of the GDA2 values has its\nmost-significant bit set (indicating that its value should be read from\nthe extended offset table in the GDO2 chunk) without having a GDO2 chunk\nto read from.\n\nThis results in the following error message being displayed to the\ncaller:\n\n fatal: commit-graph requires overflow generation data but has none\n\nThis bug arises in the following scenario:\n\n - We decide to write a commit-graph using generation number v2, and\n decide (correctly) that no GDO2 chunk is necessary (e.g., because\n all of the commiter date offsets are no larger than 2^31-1).\n\n - The v2 generation numbers are stored in the `->generation` member of\n the commit slab holding `struct commit_graph_data`'s.\n\n - Later on, `load_commit_graph_info()` is called, overwriting the\n v2 generation data in the aforementioned slab with any existing v1\n generation data.\n\nThen, when the commit-graph code goes to write the GDA2 chunk via\n`write_graph_chunk_generation_data()`, we use the overwritten generation\nv1 data in a place where we expect to use a v2 generation number:\n\n offset = commit_graph_data_at(c)->generation - c->date;\n\n...because `commit_graph_data_at(c)->generation` used to hold the v2\ngeneration data, but it was overwritten to contain the v1 generation\nnumber via `load_commit_graph_info()`.\n\nIf the `offset` computation above overflows the v2 generation number\nmax, then `write_graph_chunk_generation_data()` will update its count of\nlarge offsets and write the marker accordingly:\n\n if (offset > GENERATION_NUMBER_V2_OFFSET_MAX) {\n offset = CORRECTED_COMMIT_DATE_OFFSET_OVERFLOW | num_generation_data_overflows;\n num_generation_data_overflows++;\n }\n\nand reads will look for the GDO2 chunk containing the overflowing v2\ngeneration number, *after* the commit-graph code decided that no such\nchunk was necessary.\n\nThe main problem is that the slab containing `struct commit_graph_data`\nhas a dual purpose. It is used to hold data that we are about to write\nto disk while generating a commit-graph, as well as hold data that was\nread from an existing commit-graph.\n\nWhen the two mix, namely when the result of reading the commit-graph has\na side-effect that mixes poorly with an in-progress commit-graph write,\nwe end up with corrupt data.\n\nA complete fix might be to introduce a new slab that is used exclusively\nfor writing, and gate access between the two slabs based on context\nprovided by the caller (e.g., whether this computation is part of a\n\"read\" or \"write\" operation).\n\nBut a more minimal fix addresses the only known path which overwrites\nthe slab data, which is `compute_bloom_filters()` ->\n`get_or_compute_bloom_filter()` -> `load_commit_graph_info()` ->\n`fill_commit_graph_info()` by avoiding the last call which clobbers the\ndata altogether.\n\nThis path only needs to learn the graph position of a given commit so\nthat it can be used in `load_bloom_filter_from_graph()`. By replacing\nthe last steps of the above with one that records the graph position\ninto a temporary variable which is then used to load the existing Bloom\ndata, we eliminate the clobbering, removing the corruption.\n\nSigned-off-by: Taylor Blau \n---\n bloom.c | 10 +++++-----\n t/t5318-commit-graph.sh | 2 +-\n 2 files changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/bloom.c b/bloom.c\nindex 5e297038bb..816f063dca 100644\n--- a/bloom.c\n+++ b/bloom.c\n@@ -30,10 +30,9 @@ static inline unsigned char get_bitmask(uint32_t pos)\n \n static int load_bloom_filter_from_graph(struct commit_graph *g,\n \t\t\t\t\tstruct bloom_filter *filter,\n-\t\t\t\t\tstruct commit *c)\n+\t\t\t\t\tuint32_t graph_pos)\n {\n \tuint32_t lex_pos, start_index, end_index;\n-\tuint32_t graph_pos = commit_graph_position(c);\n \n \twhile (graph_pos < g->num_commits_in_base)\n \t\tg = g->base_graph;\n@@ -203,9 +202,10 @@ struct bloom_filter *get_or_compute_bloom_filter(struct repository *r,\n \tfilter = bloom_filter_slab_at(&bloom_filters, c);\n \n \tif (!filter->data) {\n-\t\tload_commit_graph_info(r, c);\n-\t\tif (commit_graph_position(c) != COMMIT_NOT_FROM_GRAPH)\n-\t\t\tload_bloom_filter_from_graph(r->objects->commit_graph, filter, c);\n+\t\tuint32_t graph_pos;\n+\t\tif (repo_find_commit_pos_in_graph(r, c, &graph_pos))\n+\t\t\tload_bloom_filter_from_graph(r->objects->commit_graph,\n+\t\t\t\t\t\t filter, graph_pos);\n \t}\n \n \tif (filter->data && filter->len)\ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex 4d9f62f22d..f5f0895631 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -811,7 +811,7 @@ test_expect_success 'set up and verify repo with generation data overflow chunk'\n \n graph_git_behavior 'generation data overflow chunk repo' repo left right\n \n-test_expect_failure 'overflow during generation version upgrade' '\n+test_expect_success 'overflow during generation version upgrade' '\n \tgit init overflow-v2-upgrade &&\n \t(\n \t\tcd overflow-v2-upgrade &&\n-- \n2.37.0.1.g1379af2e9d\n"},{"id":"458996","messageId":"xmqqk08gdi5i.fsf@gitster.g","threadId":"58147","inReplyTo":"cover.1657667404.git.me@ttaylorr.com","subject":"Re: [PATCH 0/3] commit-graph: fix corruption during generation v2 upgrade","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-07-13T17:41:45Z","receivedAt":"2022-07-13T17:41:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> This brief series resolves a bug where a commit-graph would become\n> corrupt when upgrading from generation number v1 to v2, as originally\n> reported in [1].\n>\n> Some speculation occurred as to what might be causing that bug in the\n> thread beginning at [1], until the problem was explained in more detail\n> by Will Chandler in [2].\n>\n> The crux of the issue, as is described in [2] and [3], is that the\n> commit_graph_data slab is reused for read and write operations involving\n> the commit-graph, leading to situations where data computed in\n> preparation of a write is clobbered by a read of existing data.\n>\n> The first patch demonstrates the issue, and the second patch prepares to\n> fix it by introducing a helper function. The crux of the issue is\n> described and fixed in the third patch.\n>\n> [1]: https://lore.kernel.org/git/YqD5dgalb9EPnz85@coredump.intra.peff.net/\n> [2]: https://lore.kernel.org/git/DD88D523-0ECA-4474-9AA5-1D4A431E532A@wfchandler.org/\n> [3]: https://lore.kernel.org/git/YsS7H4i5DqUZVQ5i@nand.local/\n\nThanks. Do we know where this breaks? Applying [1/3] on Git 2.32,\n2.34, and 2.35 seems to claim that \"known breakage vanished\".\n"},{"id":"459107","messageId":"YtDKmo72LgqzEycD@nand.local","threadId":"58147","inReplyTo":"xmqqk08gdi5i.fsf@gitster.g","subject":"Re: [PATCH 0/3] commit-graph: fix corruption during generation v2 upgrade","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-07-15T02:02:02Z","receivedAt":"2022-07-15T02:02:11Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Jul 13, 2022 at 10:41:45AM -0700, Junio C Hamano wrote:\n> Thanks. Do we know where this breaks? Applying [1/3] on Git 2.32,\n> 2.34, and 2.35 seems to claim that \"known breakage vanished\".\n\nWith a script like:\n\n--- >8 ---\n#!/bin/sh\n\nset -e\n\nrm -fr repo\ngit init -q repo\ncd repo\n\necho \"x\" >x\ngit add x\nGIT_AUTHOR_DATE=\"@2 +0000\" \\\n GIT_COMMITTER_DATE=\"@2 +0000\" git commit -q -m \"$(cat x)\"\n\ngit repack -d -q\n\ngit.compile -c commitGraph.generationVersion=1 commit-graph write\ngit.compile -c commitGraph.generationVersion=2 commit-graph write \\\n --changed-paths\n\ngit.compile rev-list --all\n--- 8< ---\n\nYou can bisect it to 3b0199d4c3 (commit-graph: start parsing generation\nv2 (again), 2022-03-01), but only because that patch teaches Git to\nrecognize the existence of the generation v2 chunks.\n\nI suspect (but haven't confirmed) that it was probably broken before\n3b0199d4c3. But such breakage wouldn't have mattered, since despite\nunderstanding generation v2, previous versions of Git never read those\nchunks, which would have masked over this bug.\n\nThanks,\nTaylor\n"},{"id":"459118","messageId":"10d12d6f-6d86-743f-518b-eb45871509a7@github.com","threadId":"58147","inReplyTo":"0a49c86037bac200bb23e1abf9f67363e99c4b7c.1657667404.git.me@ttaylorr.com","subject":"Re: [PATCH 1/3] t5318: demonstrate commit-graph generation v2 corruption","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-07-15T03:15:42Z","receivedAt":"2022-07-15T03:15:49Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 7/12/2022 7:10 PM, Taylor Blau wrote:\n> When upgrading a commit-graph using generation v1 to one using\n> generation v2, it is possible to force Git into a corrupt state where it\n> (incorrectly) believes that a GDO2 chunk is necessary, *after* deciding\n> not to write one.\n> \n> This makes subsequent reads using the commit-graph produce the following\n> error message:\n> \n> fatal: commit-graph requires overflow generation data but has none\n> \n> Demonstrate this bug by increasing our test coverage to include a\n> minimal example of upgrading a commit-graph from generation v1 to v2.\n> The only notable components of this test are:\n> \n> - The committer date of the commit is chosen carefully so that the\n> offset underflows when computed using a v1 generation number, but\n> would not overflow when using v2 generation numbers.\n> \n> - The upgrade to generation number v2 must read in the v1 generation\n> numbers, which we can do by passing `--changed-paths`, which will\n> force the commit-graph internals to call `fill_commit_graph_info()`.\n> \n> A future patch will squash this bug.\n\nThanks for finding a good test.\n\n> +\t\t# This commit will have a date at two seconds past the Epoch,\n> +\t\t# and a (v1) generation number of 1, since it is a root commit.\n> +\t\t#\n> +\t\t# The offset will then be computed as 2-1, which will underflow\n\nI have verified that your test works, but this explanation is confusing me.\n\"2 - 1\" is 1, which does not underflow. There must be something else going\non.\n\nLooking ahead, you describe the situation correctly in Patch 3 to show that\nwe take \"generation - date\", so you really just need s/2-1/1-2/ here.\n\n> +\t\t# to 2^31, which is greater than the v2 offset small limit of\n> +\t\t# 2^31-1.\n> +\t\t#\n> +\t\t# This is sufficient to need a large offset table for the v2\n> +\t\t# generation numbers.\n> +\t\ttest_commit --date \"@2 +0000\" base &&\n> +\t\tgit repack -d &&\n> +\n> +\t\t# Test that upgrading from generation v1 to v2 correctly\n> +\t\t# produces the overflow table.\n> +\t\tgit -c commitGraph.generationVersion=1 commit-graph write &&\n> +\t\tgit -c commitGraph.generationVersion=2 commit-graph write \\\n> +\t\t\t--changed-paths &&\n\nSimple and fast to set up and test. Thanks for using the config explicitly\nin both commands so it is robust to possible default changes in the future.\n\nThanks,\n-Stolee\n"},{"id":"459119","messageId":"3036d949-63b2-9d6c-b3f6-0075fe9e1ca5@github.com","threadId":"58147","inReplyTo":"e988c974119edbabc06dae6d24810fc0bafbdc94.1657667404.git.me@ttaylorr.com","subject":"Re: [PATCH 2/3] commit-graph: introduce `repo_find_commit_pos_in_graph()`","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-07-15T03:17:48Z","receivedAt":"2022-07-15T03:17:53Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 7/12/2022 7:10 PM, Taylor Blau wrote:\n\n> +int repo_find_commit_pos_in_graph(struct repository *r, struct commit *c,\n> +\t\t\t\t uint32_t *pos)\n> +{\n> +\tif (!prepare_commit_graph(r))\n> +\t\treturn 0;\n> +\treturn find_commit_pos_in_graph(c, r->objects->commit_graph, pos);\n> +}\n> +\n\nThis is absolutely correct for the given prototype.\n\n> void load_commit_graph_info(struct repository *r, struct commit *item)\n> {\n> \tuint32_t pos;\n> -\tif (!prepare_commit_graph(r))\n> -\t\treturn;\n> -\tif (find_commit_pos_in_graph(item, r->objects->commit_graph, &pos))\n> +\tif (repo_find_commit_pos_in_graph(r, item, &pos))\n> \t\tfill_commit_graph_info(item, r->objects->commit_graph, pos);\n\nNormally I would complain about referring directly to r->objects->commit_graph\nwithout an explicit prepare_commit_graph() in the method. My initial thought\nwas that we would need to know that the new method prepares the graph, but\nobviously the commit-graph needs to exist and be prepared if we are loading a\nposition from it.\n\nAll good here.\n\nThanks,\n-Stolee\n"},{"id":"459120","messageId":"14325f93-051a-c286-9dc4-1a2762554a27@github.com","threadId":"58147","inReplyTo":"cover.1657667404.git.me@ttaylorr.com","subject":"Re: [PATCH 0/3] commit-graph: fix corruption during generation v2 upgrade","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-07-15T03:20:54Z","receivedAt":"2022-07-15T03:21:00Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 7/12/2022 7:10 PM, Taylor Blau wrote:\n> This brief series resolves a bug where a commit-graph would become\n> corrupt when upgrading from generation number v1 to v2, as originally\n> reported in [1].\n> \n> Some speculation occurred as to what might be causing that bug in the\n> thread beginning at [1], until the problem was explained in more detail\n> by Will Chandler in [2].\n> \n> The crux of the issue, as is described in [2] and [3], is that the\n> commit_graph_data slab is reused for read and write operations involving\n> the commit-graph, leading to situations where data computed in\n> preparation of a write is clobbered by a read of existing data.\n> \n> The first patch demonstrates the issue, and the second patch prepares to\n> fix it by introducing a helper function. The crux of the issue is\n> described and fixed in the third patch.\n\nThanks, all, for identifying the scenario that causes this breakage.\nIt was frustrating that we had reports of corruption without knowing\nwhere it was coming from.\n\nThis solution looks like the best we can do right now, although it\nhints that some refactoring would be good to distinguish between\n\"commits as read\" and \"commits to be written\" during this upgrade\nphase. Something to think about, but not to block this fix.\n\nThanks,\n-Stolee\n"},{"id":"459173","messageId":"YtHkmb4Qwvo9n00A@nand.local","threadId":"58147","inReplyTo":"10d12d6f-6d86-743f-518b-eb45871509a7@github.com","subject":"Re: [PATCH 1/3] t5318: demonstrate commit-graph generation v2 corruption","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-07-15T22:05:13Z","receivedAt":"2022-07-15T22:10:45Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Jul 14, 2022 at 11:15:42PM -0400, Derrick Stolee wrote:\n> > +\t\t# This commit will have a date at two seconds past the Epoch,\n> > +\t\t# and a (v1) generation number of 1, since it is a root commit.\n> > +\t\t#\n> > +\t\t# The offset will then be computed as 2-1, which will underflow\n>\n> I have verified that your test works, but this explanation is confusing me.\n> \"2 - 1\" is 1, which does not underflow. There must be something else going\n> on.\n>\n> Looking ahead, you describe the situation correctly in Patch 3 to show that\n> we take \"generation - date\", so you really just need s/2-1/1-2/ here.\n\nYes, absolutely. Thanks for catching it.\n\nJunio: you may want to s/2-1/1-2 in this patch's message, or I can send\nyou a replacement or reroll, whatever is easier.\n\nThanks,\nTaylor\n"},{"id":"459179","messageId":"xmqqo7xqx6wj.fsf@gitster.g","threadId":"58147","inReplyTo":"YtHkmb4Qwvo9n00A@nand.local","subject":"Re: [PATCH 1/3] t5318: demonstrate commit-graph generation v2 corruption","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-07-16T00:01:16Z","receivedAt":"2022-07-16T00:01:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> On Thu, Jul 14, 2022 at 11:15:42PM -0400, Derrick Stolee wrote:\n>> > +\t\t# This commit will have a date at two seconds past the Epoch,\n>> > +\t\t# and a (v1) generation number of 1, since it is a root commit.\n>> > +\t\t#\n>> > +\t\t# The offset will then be computed as 2-1, which will underflow\n>>\n>> I have verified that your test works, but this explanation is confusing me.\n>> \"2 - 1\" is 1, which does not underflow. There must be something else going\n>> on.\n>>\n>> Looking ahead, you describe the situation correctly in Patch 3 to show that\n>> we take \"generation - date\", so you really just need s/2-1/1-2/ here.\n>\n> Yes, absolutely. Thanks for catching it.\n>\n> Junio: you may want to s/2-1/1-2 in this patch's message, or I can send\n> you a replacement or reroll, whatever is easier.\n\nI've already done \"rebase -i\" to do so.\n\nBut for future reference, the easiest for me is if the author said,\nafter saying \"Thanks for catching it\", \"Will reroll after waiting\nfor a bit to see if there are other comments\". That way, I only\nhave to edit the latest draft of \"What's cooking\" report to mark the\ntopic to be expecting a reroll (which will prevent me from merging\nthe topic down to 'next' prematurely by mistake) and forget about\nit, until I actually see the updated set of patches. It would be\neven easier if the updated set of patches said which topic it is\nmeant to replace. That way, I can trust other reviewers about the\ndetails of the change between iterations and play a patch monkey,\nwhen I am short of time.\n\nIt is more work to (1) look at the message you are responding to and\nunderstand what \"2 minus 1\" vs \"1 minus 2\" being discussed is about,\nand (2) go one level up in the thread to find the line in the patch\nthat was being discussed, and (3) run \"rebase -i\" and change \"pick\"\nto \"edit\" and (4) find the line in the file that was affected by the\nhunk in question and edit it. The worst part of it is I'd either\nhave to do it right away before I forget, or mark the message unread\nso that I can revisit it when I have enough time.\n\nEither way is fine, but a straight resend with two notices (one that\nsays \"will reroll\", the other that says \"this replaces topic X\") is\nthe easiest to handle for me.\n\nThanks.\n\n"},{"id":"459180","messageId":"YtIDsIVCJ8XgF1eJ@nand.local","threadId":"58147","inReplyTo":"xmqqo7xqx6wj.fsf@gitster.g","subject":"Re: [PATCH 1/3] t5318: demonstrate commit-graph generation v2 corruption","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-07-16T00:17:52Z","receivedAt":"2022-07-16T00:17:57Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Jul 15, 2022 at 05:01:16PM -0700, Junio C Hamano wrote:\n> Taylor Blau writes:\n>\n> > On Thu, Jul 14, 2022 at 11:15:42PM -0400, Derrick Stolee wrote:\n> >> > +\t\t# This commit will have a date at two seconds past the Epoch,\n> >> > +\t\t# and a (v1) generation number of 1, since it is a root commit.\n> >> > +\t\t#\n> >> > +\t\t# The offset will then be computed as 2-1, which will underflow\n> >>\n> >> I have verified that your test works, but this explanation is confusing me.\n> >> \"2 - 1\" is 1, which does not underflow. There must be something else going\n> >> on.\n> >>\n> >> Looking ahead, you describe the situation correctly in Patch 3 to show that\n> >> we take \"generation - date\", so you really just need s/2-1/1-2/ here.\n> >\n> > Yes, absolutely. Thanks for catching it.\n> >\n> > Junio: you may want to s/2-1/1-2 in this patch's message, or I can send\n> > you a replacement or reroll, whatever is easier.\n>\n> I've already done \"rebase -i\" to do so.\n\nThanks very much.\n\n> But for future reference, the easiest for me is if the author said,\n> after saying \"Thanks for catching it\", \"Will reroll after waiting\n> for a bit to see if there are other comments\". That way, I only\n> have to edit the latest draft of \"What's cooking\" report to mark the\n> topic to be expecting a reroll (which will prevent me from merging\n> the topic down to 'next' prematurely by mistake) and forget about\n> it, until I actually see the updated set of patches. It would be\n> even easier if the updated set of patches said which topic it is\n> meant to replace. That way, I can trust other reviewers about the\n> details of the change between iterations and play a patch monkey,\n> when I am short of time.\n\nMakes sense. I appreciate you clarifying it explicitly, I've wondered\nover the years what is easiest for you when fixing a trivial issue in a\nlarger series.\n\nI've tended to try and avoid resubmitting a whole series when there is\njust a typo hoping to avoid flooding the list with too many (mostly\nunchanged) messages. But that requires you to do more work to futz with\nthe patches before they hit your tree, so I try not to do it too often.\n\nIn any case, I'll try to err more often on the side of resubmitting a\nseries after acking the typo in the hopes it makes your life easier ;-).\n\nThanks,\nTaylor\n"}]}