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

Commit Message

Derrick Stolee Oct. 3, 2024, 11:58 a.m. UTC

From: Derrick Stolee <[e-mail]>

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.

Co-authored-by: Jeff King <[e-mail]>
Signed-off-by: Jeff King <[e-mail]>
Signed-off-by: Derrick Stolee <[e-mail]>
---
 line-log.c          | 10 ++++------
 t/t4211-line-log.sh | 28 ++++++++++++++++++++++++++++
 2 files changed, 32 insertions(+), 6 deletions(-)

Comments

Patrick Steinhardt Oct. 4, 2024, 4:32 a.m. UTC | #1

On Thu, Oct 03, 2024 at 11:58:42AM +0000, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <[e-mail]>
> 
> 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

Patch

diff --git a/line-log.c b/line-log.c
index 67c80b39a0d..29cf66bdd10 100644
--- a/line-log.c
+++ b/line-log.c
@@ -897,13 +897,13 @@  static void print_line(const char *prefix, char first,
 		fputs("\\ No newline at end of file\n", file);
 }
 
-static char *output_prefix(struct diff_options *opt)
+static const char *output_prefix(struct diff_options *opt)
 {
 	if (opt->output_prefix) {
 		struct strbuf *sb = opt->output_prefix(opt, opt->output_prefix_data);
 		return sb->buf;
 	} else {
-		return xstrdup("");
+		return "";
 	}
 }
 
@@ -916,7 +916,7 @@  static void dump_diff_hacky_one(struct rev_info *rev, struct line_log_data *rang
 	struct diff_ranges *diff = &range->diff;
 
 	struct diff_options *opt = &rev->diffopt;
-	char *prefix = output_prefix(opt);
+	const char *prefix = output_prefix(opt);
 	const char *c_reset = diff_get_color(opt->use_color, DIFF_RESET);
 	const char *c_frag = diff_get_color(opt->use_color, DIFF_FRAGINFO);
 	const char *c_meta = diff_get_color(opt->use_color, DIFF_METAINFO);
@@ -1003,7 +1003,6 @@  static void dump_diff_hacky_one(struct rev_info *rev, struct line_log_data *rang
 out:
 	free(p_ends);
 	free(t_ends);
-	free(prefix);
 }
 
 /*
@@ -1012,10 +1011,9 @@  out:
  */
 static void dump_diff_hacky(struct rev_info *rev, struct line_log_data *range)
 {
-	char *prefix = output_prefix(&rev->diffopt);
+	const char *prefix = output_prefix(&rev->diffopt);
 
 	fprintf(rev->diffopt.file, "%s\n", prefix);
-	free(prefix);
 
 	while (range) {
 		dump_diff_hacky_one(rev, range);
diff --git a/t/t4211-line-log.sh b/t/t4211-line-log.sh
index 02d76dca284..950451cf6a6 100755
--- a/t/t4211-line-log.sh
+++ b/t/t4211-line-log.sh
@@ -337,4 +337,32 @@  test_expect_success 'zero-width regex .* matches any function name' '
 	test_cmp expect actual
 '
 
+test_expect_success 'show line-log with graph' '
+	qz_to_tab_space >expect <<-EOF &&
+	* $head_oid Modify func2() in file.c
+	|Z
+	| diff --git a/file.c b/file.c
+	| --- a/file.c
+	| +++ b/file.c
+	| @@ -6,4 +6,4 @@
+	|  int func2()
+	|  {
+	| -    return F2;
+	| +    return F2 + 2;
+	|  }
+	* $root_oid Add func1() and func2() in file.c
+	ZZ
+	  diff --git a/file.c b/file.c
+	  --- /dev/null
+	  +++ b/file.c
+	  @@ -0,0 +6,4 @@
+	  +int func2()
+	  +{
+	  +    return F2;
+	  +}
+	EOF
+	git log --graph --oneline -L:func2:file.c >actual &&
+	test_cmp expect actual
+'
+
 test_done