On Thu, May 01, 2025 at 03:36:13PM +0100, phillip.wood123@xxxxxxxxx wrote:
> > 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
