{"thread":{"id":"62923","subject":"[PATCH] revision: fix missing null for freed memory","startedAt":"2025-02-08T06:17:14Z","lastAt":"2025-02-13T21:07:35Z","messageCount":12,"participants":["Emily M Klassen","Junio C Hamano","Emily Klassen","Patrick Steinhardt","D. Ben Knoble","Jeff King","Ben Knoble"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"512129","messageId":"20250208061702.88469-1-forivall@gmail.com","threadId":"62923","inReplyTo":null,"subject":"[PATCH] revision: fix missing null for freed memory","fromName":"Emily M Klassen","fromEmail":"forivall@gmail.com","sentAt":"2025-02-08T06:17:02Z","receivedAt":"2025-02-08T06:17:14Z","isPatch":true,"sender":{"key":"forivall@gmail.com","avatar":"https://gravatar.com/avatar/ce462c1645ae87217dd3cd48cbbf0ba7599d95cfcdd482db34ac1f78912a392d?d=mp&s=160"},"body":"\"git log --graph --no-graph\" missed cleaning up the output_prefix and\noutput_prefix_data pointers. This resulted in a segfault when using \"--patch\",\n\"--name-status\" or \"--name-only\", as the output_prefix_data continued to be in\nuse after free()\n\nSigned-off-by: Emily M Klassen <forivall@gmail.com>\n---\nI previously reported this a few hours ago, and ended up digging in and figuring\nit out. I'll make sure to bottom reply in the follow ups to this patch.\n\n revision.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/revision.c b/revision.c\nindex 474fa1e767..84cb028e11 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2615,6 +2615,8 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t\tgraph_clear(revs->graph);\n \t\trevs->graph = graph_init(revs);\n \t} else if (!strcmp(arg, \"--no-graph\")) {\n+\t\trevs->diffopt.output_prefix = NULL;\n+\t\trevs->diffopt.output_prefix_data = NULL;\n \t\tgraph_clear(revs->graph);\n \t\trevs->graph = NULL;\n \t} else if (!strcmp(arg, \"--encode-email-headers\")) {\n-- \n2.48.1\n\n"},{"id":"512142","messageId":"xmqqldugdry6.fsf@gitster.g","threadId":"62923","inReplyTo":"20250208061702.88469-1-forivall@gmail.com","subject":"Re: [PATCH] revision: fix missing null for freed memory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-02-08T21:53:05Z","receivedAt":"2025-02-08T21:53:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Emily M Klassen <forivall@gmail.com> writes:\n\n> \"git log --graph --no-graph\" missed cleaning up the output_prefix and\n> output_prefix_data pointers. This resulted in a segfault when using \"--patch\",\n> \"--name-status\" or \"--name-only\", as the output_prefix_data continued to be in\n> use after free()\n>\n> Signed-off-by: Emily M Klassen <forivall@gmail.com>\n> ---\n> I previously reported this a few hours ago, and ended up digging in and figuring\n> it out. I'll make sure to bottom reply in the follow ups to this patch.\n>\n>  revision.c | 2 ++\n>  1 file changed, 2 insertions(+)\n\nReading the symptom in the proposed log message (which is very\nclearly written, by the way), it seems that this is reproducible?\n\nCan we have a test to make sure that the fix would not be broken\nlater?\n\n> diff --git a/revision.c b/revision.c\n> index 474fa1e767..84cb028e11 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -2615,6 +2615,8 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n>  \t\tgraph_clear(revs->graph);\n>  \t\trevs->graph = graph_init(revs);\n>  \t} else if (!strcmp(arg, \"--no-graph\")) {\n> +\t\trevs->diffopt.output_prefix = NULL;\n> +\t\trevs->diffopt.output_prefix_data = NULL;\n>  \t\tgraph_clear(revs->graph);\n>  \t\trevs->graph = NULL;\n>  \t} else if (!strcmp(arg, \"--encode-email-headers\")) {\n\nInteresting.\n\nIn response to \"--graph\" (the code we can see in the context before\nthis part), we clear the revs->graph and then call graph_init(revs)\nfor ourselves, and we do not have to futz with diffopt at all, and\nit works OK because output_prefix_data and output_prefix would be\noverwritten by the graph_init() to the value we want to use anyway.\n\nBut of course, after \"--no-graph\", nobody clears these two members\nfor us, so we'd need to clear them here.\n\nIt might make the API less error-prone if the \"clear\" function\ncleared the .graph and diffopt->output_prefix{,_data} together\nbut among three existing callers of graph_clear(), only this caller\nneeds to clear these two members, so it probably would not matter.\n\nSo in short, this seems to be a good fix for the immediate issue,\nand it is unlikely that we'd need any follow-up work.\n"},{"id":"512179","messageId":"xmqqtt91dbzt.fsf@gitster.g","threadId":"62923","inReplyTo":"20250208061702.88469-1-forivall@gmail.com","subject":"Re: [PATCH] revision: fix missing null for freed memory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-02-10T16:02:14Z","receivedAt":"2025-02-10T16:02:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Emily M Klassen <forivall@gmail.com> writes:\n\n> Subject: Re: [PATCH] revision: fix missing null for freed memory\n>\n> \"git log --graph --no-graph\" missed cleaning up the output_prefix and\n> output_prefix_data pointers. This resulted in a segfault when using \"--patch\",\n> \"--name-status\" or \"--name-only\", as the output_prefix_data continued to be in\n> use after free()\n\nRereading the title, I cannot make sense out of \"fix missing null\"\nand guess what it wants to say.  Is \"null\" here used as a verb to\nmean \"to assign a NULL to a variable that points at ...\"?\n\n    revision: clear graph callback upon \"--no-graph\"\n\n    \"git log --graph --no-graph\" first populates the .output_prefix\n    member of diffopt, which is a callback function, to compute\n    \"--graph\" header, and then discards the data the callback needs\n    to compute the graph header but forgets to clear .output_prefix\n    pointer in response to \"--no-graph\".  At runtime, we end up\n    calling the function that we should not.\n\n    Clear the member to stop making callback, and for a better\n    hyginene, also clear the pointer pointing at a freed memory.\n\nor something?\n\nOther than that, as I said earlier, the patch looks good.\n\nThanks.\n\n> Signed-off-by: Emily M Klassen <forivall@gmail.com>\n> ---\n> I previously reported this a few hours ago, and ended up digging in and figuring\n> it out. I'll make sure to bottom reply in the follow ups to this patch.\n>\n>  revision.c | 2 ++\n>  1 file changed, 2 insertions(+)\n>\n> diff --git a/revision.c b/revision.c\n> index 474fa1e767..84cb028e11 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -2615,6 +2615,8 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n>  \t\tgraph_clear(revs->graph);\n>  \t\trevs->graph = graph_init(revs);\n>  \t} else if (!strcmp(arg, \"--no-graph\")) {\n> +\t\trevs->diffopt.output_prefix = NULL;\n> +\t\trevs->diffopt.output_prefix_data = NULL;\n>  \t\tgraph_clear(revs->graph);\n>  \t\trevs->graph = NULL;\n>  \t} else if (!strcmp(arg, \"--encode-email-headers\")) {\n"},{"id":"512199","messageId":"CADY4h_o_wfUpjSBhWa9TPU_G-G8qpENpUeOKGQDY8dq6Zb2+qg@mail.gmail.com","threadId":"62923","inReplyTo":"xmqqtt91dbzt.fsf@gitster.g","subject":"Re: [PATCH] revision: fix missing null for freed memory","fromName":"Emily Klassen","fromEmail":"forivall@gmail.com","sentAt":"2025-02-10T20:56:17Z","receivedAt":"2025-02-10T20:56:34Z","isPatch":true,"sender":{"key":"forivall@gmail.com","avatar":"https://gravatar.com/avatar/ce462c1645ae87217dd3cd48cbbf0ba7599d95cfcdd482db34ac1f78912a392d?d=mp&s=160"},"body":"On Mon, Feb 10, 2025 at 8:02 AM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Emily M Klassen <forivall@gmail.com> writes:\n>\n> > Subject: Re: [PATCH] revision: fix missing null for freed memory\n> >\n> > \"git log --graph --no-graph\" missed cleaning up the output_prefix and\n> > output_prefix_data pointers. This resulted in a segfault when using \"--patch\",\n> > \"--name-status\" or \"--name-only\", as the output_prefix_data continued to be in\n> > use after free()\n>\n> Rereading the title, I cannot make sense out of \"fix missing null\"\n> and guess what it wants to say.  Is \"null\" here used as a verb to\n> mean \"to assign a NULL to a variable that points at ...\"?\n\nYeah, this was meant to say something like \"fix missing null assignment after\nfreeing graph data\", and I didn't really have the energy to think of a better\nsummary at the time.\n\n>\n>     revision: clear graph callback upon \"--no-graph\"\n>\n>     \"git log --graph --no-graph\" first populates the .output_prefix\n>     member of diffopt, which is a callback function, to compute\n>     \"--graph\" header, and then discards the data the callback needs\n>     to compute the graph header but forgets to clear .output_prefix\n>     pointer in response to \"--no-graph\".  At runtime, we end up\n>     calling the function that we should not.\n>\n>     Clear the member to stop making callback, and for a better\n>     hyginene, also clear the pointer pointing at a freed memory.\n>\n> or something?\n\nYup, this works well. A small bit of rephrasing for readability:\n\n    revision: clear graph prefix callback upon \"--no-graph\"\n\n    \"git log --graph --no-graph\" misses some cleanup: handling\n    \"--graph\", it assigns the .output_prefix member of diffopt, which\n    is a callback function to compute the graph prefix when displaying\n    a diff. Then, when handling \"--no-graph\" it discards the data the\n    callback needs to compute the graph header but forgets to clear\n    .output_prefix pointer.  At runtime, we  call the function when we\n    should not. It also passes a stale pointer to the data, which leads\n    to a segfault when the callback is used for \"--patch\",\n    \"--name-status\" or \"--name-only\".\n\n    Clear the member to stop the callback from being called, and for\n    hygiene, also clear the pointer pointing at a freed memory.\n\n>\n> Other than that, as I said earlier, the patch looks good.\n>\n> Thanks.\n\nAwesome. I'll also add a test before re-submitting, as mentioned in\nyour other message.\n\nThanks for the feedback!\n\n>\n> > Signed-off-by: Emily M Klassen <forivall@gmail.com>\n> > ---\n> > I previously reported this a few hours ago, and ended up digging in and figuring\n> > it out. I'll make sure to bottom reply in the follow ups to this patch.\n> >\n> >  revision.c | 2 ++\n> >  1 file changed, 2 insertions(+)\n> >\n> > diff --git a/revision.c b/revision.c\n> > index 474fa1e767..84cb028e11 100644\n> > --- a/revision.c\n> > +++ b/revision.c\n> > @@ -2615,6 +2615,8 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n> >               graph_clear(revs->graph);\n> >               revs->graph = graph_init(revs);\n> >       } else if (!strcmp(arg, \"--no-graph\")) {\n> > +             revs->diffopt.output_prefix = NULL;\n> > +             revs->diffopt.output_prefix_data = NULL;\n> >               graph_clear(revs->graph);\n> >               revs->graph = NULL;\n> >       } else if (!strcmp(arg, \"--encode-email-headers\")) {\n"},{"id":"512213","messageId":"Z6sCeYmljrqWRFnS@pks.im","threadId":"62923","inReplyTo":"20250208061702.88469-1-forivall@gmail.com","subject":"Re: [PATCH] revision: fix missing null for freed memory","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-02-11T07:55:54Z","receivedAt":"2025-02-11T07:56:03Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Feb 07, 2025 at 10:17:02PM -0800, Emily M Klassen wrote:\n> \"git log --graph --no-graph\" missed cleaning up the output_prefix and\n> output_prefix_data pointers. This resulted in a segfault when using \"--patch\",\n> \"--name-status\" or \"--name-only\", as the output_prefix_data continued to be in\n> use after free()\n> \n> Signed-off-by: Emily M Klassen <forivall@gmail.com>\n> ---\n> I previously reported this a few hours ago, and ended up digging in and figuring\n> it out. I'll make sure to bottom reply in the follow ups to this patch.\n\nDo we know when this bug was introduced? Is it a recent regression or a\nlong-standing issue? Might be nice to point out in the commit message if\nwe do know.\n\nPatrick\n"},{"id":"512254","messageId":"CALnO6CDHZerHKaWwGc-9CmwEMiFVY+Ds5-GNWYKUi1yO7=U_Rg@mail.gmail.com","threadId":"62923","inReplyTo":"Z6sCeYmljrqWRFnS@pks.im","subject":"Re: [PATCH] revision: fix missing null for freed memory","fromName":"D. Ben Knoble","fromEmail":"ben.knoble@gmail.com","sentAt":"2025-02-11T19:31:02Z","receivedAt":"2025-02-11T19:31:15Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"On Tue, Feb 11, 2025 at 2:56 AM Patrick Steinhardt <ps@pks.im> wrote:\n>\n> On Fri, Feb 07, 2025 at 10:17:02PM -0800, Emily M Klassen wrote:\n> > \"git log --graph --no-graph\" missed cleaning up the output_prefix and\n> > output_prefix_data pointers. This resulted in a segfault when using \"--patch\",\n> > \"--name-status\" or \"--name-only\", as the output_prefix_data continued to be in\n> > use after free()\n> >\n> > Signed-off-by: Emily M Klassen <forivall@gmail.com>\n> > ---\n> > I previously reported this a few hours ago, and ended up digging in and figuring\n> > it out. I'll make sure to bottom reply in the follow ups to this patch.\n>\n> Do we know when this bug was introduced? Is it a recent regression or a\n> long-standing issue? Might be nice to point out in the commit message if\n> we do know.\n>\n> Patrick\n\nOut of morbid curiosity, I've started bisecting this, but it's proven\nslightly more subtle than I anticipated. If nobody beats me to it,\nI'll post my results when I have them.\n\n-- \nD. Ben Knoble\n"},{"id":"512256","messageId":"CALnO6CDdJ4abqxZKMaevPO+aCzSqriM98JuVOX068gQrxWZt5Q@mail.gmail.com","threadId":"62923","inReplyTo":"CALnO6CDHZerHKaWwGc-9CmwEMiFVY+Ds5-GNWYKUi1yO7=U_Rg@mail.gmail.com","subject":"Re: [PATCH] revision: fix missing null for freed memory","fromName":"D. Ben Knoble","fromEmail":"ben.knoble@gmail.com","sentAt":"2025-02-11T20:22:28Z","receivedAt":"2025-02-11T20:22:41Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"On Tue, Feb 11, 2025 at 2:31 PM D. Ben Knoble <ben.knoble@gmail.com> wrote:\n>\n> On Tue, Feb 11, 2025 at 2:56 AM Patrick Steinhardt <ps@pks.im> wrote:\n> >\n> > On Fri, Feb 07, 2025 at 10:17:02PM -0800, Emily M Klassen wrote:\n> > > \"git log --graph --no-graph\" missed cleaning up the output_prefix and\n> > > output_prefix_data pointers. This resulted in a segfault when using \"--patch\",\n> > > \"--name-status\" or \"--name-only\", as the output_prefix_data continued to be in\n> > > use after free()\n> > >\n> > > Signed-off-by: Emily M Klassen <forivall@gmail.com>\n> > > ---\n> > > I previously reported this a few hours ago, and ended up digging in and figuring\n> > > it out. I'll make sure to bottom reply in the follow ups to this patch.\n> >\n> > Do we know when this bug was introduced? Is it a recent regression or a\n> > long-standing issue? Might be nice to point out in the commit message if\n> > we do know.\n> >\n> > Patrick\n>\n> Out of morbid curiosity, I've started bisecting this, but it's proven\n> slightly more subtle than I anticipated. If nobody beats me to it,\n> I'll post my results when I have them.\n>\n> --\n> D. Ben Knoble\n\n2.{30,35}.0 fails to recognize --no-graph, so I checked \"git log --grep no-graph\norigin/master\" with \"git describe --contains\" and decided that 2.36.0 was first\nrelease recognizing --no-graph, but it didn't build for me (possibly an issue on\nmy end). I got 2.37.0 built, and it was \"good,\" so that's where I started.\n\nHere's my \"bisect run\" script.\n\n    #! /bin/sh -x\n    make || exit 125\n    # segfault has exit >128\n    ./bin-wrappers/git --no-pager log -2 --graph --no-graph --patch\n--cc || exit 1\n\nThe --cc is important, since this repro logs from where the bisect is! Without\nit, if the head commits are both merges (likely), the repro will accidentally\nmark the commit as good when looking further for a commit with a patch will\nfail. Omitting -2 might work, too, but that makes \"git log\" take longer.\n\nWith --first-parent on bisect, we find 3eb4cc451e (Merge branch\n'jk/output-prefix-cleanup', 2024-10-10), which looks like a reasonable\ncandidate. Restarting between that commit and it's first parent, we get\n19752d9c91 (diff: return line_prefix directly when possible, 2024-10-03). That\ncommit actually looks relatively innocuous, and I haven't tracked down how it\nreally impacts the problem or fix.\n\nPerhaps the topic merge is more helpful to folks in assessing where the bug came\nfrom. Otherwise, it may be that --cc is not enough and we should\nbisect with --diff-merges=1 or something else guaranteed to generate a\ndiff and trip the bug.\n\nTo my shame, I didn't save the log from the --first-parent bisect from 2.37.0 to\n388218fac7 (The ninth batch, 2025-02-10), but I did save the smaller one.\n\n    # bad: [3eb4cc451ed97123ff76e183a5be8a7dc164d1f6] Merge branch\n'jk/output-prefix-cleanup'\n    # good: [31bc4454de66c22bc8570fd3af52a99843ac69b0] Merge branch\n'ps/leakfixes-part-8'\n    git bisect start '@' '@^'\n    # good: [436728fe9d75d05fa2439f867ca2039012b86e69] diff: return\nconst char from output_prefix callback\n    git bisect good 436728fe9d75d05fa2439f867ca2039012b86e69\n    # bad: [1164e270b5af80516625b628945ec7365d992055] diff: store\ngraph prefix buf in git_graph struct\n    git bisect bad 1164e270b5af80516625b628945ec7365d992055\n    # bad: [19752d9c912478b9eef0bd83c2cf6da98974f536] diff: return\nline_prefix directly when possible\n    git bisect bad 19752d9c912478b9eef0bd83c2cf6da98974f536\n    # first bad commit: [19752d9c912478b9eef0bd83c2cf6da98974f536]\ndiff: return line_prefix directly when possible\n\n-- \nD. Ben Knoble\n"},{"id":"512261","messageId":"20250211212909.GA3113114@coredump.intra.peff.net","threadId":"62923","inReplyTo":"CALnO6CDdJ4abqxZKMaevPO+aCzSqriM98JuVOX068gQrxWZt5Q@mail.gmail.com","subject":"Re: [PATCH] revision: fix missing null for freed memory","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-02-11T21:29:09Z","receivedAt":"2025-02-11T21:29:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 11, 2025 at 03:22:28PM -0500, D. Ben Knoble wrote:\n\n> 2.{30,35}.0 fails to recognize --no-graph, so I checked \"git log --grep no-graph\n> origin/master\" with \"git describe --contains\" and decided that 2.36.0 was first\n> release recognizing --no-graph, but it didn't build for me (possibly an issue on\n> my end). I got 2.37.0 built, and it was \"good,\" so that's where I started.\n> \n> Here's my \"bisect run\" script.\n> \n>     #! /bin/sh -x\n>     make || exit 125\n>     # segfault has exit >128\n>     ./bin-wrappers/git --no-pager log -2 --graph --no-graph --patch\n> --cc || exit 1\n\nI don't think this is quite enough. The problem is a use-after-free, so\nthe behavior is undefined. Depending on whether that heap block is\nreused, it might work just fine, or output garbage data, or segfault.\n\nI'd have _thought_ it would usually just segfault, but it almost always\njust output garbage for me. Building with:\n\n  make SANITIZE=address,undefined\n\nis a good way to get reliable results for this kind of memory error.\nDoing that shows that v2.37.0 is actually bad. And bisecting shows that\nit has been broken since 087c745833 (log: add a --no-graph option,\n2022-02-11), which is not too surprising.\n\n> The --cc is important, since this repro logs from where the bisect is! Without\n> it, if the head commits are both merges (likely), the repro will accidentally\n> mark the commit as good when looking further for a commit with a patch will\n> fail. Omitting -2 might work, too, but that makes \"git log\" take longer.\n\nI've also run into non-determinism when bisecting like this, because my\ntest command depends on the value of HEAD. The best solution here is to\njust feed a stable tip to git-log. I bisected on:\n\n  git log --graph --no-graph --patch origin >/dev/null\n\n(I didn't need \"-2\" because good commits failed with \"unrecognized\nargument\" and bad ones were killed by ASan immediately ;) ).\n\n-Peff\n"},{"id":"512267","messageId":"xmqqmsesw01l.fsf@gitster.g","threadId":"62923","inReplyTo":"20250211212909.GA3113114@coredump.intra.peff.net","subject":"Re: [PATCH] revision: fix missing null for freed memory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-02-11T23:09:58Z","receivedAt":"2025-02-11T23:10:01Z","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> Doing that shows that v2.37.0 is actually bad. And bisecting shows that\n> it has been broken since 087c745833 (log: add a --no-graph option,\n> 2022-02-11), which is not too surprising.\n\nYeah, broken from its beginning is usually how these kinds of bugs\nturn out to be.\n\n> I've also run into non-determinism when bisecting like this, because my\n> test command depends on the value of HEAD. The best solution here is to\n> just feed a stable tip to git-log. I bisected on:\n>\n>   git log --graph --no-graph --patch origin >/dev/null\n>\n> (I didn't need \"-2\" because good commits failed with \"unrecognized\n> argument\" and bad ones were killed by ASan immediately ;) ).\n\nThanks for sharing a good tip.\n"},{"id":"512286","messageId":"Z6wx2a4LUcOjU79p@pks.im","threadId":"62923","inReplyTo":"20250211212909.GA3113114@coredump.intra.peff.net","subject":"Re: [PATCH] revision: fix missing null for freed memory","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-02-12T05:30:01Z","receivedAt":"2025-02-12T05:30:13Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Feb 11, 2025 at 04:29:09PM -0500, Jeff King wrote:\n> On Tue, Feb 11, 2025 at 03:22:28PM -0500, D. Ben Knoble wrote:\n> \n> > 2.{30,35}.0 fails to recognize --no-graph, so I checked \"git log --grep no-graph\n> > origin/master\" with \"git describe --contains\" and decided that 2.36.0 was first\n> > release recognizing --no-graph, but it didn't build for me (possibly an issue on\n> > my end). I got 2.37.0 built, and it was \"good,\" so that's where I started.\n> > \n> > Here's my \"bisect run\" script.\n> > \n> >     #! /bin/sh -x\n> >     make || exit 125\n> >     # segfault has exit >128\n> >     ./bin-wrappers/git --no-pager log -2 --graph --no-graph --patch\n> > --cc || exit 1\n> \n> I don't think this is quite enough. The problem is a use-after-free, so\n> the behavior is undefined. Depending on whether that heap block is\n> reused, it might work just fine, or output garbage data, or segfault.\n> \n> I'd have _thought_ it would usually just segfault, but it almost always\n> just output garbage for me. Building with:\n> \n>   make SANITIZE=address,undefined\n> \n> is a good way to get reliable results for this kind of memory error.\n> Doing that shows that v2.37.0 is actually bad. And bisecting shows that\n> it has been broken since 087c745833 (log: add a --no-graph option,\n> 2022-02-11), which is not too surprising.\n\nThanks all for bisecting :)\n\nPatrick\n"},{"id":"512335","messageId":"xmqqh64yr7y3.fsf@gitster.g","threadId":"62923","inReplyTo":"CADY4h_o_wfUpjSBhWa9TPU_G-G8qpENpUeOKGQDY8dq6Zb2+qg@mail.gmail.com","subject":"Re: [PATCH] revision: fix missing null for freed memory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-02-13T00:42:44Z","receivedAt":"2025-02-13T00:42:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Emily Klassen <forivall@gmail.com> writes:\n\n> Awesome. I'll also add a test before re-submitting, as mentioned in\n> your other message.\n\nThanks.\n"},{"id":"512384","messageId":"40281952-B43E-493C-A092-63768C708C8A@gmail.com","threadId":"62923","inReplyTo":"20250211212909.GA3113114@coredump.intra.peff.net","subject":"Re: [PATCH] revision: fix missing null for freed memory","fromName":"Ben Knoble","fromEmail":"ben.knoble@gmail.com","sentAt":"2025-02-13T21:07:23Z","receivedAt":"2025-02-13T21:07:35Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"\n> \n> Le 11 févr. 2025 à 16:29, Jeff King <peff@peff.net> a écrit :\n> \n> On Tue, Feb 11, 2025 at 03:22:28PM -0500, D. Ben Knoble wrote:\n> \n>> 2.{30,35}.0 fails to recognize --no-graph, so I checked \"git log --grep no-graph\n>> origin/master\" with \"git describe --contains\" and decided that 2.36.0 was first\n>> release recognizing --no-graph, but it didn't build for me (possibly an issue on\n>> my end). I got 2.37.0 built, and it was \"good,\" so that's where I started.\n>> \n>> Here's my \"bisect run\" script.\n>> \n>>    #! /bin/sh -x\n>>    make || exit 125\n>>    # segfault has exit >128\n>>    ./bin-wrappers/git --no-pager log -2 --graph --no-graph --patch\n>> --cc || exit 1\n> \n> I don't think this is quite enough. The problem is a use-after-free, so\n> the behavior is undefined. Depending on whether that heap block is\n> reused, it might work just fine, or output garbage data, or segfault.\n> \n> I'd have _thought_ it would usually just segfault, but it almost always\n> just output garbage for me. Building with:\n> \n>  make SANITIZE=address,undefined\n> \n> is a good way to get reliable results for this kind of memory error.\n> Doing that shows that v2.37.0 is actually bad. And bisecting shows that\n> it has been broken since 087c745833 (log: add a --no-graph option,\n> 2022-02-11), which is not too surprising.\n\nAh, fun, that’s more like what I was expecting. And thanks for the advice!\n\n> \n>> The --cc is important, since this repro logs from where the bisect is! Without\n>> it, if the head commits are both merges (likely), the repro will accidentally\n>> mark the commit as good when looking further for a commit with a patch will\n>> fail. Omitting -2 might work, too, but that makes \"git log\" take longer.\n> \n> I've also run into non-determinism when bisecting like this, because my\n> test command depends on the value of HEAD. The best solution here is to\n> just feed a stable tip to git-log. I bisected on:\n> \n>  git log --graph --no-graph --patch origin >/dev/null\n> \n> (I didn't need \"-2\" because good commits failed with \"unrecognized\n> argument\" and bad ones were killed by ASan immediately ;) ).\n> \n> -Peff\n"}]}