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

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

[Johannes Schindelin via GitGitGadget]

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

[PATCH 1/2] fsmonitor: fix memory corruption in some corner cases

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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 <johannes.schindelin@gmx.de>
---
 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

[PATCH 2/2] fsmonitor: do not forget to release the token in `discard_index()`

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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 <johannes.schindelin@gmx.de>
---
 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

Re: [PATCH 1/2] fsmonitor: fix memory corruption in some corner cases

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
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

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

[Derrick Stolee]

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

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

[Johannes Schindelin]

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