# [PATCH 0/2] Fix memory corruption with FSMonitor-enabled unpack_trees() 6 messages from 2021-03-17 to 2021-03-19. Participants: Johannes Schindelin via GitGitGadget, Junio C Hamano, Derrick Stolee, Johannes Schindelin. Thread: https://gitlist.dev/t/55338 ## Johannes Schindelin via GitGitGadget, 2021-03-17 15:30 Subject: [PATCH 0/2] Fix memory corruption with FSMonitor-enabled unpack_trees() Message-ID: URL: https://gitlist.dev/e/pull.891.git.1615995049.gitgitgadget%40gmail.com ``` While dog-fooding Jeff Hostetler's FSMonitor patches, I ran into a really obscure segmentation fault during one of my epic Git for Windows rebases. Turns out that this bug is quite old. Johannes Schindelin (2): fsmonitor: fix memory corruption in some corner cases fsmonitor: do not forget to release the token in `discard_index()` read-cache.c | 1 + unpack-trees.c | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-) base-commit: dfaed028620c2dca08d24583c7fc8b0aef9b6d0f Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-891%2Fdscho%2Ffix-fsmonitor-crash-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-891/dscho/fix-fsmonitor-crash-v1 Pull-Request: https://github.com/gitgitgadget/git/pull/891 -- gitgitgadget ``` ## Johannes Schindelin via GitGitGadget, 2021-03-17 15:30 Subject: [PATCH 1/2] fsmonitor: fix memory corruption in some corner cases Message-ID: URL: https://gitlist.dev/e/e194809e547eb5bc8e8d1ad09d874ebfde0efe4f.1615995049.git.gitgitgadget%40gmail.com In-Reply-To: ``` From: Johannes Schindelin In 56c6910028a (fsmonitor: change last update timestamp on the index_state to opaque token, 2020-01-07), we forgot to adjust the part of `unpack_trees()` that copies the FSMonitor "last-update" information that we copy from the source index to the result index since 679f2f9fdd2 (unpack-trees: skip stat on fsmonitor-valid files, 2019-11-20). Since the "last-update" information is no longer a 64-bit number, but a free-form string that has been allocated, we need to duplicate it rather than just copying it. This is important because there _are_ cases when `unpack_trees()` will perform a oneway merge that implicitly calls `refresh_fsmonitor()` (which will allocate that "last-update" token). This happens _after_ that token was copied into the result index. However, we _then_ call `check_updates()` on that index, which will _also_ call `refresh_fsmonitor()`, accessing the "last-update" string, which by now would be released already. In the instance that lead to this patch, this caused a segmentation fault during a lengthy, complicated rebase involving the todo command `reset` that (crucially) had to updated many files. Unfortunately, it seems very hard to trigger that crash, therefore this patch is not accompanied by a regression test. Signed-off-by: Johannes Schindelin --- unpack-trees.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/unpack-trees.c b/unpack-trees.c index 2399b6818be6..63e3d961b378 100644 --- a/unpack-trees.c +++ b/unpack-trees.c @@ -1544,8 +1544,8 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options o->merge_size = len; mark_all_ce_unused(o->src_index); - if (o->src_index->fsmonitor_last_update) - o->result.fsmonitor_last_update = o->src_index->fsmonitor_last_update; + o->result.fsmonitor_last_update = + xstrdup_or_null(o->src_index->fsmonitor_last_update); /* * Sparse checkout loop #1: set NEW_SKIP_WORKTREE on existing entries -- gitgitgadget ``` ## Johannes Schindelin via GitGitGadget, 2021-03-17 15:30 Subject: [PATCH 2/2] fsmonitor: do not forget to release the token in `discard_index()` Message-ID: <2b4dd0c9160ebf1d676c63585869f1a35da1a0bd.1615995049.git.gitgitgadget@gmail.com> URL: https://gitlist.dev/e/2b4dd0c9160ebf1d676c63585869f1a35da1a0bd.1615995049.git.gitgitgadget%40gmail.com In-Reply-To: ``` From: Johannes Schindelin In 56c6910028a (fsmonitor: change last update timestamp on the index_state to opaque token, 2020-01-07), we forgot to adjust `discard_index()` to release the "last-update" token: it is no longer a 64-bit number, but a free-form string that has been allocated. Signed-off-by: Johannes Schindelin --- read-cache.c | 1 + 1 file changed, 1 insertion(+) diff --git a/read-cache.c b/read-cache.c index aa427c5c170f..cf5ff3158550 100644 --- a/read-cache.c +++ b/read-cache.c @@ -2364,6 +2364,7 @@ int discard_index(struct index_state *istate) cache_tree_free(&(istate->cache_tree)); istate->initialized = 0; istate->fsmonitor_has_run_once = 0; + FREE_AND_NULL(istate->fsmonitor_last_update); FREE_AND_NULL(istate->cache); istate->cache_alloc = 0; discard_split_index(istate); -- gitgitgadget ``` ## Junio C Hamano, 2021-03-17 19:19 Subject: Re: [PATCH 1/2] fsmonitor: fix memory corruption in some corner cases Message-ID: URL: https://gitlist.dev/e/xmqqwnu5twpj.fsf%40gitster.g In-Reply-To: ``` "Johannes Schindelin via GitGitGadget" writes: > --- a/unpack-trees.c > +++ b/unpack-trees.c > @@ -1544,8 +1544,8 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options > o->merge_size = len; > mark_all_ce_unused(o->src_index); > > - if (o->src_index->fsmonitor_last_update) > - o->result.fsmonitor_last_update = o->src_index->fsmonitor_last_update; > + o->result.fsmonitor_last_update = > + xstrdup_or_null(o->src_index->fsmonitor_last_update); And this won't happen twice, so there is no need to free what is in the o->result side before assignment. And 2/2 frees it so we do not leak at the end either. Will queue; thanks. > > /* > * Sparse checkout loop #1: set NEW_SKIP_WORKTREE on existing entries ``` ## Derrick Stolee, 2021-03-17 20:21 Subject: Re: [PATCH 0/2] Fix memory corruption with FSMonitor-enabled unpack_trees() Message-ID: <07a368ee-6b45-a401-32c3-c052370150f5@gmail.com> URL: https://gitlist.dev/e/07a368ee-6b45-a401-32c3-c052370150f5%40gmail.com In-Reply-To: ``` On 3/17/2021 11:30 AM, Johannes Schindelin via GitGitGadget wrote: > While dog-fooding Jeff Hostetler's FSMonitor patches, I ran into a really > obscure segmentation fault during one of my epic Git for Windows rebases. Thanks for dogfooding! > Turns out that this bug is quite old. A little over a year, yes, since the v2 hook was committed. It's old enough that these could be applied to 'maint'. > Johannes Schindelin (2): > fsmonitor: fix memory corruption in some corner cases > fsmonitor: do not forget to release the token in `discard_index()` The patches themselves are correct and describe the problem well. They only show up during non-trivial uses of FS Monitor and index updates, so I understand your hesitance to write tests that trigger these problems. Thanks, -Stolee ``` ## Johannes Schindelin, 2021-03-19 14:49 Subject: Re: [PATCH 0/2] Fix memory corruption with FSMonitor-enabled unpack_trees() Message-ID: URL: https://gitlist.dev/e/nycvar.QRO.7.76.6.2103191546190.57%40tvgsbejvaqbjf.bet In-Reply-To: <07a368ee-6b45-a401-32c3-c052370150f5@gmail.com> ``` Hi Stolee, On Wed, 17 Mar 2021, Derrick Stolee wrote: > On 3/17/2021 11:30 AM, Johannes Schindelin via GitGitGadget wrote: > > While dog-fooding Jeff Hostetler's FSMonitor patches, I ran into a really > > obscure segmentation fault during one of my epic Git for Windows rebases. > > Thanks for dogfooding! I'm completely selfish here, as I want to benefit from the speed myself, and that's also the reason why I added this as an experimental option to Git for Windows v2.31.0: That way, I can test it everywhere (and so can others). > > Turns out that this bug is quite old. > > A little over a year, yes, since the v2 hook was committed. It's old > enough that these could be applied to 'maint'. Indeed. Even better: if you look closely at the GitGitGadget PR, you will see that I based it on `kw/fsmonitor-watchman-racefix`. > > Johannes Schindelin (2): > > fsmonitor: fix memory corruption in some corner cases > > fsmonitor: do not forget to release the token in `discard_index()` > > The patches themselves are correct and describe the problem well. > They only show up during non-trivial uses of FS Monitor and index > updates, so I understand your hesitance to write tests that trigger > these problems. Right. For me, I ran into them only in one specific instance, when rebasing Git for Windows' patch thicket onto `seen`, and then only when merging a topic branch with a rather big diff. Thanks, Dscho ```