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

On Wed, Oct 02, 2024 at 10:36:33PM -0400, Derrick Stolee wrote:

quoted

      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.

Ah, of course. I saw Patrick's name and just assumed it was part of
leak-checking fixes. But of course he also fixed -Wwrite-strings issues.

quoted

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).

Yep, exactly. I actually ended up with a third patch which is a nearby
cleanup. I'll hold them back for now, though. Your patch is a regression
fix which we should prioritize (though it sounds like it is in 2.46, not
the upcoming 2.47?). I'll post my on top as a separate series.

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.

Yeah, I think probably "struct git_graph" would need to own the buffer,
initialize it in graph_init(), and then discard it in graph_clear().
But that gets weird because apparently you can set the callback without
a git_graph? Looks like that is triggered by "--line-prefix" without
"--graph". Yuck.

But in that case we are just showing the line_prefix string, so we could
return that directly. Something like the patch below.

The whole thing feels a bit over-engineered with the callback. The graph
code is the only one that needs anything beyond a static string. And the
way --line-prefix interacts with it is odd, since some callers override
the callback (e.g., "range-diff --line-prefix=foo" is accepted, but
doesn't do anything). I don't think there's a bug anybody cares about,
but, well...it's not how I would have written it. ;)

diff --git a/graph.c b/graph.c
index c046f6285d..bf000fdbe1 100644
--- a/graph.c
+++ b/graph.c

@@ -309,21 +309,28 @@ struct git_graph {
 	 * stored as an index into the array column_colors.
 	 */
 	unsigned short default_column_color;
+
+	/*
+	 * Scratch buffer for generating prefixes to be used with
+	 * diff_output_prefix_callback().
+	 */
+	struct strbuf prefix_buf;
 };
 
 static const char *diff_output_prefix_callback(struct diff_options *opt, void *data)
 {
 	struct git_graph *graph = data;
-	static struct strbuf msgbuf = STRBUF_INIT;
 
 	assert(opt);
 
-	strbuf_reset(&msgbuf);
+	if (!graph)
+		return opt->line_prefix;
+
+	strbuf_reset(&graph->prefix_buf);
 	if (opt->line_prefix)
-		strbuf_addstr(&msgbuf, opt->line_prefix);
-	if (graph)
-		graph_padding_line(graph, &msgbuf);
-	return msgbuf.buf;
+		strbuf_addstr(&graph->prefix_buf, opt->line_prefix);
+	graph_padding_line(graph, &graph->prefix_buf);
+	return graph->prefix_buf.buf;
 }
 
 static const struct diff_options *default_diffopt;

@@ -393,6 +400,7 @@ struct git_graph *graph_init(struct rev_info *opt)
 	 * The diff output prefix callback, with this we can make
 	 * all the diff output to align with the graph lines.
 	 */
+	strbuf_init(&graph->prefix_buf, 0);
 	opt->diffopt.output_prefix = diff_output_prefix_callback;
 	opt->diffopt.output_prefix_data = graph;
 

@@ -408,6 +416,7 @@ void graph_clear(struct git_graph *graph)
 	free(graph->new_columns);
 	free(graph->mapping);
 	free(graph->old_mapping);
+	strbuf_release(&graph->prefix_buf);
 	free(graph);
 }
 

-Peff