Re: [PATCH v2 1/3] line-log: protect inner strbuf from free

[Patrick Steinhardt <ps@pks.im>]

On Thu, Oct 03, 2024 at 11:58:42AM +0000, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <stolee@gmail.com>
> 
> The output_prefix() method in line-log.c may call a function pointer via
> the diff_options struct. This function pointer returns a strbuf struct
> and then its buffer is passed back. However, that implies that the
> consumer is responsible to free the string. This is especially true
> because the default behavior is to duplicate the empty string.
> 
> The existing functions used in the output_prefix pointer include:
> 
>  1. idiff_prefix_cb() in diff-lib.c. This returns the data pointer, so
>     the value exists across multiple calls.
> 
>  2. diff_output_prefix_callback() in graph.c. This uses a static strbuf
>     struct, so it reuses buffers across calls. These should not be
>     freed.
> 
>  3. output_prefix_cb() in range-diff.c. This is similar to the
>     diff-lib.c case.
> 
> In each case, we should not be freeing this buffer. We can convert the
> output_prefix() function to return a const char pointer and stop freeing
> the result.
> 
> This choice is essentially the opposite of what was done in 394affd46d
> (line-log: always allocate the output prefix, 2024-06-07).
> 
> This was discovered via 'valgrind' while investigating a public report
> of a bug in 'git log --graph -L' [1].
> 
> [1] https://github.com/git-for-windows/git/issues/5185
> 
> This issue would have been caught by the new test, when Git is compiled
> with ASan to catch these double frees.

Thanks a bunch for fixing this! The change looks good to me.

Patrick