kernel
/
git
mirror of https://git.kernel.org/pub/scm/git/git.git
1
0
Fork 0
Code Releases Activity

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>
maint
Phillip Wood 2025-05-09 16:22:27 +00:00 committed by Junio C Hamano
parent 7472721463
commit 5dbaec628d
2 changed files with 34 additions and 27 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

50
sequencer.c
View File

@ -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);
}


11
t/t3430-rebase-merges.sh
View File

@ -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' '