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

From: Jeff King <peff@peff.net>
Date: Oct 3, 2024, 06:11 UTC
Message-ID: <20241003061136.GA351404@coredump.intra.peff.net>
In-Reply-To: <1fc0d162-9814-4d94-ac67-2ea8e40495f4@gmail.com>

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

Show 12 quoted lines

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

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

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.

Show 5 quoted lines

> 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