Sequencer: avoid use-after-free when creating merges #1919
Conversation
This file contains hidden or 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 the next commit these functions will be called from pick_one_commit() so move them above that function to avoid a forward declaration. Signed-off-by: Phillip Wood <[email protected]>
It has been reported that "git rebase --rebase-merges" can create
corrupted reflog entries like
e9c962f2ea0 HEAD@{8}: <binary>�: Merged in <branch> (pull request #4441)
This is due to a use-after-free bug that happens because
reflog_message() uses a static `struct strbuf` and is not called to
update the current reflog message stored in `ctx->reflog_message` when
creating the merge. This means `ctx->reflog_message` points to a stale
reflog message that has been freed by subsequent call to
reflog_message() by a command such as `reset` that used the return value
directly rather than storing the result in `ctx->reflog_message`.
Fix this by creating the reflog message nearer to where the commit is
created and storing it in a local variable which is passed as an
additional parameter to run_git_commit() rather than storing the message
in `struct replay_ctx`. This makes it harder to forget to call
`reflog_message()` before creating a commit and using a variable with a
narrower scope means that a stale value cannot carried across a from one
iteration of the loop to the next which should prevent any similar
use-after-free bugs in the future.
A existing test is modified to demonstrate that merges are now created
with the correct reflog message.
Reported-by: Kristoffer Haugsbakk <[email protected]>
Signed-off-by: Phillip Wood <[email protected]>
|
/submit |
|
Submitted as [email protected] 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}
| @@ -224,11 +224,6 @@ struct replay_ctx { | |||
| * current chain. | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"Phillip Wood via GitGitGadget" <[email protected]> writes:
> Fix this by creating the reflog message nearer to where the commit is
> created and storing it in a local variable which is passed as an
> additional parameter to run_git_commit() rather than storing the message
> in `struct replay_ctx`. This makes it harder to forget to call
> `reflog_message()` before creating a commit and using a variable with a
> narrower scope means that a stale value cannot carried across a from one
> iteration of the loop to the next which should prevent any similar
> use-after-free bugs in the future.
Nice.
> @@ -1124,10 +1119,10 @@ static int run_command_silent_on_success(struct child_process *cmd)
> * author metadata.
> */
> static int run_git_commit(const char *defmsg,
> + const char *reflog_action,
> struct replay_opts *opts,
> unsigned int flags)
> ...
> static int try_to_commit(struct repository *r,
> struct strbuf *msg, const char *author,
> + const char *reflog_action,
> struct replay_opts *opts, unsigned int flags,
> struct object_id *oid)
> ...
> static int do_commit(struct repository *r,
> const char *msg_file, const char *author,
> + const char *reflog_action,
> struct replay_opts *opts, unsigned int flags,
> struct object_id *oid)
OK. We no longer have the reflog_action as a part of replay_opts,
but they are almost always passed together, so making them sit
together in the list of parameters does make sense.
Will queue. Thanks.
|
This patch series was integrated into seen via git@b29799e. |
|
This branch is now known as |
|
This patch series was integrated into seen via git@88f0122. |
|
This patch series was integrated into next via git@e7b8721. |
|
There was a status update in the "New Topics" section about the branch source: <[email protected]> |
|
This patch series was integrated into seen via git@56cab85. |
|
This patch series was integrated into seen via git@bb90209. |
|
This patch series was integrated into seen via git@4396e61. |
|
This patch series was integrated into seen via git@4b090d5. |
|
There was a status update in the "Cooking" section about the branch Use-after-free fix in the sequencer. Will merge to 'master'. source: <[email protected]> |
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.
This series fixes a use-after-free bug reported in [1]. The bug is caused by storing a pointer to the buffer in a static strbuf that gets reallocated. There was some discussion about removing the buffer and making the caller responsible for freeing the returned string. In the end I decided to keep the buffer as it avoids having to remember to free the return value and instead stop storing the pointer to it in a long-lived variable.
Many thanks to Kristoffer for the detailed bug report and backtrace.
[1] https://lore.kernel.org/git/[email protected]
Cc: Kristoffer Haugsbakk [email protected]
Cc: Jeff King [email protected]