[PATCH 0/2] Sequencer: avoid use-after-free when creating merges

[PATCH 0/2] Sequencer: avoid use-after-free when creating merges

[Phillip Wood via GitGitGadget]

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/20250428194048.149348-1-code@khaugsbakk.name

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
-- 
gitgitgadget

[PATCH 1/2] sequencer: move reflog message functions

[Phillip Wood via GitGitGadget]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

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 <phillip.wood@dunelm.org.uk>
---
 sequencer.c | 66 ++++++++++++++++++++++++++---------------------------
 1 file changed, 33 insertions(+), 33 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 9ea678364d4f..bb4667190db1 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -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)
 {
-- 
gitgitgadget

[PATCH 2/2] sequencer: rework reflog message handling

[Phillip Wood via GitGitGadget]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

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>
---
 sequencer.c              | 50 +++++++++++++++++++---------------------
 t/t3430-rebase-merges.sh | 11 ++++++++-
 2 files changed, 34 insertions(+), 27 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index bb4667190db1..e23708690900 100644
--- 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.
@@ -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);
 }
 
diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
index b84d68c4b96b..ff81adab8cf6 100755
--- 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' '
-- 
gitgitgadget

Re: [PATCH 2/2] sequencer: rework reflog message handling

[Junio C Hamano]

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> 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.