{"thread":{"id":"61918","subject":"[PATCH] transport: fix leak with transport helper URLs","startedAt":"2024-08-08T01:11:12Z","lastAt":"2024-08-08T16:14:58Z","messageCount":4,"participants":["Junio C Hamano","Patrick Steinhardt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"500378","messageId":"xmqq34nfn7ip.fsf@gitster.g","threadId":"61918","inReplyTo":null,"subject":"[PATCH] transport: fix leak with transport helper URLs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-08T01:11:10Z","receivedAt":"2024-08-08T01:11:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Transport URLs can be prefixed with \"foo::\", which would tell us that\nthe transport uses a remote helper called \"foo\". We extract the helper\nname by `xstrndup()`ing the prefix before the double-colons, but never\nfree that string.\n\nFix this leak by assigning the result to a separate local variable that\nwe can then free upon returning.\n\nHelped-by: Patrick Steinhardt \nSigned-off-by: Junio C Hamano \n---\n * It turns out that Patrick planned to have an almost identical\n patch, I am queuing this \"independently invented\" one now,\n because a recent update to ls-remote and its tests started\n breaking the CI.\n\n transport.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/transport.c b/transport.c\nindex 12cc5b4d96..7c4af9f56f 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -1115,6 +1115,7 @@ static struct transport_vtable builtin_smart_vtable = {\n struct transport *transport_get(struct remote *remote, const char *url)\n {\n \tconst char *helper;\n+\tchar *helper_to_free = NULL;\n \tconst char *p;\n \tstruct transport *ret = xcalloc(1, sizeof(*ret));\n \n@@ -1139,10 +1140,11 @@ struct transport *transport_get(struct remote *remote, const char *url)\n \twhile (is_urlschemechar(p == url, *p))\n \t\tp++;\n \tif (starts_with(p, \"::\"))\n-\t\thelper = xstrndup(url, p - url);\n+\t\thelper = helper_to_free = xstrndup(url, p - url);\n \n \tif (helper) {\n \t\ttransport_helper_init(ret, helper);\n+\t\tfree(helper_to_free);\n \t} else if (starts_with(url, \"rsync:\")) {\n \t\tdie(_(\"git-over-rsync is no longer supported\"));\n \t} else if (url_is_local_not_ssh(url) && is_file(url) && is_bundle(url, 1)) {\n-- \n2.46.0-313-g2d45963c97\n\n"},{"id":"500384","messageId":"ZrRPD0ggZapZym7E@tanuki","threadId":"61918","inReplyTo":"xmqq34nfn7ip.fsf@gitster.g","subject":"Re: [PATCH] transport: fix leak with transport helper URLs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-08T04:52:31Z","receivedAt":"2024-08-08T04:52:36Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Aug 07, 2024 at 06:11:10PM -0700, Junio C Hamano wrote:\n> Transport URLs can be prefixed with \"foo::\", which would tell us that\n> the transport uses a remote helper called \"foo\". We extract the helper\n> name by `xstrndup()`ing the prefix before the double-colons, but never\n> free that string.\n> \n> Fix this leak by assigning the result to a separate local variable that\n> we can then free upon returning.\n> \n> Helped-by: Patrick Steinhardt \n> Signed-off-by: Junio C Hamano \n\nI saw that you've merged this to `next` already, but: this looks good to\nme, thanks!\n\nPatrick\n"},{"id":"500392","messageId":"ZrRV_HrUArsvRgn8@tanuki","threadId":"61918","inReplyTo":"ZrRPD0ggZapZym7E@tanuki","subject":"Re: [PATCH] transport: fix leak with transport helper URLs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-08T05:22:04Z","receivedAt":"2024-08-08T05:22:10Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Aug 08, 2024 at 06:52:31AM +0200, Patrick Steinhardt wrote:\n> On Wed, Aug 07, 2024 at 06:11:10PM -0700, Junio C Hamano wrote:\n> > Transport URLs can be prefixed with \"foo::\", which would tell us that\n> > the transport uses a remote helper called \"foo\". We extract the helper\n> > name by `xstrndup()`ing the prefix before the double-colons, but never\n> > free that string.\n> > \n> > Fix this leak by assigning the result to a separate local variable that\n> > we can then free upon returning.\n> > \n> > Helped-by: Patrick Steinhardt \n> > Signed-off-by: Junio C Hamano \n> \n> I saw that you've merged this to `next` already, but: this looks good to\n> me, thanks!\n> \n> Patrick\n\nI just noticed that this also makes a couple of test suites pass with\nleak checking enabled. So below diff should likely be applied on top.\n\nPatrick\n\ndiff --git a/t/t5509-fetch-push-namespaces.sh b/t/t5509-fetch-push-namespaces.sh\nindex 31553b48df..05090feaf9 100755\n--- a/t/t5509-fetch-push-namespaces.sh\n+++ b/t/t5509-fetch-push-namespaces.sh\n@@ -4,6 +4,7 @@ test_description='fetch/push involving ref namespaces'\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/t5802-connect-helper.sh b/t/t5802-connect-helper.sh\nindex c6c2661878..dd3e6235cd 100755\n--- a/t/t5802-connect-helper.sh\n+++ b/t/t5802-connect-helper.sh\n@@ -1,6 +1,8 @@\n #!/bin/sh\n\n test_description='ext::cmd remote \"connect\" helper'\n+\n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n\n test_expect_success setup '\ndiff --git a/t/t5814-proto-disable-ext.sh b/t/t5814-proto-disable-ext.sh\nindex 9d6f7dfa2c..6fe1a98b2a 100755\n--- a/t/t5814-proto-disable-ext.sh\n+++ b/t/t5814-proto-disable-ext.sh\n@@ -1,6 +1,8 @@\n #!/bin/sh\n\n test_description='test disabling of remote-helper paths in clone/fetch'\n+\n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n . \"$TEST_DIRECTORY/lib-proto-disable.sh\"\n\ndiff --git a/t/t5815-submodule-protos.sh b/t/t5815-submodule-protos.sh\nindex 4d5956cc18..fe899ee82d 100755\n--- a/t/t5815-submodule-protos.sh\n+++ b/t/t5815-submodule-protos.sh\n@@ -1,6 +1,8 @@\n #!/bin/sh\n\n test_description='test protocol filtering with submodules'\n+\n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-proto-disable.sh\n"},{"id":"500486","messageId":"xmqqo763j8jo.fsf@gitster.g","threadId":"61918","inReplyTo":"ZrRV_HrUArsvRgn8@tanuki","subject":"Re: [PATCH] transport: fix leak with transport helper URLs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-08T16:14:51Z","receivedAt":"2024-08-08T16:14:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n>> I saw that you've merged this to `next` already, but: this looks good to\n>> me, thanks!\n>> \n>> Patrick\n>\n> I just noticed that this also makes a couple of test suites pass with\n> leak checking enabled. So below diff should likely be applied on top.\n\nI'll think about it. I do not want to see too many \"ok, we have now\nmarked this as leak-free\" plus \"ouch, we have unrelated fix and its\ntest now triggers leaks from another subsystem we happen to use\",\nespecially when our primary business is not leak-plugging (e.g., the\n\"ls-remote outside a repo\" use case was a real regression fix even\nthough it sort of falls into \"if it hurts, don't do it\" category).\n\nThanks.\n\n"}]}