Re: [PATCH v2 1/3] line-log: protect inner strbuf from free
On Thu, Oct 03, 2024 at 11:58:42AM +0000, Derrick Stolee via GitGitGadget wrote:
Show 34 quoted lines
> 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