{"thread":{"id":"62240","subject":"[PATCH] line-log: protect inner strbuf from free","startedAt":"2024-10-02T16:07:09Z","lastAt":"2024-10-04T19:33:22Z","messageCount":24,"participants":["Derrick Stolee via GitGitGadget","Jeff King","Derrick Stolee","Jeff King via GitGitGadget","Junio C Hamano","Patrick Steinhardt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"503962","messageId":"pull.1806.git.1727885224966.gitgitgadget@gmail.com","threadId":"62240","inReplyTo":null,"subject":"[PATCH] line-log: protect inner strbuf from free","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-10-02T16:07:04Z","receivedAt":"2024-10-02T16:07:09Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <stolee@gmail.com>\n\nThe output_prefix() method in line-log.c may call a function pointer via\nthe diff_options struct. This function pointer returns a strbuf struct\nand then its buffer is passed back. However, that implies that the\nconsumer is responsible to free the string. This is especially true\nbecause the default behavior is to duplicate the empty string.\n\nThe existing functions used in the output_prefix pointer include:\n\n 1. idiff_prefix_cb() in diff-lib.c. This returns the data pointer, so\n    the value exists across multiple calls.\n\n 2. diff_output_prefix_callback() in graph.c. This uses a static strbuf\n    struct, so it reuses buffers across calls. These should not be\n    freed.\n\n 3. output_prefix_cb() in range-diff.c. This is similar to the\n    diff-lib.c case.\n\nIn each case, we should not be freeing this buffer. We can convert the\noutput_prefix() function to return a const char pointer and stop freeing\nthe result.\n\nThis choice is essentially the opposite of what was done in 394affd46d\n(line-log: always allocate the output prefix, 2024-06-07).\n\nThis was discovered via 'valgrind' while investigating a public report\nof a bug in 'git log --graph -L' [1].\n\n[1] https://github.com/git-for-windows/git/issues/5185\n\nSigned-off-by: Derrick Stolee <stolee@gmail.com>\n---\n    line-log: protect inner strbuf from free\n    \n    This fixes a regression introduced in 2.46.0.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1806%2Fderrickstolee%2Fline-log-use-after-free-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1806/derrickstolee/line-log-use-after-free-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1806\n\n line-log.c | 10 ++++------\n 1 file changed, 4 insertions(+), 6 deletions(-)\n\ndiff --git a/line-log.c b/line-log.c\nindex 67c80b39a0d..29cf66bdd10 100644\n--- a/line-log.c\n+++ b/line-log.c\n@@ -897,13 +897,13 @@ static void print_line(const char *prefix, char first,\n \t\tfputs(\"\\\\ No newline at end of file\\n\", file);\n }\n \n-static char *output_prefix(struct diff_options *opt)\n+static const char *output_prefix(struct diff_options *opt)\n {\n \tif (opt->output_prefix) {\n \t\tstruct strbuf *sb = opt->output_prefix(opt, opt->output_prefix_data);\n \t\treturn sb->buf;\n \t} else {\n-\t\treturn xstrdup(\"\");\n+\t\treturn \"\";\n \t}\n }\n \n@@ -916,7 +916,7 @@ static void dump_diff_hacky_one(struct rev_info *rev, struct line_log_data *rang\n \tstruct diff_ranges *diff = &range->diff;\n \n \tstruct diff_options *opt = &rev->diffopt;\n-\tchar *prefix = output_prefix(opt);\n+\tconst char *prefix = output_prefix(opt);\n \tconst char *c_reset = diff_get_color(opt->use_color, DIFF_RESET);\n \tconst char *c_frag = diff_get_color(opt->use_color, DIFF_FRAGINFO);\n \tconst char *c_meta = diff_get_color(opt->use_color, DIFF_METAINFO);\n@@ -1003,7 +1003,6 @@ static void dump_diff_hacky_one(struct rev_info *rev, struct line_log_data *rang\n out:\n \tfree(p_ends);\n \tfree(t_ends);\n-\tfree(prefix);\n }\n \n /*\n@@ -1012,10 +1011,9 @@ out:\n  */\n static void dump_diff_hacky(struct rev_info *rev, struct line_log_data *range)\n {\n-\tchar *prefix = output_prefix(&rev->diffopt);\n+\tconst char *prefix = output_prefix(&rev->diffopt);\n \n \tfprintf(rev->diffopt.file, \"%s\\n\", prefix);\n-\tfree(prefix);\n \n \twhile (range) {\n \t\tdump_diff_hacky_one(rev, range);\n\nbase-commit: e9356ba3ea2a6754281ff7697b3e5a1697b21e24\n-- \ngitgitgadget\n"},{"id":"504003","messageId":"20241002235639.GB3455554@coredump.intra.peff.net","threadId":"62240","inReplyTo":"pull.1806.git.1727885224966.gitgitgadget@gmail.com","subject":"Re: [PATCH] line-log: protect inner strbuf from free","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-10-02T23:56:39Z","receivedAt":"2024-10-02T23:56:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 02, 2024 at 04:07:04PM +0000, Derrick Stolee via GitGitGadget wrote:\n\n> The output_prefix() method in line-log.c may call a function pointer via\n> the diff_options struct. This function pointer returns a strbuf struct\n> and then its buffer is passed back. However, that implies that the\n> consumer is responsible to free the string. This is especially true\n> because the default behavior is to duplicate the empty string.\n> \n> The existing functions used in the output_prefix pointer include:\n> \n>  1. idiff_prefix_cb() in diff-lib.c. This returns the data pointer, so\n>     the value exists across multiple calls.\n> \n>  2. diff_output_prefix_callback() in graph.c. This uses a static strbuf\n>     struct, so it reuses buffers across calls. These should not be\n>     freed.\n> \n>  3. output_prefix_cb() in range-diff.c. This is similar to the\n>     diff-lib.c case.\n> \n> In each case, we should not be freeing this buffer. We can convert the\n> output_prefix() function to return a const char pointer and stop freeing\n> the result.\n> \n> This choice is essentially the opposite of what was done in 394affd46d\n> (line-log: always allocate the output prefix, 2024-06-07).\n> \n> This was discovered via 'valgrind' while investigating a public report\n> of a bug in 'git log --graph -L' [1].\n\nGood catch. Your analysis looks correct, though I had two questions I\nmanaged to answer myself:\n\n  1. This seems like an obvious use-after-free problem. Why didn't we\n     catch it sooner? I think the answer is that most uses of the\n     output_prefix callback happen via diff_line_prefix(), which was not\n     broken by 394affd46d. It's only line-log that was affected.\n\n     Building with ASan and running:\n\n       ./git log --graph -L :diff_line_prefix:diff.c\n\n     shows the problem immediately (and that your patch fixes it).\n     Should we include a new test in this patch?\n\n  2. If the caller isn't freeing it, then who does? Should diffopt\n     cleanup do so? The answer is \"no\", the pointer is not owned by that\n     struct. In your cases (1) and (3), the caller does so after\n     finishing with the diffopt struct. In case (2) it is effectively\n     \"leaked\", but still reachable by the static strbuf. That's not\n     great, and is a problem for eventual libification. But for now, we\n     can ignore it as it won't trigger the leak-checker.\n\n     It does make me wonder what leak Patrick saw that caused him to\n     write 394affd46d, and whether we're now leaking in some case that\n     I'm missing.\n\nI do think this would have been a harder mistake to make if the callback\nsimply returned a \"const char *\" pointer. We'd lose the ability to show\nprefixes with embedded NULs, but I'm not sure that's a big deal. It\nwould also help for line-log to use the existing helper rather than\ninventing its own. So together on top something like this (which I'd\nprobably turn into two patches if this seems to others like it's\nvaluable and not just churn):\n\n diff-lib.c   |  4 ++--\n diff.c       |  9 +++------\n diff.h       |  2 +-\n graph.c      |  4 ++--\n line-log.c   | 14 ++------------\n log-tree.c   |  7 +++----\n range-diff.c |  4 ++--\n 7 files changed, 15 insertions(+), 29 deletions(-)\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex a680768ee7..6b14b95962 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -701,7 +701,7 @@ int index_differs_from(struct repository *r,\n \treturn (has_changes != 0);\n }\n \n-static struct strbuf *idiff_prefix_cb(struct diff_options *opt UNUSED, void *data)\n+static const char *idiff_prefix_cb(struct diff_options *opt UNUSED, void *data)\n {\n \treturn data;\n }\n@@ -716,7 +716,7 @@ void show_interdiff(const struct object_id *oid1, const struct object_id *oid2,\n \topts.output_format = DIFF_FORMAT_PATCH;\n \topts.output_prefix = idiff_prefix_cb;\n \tstrbuf_addchars(&prefix, ' ', indent);\n-\topts.output_prefix_data = &prefix;\n+\topts.output_prefix_data = prefix.buf;\n \tdiff_setup_done(&opts);\n \n \tdiff_tree_oid(oid1, oid2, \"\", &opts);\ndiff --git a/diff.c b/diff.c\nindex 173cbe2bed..efde65feef 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -2317,12 +2317,9 @@ const char *diff_get_color(int diff_use_color, enum color_diff ix)\n \n const char *diff_line_prefix(struct diff_options *opt)\n {\n-\tstruct strbuf *msgbuf;\n-\tif (!opt->output_prefix)\n-\t\treturn \"\";\n-\n-\tmsgbuf = opt->output_prefix(opt, opt->output_prefix_data);\n-\treturn msgbuf->buf;\n+\treturn opt->output_prefix ?\n+\t\topt->output_prefix(opt, opt->output_prefix_data) :\n+\t\t\"\";\n }\n \n static unsigned long sane_truncate_line(char *line, unsigned long len)\ndiff --git a/diff.h b/diff.h\nindex 0cde3b34e2..2a9c9191c1 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -94,7 +94,7 @@ typedef void (*add_remove_fn_t)(struct diff_options *options,\n typedef void (*diff_format_fn_t)(struct diff_queue_struct *q,\n \t\tstruct diff_options *options, void *data);\n \n-typedef struct strbuf *(*diff_prefix_fn_t)(struct diff_options *opt, void *data);\n+typedef const char *(*diff_prefix_fn_t)(struct diff_options *opt, void *data);\n \n #define DIFF_FORMAT_RAW\t\t0x0001\n #define DIFF_FORMAT_DIFFSTAT\t0x0002\ndiff --git a/graph.c b/graph.c\nindex 091c14cf4f..ebb7d1e66f 100644\n--- a/graph.c\n+++ b/graph.c\n@@ -314,7 +314,7 @@ struct git_graph {\n \tunsigned short default_column_color;\n };\n \n-static struct strbuf *diff_output_prefix_callback(struct diff_options *opt, void *data)\n+static const char *diff_output_prefix_callback(struct diff_options *opt, void *data)\n {\n \tstruct git_graph *graph = data;\n \tstatic struct strbuf msgbuf = STRBUF_INIT;\n@@ -327,7 +327,7 @@ static struct strbuf *diff_output_prefix_callback(struct diff_options *opt, void\n \t\t\t   opt->line_prefix_length);\n \tif (graph)\n \t\tgraph_padding_line(graph, &msgbuf);\n-\treturn &msgbuf;\n+\treturn msgbuf.buf;\n }\n \n static const struct diff_options *default_diffopt;\ndiff --git a/line-log.c b/line-log.c\nindex 29cf66bdd1..63945c4729 100644\n--- a/line-log.c\n+++ b/line-log.c\n@@ -897,16 +897,6 @@ static void print_line(const char *prefix, char first,\n \t\tfputs(\"\\\\ No newline at end of file\\n\", file);\n }\n \n-static const char *output_prefix(struct diff_options *opt)\n-{\n-\tif (opt->output_prefix) {\n-\t\tstruct strbuf *sb = opt->output_prefix(opt, opt->output_prefix_data);\n-\t\treturn sb->buf;\n-\t} else {\n-\t\treturn \"\";\n-\t}\n-}\n-\n static void dump_diff_hacky_one(struct rev_info *rev, struct line_log_data *range)\n {\n \tunsigned int i, j = 0;\n@@ -916,7 +906,7 @@ static void dump_diff_hacky_one(struct rev_info *rev, struct line_log_data *rang\n \tstruct diff_ranges *diff = &range->diff;\n \n \tstruct diff_options *opt = &rev->diffopt;\n-\tconst char *prefix = output_prefix(opt);\n+\tconst char *prefix = diff_line_prefix(opt);\n \tconst char *c_reset = diff_get_color(opt->use_color, DIFF_RESET);\n \tconst char *c_frag = diff_get_color(opt->use_color, DIFF_FRAGINFO);\n \tconst char *c_meta = diff_get_color(opt->use_color, DIFF_METAINFO);\n@@ -1011,7 +1001,7 @@ static void dump_diff_hacky_one(struct rev_info *rev, struct line_log_data *rang\n  */\n static void dump_diff_hacky(struct rev_info *rev, struct line_log_data *range)\n {\n-\tconst char *prefix = output_prefix(&rev->diffopt);\n+\tconst char *prefix = diff_line_prefix(&rev->diffopt);\n \n \tfprintf(rev->diffopt.file, \"%s\\n\", prefix);\n \ndiff --git a/log-tree.c b/log-tree.c\nindex 3758e0d3b8..1b80abe7d5 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -923,10 +923,9 @@ int log_tree_diff_flush(struct rev_info *opt)\n \t\t\t */\n \t\t\tint pch = DIFF_FORMAT_DIFFSTAT | DIFF_FORMAT_PATCH;\n \t\t\tif (opt->diffopt.output_prefix) {\n-\t\t\t\tstruct strbuf *msg = NULL;\n-\t\t\t\tmsg = opt->diffopt.output_prefix(&opt->diffopt,\n-\t\t\t\t\topt->diffopt.output_prefix_data);\n-\t\t\t\tfwrite(msg->buf, msg->len, 1, opt->diffopt.file);\n+\t\t\t\tfputs(opt->diffopt.output_prefix(&opt->diffopt,\n+\t\t\t\t\t\t\t\t opt->diffopt.output_prefix_data),\n+\t\t\t\t      opt->diffopt.file);\n \t\t\t}\n \n \t\t\t/*\ndiff --git a/range-diff.c b/range-diff.c\nindex bbb0952264..10885ba301 100644\n--- a/range-diff.c\n+++ b/range-diff.c\n@@ -480,7 +480,7 @@ static void patch_diff(const char *a, const char *b,\n \tdiff_flush(diffopt);\n }\n \n-static struct strbuf *output_prefix_cb(struct diff_options *opt UNUSED, void *data)\n+static const char *output_prefix_cb(struct diff_options *opt UNUSED, void *data)\n {\n \treturn data;\n }\n@@ -508,7 +508,7 @@ static void output(struct string_list *a, struct string_list *b,\n \topts.flags.suppress_hunk_header_line_count = 1;\n \topts.output_prefix = output_prefix_cb;\n \tstrbuf_addstr(&indent, \"    \");\n-\topts.output_prefix_data = &indent;\n+\topts.output_prefix_data = indent.buf;\n \tdiff_setup_done(&opts);\n \n \t/*\n"},{"id":"504005","messageId":"20241003001945.GA3486271@coredump.intra.peff.net","threadId":"62240","inReplyTo":"20241002235639.GB3455554@coredump.intra.peff.net","subject":"Re: [PATCH] line-log: protect inner strbuf from free","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-10-03T00:19:45Z","receivedAt":"2024-10-03T00:19:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 02, 2024 at 07:56:39PM -0400, Jeff King wrote:\n\n> Good catch. Your analysis looks correct, though I had two questions I\n> managed to answer myself:\n> \n>   1. This seems like an obvious use-after-free problem. Why didn't we\n>      catch it sooner? I think the answer is that most uses of the\n>      output_prefix callback happen via diff_line_prefix(), which was not\n>      broken by 394affd46d. It's only line-log that was affected.\n> \n>      Building with ASan and running:\n> \n>        ./git log --graph -L :diff_line_prefix:diff.c\n> \n>      shows the problem immediately (and that your patch fixes it).\n>      Should we include a new test in this patch?\n\nI think this would do it if you'd like to squash it in. Note that\nshowing the actual patch is necessary to trigger the problem (since\nthat's where we free the prefix buf). Unfortunately it makes the\nexpected output a lot uglier. :(\n\ndiff --git a/t/t4211-line-log.sh b/t/t4211-line-log.sh\nindex 02d76dca28..950451cf6a 100755\n--- a/t/t4211-line-log.sh\n+++ b/t/t4211-line-log.sh\n@@ -337,4 +337,32 @@ test_expect_success 'zero-width regex .* matches any function name' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'show line-log with graph' '\n+\tqz_to_tab_space >expect <<-EOF &&\n+\t* $head_oid Modify func2() in file.c\n+\t|Z\n+\t| diff --git a/file.c b/file.c\n+\t| --- a/file.c\n+\t| +++ b/file.c\n+\t| @@ -6,4 +6,4 @@\n+\t|  int func2()\n+\t|  {\n+\t| -    return F2;\n+\t| +    return F2 + 2;\n+\t|  }\n+\t* $root_oid Add func1() and func2() in file.c\n+\tZZ\n+\t  diff --git a/file.c b/file.c\n+\t  --- /dev/null\n+\t  +++ b/file.c\n+\t  @@ -0,0 +6,4 @@\n+\t  +int func2()\n+\t  +{\n+\t  +    return F2;\n+\t  +}\n+\tEOF\n+\tgit log --graph --oneline -L:func2:file.c >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n\n-Peff\n"},{"id":"504013","messageId":"1fc0d162-9814-4d94-ac67-2ea8e40495f4@gmail.com","threadId":"62240","inReplyTo":"20241002235639.GB3455554@coredump.intra.peff.net","subject":"Re: [PATCH] line-log: protect inner strbuf from free","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2024-10-03T02:36:33Z","receivedAt":"2024-10-03T02:36:35Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 10/2/24 7:56 PM, Jeff King wrote:\n> On Wed, Oct 02, 2024 at 04:07:04PM +0000, Derrick Stolee via GitGitGadget wrote:\n...\n> Good catch. Your analysis looks correct, though I had two questions I\n> managed to answer myself:\n> \n>    1. This seems like an obvious use-after-free problem. Why didn't we\n>       catch it sooner? I think the answer is that most uses of the\n>       output_prefix callback happen via diff_line_prefix(), which was not\n>       broken by 394affd46d. It's only line-log that was affected.\n> \n>       Building with ASan and running:\n> \n>         ./git log --graph -L :diff_line_prefix:diff.c\n> \n>       shows the problem immediately (and that your patch fixes it).\n>       Should we include a new test in this patch?\n\nThank you for sending a test in your second reply. I will include it in v2.\n\n>    2. If the caller isn't freeing it, then who does? Should diffopt\n>       cleanup do so? The answer is \"no\", the pointer is not owned by that\n>       struct. In your cases (1) and (3), the caller does so after\n>       finishing with the diffopt struct. In case (2) it is effectively\n>       \"leaked\", but still reachable by the static strbuf. That's not\n>       great, and is a problem for eventual libification. But for now, we\n>       can ignore it as it won't trigger the leak-checker.\n\nI agree with this, including the potential for cleaning up the static\nstrbuf and using the 'data' parameter like the other functions.\n\n>       It does make me wonder what leak Patrick saw that caused him to\n>       write 394affd46d, and whether we're now leaking in some case that\n>       I'm missing.\n\nLooking at the change, I can only guess that it was the previous use of\n\n\tchar *prefix = \"\";\n\nthat signaled that an unfreeable string was being assigned to a non-const\npointer. This signals that _something_ is wrong with the function, but\nthe way that the buffer is returned by the function pointer is suspicious,\ntoo.\n\n> I do think this would have been a harder mistake to make if the callback\n> simply returned a \"const char *\" pointer. We'd lose the ability to show\n> prefixes with embedded NULs, but I'm not sure that's a big deal. It\n> would also help for line-log to use the existing helper rather than\n> inventing its own. So together on top something like this (which I'd\n> probably turn into two patches if this seems to others like it's\n> valuable and not just churn):\n\nI do agree that changing the return type will make this easier to prevent\nand the code should be easier to read as well.\n\nYour diffed patch looks pretty good. I made an attempt at guessing where\nyou would have split them (first remove the duplicate method, then change\nthe method prototype and callers).\n\nI even took some time to attempt to remove the static strbuf from\ndiff_output_prefix_callback() in favor of using the 'data' member of the\ndiff_options struct, but it was not incredibly obvious how to communicate\nownership of the struct which would need to store both the graph struct\nand the strbuf. Perhaps this would be good for #leftoverbits.\n\nThanks,\n-Stolee\n\n"},{"id":"504015","messageId":"20241003061136.GA351404@coredump.intra.peff.net","threadId":"62240","inReplyTo":"1fc0d162-9814-4d94-ac67-2ea8e40495f4@gmail.com","subject":"Re: [PATCH] line-log: protect inner strbuf from free","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-10-03T06:11:36Z","receivedAt":"2024-10-03T06:11:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 02, 2024 at 10:36:33PM -0400, Derrick Stolee wrote:\n\n> >       It does make me wonder what leak Patrick saw that caused him to\n> >       write 394affd46d, and whether we're now leaking in some case that\n> >       I'm missing.\n> \n> Looking at the change, I can only guess that it was the previous use of\n> \n> \tchar *prefix = \"\";\n> \n> that signaled that an unfreeable string was being assigned to a non-const\n> pointer. This signals that _something_ is wrong with the function, but\n> the way that the buffer is returned by the function pointer is suspicious,\n> too.\n\nAh, of course. I saw Patrick's name and just assumed it was part of\nleak-checking fixes. But of course he also fixed -Wwrite-strings issues.\n\n> > I do think this would have been a harder mistake to make if the callback\n> > simply returned a \"const char *\" pointer. We'd lose the ability to show\n> > prefixes with embedded NULs, but I'm not sure that's a big deal. It\n> > would also help for line-log to use the existing helper rather than\n> > inventing its own. So together on top something like this (which I'd\n> > probably turn into two patches if this seems to others like it's\n> > valuable and not just churn):\n> \n> I do agree that changing the return type will make this easier to prevent\n> and the code should be easier to read as well.\n> \n> Your diffed patch looks pretty good. I made an attempt at guessing where\n> you would have split them (first remove the duplicate method, then change\n> the method prototype and callers).\n\nYep, exactly. I actually ended up with a third patch which is a nearby\ncleanup. I'll hold them back for now, though. Your patch is a regression\nfix which we should prioritize (though it sounds like it is in 2.46, not\nthe upcoming 2.47?). I'll post my on top as a separate series.\n\n> I even took some time to attempt to remove the static strbuf from\n> diff_output_prefix_callback() in favor of using the 'data' member of the\n> diff_options struct, but it was not incredibly obvious how to communicate\n> ownership of the struct which would need to store both the graph struct\n> and the strbuf. Perhaps this would be good for #leftoverbits.\n\nYeah, I think probably \"struct git_graph\" would need to own the buffer,\ninitialize it in graph_init(), and then discard it in graph_clear().\nBut that gets weird because apparently you can set the callback without\na git_graph? Looks like that is triggered by \"--line-prefix\" without\n\"--graph\". Yuck.\n\nBut in that case we are just showing the line_prefix string, so we could\nreturn that directly. Something like the patch below.\n\nThe whole thing feels a bit over-engineered with the callback. The graph\ncode is the only one that needs anything beyond a static string. And the\nway --line-prefix interacts with it is odd, since some callers override\nthe callback (e.g., \"range-diff --line-prefix=foo\" is accepted, but\ndoesn't do anything). I don't think there's a bug anybody cares about,\nbut, well...it's not how I would have written it. ;)\n\ndiff --git a/graph.c b/graph.c\nindex c046f6285d..bf000fdbe1 100644\n--- a/graph.c\n+++ b/graph.c\n@@ -309,21 +309,28 @@ struct git_graph {\n \t * stored as an index into the array column_colors.\n \t */\n \tunsigned short default_column_color;\n+\n+\t/*\n+\t * Scratch buffer for generating prefixes to be used with\n+\t * diff_output_prefix_callback().\n+\t */\n+\tstruct strbuf prefix_buf;\n };\n \n static const char *diff_output_prefix_callback(struct diff_options *opt, void *data)\n {\n \tstruct git_graph *graph = data;\n-\tstatic struct strbuf msgbuf = STRBUF_INIT;\n \n \tassert(opt);\n \n-\tstrbuf_reset(&msgbuf);\n+\tif (!graph)\n+\t\treturn opt->line_prefix;\n+\n+\tstrbuf_reset(&graph->prefix_buf);\n \tif (opt->line_prefix)\n-\t\tstrbuf_addstr(&msgbuf, opt->line_prefix);\n-\tif (graph)\n-\t\tgraph_padding_line(graph, &msgbuf);\n-\treturn msgbuf.buf;\n+\t\tstrbuf_addstr(&graph->prefix_buf, opt->line_prefix);\n+\tgraph_padding_line(graph, &graph->prefix_buf);\n+\treturn graph->prefix_buf.buf;\n }\n \n static const struct diff_options *default_diffopt;\n@@ -393,6 +400,7 @@ struct git_graph *graph_init(struct rev_info *opt)\n \t * The diff output prefix callback, with this we can make\n \t * all the diff output to align with the graph lines.\n \t */\n+\tstrbuf_init(&graph->prefix_buf, 0);\n \topt->diffopt.output_prefix = diff_output_prefix_callback;\n \topt->diffopt.output_prefix_data = graph;\n \n@@ -408,6 +416,7 @@ void graph_clear(struct git_graph *graph)\n \tfree(graph->new_columns);\n \tfree(graph->mapping);\n \tfree(graph->old_mapping);\n+\tstrbuf_release(&graph->prefix_buf);\n \tfree(graph);\n }\n \n\n-Peff\n"},{"id":"504020","messageId":"pull.1806.v2.git.1727956724.gitgitgadget@gmail.com","threadId":"62240","inReplyTo":"pull.1806.git.1727885224966.gitgitgadget@gmail.com","subject":"[PATCH v2 0/3] line-log: protect inner strbuf from free","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-10-03T11:58:41Z","receivedAt":"2024-10-03T11:58:48Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"This fixes a regression introduced in 2.46.0.\n\nThe change made in 394affd46d (line-log: always allocate the output prefix,\n2024-06-07) made the method more consistent in that it did not return a\nstatic empty string that would fail to free(), but it does lead to\ndouble-frees when a strbuf buffer is freed multiple times.\n\nIn v2, I add Peff's test to the first patch. I also split his diff into two\nmore follow-up patches because the additional clarity seems valuable to me.\nI have forged his sign-off in all three patches.\n\nNote to the maintainer: feel free to take only the first patch, as Peff\nreplied that he may work on the remaining cleanup independently (but I had\nalready prepared patches 2 & 3).\n\nThanks, -Stolee\n\nDerrick Stolee (1):\n  line-log: protect inner strbuf from free\n\nJeff King (2):\n  line-log: remove output_prefix()\n  diff: modify output_prefix function pointer\n\n diff-lib.c          |  4 ++--\n diff.c              |  8 +++-----\n diff.h              |  2 +-\n graph.c             |  4 ++--\n line-log.c          | 16 ++--------------\n log-tree.c          |  4 ++--\n range-diff.c        |  4 ++--\n t/t4211-line-log.sh | 28 ++++++++++++++++++++++++++++\n 8 files changed, 42 insertions(+), 28 deletions(-)\n\n\nbase-commit: e9356ba3ea2a6754281ff7697b3e5a1697b21e24\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1806%2Fderrickstolee%2Fline-log-use-after-free-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1806/derrickstolee/line-log-use-after-free-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1806\n\nRange-diff vs v1:\n\n 1:  5d341e42d83 ! 1:  05c21616c35 line-log: protect inner strbuf from free\n     @@ Commit message\n      \n          [1] https://github.com/git-for-windows/git/issues/5185\n      \n     +    This issue would have been caught by the new test, when Git is compiled\n     +    with ASan to catch these double frees.\n     +\n     +    Co-authored-by: Jeff King <peff@peff.net>\n     +    Signed-off-by: Jeff King <peff@peff.net>\n          Signed-off-by: Derrick Stolee <stolee@gmail.com>\n      \n       ## line-log.c ##\n     @@ line-log.c: out:\n       \n       \twhile (range) {\n       \t\tdump_diff_hacky_one(rev, range);\n     +\n     + ## t/t4211-line-log.sh ##\n     +@@ t/t4211-line-log.sh: test_expect_success 'zero-width regex .* matches any function name' '\n     + \ttest_cmp expect actual\n     + '\n     + \n     ++test_expect_success 'show line-log with graph' '\n     ++\tqz_to_tab_space >expect <<-EOF &&\n     ++\t* $head_oid Modify func2() in file.c\n     ++\t|Z\n     ++\t| diff --git a/file.c b/file.c\n     ++\t| --- a/file.c\n     ++\t| +++ b/file.c\n     ++\t| @@ -6,4 +6,4 @@\n     ++\t|  int func2()\n     ++\t|  {\n     ++\t| -    return F2;\n     ++\t| +    return F2 + 2;\n     ++\t|  }\n     ++\t* $root_oid Add func1() and func2() in file.c\n     ++\tZZ\n     ++\t  diff --git a/file.c b/file.c\n     ++\t  --- /dev/null\n     ++\t  +++ b/file.c\n     ++\t  @@ -0,0 +6,4 @@\n     ++\t  +int func2()\n     ++\t  +{\n     ++\t  +    return F2;\n     ++\t  +}\n     ++\tEOF\n     ++\tgit log --graph --oneline -L:func2:file.c >actual &&\n     ++\ttest_cmp expect actual\n     ++'\n     ++\n     + test_done\n -:  ----------- > 2:  94d2c034b4a line-log: remove output_prefix()\n -:  ----------- > 3:  e1d825ad212 diff: modify output_prefix function pointer\n\n-- \ngitgitgadget\n"},{"id":"504021","messageId":"05c21616c350b5141c17fde1aa5d3aea881c6031.1727956724.git.gitgitgadget@gmail.com","threadId":"62240","inReplyTo":"pull.1806.v2.git.1727956724.gitgitgadget@gmail.com","subject":"[PATCH v2 1/3] line-log: protect inner strbuf from free","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-10-03T11:58:42Z","receivedAt":"2024-10-03T11:58:49Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <stolee@gmail.com>\n\nThe output_prefix() method in line-log.c may call a function pointer via\nthe diff_options struct. This function pointer returns a strbuf struct\nand then its buffer is passed back. However, that implies that the\nconsumer is responsible to free the string. This is especially true\nbecause the default behavior is to duplicate the empty string.\n\nThe existing functions used in the output_prefix pointer include:\n\n 1. idiff_prefix_cb() in diff-lib.c. This returns the data pointer, so\n    the value exists across multiple calls.\n\n 2. diff_output_prefix_callback() in graph.c. This uses a static strbuf\n    struct, so it reuses buffers across calls. These should not be\n    freed.\n\n 3. output_prefix_cb() in range-diff.c. This is similar to the\n    diff-lib.c case.\n\nIn each case, we should not be freeing this buffer. We can convert the\noutput_prefix() function to return a const char pointer and stop freeing\nthe result.\n\nThis choice is essentially the opposite of what was done in 394affd46d\n(line-log: always allocate the output prefix, 2024-06-07).\n\nThis was discovered via 'valgrind' while investigating a public report\nof a bug in 'git log --graph -L' [1].\n\n[1] https://github.com/git-for-windows/git/issues/5185\n\nThis issue would have been caught by the new test, when Git is compiled\nwith ASan to catch these double frees.\n\nCo-authored-by: Jeff King <peff@peff.net>\nSigned-off-by: Jeff King <peff@peff.net>\nSigned-off-by: Derrick Stolee <stolee@gmail.com>\n---\n line-log.c          | 10 ++++------\n t/t4211-line-log.sh | 28 ++++++++++++++++++++++++++++\n 2 files changed, 32 insertions(+), 6 deletions(-)\n\ndiff --git a/line-log.c b/line-log.c\nindex 67c80b39a0d..29cf66bdd10 100644\n--- a/line-log.c\n+++ b/line-log.c\n@@ -897,13 +897,13 @@ static void print_line(const char *prefix, char first,\n \t\tfputs(\"\\\\ No newline at end of file\\n\", file);\n }\n \n-static char *output_prefix(struct diff_options *opt)\n+static const char *output_prefix(struct diff_options *opt)\n {\n \tif (opt->output_prefix) {\n \t\tstruct strbuf *sb = opt->output_prefix(opt, opt->output_prefix_data);\n \t\treturn sb->buf;\n \t} else {\n-\t\treturn xstrdup(\"\");\n+\t\treturn \"\";\n \t}\n }\n \n@@ -916,7 +916,7 @@ static void dump_diff_hacky_one(struct rev_info *rev, struct line_log_data *rang\n \tstruct diff_ranges *diff = &range->diff;\n \n \tstruct diff_options *opt = &rev->diffopt;\n-\tchar *prefix = output_prefix(opt);\n+\tconst char *prefix = output_prefix(opt);\n \tconst char *c_reset = diff_get_color(opt->use_color, DIFF_RESET);\n \tconst char *c_frag = diff_get_color(opt->use_color, DIFF_FRAGINFO);\n \tconst char *c_meta = diff_get_color(opt->use_color, DIFF_METAINFO);\n@@ -1003,7 +1003,6 @@ static void dump_diff_hacky_one(struct rev_info *rev, struct line_log_data *rang\n out:\n \tfree(p_ends);\n \tfree(t_ends);\n-\tfree(prefix);\n }\n \n /*\n@@ -1012,10 +1011,9 @@ out:\n  */\n static void dump_diff_hacky(struct rev_info *rev, struct line_log_data *range)\n {\n-\tchar *prefix = output_prefix(&rev->diffopt);\n+\tconst char *prefix = output_prefix(&rev->diffopt);\n \n \tfprintf(rev->diffopt.file, \"%s\\n\", prefix);\n-\tfree(prefix);\n \n \twhile (range) {\n \t\tdump_diff_hacky_one(rev, range);\ndiff --git a/t/t4211-line-log.sh b/t/t4211-line-log.sh\nindex 02d76dca284..950451cf6a6 100755\n--- a/t/t4211-line-log.sh\n+++ b/t/t4211-line-log.sh\n@@ -337,4 +337,32 @@ test_expect_success 'zero-width regex .* matches any function name' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'show line-log with graph' '\n+\tqz_to_tab_space >expect <<-EOF &&\n+\t* $head_oid Modify func2() in file.c\n+\t|Z\n+\t| diff --git a/file.c b/file.c\n+\t| --- a/file.c\n+\t| +++ b/file.c\n+\t| @@ -6,4 +6,4 @@\n+\t|  int func2()\n+\t|  {\n+\t| -    return F2;\n+\t| +    return F2 + 2;\n+\t|  }\n+\t* $root_oid Add func1() and func2() in file.c\n+\tZZ\n+\t  diff --git a/file.c b/file.c\n+\t  --- /dev/null\n+\t  +++ b/file.c\n+\t  @@ -0,0 +6,4 @@\n+\t  +int func2()\n+\t  +{\n+\t  +    return F2;\n+\t  +}\n+\tEOF\n+\tgit log --graph --oneline -L:func2:file.c >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \ngitgitgadget\n\n"},{"id":"504022","messageId":"94d2c034b4a82ca1024b2a591aef990688f95862.1727956724.git.gitgitgadget@gmail.com","threadId":"62240","inReplyTo":"pull.1806.v2.git.1727956724.gitgitgadget@gmail.com","subject":"[PATCH v2 2/3] line-log: remove output_prefix()","fromName":"Jeff King via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-10-03T11:58:43Z","receivedAt":"2024-10-03T11:58:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"From: Jeff King <peff@peff.net>\n\nNow that output_prefix() returns a const char * type, it matches the\nbehavior of diff_line_prefix() and no longer needs to exist on its own.\n\nSigned-off-by: Jeff King <peff@peff.net>\nSigned-off-by: Derrick Stolee <stolee@gmail.com>\n---\n line-log.c | 14 ++------------\n 1 file changed, 2 insertions(+), 12 deletions(-)\n\ndiff --git a/line-log.c b/line-log.c\nindex 29cf66bdd10..63945c47299 100644\n--- a/line-log.c\n+++ b/line-log.c\n@@ -897,16 +897,6 @@ static void print_line(const char *prefix, char first,\n \t\tfputs(\"\\\\ No newline at end of file\\n\", file);\n }\n \n-static const char *output_prefix(struct diff_options *opt)\n-{\n-\tif (opt->output_prefix) {\n-\t\tstruct strbuf *sb = opt->output_prefix(opt, opt->output_prefix_data);\n-\t\treturn sb->buf;\n-\t} else {\n-\t\treturn \"\";\n-\t}\n-}\n-\n static void dump_diff_hacky_one(struct rev_info *rev, struct line_log_data *range)\n {\n \tunsigned int i, j = 0;\n@@ -916,7 +906,7 @@ static void dump_diff_hacky_one(struct rev_info *rev, struct line_log_data *rang\n \tstruct diff_ranges *diff = &range->diff;\n \n \tstruct diff_options *opt = &rev->diffopt;\n-\tconst char *prefix = output_prefix(opt);\n+\tconst char *prefix = diff_line_prefix(opt);\n \tconst char *c_reset = diff_get_color(opt->use_color, DIFF_RESET);\n \tconst char *c_frag = diff_get_color(opt->use_color, DIFF_FRAGINFO);\n \tconst char *c_meta = diff_get_color(opt->use_color, DIFF_METAINFO);\n@@ -1011,7 +1001,7 @@ out:\n  */\n static void dump_diff_hacky(struct rev_info *rev, struct line_log_data *range)\n {\n-\tconst char *prefix = output_prefix(&rev->diffopt);\n+\tconst char *prefix = diff_line_prefix(&rev->diffopt);\n \n \tfprintf(rev->diffopt.file, \"%s\\n\", prefix);\n \n-- \ngitgitgadget\n\n"},{"id":"504023","messageId":"e1d825ad212d91505eee9d911abbd3ba6bc170b1.1727956724.git.gitgitgadget@gmail.com","threadId":"62240","inReplyTo":"pull.1806.v2.git.1727956724.gitgitgadget@gmail.com","subject":"[PATCH v2 3/3] diff: modify output_prefix function pointer","fromName":"Jeff King via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-10-03T11:58:44Z","receivedAt":"2024-10-03T11:58:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"From: Jeff King <peff@peff.net>\n\nThe uses of the output_prefix function pointer in the diff_options\nstruct is currently difficult to work with by returning a pointer to a\nstrbuf. There is only one use that cares about the length of the string,\nwhich appears to be the only justification of the return type.\n\nWe already noticed confusing memory issues around this return type, so\nuse a const char * return type to make it clear that the caller does not\nown this string buffer.\n\nSigned-off-by: Jeff King <peff@peff.net>\nSigned-off-by: Derrick Stolee <stolee@gmail.com>\n---\n diff-lib.c   | 4 ++--\n diff.c       | 8 +++-----\n diff.h       | 2 +-\n graph.c      | 4 ++--\n log-tree.c   | 4 ++--\n range-diff.c | 4 ++--\n 6 files changed, 12 insertions(+), 14 deletions(-)\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex a680768ee75..6b14b959629 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -701,7 +701,7 @@ int index_differs_from(struct repository *r,\n \treturn (has_changes != 0);\n }\n \n-static struct strbuf *idiff_prefix_cb(struct diff_options *opt UNUSED, void *data)\n+static const char *idiff_prefix_cb(struct diff_options *opt UNUSED, void *data)\n {\n \treturn data;\n }\n@@ -716,7 +716,7 @@ void show_interdiff(const struct object_id *oid1, const struct object_id *oid2,\n \topts.output_format = DIFF_FORMAT_PATCH;\n \topts.output_prefix = idiff_prefix_cb;\n \tstrbuf_addchars(&prefix, ' ', indent);\n-\topts.output_prefix_data = &prefix;\n+\topts.output_prefix_data = prefix.buf;\n \tdiff_setup_done(&opts);\n \n \tdiff_tree_oid(oid1, oid2, \"\", &opts);\ndiff --git a/diff.c b/diff.c\nindex 84a6bb08681..82a9545afbd 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -2315,12 +2315,10 @@ const char *diff_get_color(int diff_use_color, enum color_diff ix)\n \n const char *diff_line_prefix(struct diff_options *opt)\n {\n-\tstruct strbuf *msgbuf;\n-\tif (!opt->output_prefix)\n-\t\treturn \"\";\n+\tif (opt->output_prefix)\n+\t\treturn opt->output_prefix(opt, opt->output_prefix_data);\n \n-\tmsgbuf = opt->output_prefix(opt, opt->output_prefix_data);\n-\treturn msgbuf->buf;\n+\treturn \"\";\n }\n \n static unsigned long sane_truncate_line(char *line, unsigned long len)\ndiff --git a/diff.h b/diff.h\nindex fb40c6e6d60..978450e20ac 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -94,7 +94,7 @@ typedef void (*add_remove_fn_t)(struct diff_options *options,\n typedef void (*diff_format_fn_t)(struct diff_queue_struct *q,\n \t\tstruct diff_options *options, void *data);\n \n-typedef struct strbuf *(*diff_prefix_fn_t)(struct diff_options *opt, void *data);\n+typedef const char *(*diff_prefix_fn_t)(struct diff_options *opt, void *data);\n \n #define DIFF_FORMAT_RAW\t\t0x0001\n #define DIFF_FORMAT_DIFFSTAT\t0x0002\ndiff --git a/graph.c b/graph.c\nindex 091c14cf4fb..ebb7d1e66f4 100644\n--- a/graph.c\n+++ b/graph.c\n@@ -314,7 +314,7 @@ struct git_graph {\n \tunsigned short default_column_color;\n };\n \n-static struct strbuf *diff_output_prefix_callback(struct diff_options *opt, void *data)\n+static const char *diff_output_prefix_callback(struct diff_options *opt, void *data)\n {\n \tstruct git_graph *graph = data;\n \tstatic struct strbuf msgbuf = STRBUF_INIT;\n@@ -327,7 +327,7 @@ static struct strbuf *diff_output_prefix_callback(struct diff_options *opt, void\n \t\t\t   opt->line_prefix_length);\n \tif (graph)\n \t\tgraph_padding_line(graph, &msgbuf);\n-\treturn &msgbuf;\n+\treturn msgbuf.buf;\n }\n \n static const struct diff_options *default_diffopt;\ndiff --git a/log-tree.c b/log-tree.c\nindex 3758e0d3b8e..3af34b91a59 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -923,10 +923,10 @@ int log_tree_diff_flush(struct rev_info *opt)\n \t\t\t */\n \t\t\tint pch = DIFF_FORMAT_DIFFSTAT | DIFF_FORMAT_PATCH;\n \t\t\tif (opt->diffopt.output_prefix) {\n-\t\t\t\tstruct strbuf *msg = NULL;\n+\t\t\t\tconst char *msg;\n \t\t\t\tmsg = opt->diffopt.output_prefix(&opt->diffopt,\n \t\t\t\t\topt->diffopt.output_prefix_data);\n-\t\t\t\tfwrite(msg->buf, msg->len, 1, opt->diffopt.file);\n+\t\t\t\tfwrite(msg, strlen(msg), 1, opt->diffopt.file);\n \t\t\t}\n \n \t\t\t/*\ndiff --git a/range-diff.c b/range-diff.c\nindex bbb0952264b..10885ba3013 100644\n--- a/range-diff.c\n+++ b/range-diff.c\n@@ -480,7 +480,7 @@ static void patch_diff(const char *a, const char *b,\n \tdiff_flush(diffopt);\n }\n \n-static struct strbuf *output_prefix_cb(struct diff_options *opt UNUSED, void *data)\n+static const char *output_prefix_cb(struct diff_options *opt UNUSED, void *data)\n {\n \treturn data;\n }\n@@ -508,7 +508,7 @@ static void output(struct string_list *a, struct string_list *b,\n \topts.flags.suppress_hunk_header_line_count = 1;\n \topts.output_prefix = output_prefix_cb;\n \tstrbuf_addstr(&indent, \"    \");\n-\topts.output_prefix_data = &indent;\n+\topts.output_prefix_data = indent.buf;\n \tdiff_setup_done(&opts);\n \n \t/*\n-- \ngitgitgadget\n"},{"id":"504025","messageId":"cf5fea7a-c711-4d35-9c6e-fe96778a3f85@gmail.com","threadId":"62240","inReplyTo":"20241003061136.GA351404@coredump.intra.peff.net","subject":"Re: [PATCH] line-log: protect inner strbuf from free","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2024-10-03T12:23:22Z","receivedAt":"2024-10-03T12:23:25Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 10/3/24 2:11 AM, Jeff King wrote:\n> On Wed, Oct 02, 2024 at 10:36:33PM -0400, Derrick Stolee wrote:\n\n>>> I do think this would have been a harder mistake to make if the callback\n>>> simply returned a \"const char *\" pointer. We'd lose the ability to show\n>>> prefixes with embedded NULs, but I'm not sure that's a big deal. It\n>>> would also help for line-log to use the existing helper rather than\n>>> inventing its own. So together on top something like this (which I'd\n>>> probably turn into two patches if this seems to others like it's\n>>> valuable and not just churn):\n>>\n>> I do agree that changing the return type will make this easier to prevent\n>> and the code should be easier to read as well.\n>>\n>> Your diffed patch looks pretty good. I made an attempt at guessing where\n>> you would have split them (first remove the duplicate method, then change\n>> the method prototype and callers).\n> \n> Yep, exactly. I actually ended up with a third patch which is a nearby\n> cleanup. I'll hold them back for now, though. Your patch is a regression\n> fix which we should prioritize (though it sounds like it is in 2.46, not\n> the upcoming 2.47?). I'll post my on top as a separate series.\n> \n>> I even took some time to attempt to remove the static strbuf from\n>> diff_output_prefix_callback() in favor of using the 'data' member of the\n>> diff_options struct, but it was not incredibly obvious how to communicate\n>> ownership of the struct which would need to store both the graph struct\n>> and the strbuf. Perhaps this would be good for #leftoverbits.\n> \n> Yeah, I think probably \"struct git_graph\" would need to own the buffer,\n> initialize it in graph_init(), and then discard it in graph_clear().\n> But that gets weird because apparently you can set the callback without\n> a git_graph? Looks like that is triggered by \"--line-prefix\" without\n> \"--graph\". Yuck.\n> \n> But in that case we are just showing the line_prefix string, so we could\n> return that directly. Something like the patch below.\n> \n> The whole thing feels a bit over-engineered with the callback. The graph\n> code is the only one that needs anything beyond a static string. And the\n> way --line-prefix interacts with it is odd, since some callers override\n> the callback (e.g., \"range-diff --line-prefix=foo\" is accepted, but\n> doesn't do anything). I don't think there's a bug anybody cares about,\n> but, well...it's not how I would have written it. ;)\n\nThanks for exploring this with the diff you sent. It's subtle, but you\ndid a good job of recognizing that when the callback is used without\na 'graph' value, it returns opt->line_prefix.\n\n> +\tif (!graph)\n> +\t\treturn opt->line_prefix;\n\nI was wondering why there was no need to edit graph_setup_line_prefix()\nin your diff, and this explains why. Your change is simple enough to\nhandle that change while being robust to a future assignment of the\ncallback data.\n\nFor what it's worth, I attempted creating a custom callback with a\nBUG() statement in it and the only test failure was t4013-diff-various.sh.\nThat test is very non-standard, and it did not recognize the process\nfailure, only the output difference. #leftoverbits could make that test\nfail on process failure.\n\nThanks,\n-Stolee\n\n"},{"id":"504031","messageId":"xmqqsetdi24w.fsf@gitster.g","threadId":"62240","inReplyTo":"e1d825ad212d91505eee9d911abbd3ba6bc170b1.1727956724.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 3/3] diff: modify output_prefix function pointer","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-10-03T16:26:23Z","receivedAt":"2024-10-03T16:26:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Jeff King via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Jeff King <peff@peff.net>\n>\n> The uses of the output_prefix function pointer in the diff_options\n> struct is currently difficult to work with by returning a pointer to a\n> strbuf. There is only one use that cares about the length of the string,\n> which appears to be the only justification of the return type.\n>\n> We already noticed confusing memory issues around this return type, so\n> use a const char * return type to make it clear that the caller does not\n> own this string buffer.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> Signed-off-by: Derrick Stolee <stolee@gmail.com>\n> ---\n>  diff-lib.c   | 4 ++--\n>  diff.c       | 8 +++-----\n>  diff.h       | 2 +-\n>  graph.c      | 4 ++--\n>  log-tree.c   | 4 ++--\n>  range-diff.c | 4 ++--\n>  6 files changed, 12 insertions(+), 14 deletions(-)\n\nVery nice.\n\n>  \t\t\tif (opt->diffopt.output_prefix) {\n> -\t\t\t\tstruct strbuf *msg = NULL;\n> +\t\t\t\tconst char *msg;\n>  \t\t\t\tmsg = opt->diffopt.output_prefix(&opt->diffopt,\n>  \t\t\t\t\topt->diffopt.output_prefix_data);\n> -\t\t\t\tfwrite(msg->buf, msg->len, 1, opt->diffopt.file);\n> +\t\t\t\tfwrite(msg, strlen(msg), 1, opt->diffopt.file);\n>  \t\t\t}\n\nOK.  We are not relying on the strbuf being able to have embedded\nNUL in the buffer, and this looks very sensible.\n\nThanks.\n"},{"id":"504046","messageId":"20241003210257.GA11180@coredump.intra.peff.net","threadId":"62240","inReplyTo":"cf5fea7a-c711-4d35-9c6e-fe96778a3f85@gmail.com","subject":"Re: [PATCH] line-log: protect inner strbuf from free","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-10-03T21:02:57Z","receivedAt":"2024-10-03T21:03:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 03, 2024 at 08:23:22AM -0400, Derrick Stolee wrote:\n\n> > The whole thing feels a bit over-engineered with the callback. The graph\n> > code is the only one that needs anything beyond a static string. And the\n> > way --line-prefix interacts with it is odd, since some callers override\n> > the callback (e.g., \"range-diff --line-prefix=foo\" is accepted, but\n> > doesn't do anything). I don't think there's a bug anybody cares about,\n> > but, well...it's not how I would have written it. ;)\n> \n> Thanks for exploring this with the diff you sent. It's subtle, but you\n> did a good job of recognizing that when the callback is used without\n> a 'graph' value, it returns opt->line_prefix.\n> \n> > +\tif (!graph)\n> > +\t\treturn opt->line_prefix;\n> \n> I was wondering why there was no need to edit graph_setup_line_prefix()\n> in your diff, and this explains why. Your change is simple enough to\n> handle that change while being robust to a future assignment of the\n> callback data.\n\nYeah. I actually ended up pulling that into its own patch because it's\nsufficiently subtle. I'll send that along in a minute.\n\n-Peff\n"},{"id":"504047","messageId":"20241003210548.GB11180@coredump.intra.peff.net","threadId":"62240","inReplyTo":"pull.1806.v2.git.1727956724.gitgitgadget@gmail.com","subject":"[PATCH 0/5] diff output_prefix cleanups","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-10-03T21:05:48Z","receivedAt":"2024-10-03T21:05:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 03, 2024 at 11:58:41AM +0000, Derrick Stolee via GitGitGadget wrote:\n\n> This fixes a regression introduced in 2.46.0.\n> \n> The change made in 394affd46d (line-log: always allocate the output prefix,\n> 2024-06-07) made the method more consistent in that it did not return a\n> static empty string that would fail to free(), but it does lead to\n> double-frees when a strbuf buffer is freed multiple times.\n> \n> In v2, I add Peff's test to the first patch. I also split his diff into two\n> more follow-up patches because the additional clarity seems valuable to me.\n> I have forged his sign-off in all three patches.\n> \n> Note to the maintainer: feel free to take only the first patch, as Peff\n> replied that he may work on the remaining cleanup independently (but I had\n> already prepared patches 2 & 3).\n\nOh, I wasn't expecting you to go to that trouble, and had already\npolished them up myself. :)\n\nSo certainly your patch 1 looks good to me now. Here's what I would\nput on top (but I would suggest making it a separate branch, since yours\nis a fairly urgent fix and mine is all cleanup).\n\n  [1/5]: line-log: use diff_line_prefix() instead of custom helper\n  [2/5]: diff: drop line_prefix_length field\n  [3/5]: diff: return const char from output_prefix callback\n  [4/5]: diff: return line_prefix directly when possible\n  [5/5]: diff: store graph prefix buf in git_graph struct\n\n diff-lib.c   |  4 ++--\n diff.c       | 10 +++-------\n diff.h       |  3 +--\n graph.c      | 29 +++++++++++++++++------------\n line-log.c   | 14 ++------------\n log-tree.c   |  7 +------\n range-diff.c |  4 ++--\n 7 files changed, 28 insertions(+), 43 deletions(-)\n\n-Peff\n"},{"id":"504048","messageId":"20241003210627.GA11328@coredump.intra.peff.net","threadId":"62240","inReplyTo":"20241003210548.GB11180@coredump.intra.peff.net","subject":"[PATCH 1/5] line-log: use diff_line_prefix() instead of custom helper","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-10-03T21:06:27Z","receivedAt":"2024-10-03T21:06:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Our local output_prefix() is exactly the same as the public\ndiff_line_prefix() function. Let's just use that one, saving us a little\nbit of code.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n line-log.c | 14 ++------------\n 1 file changed, 2 insertions(+), 12 deletions(-)\n\ndiff --git a/line-log.c b/line-log.c\nindex 29cf66bdd1..63945c4729 100644\n--- a/line-log.c\n+++ b/line-log.c\n@@ -897,16 +897,6 @@ static void print_line(const char *prefix, char first,\n \t\tfputs(\"\\\\ No newline at end of file\\n\", file);\n }\n \n-static const char *output_prefix(struct diff_options *opt)\n-{\n-\tif (opt->output_prefix) {\n-\t\tstruct strbuf *sb = opt->output_prefix(opt, opt->output_prefix_data);\n-\t\treturn sb->buf;\n-\t} else {\n-\t\treturn \"\";\n-\t}\n-}\n-\n static void dump_diff_hacky_one(struct rev_info *rev, struct line_log_data *range)\n {\n \tunsigned int i, j = 0;\n@@ -916,7 +906,7 @@ static void dump_diff_hacky_one(struct rev_info *rev, struct line_log_data *rang\n \tstruct diff_ranges *diff = &range->diff;\n \n \tstruct diff_options *opt = &rev->diffopt;\n-\tconst char *prefix = output_prefix(opt);\n+\tconst char *prefix = diff_line_prefix(opt);\n \tconst char *c_reset = diff_get_color(opt->use_color, DIFF_RESET);\n \tconst char *c_frag = diff_get_color(opt->use_color, DIFF_FRAGINFO);\n \tconst char *c_meta = diff_get_color(opt->use_color, DIFF_METAINFO);\n@@ -1011,7 +1001,7 @@ static void dump_diff_hacky_one(struct rev_info *rev, struct line_log_data *rang\n  */\n static void dump_diff_hacky(struct rev_info *rev, struct line_log_data *range)\n {\n-\tconst char *prefix = output_prefix(&rev->diffopt);\n+\tconst char *prefix = diff_line_prefix(&rev->diffopt);\n \n \tfprintf(rev->diffopt.file, \"%s\\n\", prefix);\n \n-- \n2.47.0.rc1.384.g9f398d04fd\n\n"},{"id":"504049","messageId":"20241003210654.GB11328@coredump.intra.peff.net","threadId":"62240","inReplyTo":"20241003210548.GB11180@coredump.intra.peff.net","subject":"[PATCH 2/5] diff: drop line_prefix_length field","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-10-03T21:06:54Z","receivedAt":"2024-10-03T21:06:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The diff_options structure holds a line_prefix string and an associated\nlength. But the length is always just the strlen() of the NUL-terminated\nstring. Let's simplify the code by just storing the string pointer and\nassuming it is NUL-terminated when we use it.\n\nThis will cause us to compute the string length in a few extra spots,\nbut I don't think any of these are particularly hot code paths.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n diff.c  | 1 -\n diff.h  | 1 -\n graph.c | 8 ++------\n 3 files changed, 2 insertions(+), 8 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 173cbe2bed..858e001993 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -5400,7 +5400,6 @@ static int diff_opt_line_prefix(const struct option *opt,\n \n \tBUG_ON_OPT_NEG(unset);\n \toptions->line_prefix = optarg;\n-\toptions->line_prefix_length = strlen(options->line_prefix);\n \tgraph_setup_line_prefix(options);\n \treturn 0;\n }\ndiff --git a/diff.h b/diff.h\nindex 0cde3b34e2..ea3634106d 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -274,7 +274,6 @@ struct diff_options {\n \tconst char *single_follow;\n \tconst char *a_prefix, *b_prefix;\n \tconst char *line_prefix;\n-\tsize_t line_prefix_length;\n \n \t/**\n \t * collection of boolean options that affects the operation, but some do\ndiff --git a/graph.c b/graph.c\nindex 091c14cf4f..2cee47294f 100644\n--- a/graph.c\n+++ b/graph.c\n@@ -76,10 +76,7 @@ static void graph_show_line_prefix(const struct diff_options *diffopt)\n \tif (!diffopt || !diffopt->line_prefix)\n \t\treturn;\n \n-\tfwrite(diffopt->line_prefix,\n-\t       sizeof(char),\n-\t       diffopt->line_prefix_length,\n-\t       diffopt->file);\n+\tfputs(diffopt->line_prefix, diffopt->file);\n }\n \n static const char **column_colors;\n@@ -323,8 +320,7 @@ static struct strbuf *diff_output_prefix_callback(struct diff_options *opt, void\n \n \tstrbuf_reset(&msgbuf);\n \tif (opt->line_prefix)\n-\t\tstrbuf_add(&msgbuf, opt->line_prefix,\n-\t\t\t   opt->line_prefix_length);\n+\t\tstrbuf_addstr(&msgbuf, opt->line_prefix);\n \tif (graph)\n \t\tgraph_padding_line(graph, &msgbuf);\n \treturn &msgbuf;\n-- \n2.47.0.rc1.384.g9f398d04fd\n\n"},{"id":"504050","messageId":"20241003210924.GC11328@coredump.intra.peff.net","threadId":"62240","inReplyTo":"20241003210548.GB11180@coredump.intra.peff.net","subject":"[PATCH 3/5] diff: return const char from output_prefix callback","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-10-03T21:09:24Z","receivedAt":"2024-10-03T21:09:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The diff_options structure has an output_prefix callback for returning a\nprefix string, but it does so by returning a pointer to a strbuf.\n\nThis makes the interface awkward. There's no reason the callback should\nneed to use a strbuf, and it creates questions about whether the\nownership of the resulting buffer should be transferred to the caller\n(it should not be, but a recent attempt to clean up this code led to a\ndouble-free in some cases).\n\nThe one advantage we get is that the strbuf contains a ptr/len pair, so\nwe could in theory have a prefix with embedded NULs. But we can observe\nthat none of the existing callbacks would ever produce such a NUL (they\nare usually just indentation or graph symbols, and even the\n\"--line-prefix\" option takes a NUL-terminated string).\n\nAnd anyway, only one caller (the one in log_tree_diff_flush) actually\nlooks at the strbuf length. In every other case we use a helper function\nwhich discards the length and just returns the NUL-terminated string.\n\nSo let's just have the callback return a \"const char *\" pointer. It's up\nto the callbacks themselves if they want to use a strbuf under the hood.\nAnd now the caller in log_tree_diff_flush() can just use the helper\nfunction along with everybody else. That lets us even simplify out the\nfunction pointer check, since the helper returns an empty string\n(technically this does mean we'll sometimes issue an empty fputs() call,\nbut I don't think this code path is hot enough to care about that).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n diff-lib.c   | 4 ++--\n diff.c       | 9 +++------\n diff.h       | 2 +-\n graph.c      | 4 ++--\n log-tree.c   | 7 +------\n range-diff.c | 4 ++--\n 6 files changed, 11 insertions(+), 19 deletions(-)\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex a680768ee7..6b14b95962 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -701,7 +701,7 @@ int index_differs_from(struct repository *r,\n \treturn (has_changes != 0);\n }\n \n-static struct strbuf *idiff_prefix_cb(struct diff_options *opt UNUSED, void *data)\n+static const char *idiff_prefix_cb(struct diff_options *opt UNUSED, void *data)\n {\n \treturn data;\n }\n@@ -716,7 +716,7 @@ void show_interdiff(const struct object_id *oid1, const struct object_id *oid2,\n \topts.output_format = DIFF_FORMAT_PATCH;\n \topts.output_prefix = idiff_prefix_cb;\n \tstrbuf_addchars(&prefix, ' ', indent);\n-\topts.output_prefix_data = &prefix;\n+\topts.output_prefix_data = prefix.buf;\n \tdiff_setup_done(&opts);\n \n \tdiff_tree_oid(oid1, oid2, \"\", &opts);\ndiff --git a/diff.c b/diff.c\nindex 858e001993..6c96154fed 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -2317,12 +2317,9 @@ const char *diff_get_color(int diff_use_color, enum color_diff ix)\n \n const char *diff_line_prefix(struct diff_options *opt)\n {\n-\tstruct strbuf *msgbuf;\n-\tif (!opt->output_prefix)\n-\t\treturn \"\";\n-\n-\tmsgbuf = opt->output_prefix(opt, opt->output_prefix_data);\n-\treturn msgbuf->buf;\n+\treturn opt->output_prefix ?\n+\t\topt->output_prefix(opt, opt->output_prefix_data) :\n+\t\t\"\";\n }\n \n static unsigned long sane_truncate_line(char *line, unsigned long len)\ndiff --git a/diff.h b/diff.h\nindex ea3634106d..5c8de79535 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -94,7 +94,7 @@ typedef void (*add_remove_fn_t)(struct diff_options *options,\n typedef void (*diff_format_fn_t)(struct diff_queue_struct *q,\n \t\tstruct diff_options *options, void *data);\n \n-typedef struct strbuf *(*diff_prefix_fn_t)(struct diff_options *opt, void *data);\n+typedef const char *(*diff_prefix_fn_t)(struct diff_options *opt, void *data);\n \n #define DIFF_FORMAT_RAW\t\t0x0001\n #define DIFF_FORMAT_DIFFSTAT\t0x0002\ndiff --git a/graph.c b/graph.c\nindex 2cee47294f..c046f6285d 100644\n--- a/graph.c\n+++ b/graph.c\n@@ -311,7 +311,7 @@ struct git_graph {\n \tunsigned short default_column_color;\n };\n \n-static struct strbuf *diff_output_prefix_callback(struct diff_options *opt, void *data)\n+static const char *diff_output_prefix_callback(struct diff_options *opt, void *data)\n {\n \tstruct git_graph *graph = data;\n \tstatic struct strbuf msgbuf = STRBUF_INIT;\n@@ -323,7 +323,7 @@ static struct strbuf *diff_output_prefix_callback(struct diff_options *opt, void\n \t\tstrbuf_addstr(&msgbuf, opt->line_prefix);\n \tif (graph)\n \t\tgraph_padding_line(graph, &msgbuf);\n-\treturn &msgbuf;\n+\treturn msgbuf.buf;\n }\n \n static const struct diff_options *default_diffopt;\ndiff --git a/log-tree.c b/log-tree.c\nindex 3758e0d3b8..33eb96b50c 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -922,12 +922,7 @@ int log_tree_diff_flush(struct rev_info *opt)\n \t\t\t * diff/diffstat output for readability.\n \t\t\t */\n \t\t\tint pch = DIFF_FORMAT_DIFFSTAT | DIFF_FORMAT_PATCH;\n-\t\t\tif (opt->diffopt.output_prefix) {\n-\t\t\t\tstruct strbuf *msg = NULL;\n-\t\t\t\tmsg = opt->diffopt.output_prefix(&opt->diffopt,\n-\t\t\t\t\topt->diffopt.output_prefix_data);\n-\t\t\t\tfwrite(msg->buf, msg->len, 1, opt->diffopt.file);\n-\t\t\t}\n+\t\t\tfputs(diff_line_prefix(&opt->diffopt), opt->diffopt.file);\n \n \t\t\t/*\n \t\t\t * We may have shown three-dashes line early\ndiff --git a/range-diff.c b/range-diff.c\nindex bbb0952264..10885ba301 100644\n--- a/range-diff.c\n+++ b/range-diff.c\n@@ -480,7 +480,7 @@ static void patch_diff(const char *a, const char *b,\n \tdiff_flush(diffopt);\n }\n \n-static struct strbuf *output_prefix_cb(struct diff_options *opt UNUSED, void *data)\n+static const char *output_prefix_cb(struct diff_options *opt UNUSED, void *data)\n {\n \treturn data;\n }\n@@ -508,7 +508,7 @@ static void output(struct string_list *a, struct string_list *b,\n \topts.flags.suppress_hunk_header_line_count = 1;\n \topts.output_prefix = output_prefix_cb;\n \tstrbuf_addstr(&indent, \"    \");\n-\topts.output_prefix_data = &indent;\n+\topts.output_prefix_data = indent.buf;\n \tdiff_setup_done(&opts);\n \n \t/*\n-- \n2.47.0.rc1.384.g9f398d04fd\n\n"},{"id":"504052","messageId":"20241003211131.GD11328@coredump.intra.peff.net","threadId":"62240","inReplyTo":"20241003210548.GB11180@coredump.intra.peff.net","subject":"[PATCH 4/5] diff: return line_prefix directly when possible","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-10-03T21:11:31Z","receivedAt":"2024-10-03T21:11:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We may point our output_prefix callback to diff_output_prefix_callback()\nin any of these cases:\n\n  1. we have a user-provided line_prefix\n\n  2. we have a graph prefix to show\n\n  3. both (1) and (2)\n\nThe function combines the available elements into a strbuf and returns\nits pointer.\n\nIn the case that we just have the line_prefix, though, there is no need\nfor the strbuf. We can return the string directly.\n\nThis is a minor optimization by itself, but also will allow us to clean\nup some memory ownership issues on top.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThe old code did handle a case \"0\" where neither is set. But in that\ncase we would never set up the callback in the first place. So I didn't\nthink it was worth being defensive there.\n\n graph.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/graph.c b/graph.c\nindex c046f6285d..0d200ca77f 100644\n--- a/graph.c\n+++ b/graph.c\n@@ -318,6 +318,9 @@ static const char *diff_output_prefix_callback(struct diff_options *opt, void *d\n \n \tassert(opt);\n \n+\tif (!graph)\n+\t\treturn opt->line_prefix;\n+\n \tstrbuf_reset(&msgbuf);\n \tif (opt->line_prefix)\n \t\tstrbuf_addstr(&msgbuf, opt->line_prefix);\n-- \n2.47.0.rc1.384.g9f398d04fd\n\n"},{"id":"504053","messageId":"20241003211317.GE11328@coredump.intra.peff.net","threadId":"62240","inReplyTo":"20241003210548.GB11180@coredump.intra.peff.net","subject":"[PATCH 5/5] diff: store graph prefix buf in git_graph struct","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-10-03T21:13:17Z","receivedAt":"2024-10-03T21:13:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The diffopt output_prefix interface makes it the callback's job to\nhandle ownership of the memory it returns, keeping it valid while\ncallers are using it and then eventually freeing it when we are done\ndiffing.\n\nIn diff_output_prefix_callback() we handle this with a static strbuf,\neffectively \"leaking\" it when the diff is done (but not triggering any\nleak detectors because it's technically still reachable). This has not\nbeen a big problem in practice, but it is a problem for libification:\ntwo diffs running in the same process could stomp on each other's prefix\nbuffers.\n\nSince we only need the strbuf when we are formatting graph padding, we\ncan give ownership of the strbuf to the git_graph struct, letting us\nfree it when that struct is no longer in use.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n graph.c | 18 ++++++++++++------\n 1 file changed, 12 insertions(+), 6 deletions(-)\n\ndiff --git a/graph.c b/graph.c\nindex 0d200ca77f..bf000fdbe1 100644\n--- a/graph.c\n+++ b/graph.c\n@@ -309,24 +309,28 @@ struct git_graph {\n \t * stored as an index into the array column_colors.\n \t */\n \tunsigned short default_column_color;\n+\n+\t/*\n+\t * Scratch buffer for generating prefixes to be used with\n+\t * diff_output_prefix_callback().\n+\t */\n+\tstruct strbuf prefix_buf;\n };\n \n static const char *diff_output_prefix_callback(struct diff_options *opt, void *data)\n {\n \tstruct git_graph *graph = data;\n-\tstatic struct strbuf msgbuf = STRBUF_INIT;\n \n \tassert(opt);\n \n \tif (!graph)\n \t\treturn opt->line_prefix;\n \n-\tstrbuf_reset(&msgbuf);\n+\tstrbuf_reset(&graph->prefix_buf);\n \tif (opt->line_prefix)\n-\t\tstrbuf_addstr(&msgbuf, opt->line_prefix);\n-\tif (graph)\n-\t\tgraph_padding_line(graph, &msgbuf);\n-\treturn msgbuf.buf;\n+\t\tstrbuf_addstr(&graph->prefix_buf, opt->line_prefix);\n+\tgraph_padding_line(graph, &graph->prefix_buf);\n+\treturn graph->prefix_buf.buf;\n }\n \n static const struct diff_options *default_diffopt;\n@@ -396,6 +400,7 @@ struct git_graph *graph_init(struct rev_info *opt)\n \t * The diff output prefix callback, with this we can make\n \t * all the diff output to align with the graph lines.\n \t */\n+\tstrbuf_init(&graph->prefix_buf, 0);\n \topt->diffopt.output_prefix = diff_output_prefix_callback;\n \topt->diffopt.output_prefix_data = graph;\n \n@@ -411,6 +416,7 @@ void graph_clear(struct git_graph *graph)\n \tfree(graph->new_columns);\n \tfree(graph->mapping);\n \tfree(graph->old_mapping);\n+\tstrbuf_release(&graph->prefix_buf);\n \tfree(graph);\n }\n \n-- \n2.47.0.rc1.384.g9f398d04fd\n"},{"id":"504062","messageId":"xmqq7caohhw3.fsf@gitster.g","threadId":"62240","inReplyTo":"20241003211317.GE11328@coredump.intra.peff.net","subject":"Re: [PATCH 5/5] diff: store graph prefix buf in git_graph struct","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-10-03T23:43:40Z","receivedAt":"2024-10-03T23:43:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Since we only need the strbuf when we are formatting graph padding, we\n> can give ownership of the strbuf to the git_graph struct, letting us\n> free it when that struct is no longer in use.\n>\n>  static const char *diff_output_prefix_callback(struct diff_options *opt, void *data)\n>  {\n>  \tstruct git_graph *graph = data;\n> -\tstatic struct strbuf msgbuf = STRBUF_INIT;\n>  \n>  \tassert(opt);\n>  \n>  \tif (!graph)\n>  \t\treturn opt->line_prefix;\n>  \n> -\tstrbuf_reset(&msgbuf);\n\nOooh, I love this change.  The fewer file scope statics (or global\nstates in general) we have, the better ;-).\n\n"},{"id":"504069","messageId":"Zv9v7THK8jlH9exj@pks.im","threadId":"62240","inReplyTo":"xmqq7caohhw3.fsf@gitster.g","subject":"Re: [PATCH 5/5] diff: store graph prefix buf in git_graph struct","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-10-04T04:32:45Z","receivedAt":"2024-10-04T04:32:51Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Oct 03, 2024 at 04:43:40PM -0700, Junio C Hamano wrote:\n> Jeff King <peff@peff.net> writes:\n> \n> > Since we only need the strbuf when we are formatting graph padding, we\n> > can give ownership of the strbuf to the git_graph struct, letting us\n> > free it when that struct is no longer in use.\n> >\n> >  static const char *diff_output_prefix_callback(struct diff_options *opt, void *data)\n> >  {\n> >  \tstruct git_graph *graph = data;\n> > -\tstatic struct strbuf msgbuf = STRBUF_INIT;\n> >  \n> >  \tassert(opt);\n> >  \n> >  \tif (!graph)\n> >  \t\treturn opt->line_prefix;\n> >  \n> > -\tstrbuf_reset(&msgbuf);\n> \n> Oooh, I love this change.  The fewer file scope statics (or global\n> states in general) we have, the better ;-).\n\nTrue, thanks for going the extra mile here! The other patches look good\nto me, as well.\n\nPatrick\n"},{"id":"504070","messageId":"Zv9v8ZKIgQpxpuVv@pks.im","threadId":"62240","inReplyTo":"05c21616c350b5141c17fde1aa5d3aea881c6031.1727956724.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/3] line-log: protect inner strbuf from free","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-10-04T04:32:49Z","receivedAt":"2024-10-04T04:32:54Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Oct 03, 2024 at 11:58:42AM +0000, Derrick Stolee via GitGitGadget wrote:\n> From: Derrick Stolee <stolee@gmail.com>\n> \n> The output_prefix() method in line-log.c may call a function pointer via\n> the diff_options struct. This function pointer returns a strbuf struct\n> and then its buffer is passed back. However, that implies that the\n> consumer is responsible to free the string. This is especially true\n> because the default behavior is to duplicate the empty string.\n> \n> The existing functions used in the output_prefix pointer include:\n> \n>  1. idiff_prefix_cb() in diff-lib.c. This returns the data pointer, so\n>     the value exists across multiple calls.\n> \n>  2. diff_output_prefix_callback() in graph.c. This uses a static strbuf\n>     struct, so it reuses buffers across calls. These should not be\n>     freed.\n> \n>  3. output_prefix_cb() in range-diff.c. This is similar to the\n>     diff-lib.c case.\n> \n> In each case, we should not be freeing this buffer. We can convert the\n> output_prefix() function to return a const char pointer and stop freeing\n> the result.\n> \n> This choice is essentially the opposite of what was done in 394affd46d\n> (line-log: always allocate the output prefix, 2024-06-07).\n> \n> This was discovered via 'valgrind' while investigating a public report\n> of a bug in 'git log --graph -L' [1].\n> \n> [1] https://github.com/git-for-windows/git/issues/5185\n> \n> This issue would have been caught by the new test, when Git is compiled\n> with ASan to catch these double frees.\n\nThanks a bunch for fixing this! The change looks good to me.\n\nPatrick\n"},{"id":"504109","messageId":"e38449d8-190a-49af-85b6-a628e14379d6@gmail.com","threadId":"62240","inReplyTo":"20241003210548.GB11180@coredump.intra.peff.net","subject":"Re: [PATCH 0/5] diff output_prefix cleanups","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2024-10-04T19:27:14Z","receivedAt":"2024-10-04T19:27:16Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 10/3/24 5:05 PM, Jeff King wrote:\n> On Thu, Oct 03, 2024 at 11:58:41AM +0000, Derrick Stolee via GitGitGadget wrote:\n\n>> Note to the maintainer: feel free to take only the first patch, as Peff\n>> replied that he may work on the remaining cleanup independently (but I had\n>> already prepared patches 2 & 3).\n> \n> Oh, I wasn't expecting you to go to that trouble, and had already\n> polished them up myself. :)\n\nIt's perfectly fine that we were attempting to save each other work.\n\n> So certainly your patch 1 looks good to me now. Here's what I would\n> put on top (but I would suggest making it a separate branch, since yours\n> is a fairly urgent fix and mine is all cleanup).\n\nI approve of this plan. Please only consider my first patch and drop\nthe others.\n\nThanks,\n-Stolee\n"},{"id":"504110","messageId":"xmqq5xq7ekb8.fsf@gitster.g","threadId":"62240","inReplyTo":"e38449d8-190a-49af-85b6-a628e14379d6@gmail.com","subject":"Re: [PATCH 0/5] diff output_prefix cleanups","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-10-04T19:31:55Z","receivedAt":"2024-10-04T19:31:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee <stolee@gmail.com> writes:\n\n> On 10/3/24 5:05 PM, Jeff King wrote:\n>> On Thu, Oct 03, 2024 at 11:58:41AM +0000, Derrick Stolee via GitGitGadget wrote:\n>\n>>> Note to the maintainer: feel free to take only the first patch, as Peff\n>>> replied that he may work on the remaining cleanup independently (but I had\n>>> already prepared patches 2 & 3).\n>> Oh, I wasn't expecting you to go to that trouble, and had already\n>> polished them up myself. :)\n>\n> It's perfectly fine that we were attempting to save each other work.\n>\n>> So certainly your patch 1 looks good to me now. Here's what I would\n>> put on top (but I would suggest making it a separate branch, since yours\n>> is a fairly urgent fix and mine is all cleanup).\n>\n> I approve of this plan. Please only consider my first patch and drop\n> the others.\n\nYup.  Thanks, both.  The result looked very sensible.\n"},{"id":"504111","messageId":"039920ed-57a8-4c49-8914-5f071a548eca@gmail.com","threadId":"62240","inReplyTo":"20241003210548.GB11180@coredump.intra.peff.net","subject":"Re: [PATCH 0/5] diff output_prefix cleanups","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2024-10-04T19:33:20Z","receivedAt":"2024-10-04T19:33:22Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 10/3/24 5:05 PM, Jeff King wrote:\n > Here's what I would> put on top (but I would suggest making it a separate \nbranch, since yours\n> is a fairly urgent fix and mine is all cleanup).\n> \n>    [1/5]: line-log: use diff_line_prefix() instead of custom helper\n>    [2/5]: diff: drop line_prefix_length field\n>    [3/5]: diff: return const char from output_prefix callback\n>    [4/5]: diff: return line_prefix directly when possible\n>    [5/5]: diff: store graph prefix buf in git_graph struct\n\nI've reviewed these patches and they look good to me. Thanks for\ntaking the time to split them up carefully to help review go so\nsmoothly.\n\n>   7 files changed, 28 insertions(+), 43 deletions(-)\n\nExcellent to see the line reduction, too.\n\nThanks,\n-Stolee\n\n"}]}