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

From: Jeff King <peff@peff.net>
Date: May 1, 2025, 15:36 UTC
Message-ID: <20250501153638.GA1793056@coredump.intra.peff.net>
In-Reply-To: <e4ea1e44-af7e-4c24-b0ee-f87091200b90@gmail.com>

On Thu, May 01, 2025 at 03:36:13PM +0100, phillip.wood123@gmail.com wrote:

Show 13 quoted lines

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

Yeah, I agree that would solve the use-after-free. I just wasn't sure if there would still be a logic bug, though. If we have a sequence like:

  /* somebody sets the variable */
  ctx->reflog_message = reflog_message(...);
  ...
  /* some caller, possibly far away, uses the function again */
  foo(reflog_message(...));
  ...
  /* now the original caller wants to use what it stored */
  printf("%s", ctx->reflog_message);

Right now that is a potential use-after-free because of reallocating the static storage inside reflog_message(). If we teach reflog_message to store things in ctx->reflog_message always, that use-after-free goes away, but will the printf() above print the wrong thing?

I wasn't clear on who is setting the value or why. It could be that doing so is actually _fixing_ a bug (because it should be printing what comes from the invocation in the foo() call).

> 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.

Great, thanks.

-Peff