Fix memory corruption with FSMonitor-enabled unpack_trees()
#891
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
In 56c6910 (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 679f2f9 (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>
In 56c6910 (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>
dscho
force-pushed
the
fix-fsmonitor-crash
branch
from
3c8e9ea
to
2b4dd0c
Compare
|
/submit |
|
Submitted as pull.891.git.1615995049.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
| @@ -1544,8 +1544,8 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options | |||
| o->merge_size = len; | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"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
|
On the Git mailing list, Derrick Stolee wrote (reply to this): |
|
User |
|
This branch is now known as |
|
This patch series was integrated into seen via git@5f24c7e. |
|
There was a status update about the branch The data structure used by fsmonitor interface was not properly duplicated during an in-core merge, leading to use-after-free etc. Will merge to 'next'. |
|
This patch series was integrated into seen via git@10078f7. |
|
This patch series was integrated into next via git@2ab614f. |
|
On the Git mailing list, Johannes Schindelin wrote (reply to this): |
|
This patch series was integrated into seen via git@1dd4e74. |
|
This patch series was integrated into next via git@1dd4e74. |
|
This patch series was integrated into master via git@1dd4e74. |
|
Closed via 1dd4e74. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
cc: Derrick Stolee stolee@gmail.com