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 <[email protected]>

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.

---
 unpack-trees.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index 2399b6818be6..63e3d961b378 100644
@@ -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" <[email protected]>

From: Johannes Schindelin <[email protected]>

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.

---
 read-cache.c | 1 +
 1 file changed, 1 insertion(+)

index aa427c5c170f..cf5ff3158550 100644
@@ -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

Thanks for dogfooding!

A little over a year, yes, since the v2 hook was committed. It's old enough that these could be applied to 'maint'.

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,

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

Indeed. Even better: if you look closely at the GitGitGadget PR, you will see that I based it on kw/fsmonitor-watchman-racefix.

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