Hi Peff and Kristoffer
On 01/05/2025 14:17, Jeff King wrote:
On Wed, Apr 30, 2025 at 05:17:38PM +0200, Kristoffer Haugsbakk wrote:
Probably the smallest solution is for ctx->reflog_message to copy the
result and always own the memory (and then remember to free it, both at
cleanup and if it is ever overwritten).
But I think the way reflog_message() returns the "buf" member of a
static strbuf is kind of an anti-pattern, exactly because you can get
this kind of subtle re-use. It probably should just return a non-const
pointer, handing over memory ownership to the caller. That would require
adjusting its other callers, too.
Getting rid of the static buffer would certainly protect us from the
use-after-free. The bug here is that we're not calling reflog_message()
and storing the result in ctx->reflog_message() to create the correct
message in do_merge(). Looking at your patch, having to remember to copy
the string returned from reflog_message() is a bit of a pain. I wonder
if we could change ctx->reflog_message to be an strbuf and update
reflog_message() like so
diff --git a/sequencer.c b/sequencer.c
index ad0ab75c8d4..59d80ddf0cc 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3941,11 +3941,10 @@ static const char
*sequencer_reflog_action(struct replay_opts *opts)
}
__attribute__((format (printf, 3, 4)))
-static const char *reflog_message(struct replay_opts *opts,
+static void reflog_message(struct replay_opts *opts,
const char *sub_action, const char *fmt, ...)
{
va_list ap;
- static struct strbuf buf = STRBUF_INIT;
+ struct strbuf *buf = &opts->ctx->reflog_message;
va_start(ap, fmt);
strbuf_reset(&buf);
@@ -3957,8 +3956,6 @@ static const char *reflog_message(struct
replay_opts *opts,
strbuf_vaddf(&buf, fmt, ap);
}
va_end(ap);
-
- return buf.buf;
}
static struct commit *lookup_label(struct repository *r, const char
*label,
All of the other callers should can then use use ctx->reflog_message.buf
where they were using the return value of reflog_message() before. That
would protect us from the use-after-free.
I'll try and put a proper patch together next week that removes the
static buffer and starts calling reflog_message() when we're merging.
Best Wishes
Phillip
So the "smallest" version is perhaps something like this, totally
untested except for confirming that t3430 no longer complains:
diff --git a/sequencer.c b/sequencer.c
index b5c4043757..07aa3b3731 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -228,7 +228,7 @@ struct replay_ctx {
* 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;
+ char *reflog_message;
/*
* The number of completed fixup and squash commands in the
* current chain.
@@ -411,6 +411,7 @@ static void replay_ctx_release(struct replay_ctx *ctx)
{
strbuf_release(&ctx->current_fixups);
strbuf_release(&ctx->message);
+ free(ctx->reflog_message);
}
void replay_opts_release(struct replay_opts *opts)
@@ -3939,7 +3940,7 @@ static const char *reflog_message(struct replay_opts *opts,
static struct strbuf buf = STRBUF_INIT;
va_start(ap, fmt);
- strbuf_reset(&buf);
+ strbuf_release(&buf); /* guarantees realloaction */
strbuf_addstr(&buf, sequencer_reflog_action(opts));
if (sub_action)
strbuf_addf(&buf, " (%s)", sub_action);
@@ -4886,9 +4887,11 @@ static int pick_one_commit(struct repository *r,
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);
+ if (is_rebase_i(opts)) {
+ free(ctx->reflog_message);
+ ctx->reflog_message = xstrdup(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 +4950,8 @@ 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);
+ free(ctx->reflog_message);
+ ctx->reflog_message = xstrdup(sequencer_reflog_action(opts));
if (opts->allow_ff)
ASSERT(!(opts->signoff || opts->no_commit ||
opts->record_origin || should_edit(opts) ||
@@ -5423,7 +5427,8 @@ int sequencer_continue(struct repository *r, struct replay_opts *opts)
unlink(rebase_path_dropped());
}
- ctx->reflog_message = reflog_message(opts, "continue", NULL);
+ free(ctx->reflog_message);
+ ctx->reflog_message = xstrdup(reflog_message(opts, "continue", NULL));
if (commit_staged_changes(r, opts, &todo_list)) {
res = -1;
goto release_todo_list;
@@ -5475,7 +5480,8 @@ static int single_pick(struct repository *r,
TODO_PICK : TODO_REVERT;
item.commit = cmit;
- opts->ctx->reflog_message = sequencer_reflog_action(opts);
+ free(opts->ctx->reflog_message);
+ opts->ctx->reflog_message = xstrdup(sequencer_reflog_action(opts));
return do_pick_commit(r, &item, opts, 0, &check_todo);
}
I'm hoping your or Phillip can decide on the best fix from here.
-Peff
