[PATCH 0/2] Fix memory corruption with FSMonitor-enabled unpack_trees()

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

From: Johannes Schindelin <redacted>

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 <redacted>
---
 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

From: Johannes Schindelin <redacted>

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 <redacted>
---
 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

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

Hi Stolee,

On Wed, 17 Mar 2021, Derrick Stolee wrote:

On 3/17/2021 11:30 AM, Johannes Schindelin via GitGitGadget wrote:

quoted

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).

quoted

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`.

quoted

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