sequencer: rework reflog message handling

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 <code@khaugsbakk.name>
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/sequencer.c b/sequencer.c
index 78db77c36ce91bb5d7817d5406932feb2077dca6..c4b3cb245ce4af17dab9a96abb75d4e3962424ba 100644 (file)
--- a/sequencer.c
+++ b/sequencer.c
@@ -224,11 +224,6 @@ struct replay_ctx {
         * current chain.
         */
        struct strbuf current_fixups;
-       /*
-        * Stores the reflog message that will be used when creating a
-        * commit. Points to a static buffer and should not be free()'d.
-        */
-       const char *reflog_message;
        /*
         * The number of completed fixup and squash commands in the
         * current chain.
@@ -1133,10 +1128,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)
 {
-       struct replay_ctx *ctx = opts->ctx;
        struct child_process cmd = CHILD_PROCESS_INIT;
 
        if ((flags & CLEANUP_MSG) && (flags & VERBATIM_MSG))
@@ -1154,7 +1149,7 @@ static int run_git_commit(const char *defmsg,
                             gpg_opt, gpg_opt);
        }
 
-       strvec_pushf(&cmd.env, GIT_REFLOG_ACTION "=%s", ctx->reflog_message);
+       strvec_pushf(&cmd.env, GIT_REFLOG_ACTION "=%s", reflog_action);
 
        if (opts->committer_date_is_author_date)
                strvec_pushf(&cmd.env, "GIT_COMMITTER_DATE=%s",
@@ -1538,10 +1533,10 @@ static int parse_head(struct repository *r, struct commit **head)
  */
 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)
 {
-       struct replay_ctx *ctx = opts->ctx;
        struct object_id tree;
        struct commit *current_head = NULL;
        struct commit_list *parents = NULL;
@@ -1703,7 +1698,7 @@ static int try_to_commit(struct repository *r,
                goto out;
        }
 
-       if (update_head_with_reflog(current_head, oid, ctx->reflog_message,
+       if (update_head_with_reflog(current_head, oid, reflog_action,
                                    msg, &err)) {
                res = error("%s", err.buf);
                goto out;
@@ -1734,6 +1729,7 @@ static int write_rebase_head(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)
 {
@@ -1749,7 +1745,7 @@ static int do_commit(struct repository *r,
                                           msg_file);
 
                res = try_to_commit(r, msg_file ? &sb : NULL,
-                                   author, opts, flags, &oid);
+                                   author, reflog_action, opts, flags, &oid);
                strbuf_release(&sb);
                if (!res) {
                        refs_delete_ref(get_main_ref_store(r), "",
@@ -1765,7 +1761,7 @@ static int do_commit(struct repository *r,
                if (is_rebase_i(opts) && oid)
                        if (write_rebase_head(oid))
                            return -1;
-               return run_git_commit(msg_file, opts, flags);
+               return run_git_commit(msg_file, reflog_action, opts, flags);
        }
 
        return res;
@@ -2278,13 +2274,19 @@ static int do_pick_commit(struct repository *r,
        const char *msg_file = should_edit(opts) ? NULL : git_path_merge_msg(r);
        struct object_id head;
        struct commit *base, *next, *parent;
-       const char *base_label, *next_label;
+       const char *base_label, *next_label, *reflog_action;
        char *author = NULL;
        struct commit_message msg = { NULL, NULL, NULL, NULL };
        int res, unborn = 0, reword = 0, allow, drop_commit;
        enum todo_command command = item->command;
        struct commit *commit = item->commit;
 
+       if (is_rebase_i(opts))
+               reflog_action = reflog_message(
+                       opts, command_to_string(item->command), NULL);
+       else
+               reflog_action = sequencer_reflog_action(opts);
+
        if (opts->no_commit) {
                /*
                 * We do not intend to commit immediately.  We just want to
@@ -2536,14 +2538,15 @@ static int do_pick_commit(struct repository *r,
        } /* else allow == 0 and there's nothing special to do */
        if (!opts->no_commit && !drop_commit) {
                if (author || command == TODO_REVERT || (flags & AMEND_MSG))
-                       res = do_commit(r, msg_file, author, opts, flags,
+                       res = do_commit(r, msg_file, author, reflog_action,
+                                       opts, flags,
                                        commit? &commit->object.oid : NULL);
                else
                        res = error(_("unable to parse commit author"));
                *check_todo = !!(flags & EDIT_MSG);
                if (!res && reword) {
 fast_forward_edit:
-                       res = run_git_commit(NULL, opts, EDIT_MSG |
+                       res = run_git_commit(NULL, reflog_action, opts, EDIT_MSG |
                                             VERIFY_MSG | AMEND_MSG |
                                             (flags & ALLOW_EMPTY));
                        *check_todo = 1;
@@ -4092,6 +4095,7 @@ static int do_merge(struct repository *r,
        int merge_arg_len, oneline_offset, can_fast_forward, ret, k;
        static struct lock_file lock;
        const char *p;
+       const char *reflog_action = reflog_message(opts, "merge", NULL);
 
        if (repo_hold_locked_index(r, &lock, LOCK_REPORT_ON_ERROR) < 0) {
                ret = -1;
@@ -4370,14 +4374,15 @@ static int do_merge(struct repository *r,
                 * value (a negative one would indicate that the `merge`
                 * command needs to be rescheduled).
                 */
-               ret = !!run_git_commit(git_path_merge_msg(r), opts,
-                                      run_commit_flags);
+               ret = !!run_git_commit(git_path_merge_msg(r), reflog_action,
+                                      opts, run_commit_flags);
 
        if (!ret && flags & TODO_EDIT_MERGE_MSG) {
        fast_forward_edit:
                *check_todo = 1;
                run_commit_flags |= AMEND_MSG | EDIT_MSG | VERIFY_MSG;
-               ret = !!run_git_commit(NULL, opts, run_commit_flags);
+               ret = !!run_git_commit(NULL, reflog_action, opts,
+                                      run_commit_flags);
        }
 
 
@@ -4892,13 +4897,9 @@ static int pick_one_commit(struct repository *r,
                           struct replay_opts *opts,
                           int *check_todo, int* reschedule)
 {
-       struct replay_ctx *ctx = opts->ctx;
        int res;
        struct todo_item *item = todo_list->items + todo_list->current;
        const char *arg = todo_item_get_arg(todo_list, item);
-       if (is_rebase_i(opts))
-               ctx->reflog_message = reflog_message(
-                       opts, command_to_string(item->command), NULL);
 
        res = do_pick_commit(r, item, opts, is_final_fixup(todo_list),
                             check_todo);
@@ -4957,7 +4958,6 @@ static int pick_commits(struct repository *r,
        struct replay_ctx *ctx = opts->ctx;
        int res = 0, reschedule = 0;
 
-       ctx->reflog_message = sequencer_reflog_action(opts);
        if (opts->allow_ff)
                assert(!(opts->signoff || opts->no_commit ||
                         opts->record_origin || should_edit(opts) ||
@@ -5218,6 +5218,7 @@ static int commit_staged_changes(struct repository *r,
        unsigned int flags = ALLOW_EMPTY | EDIT_MSG;
        unsigned int final_fixup = 0, is_clean;
        struct strbuf rev = STRBUF_INIT;
+       const char *reflog_action = reflog_message(opts, "continue", NULL);
        int ret;
 
        if (has_unstaged_changes(r, 1)) {
@@ -5380,7 +5381,7 @@ static int commit_staged_changes(struct repository *r,
        }
 
        if (run_git_commit(final_fixup ? NULL : rebase_path_message(),
-                          opts, flags)) {
+                          reflog_action, opts, flags)) {
                ret = error(_("could not commit staged changes."));
                goto out;
        }
@@ -5412,7 +5413,6 @@ out:
 
 int sequencer_continue(struct repository *r, struct replay_opts *opts)
 {
-       struct replay_ctx *ctx = opts->ctx;
        struct todo_list todo_list = TODO_LIST_INIT;
        int res;
 
@@ -5433,7 +5433,6 @@ int sequencer_continue(struct repository *r, struct replay_opts *opts)
                        unlink(rebase_path_dropped());
                }
 
-               ctx->reflog_message = reflog_message(opts, "continue", NULL);
                if (commit_staged_changes(r, opts, &todo_list)) {
                        res = -1;
                        goto release_todo_list;
@@ -5485,7 +5484,6 @@ static int single_pick(struct repository *r,
                        TODO_PICK : TODO_REVERT;
        item.commit = cmit;
 
-       opts->ctx->reflog_message = sequencer_reflog_action(opts);
        return do_pick_commit(r, &item, opts, 0, &check_todo);
 }
 

diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
index 2593711fecdc9f18c1b83b8d921ae06fbd2495ac..b7da7c364e200c3c14cb398ce187ed8fedeb6938 100755 (executable)
--- a/t/t3430-rebase-merges.sh
+++ b/t/t3430-rebase-merges.sh
@@ -86,7 +86,7 @@ test_expect_success 'create completely different structure' '
        test_config sequence.editor \""$PWD"/replace-editor.sh\" &&
        test_tick &&
        git rebase -i -r A main &&
-       test_cmp_graph <<-\EOF
+       test_cmp_graph <<-\EOF &&
        *   Merge the topic branch '\''onebranch'\''
        |\
        | * D
@@ -99,6 +99,15 @@ test_expect_success 'create completely different structure' '
        |/
        * A
        EOF
+
+       head="$(git show-ref --verify -s --abbrev HEAD)" &&
+       cat >expect <<-EOF &&
+       $head HEAD@{0}: rebase (finish): returning to refs/heads/main
+       $head HEAD@{1}: rebase (merge): Merge the topic branch ${SQ}onebranch${SQ}
+       EOF
+
+       git reflog -n2 HEAD >actual &&
+       test_cmp expect actual
 '
 
 test_expect_success 'generate correct todo list' '