{"thread":{"id":"58511","subject":"[PATCH] ref-filter.c: fix a leak in get_head_description","startedAt":"2022-09-24T22:53:26Z","lastAt":"2022-10-08T00:35:10Z","messageCount":7,"participants":["Rubén Justo","Martin Ågren","Ævar Arnfjörð Bjarmason","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"463589","messageId":"6ff29e96-7f8d-c354-dced-b1b363e54467@gmail.com","threadId":"58511","inReplyTo":null,"subject":"[PATCH] ref-filter.c: fix a leak in get_head_description","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2022-09-24T22:53:18Z","receivedAt":"2022-09-24T22:53:26Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"In 2708ce62d2 (branch: sort detached HEAD based on a flag, 2021-01-07) a\ncall to wt_status_state_free_buffers, responsible of freeing the\nresources that could be allocated in the local struct wt_status_state\nstate, was eliminated.\n\nThe call to wt_status_state_free_buffers was introduced in 962dd7ebc3\n(wt-status: introduce wt_status_state_free_buffers(), 2020-09-27). This\ncommit brings back that call in get_head_description.\n\nSigned-off-by: Rubén Justo \n\n---\n ref-filter.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex fd1cb14b0f..914908fac5 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1722,6 +1722,8 @@ char *get_head_description(void)\n \t} else\n \t\tstrbuf_addstr(&desc, _(\"(no branch)\"));\n \n+\twt_status_state_free_buffers(&state);\n+\n \treturn strbuf_detach(&desc, NULL);\n }\n \n-- \n2.36.1\n"},{"id":"463604","messageId":"CAN0heSra_LsBzYCNFh0cZOZ0pmk1Wb9RtNTLwi93UM=f-53Uxw@mail.gmail.com","threadId":"58511","inReplyTo":"6ff29e96-7f8d-c354-dced-b1b363e54467@gmail.com","subject":"Re: [PATCH] ref-filter.c: fix a leak in get_head_description","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2022-09-26T06:59:34Z","receivedAt":"2022-09-26T06:59:54Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Sun, 25 Sept 2022 at 00:53, Rubén Justo wrote:\n>\n> In 2708ce62d2 (branch: sort detached HEAD based on a flag, 2021-01-07) a\n> call to wt_status_state_free_buffers, responsible of freeing the\n> resources that could be allocated in the local struct wt_status_state\n> state, was eliminated.\n>\n> The call to wt_status_state_free_buffers was introduced in 962dd7ebc3\n> (wt-status: introduce wt_status_state_free_buffers(), 2020-09-27). This\n> commit brings back that call in get_head_description.\n\n> + wt_status_state_free_buffers(&state);\n> +\n> return strbuf_detach(&desc, NULL);\n> }\n\nGood catch, and excellent history digging. From the original submission\n[1] of the patch that dropped this call, I get the feeling that it was\noriginally developed some time earlier. I suspect this call was then\naccidentally dropped in a rebase before submission.\n\nFWIW, this patch is\n\nReviewed-by: Martin Ågren \n\n[1] https://lore.kernel.org/git/20210106100139.14651-1-avarab@gmail.com/\n\nMartin\n"},{"id":"463606","messageId":"220926.86bkr24kjw.gmgdl@evledraar.gmail.com","threadId":"58511","inReplyTo":"6ff29e96-7f8d-c354-dced-b1b363e54467@gmail.com","subject":"Re: [PATCH] ref-filter.c: fix a leak in get_head_description","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-09-26T08:14:38Z","receivedAt":"2022-09-26T08:17:31Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sun, Sep 25 2022, Rubén Justo wrote:\n\n> In 2708ce62d2 (branch: sort detached HEAD based on a flag, 2021-01-07) a\n> call to wt_status_state_free_buffers, responsible of freeing the\n> resources that could be allocated in the local struct wt_status_state\n> state, was eliminated.\n>\n> The call to wt_status_state_free_buffers was introduced in 962dd7ebc3\n> (wt-status: introduce wt_status_state_free_buffers(), 2020-09-27). This\n> commit brings back that call in get_head_description.\n>\n> Signed-off-by: Rubén Justo \n>\n> ---\n> ref-filter.c | 2 ++\n> 1 file changed, 2 insertions(+)\n>\n> diff --git a/ref-filter.c b/ref-filter.c\n> index fd1cb14b0f..914908fac5 100644\n> --- a/ref-filter.c\n> +++ b/ref-filter.c\n> @@ -1722,6 +1722,8 @@ char *get_head_description(void)\n> \t} else\n> \t\tstrbuf_addstr(&desc, _(\"(no branch)\"));\n> \n> +\twt_status_state_free_buffers(&state);\n> +\n> \treturn strbuf_detach(&desc, NULL);\n> }\n\nThanks, this looks good to me. FWIW I have a local (still unsubmitted)\nseries of leak fixes across the tree which fixed this leak, that commit\nis presented below as a \"I've already been running with this for a\nwhile\" data point.\n\nThanks!\n\n-- >8 --\nSubject: [PATCH] wt-status API users: use wt_status_state_free_buffers(), fix\n leaks\n\nFix a memory that was accidentally introduced in ref-filter.c in\n2708ce62d21 (branch: sort detached HEAD based on a flag, 2021-01-07),\nand one that's been present in builtin/checkout.c since\nc45f0f525de (switch: reject if some operation is in progress,\n2019-03-29).\n\nIn both cases we should be using the wt_status_state_free_buffers()\nAPI introduced in 962dd7ebc3e (wt-status: introduce\nwt_status_state_free_buffers(), 2020-09-27).\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/checkout.c | 2 ++\n ref-filter.c | 2 ++\n 2 files changed, 4 insertions(+)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 54373424403..549c3d17a1a 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -1469,6 +1469,8 @@ static void die_if_some_operation_in_progress(void)\n \t\t \"or \\\"git worktree add\\\".\"));\n \tif (state.bisect_in_progress)\n \t\twarning(_(\"you are switching branch while bisecting\"));\n+\n+\twt_status_state_free_buffers(&state);\n }\n \n static int checkout_branch(struct checkout_opts *opts,\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 45908d4bdfc..81278ec3cf7 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1722,6 +1722,8 @@ char *get_head_description(void)\n \t} else\n \t\tstrbuf_addstr(&desc, _(\"(no branch)\"));\n \n+\twt_status_state_free_buffers(&state);\n+\n \treturn strbuf_detach(&desc, NULL);\n }\n"},{"id":"463666","messageId":"xmqqfsgene5i.fsf@gitster.g","threadId":"58511","inReplyTo":"CAN0heSra_LsBzYCNFh0cZOZ0pmk1Wb9RtNTLwi93UM=f-53Uxw@mail.gmail.com","subject":"Re: [PATCH] ref-filter.c: fix a leak in get_head_description","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-26T19:12:25Z","receivedAt":"2022-09-26T19:12:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Ågren writes:\n\n> On Sun, 25 Sept 2022 at 00:53, Rubén Justo wrote:\n>>\n>> In 2708ce62d2 (branch: sort detached HEAD based on a flag, 2021-01-07) a\n>> call to wt_status_state_free_buffers, responsible of freeing the\n>> resources that could be allocated in the local struct wt_status_state\n>> state, was eliminated.\n>>\n>> The call to wt_status_state_free_buffers was introduced in 962dd7ebc3\n>> (wt-status: introduce wt_status_state_free_buffers(), 2020-09-27). This\n>> commit brings back that call in get_head_description.\n>\n>> + wt_status_state_free_buffers(&state);\n>> +\n>> return strbuf_detach(&desc, NULL);\n>> }\n>\n> Good catch, and excellent history digging. From the original submission\n> [1] of the patch that dropped this call, I get the feeling that it was\n> originally developed some time earlier. I suspect this call was then\n> accidentally dropped in a rebase before submission.\n>\n> FWIW, this patch is\n>\n> Reviewed-by: Martin Ågren \n>\n> [1] https://lore.kernel.org/git/20210106100139.14651-1-avarab@gmail.com/\n>\n> Martin\n\nThanks, all. Will queue lest I forget, but I presume this is not\nrelease critical?\n\n"},{"id":"463668","messageId":"8288e9e7-dbea-a5de-c05e-5a22a6a853f6@gmail.com","threadId":"58511","inReplyTo":"220926.86bkr24kjw.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] ref-filter.c: fix a leak in get_head_description","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2022-09-26T19:13:45Z","receivedAt":"2022-09-26T19:13:51Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On 26/9/22 10:14, Ævar Arnfjörð Bjarmason wrote:\n> \n> Thanks, this looks good to me. FWIW I have a local (still unsubmitted)\n> series of leak fixes across the tree which fixed this leak, that commit\n> is presented below as a \"I've already been running with this for a\n> while\" data point.\n\nThat call in die_if_some_operation_in_progress is a good addition to this fix.\nI haven't found a happy path that leaks there yet, but it is calling for it\nanyway. And the commit message looks good to me too. So you can add to that\npatch, if you want:\n\nReviewed-by: Rubén Justo \n\n\nThanks Martin for your review!\n\nUn saludo.\n\n> \n> Thanks!\n> \n> -- >8 --\n> Subject: [PATCH] wt-status API users: use wt_status_state_free_buffers(), fix\n> leaks\n> \n> Fix a memory that was accidentally introduced in ref-filter.c in\n> 2708ce62d21 (branch: sort detached HEAD based on a flag, 2021-01-07),\n> and one that's been present in builtin/checkout.c since\n> c45f0f525de (switch: reject if some operation is in progress,\n> 2019-03-29).\n> \n> In both cases we should be using the wt_status_state_free_buffers()\n> API introduced in 962dd7ebc3e (wt-status: introduce\n> wt_status_state_free_buffers(), 2020-09-27).\n> \n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> builtin/checkout.c | 2 ++\n> ref-filter.c | 2 ++\n> 2 files changed, 4 insertions(+)\n> \n> diff --git a/builtin/checkout.c b/builtin/checkout.c\n> index 54373424403..549c3d17a1a 100644\n> --- a/builtin/checkout.c\n> +++ b/builtin/checkout.c\n> @@ -1469,6 +1469,8 @@ static void die_if_some_operation_in_progress(void)\n> \t\t \"or \\\"git worktree add\\\".\"));\n> \tif (state.bisect_in_progress)\n> \t\twarning(_(\"you are switching branch while bisecting\"));\n> +\n> +\twt_status_state_free_buffers(&state);\n> }\n> \n> static int checkout_branch(struct checkout_opts *opts,\n> diff --git a/ref-filter.c b/ref-filter.c\n> index 45908d4bdfc..81278ec3cf7 100644\n> --- a/ref-filter.c\n> +++ b/ref-filter.c\n> @@ -1722,6 +1722,8 @@ char *get_head_description(void)\n> \t} else\n> \t\tstrbuf_addstr(&desc, _(\"(no branch)\"));\n> \n> +\twt_status_state_free_buffers(&state);\n> +\n> \treturn strbuf_detach(&desc, NULL);\n> }\n> \n"},{"id":"463697","messageId":"CAN0heSr0V+coALLaUkN2Su-xQ-xRpWuMz0TMr5kYpO0n6LNbaw@mail.gmail.com","threadId":"58511","inReplyTo":"xmqqfsgene5i.fsf@gitster.g","subject":"Re: [PATCH] ref-filter.c: fix a leak in get_head_description","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2022-09-27T05:59:52Z","receivedAt":"2022-09-27T06:00:09Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Mon, 26 Sept 2022 at 21:12, Junio C Hamano wrote:\n>\n> Thanks, all. Will queue lest I forget, but I presume this is not\n> release critical?\n\nCorrect. This regressed already in v2.30.1 and v2.31.0 after the leak\nwas originally plugged in v2.29.0. So almost all recentish releases have\nthis minor leak. I'm not even sure it's possible for a single git\nprocess to hit this leak more than once today.\n\nMartin\n"},{"id":"464406","messageId":"b6be0af4-8b47-d89d-13fe-a307c0e96797@gmail.com","threadId":"58511","inReplyTo":"6ff29e96-7f8d-c354-dced-b1b363e54467@gmail.com","subject":"Re: [PATCH v2] wt-status: using of wt_status_state_free_buffers","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2022-10-08T00:35:02Z","receivedAt":"2022-10-08T00:35:10Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"In 2708ce62d2 (branch: sort detached HEAD based on a flag, 2021-01-07) a\ncall to wt_status_state_free_buffers, responsible of freeing the\nresources that could be allocated in the local struct wt_status_state\nstate, was eliminated.\n\nThat introduced a leak that occurs with \"branch list\" in a detached HEAD\nstate. The number of leaks depends on the references to \"refname\" in\n--sort and --format. With defaults, 3 are leaked: default sort is by\n\"refname\" and default format needs two references of \"refname\":\n%(refname:lstrip=2) and %(refname:rstrip-2).\n\nThis doesn't leak:\n $ git branch --sort objectname --format '%(objectname)'\n\nThis leak once:\n $ git branch --sort refname --format '%(refname)'\n\nThis leak twice:\n $ git branch --sort refname --format '%(refname:lstrip=2)'\n\nNote that each different \"atom\" is only resolved once (see ref-filter.c).\n\nThe call to wt_status_state_free_buffers was introduced in 962dd7ebc3\n(wt-status: introduce wt_status_state_free_buffers(), 2020-09-27).\nBring back that call in get_head_description.\n\nÆvar Arnfjörð pointed out [1] that a similar leak is present in\nbuiltin/checkout.c:die_if_some_operation_in_progress(void), since\nc45f0f525de (switch: reject if some operation is in progress,\n2019-03-29). The leak occurs once with \"switch\" while bisecting.\n\nLets call to wc_status_state_free_buffers() there too.\n\n[1] 220926.86bkr24kjw.gmgdl@evledraar.gmail.com\n\nSigned-off-by: Rubén Justo \n---\n\nChanges since v1:\n\t- Add details about the leak\n\t- Fix the leak in builtin/checkout.c\n\n\nRange-diff:\n1: 86b7803955 ! 1: 3fa1ddbb30 ref-filter.c: fix a leak in get_head_description\n @@ Commit message\n resources that could be allocated in the local struct wt_status_state\n state, was eliminated.\n \n + That introduced a leak that occurs with \"branch list\" in a detached HEAD\n + state. The number of leaks depends on the references to \"refname\" in\n + --sort and --format. With defaults, 3 are leaked: default sort is by\n + \"refname\" and default format needs two references of \"refname\",\n + %(refname:lstrip=2) and %(refname:rstrip-2).\n +\n + This doesn't leak:\n + $ git branch --sort objectname --format '%(objectname)'\n +\n + This leak once:\n + $ git branch --sort refname --format '%(refname)'\n +\n + This leak twice:\n + $ git branch --sort refname --format '%(refname:lstrip=2)'\n +\n + Note that each different \"atom\" is only resolved once (see ref-filter.c).\n +\n The call to wt_status_state_free_buffers was introduced in 962dd7ebc3\n - (wt-status: introduce wt_status_state_free_buffers(), 2020-09-27). This\n - commit brings back that call in get_head_description.\n + (wt-status: introduce wt_status_state_free_buffers(), 2020-09-27).\n + Bring back that call in get_head_description.\n +\n + Ævar Arnfjörð pointed out [1] that a similar leak is present in\n + builtin/checkout.c:die_if_some_operation_in_progress(void), since\n + c45f0f525de (switch: reject if some operation is in progress,\n + 2019-03-29). The leak ocurrs once with checkout while bisecting.\n +\n + Lets call to wc_status_state_free_buffers() there too.\n +\n + [1] 220926.86bkr24kjw.gmgdl@evledraar.gmail.com\n \n Signed-off-by: Rubén Justo \n \n + ## builtin/checkout.c ##\n +@@ builtin/checkout.c: static void die_if_some_operation_in_progress(void)\n + \t\t \"or \\\"git worktree add\\\".\"));\n + \tif (state.bisect_in_progress)\n + \t\twarning(_(\"you are switching branch while bisecting\"));\n ++\n ++\twt_status_state_free_buffers(&state);\n + }\n + \n + static int checkout_branch(struct checkout_opts *opts,\n +\n ## ref-filter.c ##\n @@ ref-filter.c: char *get_head_description(void)\n \t} else\n\n builtin/checkout.c | 2 ++\n ref-filter.c | 2 ++\n 2 files changed, 4 insertions(+)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 2a132392fb..659dd5c430 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -1470,6 +1470,8 @@ static void die_if_some_operation_in_progress(void)\n \t\t \"or \\\"git worktree add\\\".\"));\n \tif (state.bisect_in_progress)\n \t\twarning(_(\"you are switching branch while bisecting\"));\n+\n+\twt_status_state_free_buffers(&state);\n }\n \n static int checkout_branch(struct checkout_opts *opts,\ndiff --git a/ref-filter.c b/ref-filter.c\nindex fd1cb14b0f..914908fac5 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1722,6 +1722,8 @@ char *get_head_description(void)\n \t} else\n \t\tstrbuf_addstr(&desc, _(\"(no branch)\"));\n \n+\twt_status_state_free_buffers(&state);\n+\n \treturn strbuf_detach(&desc, NULL);\n }\n \n-- \n2.32.0\n"}]}