Sequencer: avoid use-after-free when creating merges
cover Sequencer: avoid use-after-free when creating merges
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]
Phillip Wood (2):
sequencer: move reflog message functions
sequencer: rework reflog message handling
sequencer.c | 116 +++++++++++++++++++-------------------- t/t3430-rebase-merges.sh | 11 +++- 2 files changed, 67 insertions(+), 60 deletions(-)
base-commit: 4bbb303af69990ccd05fe3a2eb58a1ce036f8220
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1919%2Fphillipwood%2Fsequencer-explicit-reflog-message-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1919/phillipwood/sequencer-explicit-reflog-message-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1919
Signature · 1 line
gitgitgadget
PATCH 1/2 sequencer: move reflog message functions
From: Phillip Wood <[email protected]>
In the next commit these functions will be called from pick_one_commit() so move them above that function to avoid a forward declaration.
--- sequencer.c | 66 ++++++++++++++++++++++++++--------------------------- 1 file changed, 33 insertions(+), 33 deletions(-)
sequencer.c +33 -33
@@ -2226,6 +2226,39 @@ static void refer_to_commit(struct replay_opts *opts, } } +static const char *sequencer_reflog_action (struct replay_opts *opts) +{ +if (!opts->reflog_action) { + opts->reflog_action =getenv (GIT_REFLOG_ACTION ); + opts->reflog_action = +xstrdup (opts->reflog_action ? opts->reflog_action + :action_name (opts)); + } + +return opts->reflog_action; +} + +__attribute__((format (printf,3 ,4 ))) +static const char *reflog_message (struct replay_opts *opts, +const char *sub_action,const char *fmt, ...) +{ +va_list ap; +static struct strbuf buf =STRBUF_INIT ; + +va_start (ap, fmt); +strbuf_reset (&buf); +strbuf_addstr (&buf,sequencer_reflog_action (opts)); +if (sub_action) +strbuf_addf (&buf," (%s)" , sub_action); +if (fmt) { +strbuf_addstr (&buf,": " ); +strbuf_vaddf (&buf, fmt, ap); + } +va_end (ap); + +return buf.buf; +} + static int do_pick_commit(struct repository *r, struct todo_item *item, struct replay_opts *opts,
@@ -3919,39 +3952,6 @@ static int do_label(struct repository *r, const char *name, int len)return ret; } -static const char *sequencer_reflog_action (struct replay_opts *opts) -{ -if (!opts->reflog_action) { - opts->reflog_action =getenv (GIT_REFLOG_ACTION ); - opts->reflog_action = -xstrdup (opts->reflog_action ? opts->reflog_action - :action_name (opts)); - } - -return opts->reflog_action; -} - -__attribute__((format (printf,3 ,4 ))) -static const char *reflog_message (struct replay_opts *opts, -const char *sub_action,const char *fmt, ...) -{ -va_list ap; -static struct strbuf buf =STRBUF_INIT ; - -va_start (ap, fmt); -strbuf_reset (&buf); -strbuf_addstr (&buf,sequencer_reflog_action (opts)); -if (sub_action) -strbuf_addf (&buf," (%s)" , sub_action); -if (fmt) { -strbuf_addstr (&buf,": " ); -strbuf_vaddf (&buf, fmt, ap); - } -va_end (ap); - -return buf.buf; -} -static struct commit *lookup_label (struct repository *r,const char *label,int len,struct strbuf *buf) {
Signature · 1 line
gitgitgadget
PATCH 2/2 sequencer: rework reflog message handling
In reply to Phillip Wood via GitGitGadgetFrom: 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.
--- sequencer.c | 50 +++++++++++++++++++--------------------- t/t3430-rebase-merges.sh | 11 ++++++++- 2 files changed, 34 insertions(+), 27 deletions(-)
sequencer.c +24 -26
@@ -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.
@@ -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) { -struct replay_ctx *ctx = opts->ctx;struct child_process cmd =CHILD_PROCESS_INIT ;if ((flags &CLEANUP_MSG ) && (flags &VERBATIM_MSG ))
@@ -1145,7 +1140,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",
@@ -1529,10 +1524,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 ;
@@ -1694,7 +1689,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;
@@ -1725,6 +1720,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) {
@@ -1740,7 +1736,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), "",
@@ -1756,7 +1752,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;
@@ -2269,13 +2265,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
@@ -2527,7 +2529,8 @@ 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" ));
@@ -2542,7 +2545,7 @@ fast_forward_edit:* got here. */ flags =EDIT_MSG |VERIFY_MSG |AMEND_MSG |ALLOW_EMPTY ; - res =run_git_commit (NULL , opts, flags); + res =run_git_commit (NULL , reflog_action, opts, flags); *check_todo =1 ; } }
@@ -4089,6 +4092,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 ;
@@ -4360,14 +4364,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); }
@@ -4882,13 +4887,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);
@@ -4947,7 +4948,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) ||
@@ -5208,6 +5208,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 )) {
@@ -5370,7 +5371,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; }
@@ -5402,7 +5403,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;
@@ -5423,7 +5423,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;
@@ -5475,7 +5474,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); }
t/t3430-rebase-merges.sh +10 -1
@@ -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' '
Signature · 1 line
gitgitgadget
Re: [PATCH 2/2] sequencer: rework reflog message handling
Quoted 8 lines · Phillip Wood via GitGitGadget
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 instruct replay_ctx. This makes it harder to forget to callreflog_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.
Quoted 9 lines
(quoted diff) +1
-0@@ -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,
Review static int run_command_silent_on_success(struct child_process *cmd)
Quoted 10 lines
(quoted diff) +2
-0struct 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.