Re: [PATCH] ls-remote & transport API: release "struct transport_ls_refs_options"
From: Junio C Hamano <hidden>
Date: 2022-02-07 02:11:43
Ævar Arnfjörð Bjarmason [off-list ref] writes:
Fix a memory leak in codepaths that use the "struct transport_ls_refs_options" API. Since the introduction of the struct in 39835409d10 (connect, transport: encapsulate arg in struct, 2021-02-05) the caller has been responsible for freeing it. That commit in turn migrated code originally added in 402c47d9391 (clone: send ref-prefixes when using protocol v2, 2018-07-20) and b4be74105fe (ls-remote: pass ref prefixes when requesting a remote's refs, 2018-03-15). Only some of those codepaths were releasing the allocated resources of the struct, now all of them will. Mark the "t/t5511-refspec.sh" test as passing when git is compiled with SANITIZE=leak. They'll now be listed as running under the "GIT_TEST_PASSING_SANITIZE_LEAK=true" test mode (the "linux-leaks" CI target). Previously 24/47 tests would fail. Signed-off-by: Ævar Arnfjörð Bjarmason <redacted> --- builtin/clone.c | 13 ++++++------- builtin/fetch.c | 2 +- builtin/ls-remote.c | 3 ++- connect.c | 4 ++-- t/t5511-refspec.sh | 1 + transport.c | 8 +++++++- transport.h | 10 +++++++--- 7 files changed, 26 insertions(+), 15 deletions(-)
This ...
+void transport_ls_refs_options_release(struct transport_ls_refs_options *opts)
+{
+ strvec_clear(&opts->ref_prefixes);
+ free((char *)opts->unborn_head_target);
+}
+... addition is very much welcomed. And instead of different code paths doing "we used this member, so clear only that" ad-hoc, making them all call it makes it very much pleasant read.
quoted hunk
diff --git a/builtin/clone.c b/builtin/clone.c index 727e16e0aea..8564e5f603f 100644 --- a/builtin/clone.c +++ b/builtin/clone.c@@ -1233,7 +1233,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix) } else { const char *branch; - char *ref; + const char *ref; + char *ref_free = NULL; if (option_branch) die(_("Remote branch %s not found in upstream %s"),@@ -1250,17 +1251,16 @@ int cmd_clone(int argc, const char **argv, const char *prefix) skip_prefix(transport_ls_refs_options.unborn_head_target, "refs/heads/", &branch)) { ref = transport_ls_refs_options.unborn_head_target; - transport_ls_refs_options.unborn_head_target = NULL; create_symref("HEAD", ref, reflog_msg.buf); } else { branch = git_default_branch_name(0); - ref = xstrfmt("refs/heads/%s", branch); + ref_free = xstrfmt("refs/heads/%s", branch); + ref = ref_free; } if (!option_bare) install_branch_config(0, branch, remote_name, ref); - - free(ref); + free(ref_free); }
It is a bit unfortunate that "ref" has to be sometimes a borrowed pointer and some other times own the storage, only to allow us write the call that uses the variable only once. But under the constraints of the current code, I think this is the best we could do. In our code base, we would usually call the auxiliary variable "to_free", because its "ref"-ness does not matter and its sole reason to exist is to be the "other owner" of the piece of memory, to relieve the "ref" variable from the responsibility of releasing resources. With it, "ref" consistently borrows from somebody else, either "to_free", or the .unborn_head_target member, and does not have to be (and should not be) freed itself.