Re: [PATCH v4 19/19] push: free_refs() the "local_refs" in set_refspecs()
- From
René Scharfe <l.s.r@web.de>
- Date
- Jan 17, 2023, 19:58 UTC
- Message-ID
- <c0c07b89-7eaf-21cd-748e-e14ea57f09fd@web.de>
- In-Reply-To
- <patch-v4-19.19-b3aee41d0b4-20230117T151202Z-avarab@gmail.com>
Am 17.01.23 um 18:11 schrieb Ævar Arnfjörð Bjarmason:
Show 30 quoted lines
> Fix a memory leak that's been with us since this code was added in > ca02465b413 (push: use remote.$name.push as a refmap, 2013-12-03). > > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > --- > builtin/push.c | 1 + > t/t1416-ref-transaction-hooks.sh | 1 + > t/t2402-worktree-list.sh | 1 + > t/t5504-fetch-receive-strict.sh | 1 + > t/t5523-push-upstream.sh | 1 + > t/t5529-push-errors.sh | 2 ++ > t/t5546-receive-limits.sh | 2 ++ > t/t5547-push-quarantine.sh | 2 ++ > t/t5606-clone-options.sh | 1 + > t/t5810-proto-disable-local.sh | 2 ++ > t/t5813-proto-disable-ssh.sh | 2 ++ > t/t7409-submodule-detached-work-tree.sh | 1 + > t/t7416-submodule-dash-url.sh | 2 ++ > t/t7450-bad-git-dotfiles.sh | 2 ++ > 14 files changed, 21 insertions(+) > > diff --git a/builtin/push.c b/builtin/push.c > index 60ac8017e52..f48e4c6a856 100644 > --- a/builtin/push.c > +++ b/builtin/push.c > @@ -129,6 +129,7 @@ static void set_refspecs(const char **refs, int nr, const char *repo) > } else > refspec_append(&rs, ref); > } > + free_refs(local_refs);
OK.
This can still leak local_refs if remote_get() returns NULL and lazy- loading is done over and over. Unlikely to occur in the wild, I bet -- who pushes without a remote? Does it make sense to also check local_refs for NULL already in this patch or is it worth its own series? Not sure.
remote is still leaked if it isn't NULL, though. We'd need to export remote_clear() to release it properly, no? And shouldn't remotes_remote_get_1() call remote_clear() itself before returning NULL? Not simple, separate series.
Show 166 quoted lines
> } > > static int push_url_of_remote(struct remote *remote, const char ***url_p) > diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh > index 27731722a5b..b32ca798f9f 100755 > --- a/t/t1416-ref-transaction-hooks.sh > +++ b/t/t1416-ref-transaction-hooks.sh > @@ -5,6 +5,7 @@ test_description='reference transaction hooks' > 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/t2402-worktree-list.sh b/t/t2402-worktree-list.sh > index 79e0fce2d90..9ad9be0c208 100755 > --- a/t/t2402-worktree-list.sh > +++ b/t/t2402-worktree-list.sh > @@ -5,6 +5,7 @@ test_description='test git worktree list' > 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/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh > index ac4099ca893..14e8af1f3b7 100755 > --- a/t/t5504-fetch-receive-strict.sh > +++ b/t/t5504-fetch-receive-strict.sh > @@ -4,6 +4,7 @@ test_description='fetch/receive strict mode' > 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 and inject "corrupt or missing" object' ' > diff --git a/t/t5523-push-upstream.sh b/t/t5523-push-upstream.sh > index fdb42920564..c9acc076353 100755 > --- a/t/t5523-push-upstream.sh > +++ b/t/t5523-push-upstream.sh > @@ -4,6 +4,7 @@ test_description='push with --set-upstream' > GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main > export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME > > +TEST_PASSES_SANITIZE_LEAK=true > . ./test-lib.sh > . "$TEST_DIRECTORY"/lib-terminal.sh > > diff --git a/t/t5529-push-errors.sh b/t/t5529-push-errors.sh > index ce85fd30ad1..0247137cb36 100755 > --- a/t/t5529-push-errors.sh > +++ b/t/t5529-push-errors.sh > @@ -1,6 +1,8 @@ > #!/bin/sh > > test_description='detect some push errors early (before contacting remote)' > + > +TEST_PASSES_SANITIZE_LEAK=true > . ./test-lib.sh > > test_expect_success 'setup commits' ' > diff --git a/t/t5546-receive-limits.sh b/t/t5546-receive-limits.sh > index 0b0e987fdb7..eed3c9d81ab 100755 > --- a/t/t5546-receive-limits.sh > +++ b/t/t5546-receive-limits.sh > @@ -1,6 +1,8 @@ > #!/bin/sh > > test_description='check receive input limits' > + > +TEST_PASSES_SANITIZE_LEAK=true > . ./test-lib.sh > > # Let's run tests with different unpack limits: 1 and 10000 > diff --git a/t/t5547-push-quarantine.sh b/t/t5547-push-quarantine.sh > index 1876fb34e51..9f899b8c7d7 100755 > --- a/t/t5547-push-quarantine.sh > +++ b/t/t5547-push-quarantine.sh > @@ -1,6 +1,8 @@ > #!/bin/sh > > test_description='check quarantine of objects during push' > + > +TEST_PASSES_SANITIZE_LEAK=true > . ./test-lib.sh > > test_expect_success 'create picky dest repo' ' > diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh > index cf221e92c4d..27f9f776389 100755 > --- a/t/t5606-clone-options.sh > +++ b/t/t5606-clone-options.sh > @@ -4,6 +4,7 @@ test_description='basic clone options' > 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/t5810-proto-disable-local.sh b/t/t5810-proto-disable-local.sh > index c1ef99b85c2..862610256fb 100755 > --- a/t/t5810-proto-disable-local.sh > +++ b/t/t5810-proto-disable-local.sh > @@ -1,6 +1,8 @@ > #!/bin/sh > > test_description='test disabling of local paths in clone/fetch' > + > +TEST_PASSES_SANITIZE_LEAK=true > . ./test-lib.sh > . "$TEST_DIRECTORY/lib-proto-disable.sh" > > diff --git a/t/t5813-proto-disable-ssh.sh b/t/t5813-proto-disable-ssh.sh > index 3f084ee3065..2e975dc70ec 100755 > --- a/t/t5813-proto-disable-ssh.sh > +++ b/t/t5813-proto-disable-ssh.sh > @@ -1,6 +1,8 @@ > #!/bin/sh > > test_description='test disabling of git-over-ssh in clone/fetch' > + > +TEST_PASSES_SANITIZE_LEAK=true > . ./test-lib.sh > . "$TEST_DIRECTORY/lib-proto-disable.sh" > > diff --git a/t/t7409-submodule-detached-work-tree.sh b/t/t7409-submodule-detached-work-tree.sh > index 374ed481e9c..574a6fc526e 100755 > --- a/t/t7409-submodule-detached-work-tree.sh > +++ b/t/t7409-submodule-detached-work-tree.sh > @@ -13,6 +13,7 @@ TEST_NO_CREATE_REPO=1 > 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/t7416-submodule-dash-url.sh b/t/t7416-submodule-dash-url.sh > index 3ebd9859814..7cf72b9a076 100755 > --- a/t/t7416-submodule-dash-url.sh > +++ b/t/t7416-submodule-dash-url.sh > @@ -1,6 +1,8 @@ > #!/bin/sh > > test_description='check handling of disallowed .gitmodule urls' > + > +TEST_PASSES_SANITIZE_LEAK=true > . ./test-lib.sh > > test_expect_success 'setup' ' > diff --git a/t/t7450-bad-git-dotfiles.sh b/t/t7450-bad-git-dotfiles.sh > index ba1f569bcbb..0d0c3f2c683 100755 > --- a/t/t7450-bad-git-dotfiles.sh > +++ b/t/t7450-bad-git-dotfiles.sh > @@ -12,6 +12,8 @@ Such as: > > - symlinked .gitmodules, etc > ' > + > +TEST_PASSES_SANITIZE_LEAK=true > . ./test-lib.sh > . "$TEST_DIRECTORY"/lib-pack.sh >