diff mbox series

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

Message ID e194809e547eb5bc8e8d1ad09d874ebfde0efe4f.1615995049.git.gitgitgadget@gmail.com (mailing list archive)
State Accepted
Commit 3dfd30598bdb34078329aa9bd9b03c5ab5cdbcce
Headers show
Series Fix memory corruption with FSMonitor-enabled unpack_trees() | expand

Commit Message

Johannes Schindelin March 17, 2021, 3:30 p.m. UTC 
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(-)

Comments

Junio C Hamano March 17, 2021, 7:19 p.m. UTC | #1 
"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
diff mbox series

Patch

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