{"thread":{"id":"63428","subject":"[PATCH 0/2] Sequencer: avoid use-after-free when creating merges","startedAt":"2025-05-09T16:22:30Z","lastAt":"2025-05-09T20:37:47Z","messageCount":4,"participants":["Phillip Wood via GitGitGadget","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"517692","messageId":"pull.1919.git.1746807747.gitgitgadget@gmail.com","threadId":"63428","inReplyTo":null,"subject":"[PATCH 0/2] Sequencer: avoid use-after-free when creating merges","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-05-09T16:22:25Z","receivedAt":"2025-05-09T16:22:30Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"This series fixes a use-after-free bug reported in [1]. The bug is caused by\nstoring a pointer to the buffer in a static strbuf that gets reallocated.\nThere was some discussion about removing the buffer and making the caller\nresponsible for freeing the returned string. In the end I decided to keep\nthe buffer as it avoids having to remember to free the return value and\ninstead stop storing the pointer to it in a long-lived variable.\n\nMany thanks to Kristoffer for the detailed bug report and backtrace.\n\n[1] https://lore.kernel.org/git/20250428194048.149348-1-code@khaugsbakk.name\n\nPhillip Wood (2):\n  sequencer: move reflog message functions\n  sequencer: rework reflog message handling\n\n sequencer.c              | 116 +++++++++++++++++++--------------------\n t/t3430-rebase-merges.sh |  11 +++-\n 2 files changed, 67 insertions(+), 60 deletions(-)\n\n\nbase-commit: 4bbb303af69990ccd05fe3a2eb58a1ce036f8220\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1919%2Fphillipwood%2Fsequencer-explicit-reflog-message-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1919/phillipwood/sequencer-explicit-reflog-message-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1919\n-- \ngitgitgadget\n"},{"id":"517693","messageId":"fa5442cb805793adfe1f0b26ec79a75388317d2b.1746807747.git.gitgitgadget@gmail.com","threadId":"63428","inReplyTo":"pull.1919.git.1746807747.gitgitgadget@gmail.com","subject":"[PATCH 1/2] sequencer: move reflog message functions","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-05-09T16:22:26Z","receivedAt":"2025-05-09T16:22:31Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nIn the next commit these functions will be called from pick_one_commit()\nso move them above that function to avoid a forward declaration.\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n sequencer.c | 66 ++++++++++++++++++++++++++---------------------------\n 1 file changed, 33 insertions(+), 33 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 9ea678364d4f..bb4667190db1 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2226,6 +2226,39 @@ static void refer_to_commit(struct replay_opts *opts,\n \t}\n }\n \n+static const char *sequencer_reflog_action(struct replay_opts *opts)\n+{\n+\tif (!opts->reflog_action) {\n+\t\topts->reflog_action = getenv(GIT_REFLOG_ACTION);\n+\t\topts->reflog_action =\n+\t\t\txstrdup(opts->reflog_action ? opts->reflog_action\n+\t\t\t\t\t\t    : action_name(opts));\n+\t}\n+\n+\treturn opts->reflog_action;\n+}\n+\n+__attribute__((format (printf, 3, 4)))\n+static const char *reflog_message(struct replay_opts *opts,\n+\tconst char *sub_action, const char *fmt, ...)\n+{\n+\tva_list ap;\n+\tstatic struct strbuf buf = STRBUF_INIT;\n+\n+\tva_start(ap, fmt);\n+\tstrbuf_reset(&buf);\n+\tstrbuf_addstr(&buf, sequencer_reflog_action(opts));\n+\tif (sub_action)\n+\t\tstrbuf_addf(&buf, \" (%s)\", sub_action);\n+\tif (fmt) {\n+\t\tstrbuf_addstr(&buf, \": \");\n+\t\tstrbuf_vaddf(&buf, fmt, ap);\n+\t}\n+\tva_end(ap);\n+\n+\treturn buf.buf;\n+}\n+\n static int do_pick_commit(struct repository *r,\n \t\t\t  struct todo_item *item,\n \t\t\t  struct replay_opts *opts,\n@@ -3919,39 +3952,6 @@ static int do_label(struct repository *r, const char *name, int len)\n \treturn ret;\n }\n \n-static const char *sequencer_reflog_action(struct replay_opts *opts)\n-{\n-\tif (!opts->reflog_action) {\n-\t\topts->reflog_action = getenv(GIT_REFLOG_ACTION);\n-\t\topts->reflog_action =\n-\t\t\txstrdup(opts->reflog_action ? opts->reflog_action\n-\t\t\t\t\t\t    : action_name(opts));\n-\t}\n-\n-\treturn opts->reflog_action;\n-}\n-\n-__attribute__((format (printf, 3, 4)))\n-static const char *reflog_message(struct replay_opts *opts,\n-\tconst char *sub_action, const char *fmt, ...)\n-{\n-\tva_list ap;\n-\tstatic struct strbuf buf = STRBUF_INIT;\n-\n-\tva_start(ap, fmt);\n-\tstrbuf_reset(&buf);\n-\tstrbuf_addstr(&buf, sequencer_reflog_action(opts));\n-\tif (sub_action)\n-\t\tstrbuf_addf(&buf, \" (%s)\", sub_action);\n-\tif (fmt) {\n-\t\tstrbuf_addstr(&buf, \": \");\n-\t\tstrbuf_vaddf(&buf, fmt, ap);\n-\t}\n-\tva_end(ap);\n-\n-\treturn buf.buf;\n-}\n-\n static struct commit *lookup_label(struct repository *r, const char *label,\n \t\t\t\t   int len, struct strbuf *buf)\n {\n-- \ngitgitgadget\n\n"},{"id":"517694","messageId":"90c6912478683d96ef18b521506c703a2467585e.1746807747.git.gitgitgadget@gmail.com","threadId":"63428","inReplyTo":"pull.1919.git.1746807747.gitgitgadget@gmail.com","subject":"[PATCH 2/2] sequencer: rework reflog message handling","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-05-09T16:22:27Z","receivedAt":"2025-05-09T16:22:32Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nIt has been reported that \"git rebase --rebase-merges\" can create\ncorrupted reflog entries like\n\n    e9c962f2ea0 HEAD@{8}: <binary>�: Merged in <branch> (pull request #4441)\n\nThis is due to a use-after-free bug that happens because\nreflog_message() uses a static `struct strbuf` and is not called to\nupdate the current reflog message stored in `ctx->reflog_message` when\ncreating the merge. This means `ctx->reflog_message` points to a stale\nreflog message that has been freed by subsequent call to\nreflog_message() by a command such as `reset` that used the return value\ndirectly rather than storing the result in `ctx->reflog_message`.\n\nFix this by creating the reflog message nearer to where the commit is\ncreated and storing it in a local variable which is passed as an\nadditional parameter to run_git_commit() rather than storing the message\nin `struct replay_ctx`. This makes it harder to forget to call\n`reflog_message()` before creating a commit and using a variable with a\nnarrower scope means that a stale value cannot carried across a from one\niteration of the loop to the next which should prevent any similar\nuse-after-free bugs in the future.\n\nA existing test is modified to demonstrate that merges are now created\nwith the correct reflog message.\n\nReported-by: Kristoffer Haugsbakk <code@khaugsbakk.name>\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n sequencer.c              | 50 +++++++++++++++++++---------------------\n t/t3430-rebase-merges.sh | 11 ++++++++-\n 2 files changed, 34 insertions(+), 27 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex bb4667190db1..e23708690900 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -224,11 +224,6 @@ struct replay_ctx {\n \t * current chain.\n \t */\n \tstruct strbuf current_fixups;\n-\t/*\n-\t * Stores the reflog message that will be used when creating a\n-\t * commit. Points to a static buffer and should not be free()'d.\n-\t */\n-\tconst char *reflog_message;\n \t/*\n \t * The number of completed fixup and squash commands in the\n \t * current chain.\n@@ -1124,10 +1119,10 @@ static int run_command_silent_on_success(struct child_process *cmd)\n  * author metadata.\n  */\n static int run_git_commit(const char *defmsg,\n+\t\t\t  const char *reflog_action,\n \t\t\t  struct replay_opts *opts,\n \t\t\t  unsigned int flags)\n {\n-\tstruct replay_ctx *ctx = opts->ctx;\n \tstruct child_process cmd = CHILD_PROCESS_INIT;\n \n \tif ((flags & CLEANUP_MSG) && (flags & VERBATIM_MSG))\n@@ -1145,7 +1140,7 @@ static int run_git_commit(const char *defmsg,\n \t\t\t     gpg_opt, gpg_opt);\n \t}\n \n-\tstrvec_pushf(&cmd.env, GIT_REFLOG_ACTION \"=%s\", ctx->reflog_message);\n+\tstrvec_pushf(&cmd.env, GIT_REFLOG_ACTION \"=%s\", reflog_action);\n \n \tif (opts->committer_date_is_author_date)\n \t\tstrvec_pushf(&cmd.env, \"GIT_COMMITTER_DATE=%s\",\n@@ -1529,10 +1524,10 @@ static int parse_head(struct repository *r, struct commit **head)\n  */\n static int try_to_commit(struct repository *r,\n \t\t\t struct strbuf *msg, const char *author,\n+\t\t\t const char *reflog_action,\n \t\t\t struct replay_opts *opts, unsigned int flags,\n \t\t\t struct object_id *oid)\n {\n-\tstruct replay_ctx *ctx = opts->ctx;\n \tstruct object_id tree;\n \tstruct commit *current_head = NULL;\n \tstruct commit_list *parents = NULL;\n@@ -1694,7 +1689,7 @@ static int try_to_commit(struct repository *r,\n \t\tgoto out;\n \t}\n \n-\tif (update_head_with_reflog(current_head, oid, ctx->reflog_message,\n+\tif (update_head_with_reflog(current_head, oid, reflog_action,\n \t\t\t\t    msg, &err)) {\n \t\tres = error(\"%s\", err.buf);\n \t\tgoto out;\n@@ -1725,6 +1720,7 @@ static int write_rebase_head(struct object_id *oid)\n \n static int do_commit(struct repository *r,\n \t\t     const char *msg_file, const char *author,\n+\t\t     const char *reflog_action,\n \t\t     struct replay_opts *opts, unsigned int flags,\n \t\t     struct object_id *oid)\n {\n@@ -1740,7 +1736,7 @@ static int do_commit(struct repository *r,\n \t\t\t\t\t   msg_file);\n \n \t\tres = try_to_commit(r, msg_file ? &sb : NULL,\n-\t\t\t\t    author, opts, flags, &oid);\n+\t\t\t\t    author, reflog_action, opts, flags, &oid);\n \t\tstrbuf_release(&sb);\n \t\tif (!res) {\n \t\t\trefs_delete_ref(get_main_ref_store(r), \"\",\n@@ -1756,7 +1752,7 @@ static int do_commit(struct repository *r,\n \t\tif (is_rebase_i(opts) && oid)\n \t\t\tif (write_rebase_head(oid))\n \t\t\t    return -1;\n-\t\treturn run_git_commit(msg_file, opts, flags);\n+\t\treturn run_git_commit(msg_file, reflog_action, opts, flags);\n \t}\n \n \treturn res;\n@@ -2269,13 +2265,19 @@ static int do_pick_commit(struct repository *r,\n \tconst char *msg_file = should_edit(opts) ? NULL : git_path_merge_msg(r);\n \tstruct object_id head;\n \tstruct commit *base, *next, *parent;\n-\tconst char *base_label, *next_label;\n+\tconst char *base_label, *next_label, *reflog_action;\n \tchar *author = NULL;\n \tstruct commit_message msg = { NULL, NULL, NULL, NULL };\n \tint res, unborn = 0, reword = 0, allow, drop_commit;\n \tenum todo_command command = item->command;\n \tstruct commit *commit = item->commit;\n \n+\tif (is_rebase_i(opts))\n+\t\treflog_action = reflog_message(\n+\t\t\topts, command_to_string(item->command), NULL);\n+\telse\n+\t\treflog_action = sequencer_reflog_action(opts);\n+\n \tif (opts->no_commit) {\n \t\t/*\n \t\t * We do not intend to commit immediately.  We just want to\n@@ -2527,7 +2529,8 @@ static int do_pick_commit(struct repository *r,\n \t} /* else allow == 0 and there's nothing special to do */\n \tif (!opts->no_commit && !drop_commit) {\n \t\tif (author || command == TODO_REVERT || (flags & AMEND_MSG))\n-\t\t\tres = do_commit(r, msg_file, author, opts, flags,\n+\t\t\tres = do_commit(r, msg_file, author, reflog_action,\n+\t\t\t\t\topts, flags,\n \t\t\t\t\tcommit? &commit->object.oid : NULL);\n \t\telse\n \t\t\tres = error(_(\"unable to parse commit author\"));\n@@ -2542,7 +2545,7 @@ fast_forward_edit:\n \t\t\t * got here.\n \t\t\t */\n \t\t\tflags = EDIT_MSG | VERIFY_MSG | AMEND_MSG | ALLOW_EMPTY;\n-\t\t\tres = run_git_commit(NULL, opts, flags);\n+\t\t\tres = run_git_commit(NULL, reflog_action, opts, flags);\n \t\t\t*check_todo = 1;\n \t\t}\n \t}\n@@ -4089,6 +4092,7 @@ static int do_merge(struct repository *r,\n \tint merge_arg_len, oneline_offset, can_fast_forward, ret, k;\n \tstatic struct lock_file lock;\n \tconst char *p;\n+\tconst char *reflog_action = reflog_message(opts, \"merge\", NULL);\n \n \tif (repo_hold_locked_index(r, &lock, LOCK_REPORT_ON_ERROR) < 0) {\n \t\tret = -1;\n@@ -4360,14 +4364,15 @@ static int do_merge(struct repository *r,\n \t\t * value (a negative one would indicate that the `merge`\n \t\t * command needs to be rescheduled).\n \t\t */\n-\t\tret = !!run_git_commit(git_path_merge_msg(r), opts,\n-\t\t\t\t       run_commit_flags);\n+\t\tret = !!run_git_commit(git_path_merge_msg(r), reflog_action,\n+\t\t\t\t       opts, run_commit_flags);\n \n \tif (!ret && flags & TODO_EDIT_MERGE_MSG) {\n \tfast_forward_edit:\n \t\t*check_todo = 1;\n \t\trun_commit_flags |= AMEND_MSG | EDIT_MSG | VERIFY_MSG;\n-\t\tret = !!run_git_commit(NULL, opts, run_commit_flags);\n+\t\tret = !!run_git_commit(NULL, reflog_action, opts,\n+\t\t\t\t       run_commit_flags);\n \t}\n \n \n@@ -4882,13 +4887,9 @@ static int pick_one_commit(struct repository *r,\n \t\t\t   struct replay_opts *opts,\n \t\t\t   int *check_todo, int* reschedule)\n {\n-\tstruct replay_ctx *ctx = opts->ctx;\n \tint res;\n \tstruct todo_item *item = todo_list->items + todo_list->current;\n \tconst char *arg = todo_item_get_arg(todo_list, item);\n-\tif (is_rebase_i(opts))\n-\t\tctx->reflog_message = reflog_message(\n-\t\t\topts, command_to_string(item->command), NULL);\n \n \tres = do_pick_commit(r, item, opts, is_final_fixup(todo_list),\n \t\t\t     check_todo);\n@@ -4947,7 +4948,6 @@ static int pick_commits(struct repository *r,\n \tstruct replay_ctx *ctx = opts->ctx;\n \tint res = 0, reschedule = 0;\n \n-\tctx->reflog_message = sequencer_reflog_action(opts);\n \tif (opts->allow_ff)\n \t\tASSERT(!(opts->signoff || opts->no_commit ||\n \t\t\t opts->record_origin || should_edit(opts) ||\n@@ -5208,6 +5208,7 @@ static int commit_staged_changes(struct repository *r,\n \tunsigned int flags = ALLOW_EMPTY | EDIT_MSG;\n \tunsigned int final_fixup = 0, is_clean;\n \tstruct strbuf rev = STRBUF_INIT;\n+\tconst char *reflog_action = reflog_message(opts, \"continue\", NULL);\n \tint ret;\n \n \tif (has_unstaged_changes(r, 1)) {\n@@ -5370,7 +5371,7 @@ static int commit_staged_changes(struct repository *r,\n \t}\n \n \tif (run_git_commit(final_fixup ? NULL : rebase_path_message(),\n-\t\t\t   opts, flags)) {\n+\t\t\t   reflog_action, opts, flags)) {\n \t\tret = error(_(\"could not commit staged changes.\"));\n \t\tgoto out;\n \t}\n@@ -5402,7 +5403,6 @@ out:\n \n int sequencer_continue(struct repository *r, struct replay_opts *opts)\n {\n-\tstruct replay_ctx *ctx = opts->ctx;\n \tstruct todo_list todo_list = TODO_LIST_INIT;\n \tint res;\n \n@@ -5423,7 +5423,6 @@ int sequencer_continue(struct repository *r, struct replay_opts *opts)\n \t\t\tunlink(rebase_path_dropped());\n \t\t}\n \n-\t\tctx->reflog_message = reflog_message(opts, \"continue\", NULL);\n \t\tif (commit_staged_changes(r, opts, &todo_list)) {\n \t\t\tres = -1;\n \t\t\tgoto release_todo_list;\n@@ -5475,7 +5474,6 @@ static int single_pick(struct repository *r,\n \t\t\tTODO_PICK : TODO_REVERT;\n \titem.commit = cmit;\n \n-\topts->ctx->reflog_message = sequencer_reflog_action(opts);\n \treturn do_pick_commit(r, &item, opts, 0, &check_todo);\n }\n \ndiff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh\nindex b84d68c4b96b..ff81adab8cf6 100755\n--- a/t/t3430-rebase-merges.sh\n+++ b/t/t3430-rebase-merges.sh\n@@ -86,7 +86,7 @@ test_expect_success 'create completely different structure' '\n \ttest_config sequence.editor \\\"\"$PWD\"/replace-editor.sh\\\" &&\n \ttest_tick &&\n \tgit rebase -i -r A main &&\n-\ttest_cmp_graph <<-\\EOF\n+\ttest_cmp_graph <<-\\EOF &&\n \t*   Merge the topic branch '\\''onebranch'\\''\n \t|\\\n \t| * D\n@@ -99,6 +99,15 @@ test_expect_success 'create completely different structure' '\n \t|/\n \t* A\n \tEOF\n+\n+\thead=\"$(git show-ref --verify -s --abbrev HEAD)\" &&\n+\tcat >expect <<-EOF &&\n+\t$head HEAD@{0}: rebase (finish): returning to refs/heads/main\n+\t$head HEAD@{1}: rebase (merge): Merge the topic branch ${SQ}onebranch${SQ}\n+\tEOF\n+\n+\tgit reflog -n2 HEAD >actual &&\n+\ttest_cmp expect actual\n '\n \n test_expect_success 'generate correct todo list' '\n-- \ngitgitgadget\n"},{"id":"517713","messageId":"xmqqfrhdzfjc.fsf@gitster.g","threadId":"63428","inReplyTo":"90c6912478683d96ef18b521506c703a2467585e.1746807747.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/2] sequencer: rework reflog message handling","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-09T20:37:43Z","receivedAt":"2025-05-09T20:37:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Phillip Wood via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> Fix this by creating the reflog message nearer to where the commit is\n> created and storing it in a local variable which is passed as an\n> additional parameter to run_git_commit() rather than storing the message\n> in `struct replay_ctx`. This makes it harder to forget to call\n> `reflog_message()` before creating a commit and using a variable with a\n> narrower scope means that a stale value cannot carried across a from one\n> iteration of the loop to the next which should prevent any similar\n> use-after-free bugs in the future.\n\nNice.\n\n> @@ -1124,10 +1119,10 @@ static int run_command_silent_on_success(struct child_process *cmd)\n>   * author metadata.\n>   */\n>  static int run_git_commit(const char *defmsg,\n> +\t\t\t  const char *reflog_action,\n>  \t\t\t  struct replay_opts *opts,\n>  \t\t\t  unsigned int flags)\n> ...\n>  static int try_to_commit(struct repository *r,\n>  \t\t\t struct strbuf *msg, const char *author,\n> +\t\t\t const char *reflog_action,\n>  \t\t\t struct replay_opts *opts, unsigned int flags,\n>  \t\t\t struct object_id *oid)\n> ...\n>  static int do_commit(struct repository *r,\n>  \t\t     const char *msg_file, const char *author,\n> +\t\t     const char *reflog_action,\n>  \t\t     struct replay_opts *opts, unsigned int flags,\n>  \t\t     struct object_id *oid)\n\nOK.  We no longer have the reflog_action as a part of replay_opts,\nbut they are almost always passed together, so making them sit\ntogether in the list of parameters does make sense.\n\nWill queue.  Thanks.\n"}]}