Re: [PATCH] line-log: protect inner strbuf from free

From: Derrick Stolee <stolee@gmail.com>
Date: Oct 3, 2024, 02:36 UTC
Message-ID: <1fc0d162-9814-4d94-ac67-2ea8e40495f4@gmail.com>
In-Reply-To: <20241002235639.GB3455554@coredump.intra.peff.net>

On 10/2/24 7:56 PM, Jeff King wrote:

> On Wed, Oct 02, 2024 at 04:07:04PM +0000, Derrick Stolee via GitGitGadget wrote:

...

Show 14 quoted lines

> Good catch. Your analysis looks correct, though I had two questions I
> managed to answer myself:
> 
>    1. This seems like an obvious use-after-free problem. Why didn't we
>       catch it sooner? I think the answer is that most uses of the
>       output_prefix callback happen via diff_line_prefix(), which was not
>       broken by 394affd46d. It's only line-log that was affected.
> 
>       Building with ASan and running:
> 
>         ./git log --graph -L :diff_line_prefix:diff.c
> 
>       shows the problem immediately (and that your patch fixes it).
>       Should we include a new test in this patch?

Thank you for sending a test in your second reply. I will include it in v2.

Show 7 quoted lines

>    2. If the caller isn't freeing it, then who does? Should diffopt
>       cleanup do so? The answer is "no", the pointer is not owned by that
>       struct. In your cases (1) and (3), the caller does so after
>       finishing with the diffopt struct. In case (2) it is effectively
>       "leaked", but still reachable by the static strbuf. That's not
>       great, and is a problem for eventual libification. But for now, we
>       can ignore it as it won't trigger the leak-checker.

I agree with this, including the potential for cleaning up the static strbuf and using the 'data' parameter like the other functions.

>       It does make me wonder what leak Patrick saw that caused him to
>       write 394affd46d, and whether we're now leaking in some case that
>       I'm missing.

Looking at the change, I can only guess that it was the previous use of

	char *prefix = "";

that signaled that an unfreeable string was being assigned to a non-const pointer. This signals that _something_ is wrong with the function, but the way that the buffer is returned by the function pointer is suspicious, too.

Show 7 quoted lines

> I do think this would have been a harder mistake to make if the callback
> simply returned a "const char *" pointer. We'd lose the ability to show
> prefixes with embedded NULs, but I'm not sure that's a big deal. It
> would also help for line-log to use the existing helper rather than
> inventing its own. So together on top something like this (which I'd
> probably turn into two patches if this seems to others like it's
> valuable and not just churn):

I do agree that changing the return type will make this easier to prevent and the code should be easier to read as well.

Your diffed patch looks pretty good. I made an attempt at guessing where you would have split them (first remove the duplicate method, then change the method prototype and callers).

I even took some time to attempt to remove the static strbuf from diff_output_prefix_callback() in favor of using the 'data' member of the diff_options struct, but it was not incredibly obvious how to communicate ownership of the struct which would need to store both the graph struct and the strbuf. Perhaps this would be good for #leftoverbits.

Thanks, -Stolee