threads / patch / 55338

patch, 2 partsFix memory corruption with FSMonitor-enabled unpack_trees()

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

## tl;dr

6 messages between Mar 17, 2021 and Mar 19, 2021. Diffs are folded; open one to read it.

replies: 5people: 3as markdown or json

Johannes Schindelin via GitGitGadget· Mar 17, 2021, 15:30 UTC · lore

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· Mar 17, 2021, 15:30 UTC · re: Johannes Schindelin via GitGitGadget · lore

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

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

Show changes to unpack-trees.c +2 −2

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

Junio C Hamano· Mar 17, 2021, 19:19 UTC · re: Johannes Schindelin via GitGitGadget · lore

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

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 10 quoted lines

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

Johannes Schindelin via GitGitGadget· Mar 17, 2021, 15:30 UTC · re: Johannes Schindelin via GitGitGadget · lore

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

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(+)

Show changes to read-cache.c +1 −0

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

Derrick Stolee· Mar 17, 2021, 20:21 UTC · re: Johannes Schindelin via GitGitGadget · lore

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

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· Mar 19, 2021, 14:49 UTC · re: Derrick Stolee · lore

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

Hi Stolee,

On Wed, 17 Mar 2021, Derrick Stolee wrote:

Show 5 quoted lines

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

Show 8 quoted lines

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