{"thread":{"id":"55338","subject":"[PATCH 0/2] Fix memory corruption with FSMonitor-enabled unpack_trees()","startedAt":"2021-03-17T15:59:23Z","lastAt":"2021-03-19T14:50:23Z","messageCount":6,"participants":["Johannes Schindelin via GitGitGadget","Junio C Hamano","Derrick Stolee","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"419476","messageId":"pull.891.git.1615995049.gitgitgadget@gmail.com","threadId":"55338","inReplyTo":null,"subject":"[PATCH 0/2] Fix memory corruption with FSMonitor-enabled unpack_trees()","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-03-17T15:30:47Z","receivedAt":"2021-03-17T15:59:23Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"While dog-fooding Jeff Hostetler's FSMonitor patches, I ran into a really\nobscure segmentation fault during one of my epic Git for Windows rebases.\nTurns out that this bug is quite old.\n\nJohannes Schindelin (2):\n fsmonitor: fix memory corruption in some corner cases\n fsmonitor: do not forget to release the token in `discard_index()`\n\n read-cache.c | 1 +\n unpack-trees.c | 4 ++--\n 2 files changed, 3 insertions(+), 2 deletions(-)\n\n\nbase-commit: dfaed028620c2dca08d24583c7fc8b0aef9b6d0f\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-891%2Fdscho%2Ffix-fsmonitor-crash-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-891/dscho/fix-fsmonitor-crash-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/891\n-- \ngitgitgadget\n"},{"id":"419477","messageId":"e194809e547eb5bc8e8d1ad09d874ebfde0efe4f.1615995049.git.gitgitgadget@gmail.com","threadId":"55338","inReplyTo":"pull.891.git.1615995049.gitgitgadget@gmail.com","subject":"[PATCH 1/2] fsmonitor: fix memory corruption in some corner cases","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-03-17T15:30:48Z","receivedAt":"2021-03-17T16:01:03Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn 56c6910028a (fsmonitor: change last update timestamp on the\nindex_state to opaque token, 2020-01-07), we forgot to adjust the part\nof `unpack_trees()` that copies the FSMonitor \"last-update\" information\nthat we copy from the source index to the result index since 679f2f9fdd2\n(unpack-trees: skip stat on fsmonitor-valid files, 2019-11-20).\n\nSince the \"last-update\" information is no longer a 64-bit number, but a\nfree-form string that has been allocated, we need to duplicate it rather\nthan just copying it.\n\nThis is important because there _are_ cases when `unpack_trees()` will\nperform a oneway merge that implicitly calls `refresh_fsmonitor()`\n(which will allocate that \"last-update\" token). This happens _after_\nthat token was copied into the result index. However, we _then_ call\n`check_updates()` on that index, which will _also_ call\n`refresh_fsmonitor()`, accessing the \"last-update\" string, which by now\nwould be released already.\n\nIn the instance that lead to this patch, this caused a segmentation\nfault during a lengthy, complicated rebase involving the todo command\n`reset` that (crucially) had to updated many files. Unfortunately, it\nseems very hard to trigger that crash, therefore this patch is not\naccompanied by a regression test.\n\nSigned-off-by: Johannes Schindelin \n---\n unpack-trees.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 2399b6818be6..63e3d961b378 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -1544,8 +1544,8 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n \to->merge_size = len;\n \tmark_all_ce_unused(o->src_index);\n \n-\tif (o->src_index->fsmonitor_last_update)\n-\t\to->result.fsmonitor_last_update = o->src_index->fsmonitor_last_update;\n+\to->result.fsmonitor_last_update =\n+\t\txstrdup_or_null(o->src_index->fsmonitor_last_update);\n \n \t/*\n \t * Sparse checkout loop #1: set NEW_SKIP_WORKTREE on existing entries\n-- \ngitgitgadget\n\n"},{"id":"419478","messageId":"2b4dd0c9160ebf1d676c63585869f1a35da1a0bd.1615995049.git.gitgitgadget@gmail.com","threadId":"55338","inReplyTo":"pull.891.git.1615995049.gitgitgadget@gmail.com","subject":"[PATCH 2/2] fsmonitor: do not forget to release the token in `discard_index()`","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-03-17T15:30:49Z","receivedAt":"2021-03-17T16:02:06Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn 56c6910028a (fsmonitor: change last update timestamp on the\nindex_state to opaque token, 2020-01-07), we forgot to adjust\n`discard_index()` to release the \"last-update\" token: it is no longer a\n64-bit number, but a free-form string that has been allocated.\n\nSigned-off-by: Johannes Schindelin \n---\n read-cache.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex aa427c5c170f..cf5ff3158550 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -2364,6 +2364,7 @@ int discard_index(struct index_state *istate)\n \tcache_tree_free(&(istate->cache_tree));\n \tistate->initialized = 0;\n \tistate->fsmonitor_has_run_once = 0;\n+\tFREE_AND_NULL(istate->fsmonitor_last_update);\n \tFREE_AND_NULL(istate->cache);\n \tistate->cache_alloc = 0;\n \tdiscard_split_index(istate);\n-- \ngitgitgadget\n"},{"id":"419538","messageId":"xmqqwnu5twpj.fsf@gitster.g","threadId":"55338","inReplyTo":"e194809e547eb5bc8e8d1ad09d874ebfde0efe4f.1615995049.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/2] fsmonitor: fix memory corruption in some corner cases","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-03-17T19:19:04Z","receivedAt":"2021-03-17T19:20:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> --- a/unpack-trees.c\n> +++ b/unpack-trees.c\n> @@ -1544,8 +1544,8 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n> \to->merge_size = len;\n> \tmark_all_ce_unused(o->src_index);\n> \n> -\tif (o->src_index->fsmonitor_last_update)\n> -\t\to->result.fsmonitor_last_update = o->src_index->fsmonitor_last_update;\n> +\to->result.fsmonitor_last_update =\n> +\t\txstrdup_or_null(o->src_index->fsmonitor_last_update);\n\nAnd this won't happen twice, so there is no need to free what is in\nthe o->result side before assignment. And 2/2 frees it so we do not\nleak at the end either.\n\nWill queue; thanks.\n\n> \n> \t/*\n> \t * Sparse checkout loop #1: set NEW_SKIP_WORKTREE on existing entries\n"},{"id":"419547","messageId":"07a368ee-6b45-a401-32c3-c052370150f5@gmail.com","threadId":"55338","inReplyTo":"pull.891.git.1615995049.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/2] Fix memory corruption with FSMonitor-enabled unpack_trees()","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-03-17T20:21:33Z","receivedAt":"2021-03-17T20:22:33Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 3/17/2021 11:30 AM, Johannes Schindelin via GitGitGadget wrote:\n> While dog-fooding Jeff Hostetler's FSMonitor patches, I ran into a really\n> obscure segmentation fault during one of my epic Git for Windows rebases.\n\nThanks for dogfooding!\n\n> Turns out that this bug is quite old.\n\nA little over a year, yes, since the v2 hook was committed. It's old\nenough that these could be applied to 'maint'.\n \n> Johannes Schindelin (2):\n> fsmonitor: fix memory corruption in some corner cases\n> fsmonitor: do not forget to release the token in `discard_index()`\n\nThe patches themselves are correct and describe the problem well.\nThey only show up during non-trivial uses of FS Monitor and index\nupdates, so I understand your hesitance to write tests that trigger\nthese problems.\n\nThanks,\n-Stolee\n"},{"id":"419711","messageId":"nycvar.QRO.7.76.6.2103191546190.57@tvgsbejvaqbjf.bet","threadId":"55338","inReplyTo":"07a368ee-6b45-a401-32c3-c052370150f5@gmail.com","subject":"Re: [PATCH 0/2] Fix memory corruption with FSMonitor-enabled unpack_trees()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-03-19T14:49:32Z","receivedAt":"2021-03-19T14:50:23Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Stolee,\n\nOn Wed, 17 Mar 2021, Derrick Stolee wrote:\n\n> On 3/17/2021 11:30 AM, Johannes Schindelin via GitGitGadget wrote:\n> > While dog-fooding Jeff Hostetler's FSMonitor patches, I ran into a really\n> > obscure segmentation fault during one of my epic Git for Windows rebases.\n>\n> Thanks for dogfooding!\n\nI'm completely selfish here, as I want to benefit from the speed myself,\nand that's also the reason why I added this as an experimental option to\nGit for Windows v2.31.0: That way, I can test it everywhere (and so can\nothers).\n\n> > Turns out that this bug is quite old.\n>\n> A little over a year, yes, since the v2 hook was committed. It's old\n> enough that these could be applied to 'maint'.\n\nIndeed. Even better: if you look closely at the GitGitGadget PR, you will\nsee that I based it on `kw/fsmonitor-watchman-racefix`.\n\n> > Johannes Schindelin (2):\n> > fsmonitor: fix memory corruption in some corner cases\n> > fsmonitor: do not forget to release the token in `discard_index()`\n>\n> The patches themselves are correct and describe the problem well.\n> They only show up during non-trivial uses of FS Monitor and index\n> updates, so I understand your hesitance to write tests that trigger\n> these problems.\n\nRight. For me, I ran into them only in one specific instance, when\nrebasing Git for Windows' patch thicket onto `seen`, and then only when\nmerging a topic branch with a rather big diff.\n\nThanks,\nDscho\n"}]}