| Message ID | xmqq34nfn7ip.fsf@gitster.g (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 448d51d549179bafe47e07e9434210d48fdf55c6 |
| Headers | show |
| Series | transport: fix leak with transport helper URLs | expand |
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
On Thu, Aug 08, 2024 at 06:52:31AM +0200, Patrick Steinhardt wrote: > 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 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
Patrick Steinhardt <ps@pks.im> writes: >> 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.
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)) {
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(-)