{"thread":{"id":"61347","subject":"[PATCH 0/4] mark t3701-add-interactive.sh as leak-free","startedAt":"2024-04-21T10:22:55Z","lastAt":"2024-04-22T23:27:00Z","messageCount":19,"participants":["Rubén Justo","Patrick Steinhardt","Junio C Hamano","Phillip Wood"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"493256","messageId":"b7b2d0d2-245e-440f-a7cc-fa0df1ce73ad@gmail.com","threadId":"61347","inReplyTo":null,"subject":"[PATCH 0/4] mark t3701-add-interactive.sh as leak-free","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-04-21T10:22:46Z","receivedAt":"2024-04-21T10:22:55Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"Rubén Justo (4):\n  apply: plug a leak in apply_data\n  add-interactive: plug a leak in get_untracked_files\n  add-patch: plug a leak handling the '/' command\n  add: plug a leak on interactive_add\n\n add-interactive.c          | 1 +\n add-patch.c                | 1 +\n apply.c                    | 4 +++-\n builtin/add.c              | 9 ++++++---\n t/t2016-checkout-patch.sh  | 1 +\n t/t3701-add-interactive.sh | 1 +\n t/t4103-apply-binary.sh    | 1 +\n t/t4104-apply-boundary.sh  | 1 +\n t/t4113-apply-ending.sh    | 1 +\n t/t4117-apply-reject.sh    | 1 +\n t/t4123-apply-shrink.sh    | 1 +\n t/t4252-am-options.sh      | 2 ++\n t/t4258-am-quoted-cr.sh    | 1 +\n t/t7514-commit-patch.sh    | 2 ++\n 14 files changed, 23 insertions(+), 4 deletions(-)\n\n-- \n2.45.0.rc0.4.g08f77eb516\n"},{"id":"493257","messageId":"88b119aa-c1c2-435a-baaa-9422388bf0ec@gmail.com","threadId":"61347","inReplyTo":"b7b2d0d2-245e-440f-a7cc-fa0df1ce73ad@gmail.com","subject":"[PATCH 2/4] add-interactive: plug a leak in get_untracked_files","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-04-21T10:27:06Z","receivedAt":"2024-04-21T10:27:16Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"Plug a leak we have since ab1e1cccaf (built-in add -i: re-implement\n`add-untracked` in C, 2019-11-29).\n\nThis leak can be triggered with:\n\n\t$ echo a | git add -i\n\nAs a curiosity, we have a somewhat similar function in builtin/stash.c,\nwhich correctly frees the memory.\n\nSigned-off-by: Rubén Justo <rjusto@gmail.com>\n---\n add-interactive.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/add-interactive.c b/add-interactive.c\nindex 6bf87e7ae7..e17602b5e4 100644\n--- a/add-interactive.c\n+++ b/add-interactive.c\n@@ -865,6 +865,7 @@ static int get_untracked_files(struct repository *r,\n \t}\n \n \tstrbuf_release(&buf);\n+\tdir_clear(&dir);\n \treturn 0;\n }\n \n-- \n2.45.0.rc0.4.g08f77eb516\n"},{"id":"493258","messageId":"8e6ab088-2026-43e7-a869-b1c7185ee765@gmail.com","threadId":"61347","inReplyTo":"b7b2d0d2-245e-440f-a7cc-fa0df1ce73ad@gmail.com","subject":"[PATCH 1/4] apply: plug a leak in apply_data","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-04-21T10:28:05Z","receivedAt":"2024-04-21T10:28:24Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"Plug a leak we have since cfb6f9acc3 (apply: accept -3/--3way command\nline option, 2012-05-08).\n\nThis leak can be triggered with:\n\n    $ echo foo >file\n    $ git add file && git commit -m file\n    $ echo bar >file\n    $ git diff file >diff\n    $ sed s/foo/frotz/ <diff >baddiff\n    $ git apply --cached <baddiff\n\nFixing this leak allows us to mark as leak-free the following tests:\n\n    + t2016-checkout-patch.sh\n    + t4103-apply-binary.sh\n    + t4104-apply-boundary.sh\n    + t4113-apply-ending.sh\n    + t4117-apply-reject.sh\n    + t4123-apply-shrink.sh\n    + t4252-am-options.sh\n    + t4258-am-quoted-cr.sh\n\nMark them with \"TEST_PASSES_SANITIZE_LEAK=true\" to notice and fix\npromply any new leak that may be introduced and triggered by them in the\nfuture.\n\nSigned-off-by: Rubén Justo <rjusto@gmail.com>\n---\n apply.c                   | 4 +++-\n t/t2016-checkout-patch.sh | 1 +\n t/t4103-apply-binary.sh   | 1 +\n t/t4104-apply-boundary.sh | 1 +\n t/t4113-apply-ending.sh   | 1 +\n t/t4117-apply-reject.sh   | 1 +\n t/t4123-apply-shrink.sh   | 1 +\n t/t4252-am-options.sh     | 2 ++\n t/t4258-am-quoted-cr.sh   | 1 +\n 9 files changed, 12 insertions(+), 1 deletion(-)\n\ndiff --git a/apply.c b/apply.c\nindex 34f20326a7..2f752d71a8 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -3712,8 +3712,10 @@ static int apply_data(struct apply_state *state, struct patch *patch,\n \t\t\tfprintf(stderr, _(\"Falling back to direct application...\\n\"));\n \n \t\t/* Note: with --reject, apply_fragments() returns 0 */\n-\t\tif (patch->direct_to_threeway || apply_fragments(state, &image, patch) < 0)\n+\t\tif (patch->direct_to_threeway || apply_fragments(state, &image, patch) < 0) {\n+\t\t\tclear_image(&image);\n \t\t\treturn -1;\n+\t\t}\n \t}\n \tpatch->result = image.buf;\n \tpatch->resultsize = image.len;\ndiff --git a/t/t2016-checkout-patch.sh b/t/t2016-checkout-patch.sh\nindex c4f9bf09aa..c40b661ac1 100755\n--- a/t/t2016-checkout-patch.sh\n+++ b/t/t2016-checkout-patch.sh\n@@ -2,6 +2,7 @@\n \n test_description='git checkout --patch'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./lib-patch-mode.sh\n \n test_expect_success 'setup' '\ndiff --git a/t/t4103-apply-binary.sh b/t/t4103-apply-binary.sh\nindex d370ecfe0d..144619ab87 100755\n--- a/t/t4103-apply-binary.sh\n+++ b/t/t4103-apply-binary.sh\n@@ -9,6 +9,7 @@ test_description='git apply handling binary patches\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_expect_success 'setup' '\ndiff --git a/t/t4104-apply-boundary.sh b/t/t4104-apply-boundary.sh\nindex 71ef4132d1..dc501aac38 100755\n--- a/t/t4104-apply-boundary.sh\n+++ b/t/t4104-apply-boundary.sh\n@@ -5,6 +5,7 @@\n \n test_description='git apply boundary tests'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n L=\"c d e f g h i j k l m n o p q r s t u v w x\"\ndiff --git a/t/t4113-apply-ending.sh b/t/t4113-apply-ending.sh\nindex 66fa51591e..2c65c6a169 100755\n--- a/t/t4113-apply-ending.sh\n+++ b/t/t4113-apply-ending.sh\n@@ -6,6 +6,7 @@\n test_description='git apply trying to add an ending line.\n \n '\n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n # setup\ndiff --git a/t/t4117-apply-reject.sh b/t/t4117-apply-reject.sh\nindex c86d05a96f..4d15ccd28e 100755\n--- a/t/t4117-apply-reject.sh\n+++ b/t/t4117-apply-reject.sh\n@@ -7,6 +7,7 @@ test_description='git apply with rejects\n \n '\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_expect_success setup '\ndiff --git a/t/t4123-apply-shrink.sh b/t/t4123-apply-shrink.sh\nindex 3ef84619f5..3601c0c5dc 100755\n--- a/t/t4123-apply-shrink.sh\n+++ b/t/t4123-apply-shrink.sh\n@@ -2,6 +2,7 @@\n \n test_description='apply a patch that is larger than the preimage'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n cat >F  <<\\EOF\ndiff --git a/t/t4252-am-options.sh b/t/t4252-am-options.sh\nindex e758e634a3..5b680dc755 100755\n--- a/t/t4252-am-options.sh\n+++ b/t/t4252-am-options.sh\n@@ -1,6 +1,8 @@\n #!/bin/sh\n \n test_description='git am with options and not losing them'\n+\n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n tm=\"$TEST_DIRECTORY/t4252\"\ndiff --git a/t/t4258-am-quoted-cr.sh b/t/t4258-am-quoted-cr.sh\nindex 201915b45a..3573c9147f 100755\n--- a/t/t4258-am-quoted-cr.sh\n+++ b/t/t4258-am-quoted-cr.sh\n@@ -2,6 +2,7 @@\n \n test_description='test am --quoted-cr=<action>'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n DATA=\"$TEST_DIRECTORY/t4258\"\n-- \n2.45.0.rc0.4.g08f77eb516\n"},{"id":"493259","messageId":"506130db-63b2-4e83-8545-66a71c728693@gmail.com","threadId":"61347","inReplyTo":"b7b2d0d2-245e-440f-a7cc-fa0df1ce73ad@gmail.com","subject":"[PATCH 3/4] add-patch: plug a leak handling the '/' command","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-04-21T10:28:41Z","receivedAt":"2024-04-21T10:28:49Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"Plug a leak we have since d6cf873340 (built-in add -p: implement the '/'\n(\"search regex\") command, 2019-12-13).\n\nThis leak can be triggered with:\n\n    $ printf \"A\\n\\nB\\n\" >file\n    $ git add file && git commit -m file\n    $ printf \"AA\\n\\nBB\\n\" >file\n    $ printf \"s\\n/ .\\n\" >lines\n    $ git add -p <lines\n\nSigned-off-by: Rubén Justo <rjusto@gmail.com>\n---\n add-patch.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/add-patch.c b/add-patch.c\nindex a06dd18985..0997d4af73 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -1646,6 +1646,7 @@ static int patch_update_file(struct add_p_state *s,\n \t\t\t\terr(s, _(\"No hunk matches the given pattern\"));\n \t\t\t\tbreak;\n \t\t\t}\n+\t\t\tregfree(&regex);\n \t\t\thunk_index = i;\n \t\t} else if (s->answer.buf[0] == 's') {\n \t\t\tsize_t splittable_into = hunk->splittable_into;\n-- \n2.45.0.rc0.4.g08f77eb516\n"},{"id":"493260","messageId":"a886c086-4b2e-4d41-ad40-b3fca20de42a@gmail.com","threadId":"61347","inReplyTo":"b7b2d0d2-245e-440f-a7cc-fa0df1ce73ad@gmail.com","subject":"[PATCH 4/4] add: plug a leak on interactive_add","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-04-21T10:29:00Z","receivedAt":"2024-04-21T10:29:10Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"Plug a leak we have since 5a76aff1a6 (add: convert to use\nparse_pathspec, 2013-07-14).\n\nThis leak can be triggered with:\n    $ git add -p anything\n\nFixing this leak allows us to mark as leak-free the following tests:\n\n    + t3701-add-interactive.sh\n    + t7514-commit-patch.sh\n\nMark them with \"TEST_PASSES_SANITIZE_LEAK=true\" to notice and fix\npromply any new leak that may be introduced and triggered by them in the\nfuture.\n\nSigned-off-by: Rubén Justo <rjusto@gmail.com>\n---\n builtin/add.c              | 9 ++++++---\n t/t3701-add-interactive.sh | 1 +\n t/t7514-commit-patch.sh    | 2 ++\n 3 files changed, 9 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex ae723bc85e..b7d3ff1e28 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -150,7 +150,7 @@ static int refresh(int verbose, const struct pathspec *pathspec)\n int interactive_add(const char **argv, const char *prefix, int patch)\n {\n \tstruct pathspec pathspec;\n-\tint unused;\n+\tint unused, ret;\n \n \tif (!git_config_get_bool(\"add.interactive.usebuiltin\", &unused))\n \t\twarning(_(\"the add.interactive.useBuiltin setting has been removed!\\n\"\n@@ -163,9 +163,12 @@ int interactive_add(const char **argv, const char *prefix, int patch)\n \t\t       prefix, argv);\n \n \tif (patch)\n-\t\treturn !!run_add_p(the_repository, ADD_P_ADD, NULL, &pathspec);\n+\t\tret = !!run_add_p(the_repository, ADD_P_ADD, NULL, &pathspec);\n \telse\n-\t\treturn !!run_add_i(the_repository, &pathspec);\n+\t\tret = !!run_add_i(the_repository, &pathspec);\n+\n+\tclear_pathspec(&pathspec);\n+\treturn ret;\n }\n \n static int edit_patch(int argc, const char **argv, const char *prefix)\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex bc55255b0a..04d8333373 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -4,6 +4,7 @@ test_description='add -i basic tests'\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-terminal.sh\n \ndiff --git a/t/t7514-commit-patch.sh b/t/t7514-commit-patch.sh\nindex b4de10a5dd..03ba0c0e73 100755\n--- a/t/t7514-commit-patch.sh\n+++ b/t/t7514-commit-patch.sh\n@@ -1,6 +1,8 @@\n #!/bin/sh\n \n test_description='hunk edit with \"commit -p -m\"'\n+\n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_expect_success 'setup (initial)' '\n-- \n2.45.0.rc0.4.g08f77eb516\n"},{"id":"493299","messageId":"ZiX-UB50y03h5JJc@tanuki","threadId":"61347","inReplyTo":"b7b2d0d2-245e-440f-a7cc-fa0df1ce73ad@gmail.com","subject":"Re: [PATCH 0/4] mark t3701-add-interactive.sh as leak-free","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-22T06:06:08Z","receivedAt":"2024-04-22T06:06:16Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sun, Apr 21, 2024 at 12:22:46PM +0200, Rubén Justo wrote:\n> Rubén Justo (4):\n>   apply: plug a leak in apply_data\n>   add-interactive: plug a leak in get_untracked_files\n>   add-patch: plug a leak handling the '/' command\n>   add: plug a leak on interactive_add\n\nAll of these patches look obviously good to me. Thanks!\n\nPatrick\n"},{"id":"493305","messageId":"xmqqo7a1za6q.fsf@gitster.g","threadId":"61347","inReplyTo":"b7b2d0d2-245e-440f-a7cc-fa0df1ce73ad@gmail.com","subject":"Re: [PATCH 0/4] mark t3701-add-interactive.sh as leak-free","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-22T15:30:53Z","receivedAt":"2024-04-22T15:31:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo <rjusto@gmail.com> writes:\n\n> Rubén Justo (4):\n>   apply: plug a leak in apply_data\n>   add-interactive: plug a leak in get_untracked_files\n>   add-patch: plug a leak handling the '/' command\n>   add: plug a leak on interactive_add\n\nThanks, will queue.\n\n>\n>  add-interactive.c          | 1 +\n>  add-patch.c                | 1 +\n>  apply.c                    | 4 +++-\n>  builtin/add.c              | 9 ++++++---\n>  t/t2016-checkout-patch.sh  | 1 +\n>  t/t3701-add-interactive.sh | 1 +\n>  t/t4103-apply-binary.sh    | 1 +\n>  t/t4104-apply-boundary.sh  | 1 +\n>  t/t4113-apply-ending.sh    | 1 +\n>  t/t4117-apply-reject.sh    | 1 +\n>  t/t4123-apply-shrink.sh    | 1 +\n>  t/t4252-am-options.sh      | 2 ++\n>  t/t4258-am-quoted-cr.sh    | 1 +\n>  t/t7514-commit-patch.sh    | 2 ++\n>  14 files changed, 23 insertions(+), 4 deletions(-)\n"},{"id":"493306","messageId":"74a6465c-01b6-4a65-8491-751244d37c82@gmail.com","threadId":"61347","inReplyTo":"8e6ab088-2026-43e7-a869-b1c7185ee765@gmail.com","subject":"Re: [PATCH 1/4] apply: plug a leak in apply_data","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-04-22T15:41:18Z","receivedAt":"2024-04-22T15:41:22Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Rubén\n\nOn 21/04/2024 11:28, Rubén Justo wrote:\n> Plug a leak we have since cfb6f9acc3 (apply: accept -3/--3way command\n> line option, 2012-05-08).\n\nLooking at that commit I see\n\n-        if (apply_fragments(&image, patch) < 0)\n-                return -1; /* note with --reject this succeeds. */\n+        if (apply_fragments(&image, patch) < 0) {\n+                /* Note: with --reject, apply_fragments() returns 0 */\n+                if (!threeway || try_threeway(&image, patch, st, ce) < 0)\n+                        return -1;\n+        }\n\nSo the leak existed before that commit. Indeed it looks\nlike the leak predates the introduction of struct image in b94f2eda99\n(builtin-apply.c: make it more line oriented, 2008-01-26) and when\nthe patch does not apply we have been leaking the buffer passed to\napply_fragments() since the beginning of the builtin apply added in\nac6245e31a3 (Builtin git-apply., 2006-05-23)\n\nThe fix itself looks good to me\n\nBest Wishes\n\nPhillip\n\n\n> This leak can be triggered with:\n> \n>      $ echo foo >file\n>      $ git add file && git commit -m file\n>      $ echo bar >file\n>      $ git diff file >diff\n>      $ sed s/foo/frotz/ <diff >baddiff\n>      $ git apply --cached <baddiffén\n> \n> Fixing this leak allows us to mark as leak-free the following tests:\n> \n>      + t2016-checkout-patch.sh\n>      + t4103-apply-binary.sh\n>      + t4104-apply-boundary.sh\n>      + t4113-apply-ending.sh\n>      + t4117-apply-reject.sh\n>      + t4123-apply-shrink.sh\n>      + t4252-am-options.sh\n>      + t4258-am-quoted-cr.sh\n> \n> Mark them with \"TEST_PASSES_SANITIZE_LEAK=true\" to notice and fix\n> promply any new leak that may be introduced and triggered by them in the\n> future.\n> \n> Signed-off-by: Rubén Justo <rjusto@gmail.com>\n> ---\n>   apply.c                   | 4 +++-\n>   t/t2016-checkout-patch.sh | 1 +\n>   t/t4103-apply-binary.sh   | 1 +\n>   t/t4104-apply-boundary.sh | 1 +\n>   t/t4113-apply-ending.sh   | 1 +\n>   t/t4117-apply-reject.sh   | 1 +\n>   t/t4123-apply-shrink.sh   | 1 +\n>   t/t4252-am-options.sh     | 2 ++\n>   t/t4258-am-quoted-cr.sh   | 1 +\n>   9 files changed, 12 insertions(+), 1 deletion(-)\n> \n> diff --git a/apply.c b/apply.c\n> index 34f20326a7..2f752d71a8 100644\n> --- a/apply.c\n> +++ b/apply.c\n> @@ -3712,8 +3712,10 @@ static int apply_data(struct apply_state *state, struct patch *patch,\n>   \t\t\tfprintf(stderr, _(\"Falling back to direct application...\\n\"));\n>   \n>   \t\t/* Note: with --reject, apply_fragments() returns 0 */\n> -\t\tif (patch->direct_to_threeway || apply_fragments(state, &image, patch) < 0)\n> +\t\tif (patch->direct_to_threeway || apply_fragments(state, &image, patch) < 0) {\n> +\t\t\tclear_image(&image);\n>   \t\t\treturn -1;\n> +\t\t}\n>   \t}\n>   \tpatch->result = image.buf;\n>   \tpatch->resultsize = image.len;\n> diff --git a/t/t2016-checkout-patch.sh b/t/t2016-checkout-patch.sh\n> index c4f9bf09aa..c40b661ac1 100755\n> --- a/t/t2016-checkout-patch.sh\n> +++ b/t/t2016-checkout-patch.sh\n> @@ -2,6 +2,7 @@\n>   \n>   test_description='git checkout --patch'\n>   \n> +TEST_PASSES_SANITIZE_LEAK=true\n>   . ./lib-patch-mode.sh\n>   \n>   test_expect_success 'setup' '\n> diff --git a/t/t4103-apply-binary.sh b/t/t4103-apply-binary.sh\n> index d370ecfe0d..144619ab87 100755\n> --- a/t/t4103-apply-binary.sh\n> +++ b/t/t4103-apply-binary.sh\n> @@ -9,6 +9,7 @@ test_description='git apply handling binary patches\n>   GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n>   export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n>   \n> +TEST_PASSES_SANITIZE_LEAK=true\n>   . ./test-lib.sh\n>   \n>   test_expect_success 'setup' '\n> diff --git a/t/t4104-apply-boundary.sh b/t/t4104-apply-boundary.sh\n> index 71ef4132d1..dc501aac38 100755\n> --- a/t/t4104-apply-boundary.sh\n> +++ b/t/t4104-apply-boundary.sh\n> @@ -5,6 +5,7 @@\n>   \n>   test_description='git apply boundary tests'\n>   \n> +TEST_PASSES_SANITIZE_LEAK=true\n>   . ./test-lib.sh\n>   \n>   L=\"c d e f g h i j k l m n o p q r s t u v w x\"\n> diff --git a/t/t4113-apply-ending.sh b/t/t4113-apply-ending.sh\n> index 66fa51591e..2c65c6a169 100755\n> --- a/t/t4113-apply-ending.sh\n> +++ b/t/t4113-apply-ending.sh\n> @@ -6,6 +6,7 @@\n>   test_description='git apply trying to add an ending line.\n>   \n>   '\n> +TEST_PASSES_SANITIZE_LEAK=true\n>   . ./test-lib.sh\n>   \n>   # setup\n> diff --git a/t/t4117-apply-reject.sh b/t/t4117-apply-reject.sh\n> index c86d05a96f..4d15ccd28e 100755\n> --- a/t/t4117-apply-reject.sh\n> +++ b/t/t4117-apply-reject.sh\n> @@ -7,6 +7,7 @@ test_description='git apply with rejects\n>   \n>   '\n>   \n> +TEST_PASSES_SANITIZE_LEAK=true\n>   . ./test-lib.sh\n>   \n>   test_expect_success setup '\n> diff --git a/t/t4123-apply-shrink.sh b/t/t4123-apply-shrink.sh\n> index 3ef84619f5..3601c0c5dc 100755\n> --- a/t/t4123-apply-shrink.sh\n> +++ b/t/t4123-apply-shrink.sh\n> @@ -2,6 +2,7 @@\n>   \n>   test_description='apply a patch that is larger than the preimage'\n>   \n> +TEST_PASSES_SANITIZE_LEAK=true\n>   . ./test-lib.sh\n>   \n>   cat >F  <<\\EOF\n> diff --git a/t/t4252-am-options.sh b/t/t4252-am-options.sh\n> index e758e634a3..5b680dc755 100755\n> --- a/t/t4252-am-options.sh\n> +++ b/t/t4252-am-options.sh\n> @@ -1,6 +1,8 @@\n>   #!/bin/sh\n>   \n>   test_description='git am with options and not losing them'\n> +\n> +TEST_PASSES_SANITIZE_LEAK=true\n>   . ./test-lib.sh\n>   \n>   tm=\"$TEST_DIRECTORY/t4252\"\n> diff --git a/t/t4258-am-quoted-cr.sh b/t/t4258-am-quoted-cr.sh\n> index 201915b45a..3573c9147f 100755\n> --- a/t/t4258-am-quoted-cr.sh\n> +++ b/t/t4258-am-quoted-cr.sh\n> @@ -2,6 +2,7 @@\n>   \n>   test_description='test am --quoted-cr=<action>'\n>   \n> +TEST_PASSES_SANITIZE_LEAK=true\n>   . ./test-lib.sh\n>   \n>   DATA=\"$TEST_DIRECTORY/t4258\"\n"},{"id":"493307","messageId":"55f1da17-6f07-4b25-9cf2-0de69bfb5005@gmail.com","threadId":"61347","inReplyTo":"a886c086-4b2e-4d41-ad40-b3fca20de42a@gmail.com","subject":"Re: [PATCH 4/4] add: plug a leak on interactive_add","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-04-22T15:43:59Z","receivedAt":"2024-04-22T15:44:04Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Rubén\n\nOn 21/04/2024 11:29, Rubén Justo wrote:\n> Plug a leak we have since 5a76aff1a6 (add: convert to use\n> parse_pathspec, 2013-07-14).\n> \n> This leak can be triggered with:\n>      $ git add -p anything\n> \n> Fixing this leak allows us to mark as leak-free the following tests:\n> \n>      + t3701-add-interactive.sh\n>      + t7514-commit-patch.sh\n> \n> Mark them with \"TEST_PASSES_SANITIZE_LEAK=true\" to notice and fix\n> promply any new leak that may be introduced and triggered by them in the\n> future.\n\nThis makes me wonder if we're freeing the pathspec properly when using \n'--patch' in checkout, reset, restore and stash.\n\nBest Wishes\n\nPhillip\n\n> Signed-off-by: Rubén Justo <rjusto@gmail.com>\n> ---\n>   builtin/add.c              | 9 ++++++---\n>   t/t3701-add-interactive.sh | 1 +\n>   t/t7514-commit-patch.sh    | 2 ++\n>   3 files changed, 9 insertions(+), 3 deletions(-)\n> \n> diff --git a/builtin/add.c b/builtin/add.c\n> index ae723bc85e..b7d3ff1e28 100644\n> --- a/builtin/add.c\n> +++ b/builtin/add.c\n> @@ -150,7 +150,7 @@ static int refresh(int verbose, const struct pathspec *pathspec)\n>   int interactive_add(const char **argv, const char *prefix, int patch)\n>   {\n>   \tstruct pathspec pathspec;\n> -\tint unused;\n> +\tint unused, ret;\n>   \n>   \tif (!git_config_get_bool(\"add.interactive.usebuiltin\", &unused))\n>   \t\twarning(_(\"the add.interactive.useBuiltin setting has been removed!\\n\"\n> @@ -163,9 +163,12 @@ int interactive_add(const char **argv, const char *prefix, int patch)\n>   \t\t       prefix, argv);\n>   \n>   \tif (patch)\n> -\t\treturn !!run_add_p(the_repository, ADD_P_ADD, NULL, &pathspec);\n> +\t\tret = !!run_add_p(the_repository, ADD_P_ADD, NULL, &pathspec);\n>   \telse\n> -\t\treturn !!run_add_i(the_repository, &pathspec);\n> +\t\tret = !!run_add_i(the_repository, &pathspec);\n> +\n> +\tclear_pathspec(&pathspec);\n> +\treturn ret;\n>   }\n>   \n>   static int edit_patch(int argc, const char **argv, const char *prefix)\n> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\n> index bc55255b0a..04d8333373 100755\n> --- a/t/t3701-add-interactive.sh\n> +++ b/t/t3701-add-interactive.sh\n> @@ -4,6 +4,7 @@ test_description='add -i basic tests'\n>   GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n>   export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n>   \n> +TEST_PASSES_SANITIZE_LEAK=true\n>   . ./test-lib.sh\n>   . \"$TEST_DIRECTORY\"/lib-terminal.sh\n>   \n> diff --git a/t/t7514-commit-patch.sh b/t/t7514-commit-patch.sh\n> index b4de10a5dd..03ba0c0e73 100755\n> --- a/t/t7514-commit-patch.sh\n> +++ b/t/t7514-commit-patch.sh\n> @@ -1,6 +1,8 @@\n>   #!/bin/sh\n>   \n>   test_description='hunk edit with \"commit -p -m\"'\n> +\n> +TEST_PASSES_SANITIZE_LEAK=true\n>   . ./test-lib.sh\n>   \n>   test_expect_success 'setup (initial)' '\n"},{"id":"493308","messageId":"xmqqwmopxuqe.fsf@gitster.g","threadId":"61347","inReplyTo":"88b119aa-c1c2-435a-baaa-9422388bf0ec@gmail.com","subject":"Re: [PATCH 2/4] add-interactive: plug a leak in get_untracked_files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-22T15:50:01Z","receivedAt":"2024-04-22T15:50:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo <rjusto@gmail.com> writes:\n\n> Plug a leak we have since ab1e1cccaf (built-in add -i: re-implement\n> `add-untracked` in C, 2019-11-29).\n>\n> This leak can be triggered with:\n>\n> \t$ echo a | git add -i\n>\n> As a curiosity, we have a somewhat similar function in builtin/stash.c,\n> which correctly frees the memory.\n\nYup, \n\n    $ git grep -W -e fill_directory -e dir_clear -e setup_standard_excludes \\*.c\n\nmakes an interesting read ;-)\n\nThe fix looks good.  Thanks.\n\n> Signed-off-by: Rubén Justo <rjusto@gmail.com>\n> ---\n>  add-interactive.c | 1 +\n>  1 file changed, 1 insertion(+)\n>\n> diff --git a/add-interactive.c b/add-interactive.c\n> index 6bf87e7ae7..e17602b5e4 100644\n> --- a/add-interactive.c\n> +++ b/add-interactive.c\n> @@ -865,6 +865,7 @@ static int get_untracked_files(struct repository *r,\n>  \t}\n>  \n>  \tstrbuf_release(&buf);\n> +\tdir_clear(&dir);\n>  \treturn 0;\n>  }\n"},{"id":"493324","messageId":"78f38e0f-7456-42d7-b0d1-887944bb01c9@gmail.com","threadId":"61347","inReplyTo":"74a6465c-01b6-4a65-8491-751244d37c82@gmail.com","subject":"Re: [PATCH 1/4] apply: plug a leak in apply_data","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-04-22T22:04:16Z","receivedAt":"2024-04-22T22:04:20Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On Mon, Apr 22, 2024 at 04:41:18PM +0100, Phillip Wood wrote:\n\n> On 21/04/2024 11:28, Rubén Justo wrote:\n> > Plug a leak we have since cfb6f9acc3 (apply: accept -3/--3way command\n> > line option, 2012-05-08).\n> \n> Looking at that commit I see\n> \n> -        if (apply_fragments(&image, patch) < 0)\n> -                return -1; /* note with --reject this succeeds. */\n> +        if (apply_fragments(&image, patch) < 0) {\n> +                /* Note: with --reject, apply_fragments() returns 0 */\n> +                if (!threeway || try_threeway(&image, patch, st, ce) < 0)\n> +                        return -1;\n> +        }\n> \n> So the leak existed before that commit. Indeed it looks\n> like the leak predates the introduction of struct image in b94f2eda99\n> (builtin-apply.c: make it more line oriented, 2008-01-26) and when\n> the patch does not apply we have been leaking the buffer passed to\n> apply_fragments() since the beginning of the builtin apply added in\n> ac6245e31a3 (Builtin git-apply., 2006-05-23)\n\nYou are very right.\n\nI saw that commit.  I reviewed again my notes and I recorded the hash of\nthe previous hop;  the one that my message, incorrectly, refers to.\nI'll reroll to fix it.\n\nThank you very much for your attention.\n"},{"id":"493325","messageId":"xmqqjzkpt4n2.fsf@gitster.g","threadId":"61347","inReplyTo":"78f38e0f-7456-42d7-b0d1-887944bb01c9@gmail.com","subject":"Re: [PATCH 1/4] apply: plug a leak in apply_data","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-22T22:27:13Z","receivedAt":"2024-04-22T22:27:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo <rjusto@gmail.com> writes:\n\n>> So the leak existed before that commit. Indeed it looks\n>> like the leak predates the introduction of struct image in b94f2eda99\n>> (builtin-apply.c: make it more line oriented, 2008-01-26) and when\n>> the patch does not apply we have been leaking the buffer passed to\n>> apply_fragments() since the beginning of the builtin apply added in\n>> ac6245e31a3 (Builtin git-apply., 2006-05-23)\n>\n> You are very right.\n>\n> I saw that commit.  I reviewed again my notes and I recorded the hash of\n> the previous hop;  the one that my message, incorrectly, refers to.\n> I'll reroll to fix it.\n\nFWIW, the very first version of apply_one_fragment() in 3cca928d\n(git-apply: first cut at actually checking fragment data,\n2005-06-05) already had leak; it \"returns -1\" upon an error without\nfreeing old/new buffers.  Over the years, the shape of the code\nchanged, but the leak survived.\n\n"},{"id":"493326","messageId":"69f86760-96ff-4c62-9649-4627652b7c19@gmail.com","threadId":"61347","inReplyTo":"b7b2d0d2-245e-440f-a7cc-fa0df1ce73ad@gmail.com","subject":"[PATCH v2 0/4] mark t3701-add-interactive.sh as leak-free","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-04-22T22:51:04Z","receivedAt":"2024-04-22T22:51:07Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"Rubén Justo (4):\n  apply: plug a leak in apply_data\n  add-interactive: plug a leak in get_untracked_files\n  add-patch: plug a leak handling the '/' command\n  add: plug a leak on interactive_add\n\n add-interactive.c          | 1 +\n add-patch.c                | 1 +\n apply.c                    | 4 +++-\n builtin/add.c              | 9 ++++++---\n t/t2016-checkout-patch.sh  | 1 +\n t/t3701-add-interactive.sh | 1 +\n t/t4103-apply-binary.sh    | 1 +\n t/t4104-apply-boundary.sh  | 1 +\n t/t4113-apply-ending.sh    | 1 +\n t/t4117-apply-reject.sh    | 1 +\n t/t4123-apply-shrink.sh    | 1 +\n t/t4252-am-options.sh      | 2 ++\n t/t4258-am-quoted-cr.sh    | 1 +\n t/t7514-commit-patch.sh    | 2 ++\n 14 files changed, 23 insertions(+), 4 deletions(-)\n\nRange-diff against v1:\n1:  18e4c7f653 ! 1:  75cb700eab apply: plug a leak in apply_data\n    @@ Metadata\n      ## Commit message ##\n         apply: plug a leak in apply_data\n     \n    -    Plug a leak we have since cfb6f9acc3 (apply: accept -3/--3way command\n    -    line option, 2012-05-08).\n    +    We have an execution path in apply_data that leaks the local struct\n    +    image.  Plug it.\n     \n         This leak can be triggered with:\n     \n2:  21d6c2dd16 = 2:  bee002b0ae add-interactive: plug a leak in get_untracked_files\n3:  f69a6a30a1 = 3:  7d1a94dd91 add-patch: plug a leak handling the '/' command\n4:  5d9607f153 = 4:  fff7e48949 add: plug a leak on interactive_add\n-- \n2.45.0.rc0.4.gfff7e48949\n"},{"id":"493327","messageId":"5e97fd0e-8aca-43dd-9f6e-a8cb28fa01b8@gmail.com","threadId":"61347","inReplyTo":"69f86760-96ff-4c62-9649-4627652b7c19@gmail.com","subject":"[PATCH v2 1/4] apply: plug a leak in apply_data","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-04-22T22:54:05Z","receivedAt":"2024-04-22T22:54:08Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"We have an execution path in apply_data that leaks the local struct\nimage.  Plug it.\n\nThis leak can be triggered with:\n\n    $ echo foo >file\n    $ git add file && git commit -m file\n    $ echo bar >file\n    $ git diff file >diff\n    $ sed s/foo/frotz/ <diff >baddiff\n    $ git apply --cached <baddiff\n\nFixing this leak allows us to mark as leak-free the following tests:\n\n    + t2016-checkout-patch.sh\n    + t4103-apply-binary.sh\n    + t4104-apply-boundary.sh\n    + t4113-apply-ending.sh\n    + t4117-apply-reject.sh\n    + t4123-apply-shrink.sh\n    + t4252-am-options.sh\n    + t4258-am-quoted-cr.sh\n\nMark them with \"TEST_PASSES_SANITIZE_LEAK=true\" to notice and fix\npromply any new leak that may be introduced and triggered by them in the\nfuture.\n\nSigned-off-by: Rubén Justo <rjusto@gmail.com>\n---\n apply.c                   | 4 +++-\n t/t2016-checkout-patch.sh | 1 +\n t/t4103-apply-binary.sh   | 1 +\n t/t4104-apply-boundary.sh | 1 +\n t/t4113-apply-ending.sh   | 1 +\n t/t4117-apply-reject.sh   | 1 +\n t/t4123-apply-shrink.sh   | 1 +\n t/t4252-am-options.sh     | 2 ++\n t/t4258-am-quoted-cr.sh   | 1 +\n 9 files changed, 12 insertions(+), 1 deletion(-)\n\ndiff --git a/apply.c b/apply.c\nindex 34f20326a7..2f752d71a8 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -3712,8 +3712,10 @@ static int apply_data(struct apply_state *state, struct patch *patch,\n \t\t\tfprintf(stderr, _(\"Falling back to direct application...\\n\"));\n \n \t\t/* Note: with --reject, apply_fragments() returns 0 */\n-\t\tif (patch->direct_to_threeway || apply_fragments(state, &image, patch) < 0)\n+\t\tif (patch->direct_to_threeway || apply_fragments(state, &image, patch) < 0) {\n+\t\t\tclear_image(&image);\n \t\t\treturn -1;\n+\t\t}\n \t}\n \tpatch->result = image.buf;\n \tpatch->resultsize = image.len;\ndiff --git a/t/t2016-checkout-patch.sh b/t/t2016-checkout-patch.sh\nindex c4f9bf09aa..c40b661ac1 100755\n--- a/t/t2016-checkout-patch.sh\n+++ b/t/t2016-checkout-patch.sh\n@@ -2,6 +2,7 @@\n \n test_description='git checkout --patch'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./lib-patch-mode.sh\n \n test_expect_success 'setup' '\ndiff --git a/t/t4103-apply-binary.sh b/t/t4103-apply-binary.sh\nindex d370ecfe0d..144619ab87 100755\n--- a/t/t4103-apply-binary.sh\n+++ b/t/t4103-apply-binary.sh\n@@ -9,6 +9,7 @@ test_description='git apply handling binary patches\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_expect_success 'setup' '\ndiff --git a/t/t4104-apply-boundary.sh b/t/t4104-apply-boundary.sh\nindex 71ef4132d1..dc501aac38 100755\n--- a/t/t4104-apply-boundary.sh\n+++ b/t/t4104-apply-boundary.sh\n@@ -5,6 +5,7 @@\n \n test_description='git apply boundary tests'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n L=\"c d e f g h i j k l m n o p q r s t u v w x\"\ndiff --git a/t/t4113-apply-ending.sh b/t/t4113-apply-ending.sh\nindex 66fa51591e..2c65c6a169 100755\n--- a/t/t4113-apply-ending.sh\n+++ b/t/t4113-apply-ending.sh\n@@ -6,6 +6,7 @@\n test_description='git apply trying to add an ending line.\n \n '\n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n # setup\ndiff --git a/t/t4117-apply-reject.sh b/t/t4117-apply-reject.sh\nindex c86d05a96f..4d15ccd28e 100755\n--- a/t/t4117-apply-reject.sh\n+++ b/t/t4117-apply-reject.sh\n@@ -7,6 +7,7 @@ test_description='git apply with rejects\n \n '\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_expect_success setup '\ndiff --git a/t/t4123-apply-shrink.sh b/t/t4123-apply-shrink.sh\nindex 3ef84619f5..3601c0c5dc 100755\n--- a/t/t4123-apply-shrink.sh\n+++ b/t/t4123-apply-shrink.sh\n@@ -2,6 +2,7 @@\n \n test_description='apply a patch that is larger than the preimage'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n cat >F  <<\\EOF\ndiff --git a/t/t4252-am-options.sh b/t/t4252-am-options.sh\nindex e758e634a3..5b680dc755 100755\n--- a/t/t4252-am-options.sh\n+++ b/t/t4252-am-options.sh\n@@ -1,6 +1,8 @@\n #!/bin/sh\n \n test_description='git am with options and not losing them'\n+\n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n tm=\"$TEST_DIRECTORY/t4252\"\ndiff --git a/t/t4258-am-quoted-cr.sh b/t/t4258-am-quoted-cr.sh\nindex 201915b45a..3573c9147f 100755\n--- a/t/t4258-am-quoted-cr.sh\n+++ b/t/t4258-am-quoted-cr.sh\n@@ -2,6 +2,7 @@\n \n test_description='test am --quoted-cr=<action>'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n DATA=\"$TEST_DIRECTORY/t4258\"\n-- \n2.45.0.rc0.4.gfff7e48949\n"},{"id":"493328","messageId":"791e97af-f233-4c06-9c5b-2b9ff758736e@gmail.com","threadId":"61347","inReplyTo":"69f86760-96ff-4c62-9649-4627652b7c19@gmail.com","subject":"[PATCH v2 2/4] add-interactive: plug a leak in get_untracked_files","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-04-22T22:54:08Z","receivedAt":"2024-04-22T22:54:10Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"Plug a leak we have since ab1e1cccaf (built-in add -i: re-implement\n`add-untracked` in C, 2019-11-29).\n\nThis leak can be triggered with:\n\n\t$ echo a | git add -i\n\nAs a curiosity, we have a somewhat similar function in builtin/stash.c,\nwhich correctly frees the memory.\n\nSigned-off-by: Rubén Justo <rjusto@gmail.com>\n---\n add-interactive.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/add-interactive.c b/add-interactive.c\nindex 6bf87e7ae7..e17602b5e4 100644\n--- a/add-interactive.c\n+++ b/add-interactive.c\n@@ -865,6 +865,7 @@ static int get_untracked_files(struct repository *r,\n \t}\n \n \tstrbuf_release(&buf);\n+\tdir_clear(&dir);\n \treturn 0;\n }\n \n-- \n2.45.0.rc0.4.gfff7e48949\n"},{"id":"493329","messageId":"f35a22a9-bbb1-491c-9f1f-e8c77f18fb9f@gmail.com","threadId":"61347","inReplyTo":"69f86760-96ff-4c62-9649-4627652b7c19@gmail.com","subject":"[PATCH v2 3/4] add-patch: plug a leak handling the '/' command","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-04-22T22:54:14Z","receivedAt":"2024-04-22T22:54:17Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"Plug a leak we have since d6cf873340 (built-in add -p: implement the '/'\n(\"search regex\") command, 2019-12-13).\n\nThis leak can be triggered with:\n\n    $ printf \"A\\n\\nB\\n\" >file\n    $ git add file && git commit -m file\n    $ printf \"AA\\n\\nBB\\n\" >file\n    $ printf \"s\\n/ .\\n\" >lines\n    $ git add -p <lines\n\nSigned-off-by: Rubén Justo <rjusto@gmail.com>\n---\n add-patch.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/add-patch.c b/add-patch.c\nindex a06dd18985..0997d4af73 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -1646,6 +1646,7 @@ static int patch_update_file(struct add_p_state *s,\n \t\t\t\terr(s, _(\"No hunk matches the given pattern\"));\n \t\t\t\tbreak;\n \t\t\t}\n+\t\t\tregfree(&regex);\n \t\t\thunk_index = i;\n \t\t} else if (s->answer.buf[0] == 's') {\n \t\t\tsize_t splittable_into = hunk->splittable_into;\n-- \n2.45.0.rc0.4.gfff7e48949\n"},{"id":"493330","messageId":"f20291e7-fc2b-4ce4-a471-84cf76988f6e@gmail.com","threadId":"61347","inReplyTo":"69f86760-96ff-4c62-9649-4627652b7c19@gmail.com","subject":"[PATCH v2 4/4] add: plug a leak on interactive_add","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-04-22T22:54:18Z","receivedAt":"2024-04-22T22:54:20Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"Plug a leak we have since 5a76aff1a6 (add: convert to use\nparse_pathspec, 2013-07-14).\n\nThis leak can be triggered with:\n    $ git add -p anything\n\nFixing this leak allows us to mark as leak-free the following tests:\n\n    + t3701-add-interactive.sh\n    + t7514-commit-patch.sh\n\nMark them with \"TEST_PASSES_SANITIZE_LEAK=true\" to notice and fix\npromply any new leak that may be introduced and triggered by them in the\nfuture.\n\nSigned-off-by: Rubén Justo <rjusto@gmail.com>\n---\n builtin/add.c              | 9 ++++++---\n t/t3701-add-interactive.sh | 1 +\n t/t7514-commit-patch.sh    | 2 ++\n 3 files changed, 9 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex ae723bc85e..b7d3ff1e28 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -150,7 +150,7 @@ static int refresh(int verbose, const struct pathspec *pathspec)\n int interactive_add(const char **argv, const char *prefix, int patch)\n {\n \tstruct pathspec pathspec;\n-\tint unused;\n+\tint unused, ret;\n \n \tif (!git_config_get_bool(\"add.interactive.usebuiltin\", &unused))\n \t\twarning(_(\"the add.interactive.useBuiltin setting has been removed!\\n\"\n@@ -163,9 +163,12 @@ int interactive_add(const char **argv, const char *prefix, int patch)\n \t\t       prefix, argv);\n \n \tif (patch)\n-\t\treturn !!run_add_p(the_repository, ADD_P_ADD, NULL, &pathspec);\n+\t\tret = !!run_add_p(the_repository, ADD_P_ADD, NULL, &pathspec);\n \telse\n-\t\treturn !!run_add_i(the_repository, &pathspec);\n+\t\tret = !!run_add_i(the_repository, &pathspec);\n+\n+\tclear_pathspec(&pathspec);\n+\treturn ret;\n }\n \n static int edit_patch(int argc, const char **argv, const char *prefix)\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex bc55255b0a..04d8333373 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -4,6 +4,7 @@ test_description='add -i basic tests'\n GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-terminal.sh\n \ndiff --git a/t/t7514-commit-patch.sh b/t/t7514-commit-patch.sh\nindex b4de10a5dd..03ba0c0e73 100755\n--- a/t/t7514-commit-patch.sh\n+++ b/t/t7514-commit-patch.sh\n@@ -1,6 +1,8 @@\n #!/bin/sh\n \n test_description='hunk edit with \"commit -p -m\"'\n+\n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_expect_success 'setup (initial)' '\n-- \n2.45.0.rc0.4.gfff7e48949\n"},{"id":"493331","messageId":"effc5e33-562b-4545-a58e-0c6e9a10ad42@gmail.com","threadId":"61347","inReplyTo":"55f1da17-6f07-4b25-9cf2-0de69bfb5005@gmail.com","subject":"Re: [PATCH 4/4] add: plug a leak on interactive_add","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-04-22T23:04:00Z","receivedAt":"2024-04-22T23:04:03Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On 4/22/24 5:43 PM, Phillip Wood wrote:\n\n>> Plug a leak we have since 5a76aff1a6 (add: convert to use\n>> parse_pathspec, 2013-07-14).\n>>\n>> This leak can be triggered with:\n>>      $ git add -p anything\n>>\n>> Fixing this leak allows us to mark as leak-free the following tests:\n>>\n>>      + t3701-add-interactive.sh\n>>      + t7514-commit-patch.sh\n>>\n>> Mark them with \"TEST_PASSES_SANITIZE_LEAK=true\" to notice and fix\n>> promply any new leak that may be introduced and triggered by them in the\n>> future.\n> \n> This makes me wonder if we're freeing the pathspec properly when using '--patch' in checkout, reset, restore and stash.\n\nI haven't checked it thoroughly, but I think we're fine in those cases.\n"},{"id":"493332","messageId":"xmqq4jbtt1vh.fsf@gitster.g","threadId":"61347","inReplyTo":"69f86760-96ff-4c62-9649-4627652b7c19@gmail.com","subject":"Re: [PATCH v2 0/4] mark t3701-add-interactive.sh as leak-free","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-22T23:26:58Z","receivedAt":"2024-04-22T23:27:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo <rjusto@gmail.com> writes:\n\n> Range-diff against v1:\n> 1:  18e4c7f653 ! 1:  75cb700eab apply: plug a leak in apply_data\n>     @@ Metadata\n>       ## Commit message ##\n>          apply: plug a leak in apply_data\n>      \n>     -    Plug a leak we have since cfb6f9acc3 (apply: accept -3/--3way command\n>     -    line option, 2012-05-08).\n>     +    We have an execution path in apply_data that leaks the local struct\n>     +    image.  Plug it.\n\nThis is a nice improvement.\n\nIf a bug existed in an ancient version and survived across evolution\nof the code for a long time, naming a random old version that\nhappened to already have it does not help very much.\n\nSaying something like\n\n    This is an ancient bug whose moral equivalent existed even\n    before the data & code structure became the current shape in\n    commit X\n\nwould give a bit more information, in that X gives a rough estimate\nhow far back in the codebase the fix in the patch can be applied\nmore or less cleanly.  In this case, X would be where the struct\nimage was introduced, I guess?\n\nBut the updated text is good enough and I do not see a need for\nreroll.  Will queue.  Thanks.\n"}]}