Re: [BUG] rebase: can write reflog with uninit. `action` string

From

Jeff King <peff@peff.net>

Date

May 1, 2025, 13:17 UTC

Message-ID

<20250501131751.GA1725607@coredump.intra.peff.net>

In-Reply-To

<6743a9fc-11ca-45ac-bc40-4148f5d85d27@app.fastmail.com>

On Wed, Apr 30, 2025 at 05:17:38PM +0200, Kristoffer Haugsbakk wrote:

> > Have you tried building with "make SANITIZE=address,undefined"?
> 
> No I haven’t.  Thank you.  The following is with that `make`.
> 
> Still on f65182a99e5 (The ninth batch, 2025-04-24).  I eventually[1]
> got this:
> 
> [1] I run through 19 merge conflicts which I `--continue` (using rerere)
>     until the rebase is done
> 
> ```
> detached HEAD 5d96584c836] Merge branch '<branch>' into <something else>
>  Author: [author]
> =================================================================
> ==87324==ERROR: AddressSanitizer: heap-use-after-free on address 0x60300001daa0 at pc 0x79371ca5df89 bp 0x7fff8e215a50 sp 0x7fff8e2151c8

But we should be able to trigger it reliably with this:

diff --git a/sequencer.c b/sequencer.c
index b5c4043757..43db0ce66b 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3939,7 +3939,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 reallocation */
 	strbuf_addstr(&buf, sequencer_reflog_action(opts));
 	if (sub_action)
 		strbuf_addf(&buf, " (%s)", sub_action);

And indeed, building with that patch and SANITIZE=address seems to show
the problem reliably with the existing tests in t3430 (it might also
show up without ASan, but probably not as reliably).

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.

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