Re: [BUG] rebase: can write reflog with uninit. `action` string
- From
- phillip.wood123@gmail.com <phillip.wood123@gmail.com>
- Date
- May 1, 2025, 14:36 UTC
- Message-ID
- <e4ea1e44-af7e-4c24-b0ee-f87091200b90@gmail.com>
- In-Reply-To
- <20250501131751.GA1725607@coredump.intra.peff.net>
Hi Peff and Kristoffer
On 01/05/2025 14:17, Jeff King wrote:
Show 11 quoted lines
> 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