threads / patch / 61918

patchtransport: fix leak with transport helper URLs

Subject: [PATCH] transport: fix leak with transport helper URLs

## tl;dr

4 messages between Aug 8, 2024 and Aug 8, 2024. Diffs are folded; open one to read it.

replies: 3people: 2as markdown or json

Junio C Hamano· Aug 8, 2024, 01:11 UTC · lore

Transport URLs can be prefixed with "foo::", which would tell us that the transport uses a remote helper called "foo". We extract the helper name by `xstrndup()`ing the prefix before the double-colons, but never free that string.

Fix this leak by assigning the result to a separate local variable that we can then free upon returning.

Helped-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 * It turns out that Patrick planned to have an almost identical
   patch, I am queuing this "independently invented" one now,
   because a recent update to ls-remote and its tests started
   breaking the CI.

 transport.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Show changes to transport.c +3 −1

diff --git a/transport.c b/transport.c
index 12cc5b4d96..7c4af9f56f 100644
--- a/transport.c
+++ b/transport.c
@@ -1115,6 +1115,7 @@ static struct transport_vtable builtin_smart_vtable = {
 struct transport *transport_get(struct remote *remote, const char *url)
 {
 	const char *helper;
+	char *helper_to_free = NULL;
 	const char *p;
 	struct transport *ret = xcalloc(1, sizeof(*ret));
 
@@ -1139,10 +1140,11 @@ struct transport *transport_get(struct remote *remote, const char *url)
 	while (is_urlschemechar(p == url, *p))
 		p++;
 	if (starts_with(p, "::"))
-		helper = xstrndup(url, p - url);
+		helper = helper_to_free = xstrndup(url, p - url);
 
 	if (helper) {
 		transport_helper_init(ret, helper);
+		free(helper_to_free);
 	} else if (starts_with(url, "rsync:")) {
 		die(_("git-over-rsync is no longer supported"));
 	} else if (url_is_local_not_ssh(url) && is_file(url) && is_bundle(url, 1)) {

-- 
2.46.0-313-g2d45963c97

Patrick Steinhardt· Aug 8, 2024, 04:52 UTC · re: Junio C Hamano · lore

Re: [PATCH] transport: fix leak with transport helper URLs

On Wed, Aug 07, 2024 at 06:11:10PM -0700, Junio C Hamano wrote:

Show 10 quoted lines

> Transport URLs can be prefixed with "foo::", which would tell us that
> the transport uses a remote helper called "foo". We extract the helper
> name by `xstrndup()`ing the prefix before the double-colons, but never
> free that string.
> 
> Fix this leak by assigning the result to a separate local variable that
> we can then free upon returning.
> 
> Helped-by: Patrick Steinhardt <ps@pks.im>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>

I saw that you've merged this to `next` already, but: this looks good to me, thanks!

Patrick

Patrick Steinhardt· Aug 8, 2024, 05:22 UTC · re: Patrick Steinhardt · lore

Re: [PATCH] transport: fix leak with transport helper URLs

On Thu, Aug 08, 2024 at 06:52:31AM +0200, Patrick Steinhardt wrote:

Show 16 quoted lines

> On Wed, Aug 07, 2024 at 06:11:10PM -0700, Junio C Hamano wrote:
> > Transport URLs can be prefixed with "foo::", which would tell us that
> > the transport uses a remote helper called "foo". We extract the helper
> > name by `xstrndup()`ing the prefix before the double-colons, but never
> > free that string.
> > 
> > Fix this leak by assigning the result to a separate local variable that
> > we can then free upon returning.
> > 
> > Helped-by: Patrick Steinhardt <ps@pks.im>
> > Signed-off-by: Junio C Hamano <gitster@pobox.com>
> 
> I saw that you've merged this to `next` already, but: this looks good to
> me, thanks!
> 
> Patrick

I just noticed that this also makes a couple of test suites pass with leak checking enabled. So below diff should likely be applied on top.

Patrick

Show changes to 4 files +7 −0

t/t5509-fetch-push-namespaces.sh, t/t5802-connect-helper.sh, t/t5814-proto-disable-ext.sh, t/t5815-submodule-protos.sh

diff --git a/t/t5509-fetch-push-namespaces.sh b/t/t5509-fetch-push-namespaces.sh
index 31553b48df..05090feaf9 100755
--- a/t/t5509-fetch-push-namespaces.sh
+++ b/t/t5509-fetch-push-namespaces.sh
@@ -4,6 +4,7 @@ test_description='fetch/push involving ref namespaces'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME

+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh

 test_expect_success setup '
diff --git a/t/t5802-connect-helper.sh b/t/t5802-connect-helper.sh
index c6c2661878..dd3e6235cd 100755
--- a/t/t5802-connect-helper.sh
+++ b/t/t5802-connect-helper.sh
@@ -1,6 +1,8 @@
 #!/bin/sh

 test_description='ext::cmd remote "connect" helper'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh

 test_expect_success setup '
diff --git a/t/t5814-proto-disable-ext.sh b/t/t5814-proto-disable-ext.sh
index 9d6f7dfa2c..6fe1a98b2a 100755
--- a/t/t5814-proto-disable-ext.sh
+++ b/t/t5814-proto-disable-ext.sh
@@ -1,6 +1,8 @@
 #!/bin/sh

 test_description='test disabling of remote-helper paths in clone/fetch'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY/lib-proto-disable.sh"

diff --git a/t/t5815-submodule-protos.sh b/t/t5815-submodule-protos.sh
index 4d5956cc18..fe899ee82d 100755
--- a/t/t5815-submodule-protos.sh
+++ b/t/t5815-submodule-protos.sh
@@ -1,6 +1,8 @@
 #!/bin/sh

 test_description='test protocol filtering with submodules'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-proto-disable.sh

Junio C Hamano· Aug 8, 2024, 16:14 UTC · re: Patrick Steinhardt · lore

Re: [PATCH] transport: fix leak with transport helper URLs

Patrick Steinhardt <ps@pks.im> writes:

Show 7 quoted lines

>> I saw that you've merged this to `next` already, but: this looks good to
>> me, thanks!
>> 
>> Patrick
>
> I just noticed that this also makes a couple of test suites pass with
> leak checking enabled. So below diff should likely be applied on top.

I'll think about it. I do not want to see too many "ok, we have now marked this as leak-free" plus "ouch, we have unrelated fix and its test now triggers leaks from another subsystem we happen to use", especially when our primary business is not leak-plugging (e.g., the "ls-remote outside a repo" use case was a real regression fix even though it sort of falls into "if it hurts, don't do it" category).

Thanks.