Skip to content

Sequencer: avoid use-after-free when creating merges #1919

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
116 changes: 57 additions & 59 deletions sequencer.c
Original file line number Diff line number Diff line change
Expand Up @@ -224,11 +224,6 @@ struct replay_ctx {
* current chain.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Phillip Wood via GitGitGadget" <[email protected]> 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.

*/
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.
Expand Down Expand Up @@ -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))
Expand All @@ -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",
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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)
{
Expand All @@ -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), "",
Expand All @@ -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;
Expand Down Expand Up @@ -2226,6 +2222,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,
Expand All @@ -2236,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
Expand Down Expand Up @@ -2494,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"));
Expand All @@ -2509,7 +2545,7 @@ static int do_pick_commit(struct repository *r,
* 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;
}
}
Expand Down Expand Up @@ -3919,39 +3955,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)
{
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}


Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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) ||
Expand Down Expand Up @@ -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)) {
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -5402,7 +5403,6 @@ static int commit_staged_changes(struct repository *r,

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;

Expand All @@ -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;
Expand Down Expand Up @@ -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);
}

Expand Down
11 changes: 10 additions & 1 deletion t/t3430-rebase-merges.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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' '
Expand Down
Loading