{"thread":{"id":"57369","subject":"[PATCH] ls-remote & transport API: release \"struct transport_ls_refs_options\"","startedAt":"2022-02-05T00:08:21Z","lastAt":"2022-02-07T02:11:43Z","messageCount":2,"participants":["Ævar Arnfjörð Bjarmason","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"447807","messageId":"patch-1.1-e80e8f64eae-20220205T000809Z-avarab@gmail.com","threadId":"57369","inReplyTo":null,"subject":"[PATCH] ls-remote & transport API: release \"struct transport_ls_refs_options\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-02-05T00:08:14Z","receivedAt":"2022-02-05T00:08:21Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Fix a memory leak in codepaths that use the \"struct\ntransport_ls_refs_options\" API. Since the introduction of the struct\nin 39835409d10 (connect, transport: encapsulate arg in struct,\n2021-02-05) the caller has been responsible for freeing it.\n\nThat commit in turn migrated code originally added in\n402c47d9391 (clone: send ref-prefixes when using protocol v2,\n2018-07-20) and b4be74105fe (ls-remote: pass ref prefixes when\nrequesting a remote's refs, 2018-03-15). Only some of those codepaths\nwere releasing the allocated resources of the struct, now all of them\nwill.\n\nMark the \"t/t5511-refspec.sh\" test as passing when git is compiled\nwith SANITIZE=leak. They'll now be listed as running under the\n\"GIT_TEST_PASSING_SANITIZE_LEAK=true\" test mode (the \"linux-leaks\" CI\ntarget). Previously 24/47 tests would fail.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n builtin/clone.c     | 13 ++++++-------\n builtin/fetch.c     |  2 +-\n builtin/ls-remote.c |  3 ++-\n connect.c           |  4 ++--\n t/t5511-refspec.sh  |  1 +\n transport.c         |  8 +++++++-\n transport.h         | 10 +++++++---\n 7 files changed, 26 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 727e16e0aea..8564e5f603f 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -1233,7 +1233,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t}\n \telse {\n \t\tconst char *branch;\n-\t\tchar *ref;\n+\t\tconst char *ref;\n+\t\tchar *ref_free = NULL;\n \n \t\tif (option_branch)\n \t\t\tdie(_(\"Remote branch %s not found in upstream %s\"),\n@@ -1250,17 +1251,16 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\t    skip_prefix(transport_ls_refs_options.unborn_head_target,\n \t\t\t\t\"refs/heads/\", &branch)) {\n \t\t\tref = transport_ls_refs_options.unborn_head_target;\n-\t\t\ttransport_ls_refs_options.unborn_head_target = NULL;\n \t\t\tcreate_symref(\"HEAD\", ref, reflog_msg.buf);\n \t\t} else {\n \t\t\tbranch = git_default_branch_name(0);\n-\t\t\tref = xstrfmt(\"refs/heads/%s\", branch);\n+\t\t\tref_free = xstrfmt(\"refs/heads/%s\", branch);\n+\t\t\tref = ref_free;\n \t\t}\n \n \t\tif (!option_bare)\n \t\t\tinstall_branch_config(0, branch, remote_name, ref);\n-\n-\t\tfree(ref);\n+\t\tfree(ref_free);\n \t}\n \n \twrite_refspec_config(src_ref_prefix, our_head_points_at,\n@@ -1312,7 +1312,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tUNLEAK(repo);\n \tjunk_mode = JUNK_LEAVE_ALL;\n \n-\tstrvec_clear(&transport_ls_refs_options.ref_prefixes);\n-\tfree(transport_ls_refs_options.unborn_head_target);\n+\ttransport_ls_refs_options_release(&transport_ls_refs_options);\n \treturn err;\n }\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 5f06b21f8e9..a3ffab727eb 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1593,7 +1593,7 @@ static int do_fetch(struct transport *transport,\n \t} else\n \t\tremote_refs = NULL;\n \n-\tstrvec_clear(&transport_ls_refs_options.ref_prefixes);\n+\ttransport_ls_refs_options_release(&transport_ls_refs_options);\n \n \tref_map = get_ref_map(transport->remote, remote_refs, rs,\n \t\t\t      tags, &autotags);\ndiff --git a/builtin/ls-remote.c b/builtin/ls-remote.c\nindex 44448fa61d1..d856085e941 100644\n--- a/builtin/ls-remote.c\n+++ b/builtin/ls-remote.c\n@@ -155,6 +155,7 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)\n \n \tref_array_clear(&ref_array);\n \tif (transport_disconnect(transport))\n-\t\treturn 1;\n+\t\tstatus = 1;\n+\ttransport_ls_refs_options_release(&transport_options);\n \treturn status;\n }\ndiff --git a/connect.c b/connect.c\nindex eaf7d6d2618..afc79a6236e 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -379,7 +379,7 @@ struct ref **get_remote_heads(struct packet_reader *reader,\n \n /* Returns 1 when a valid ref has been added to `list`, 0 otherwise */\n static int process_ref_v2(struct packet_reader *reader, struct ref ***list,\n-\t\t\t  char **unborn_head_target)\n+\t\t\t  const char **unborn_head_target)\n {\n \tint ret = 1;\n \tint i = 0;\n@@ -483,7 +483,7 @@ struct ref **get_remote_refs(int fd_out, struct packet_reader *reader,\n \tconst char *hash_name;\n \tstruct strvec *ref_prefixes = transport_options ?\n \t\t&transport_options->ref_prefixes : NULL;\n-\tchar **unborn_head_target = transport_options ?\n+\tconst char **unborn_head_target = transport_options ?\n \t\t&transport_options->unborn_head_target : NULL;\n \t*list = NULL;\n \ndiff --git a/t/t5511-refspec.sh b/t/t5511-refspec.sh\nindex be025b90f98..fc55681a3f2 100755\n--- a/t/t5511-refspec.sh\n+++ b/t/t5511-refspec.sh\n@@ -2,6 +2,7 @@\n \n test_description='refspec parsing'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_refspec () {\ndiff --git a/transport.c b/transport.c\nindex 2a3e3241545..253d6671b1f 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -1292,7 +1292,7 @@ int transport_push(struct repository *r,\n \t\t\t\t\t\t\t       &transport_options);\n \t\ttrace2_region_leave(\"transport_push\", \"get_refs_list\", r);\n \n-\t\tstrvec_clear(&transport_options.ref_prefixes);\n+\t\ttransport_ls_refs_options_release(&transport_options);\n \n \t\tif (flags & TRANSPORT_PUSH_ALL)\n \t\t\tmatch_flags |= MATCH_REFS_ALL;\n@@ -1420,6 +1420,12 @@ const struct ref *transport_get_remote_refs(struct transport *transport,\n \treturn transport->remote_refs;\n }\n \n+void transport_ls_refs_options_release(struct transport_ls_refs_options *opts)\n+{\n+\tstrvec_clear(&opts->ref_prefixes);\n+\tfree((char *)opts->unborn_head_target);\n+}\n+\n int transport_fetch_refs(struct transport *transport, struct ref *refs)\n {\n \tint rc;\ndiff --git a/transport.h b/transport.h\nindex 3f16e50c196..a0bc6a1e9eb 100644\n--- a/transport.h\n+++ b/transport.h\n@@ -257,15 +257,19 @@ struct transport_ls_refs_options {\n \t/*\n \t * If unborn_head_target is not NULL, and the remote reports HEAD as\n \t * pointing to an unborn branch, transport_get_remote_refs() stores the\n-\t * unborn branch in unborn_head_target. It should be freed by the\n-\t * caller.\n+\t * unborn branch in unborn_head_target.\n \t */\n-\tchar *unborn_head_target;\n+\tconst char *unborn_head_target;\n };\n #define TRANSPORT_LS_REFS_OPTIONS_INIT { \\\n \t.ref_prefixes = STRVEC_INIT, \\\n }\n \n+/**\n+ * Release the \"struct transport_ls_refs_options\".\n+ */\n+void transport_ls_refs_options_release(struct transport_ls_refs_options *opts);\n+\n /*\n  * Retrieve refs from a remote.\n  */\n-- \n2.35.1.945.g180f8b8dd92\n\n"},{"id":"447862","messageId":"xmqqwni7h1d0.fsf@gitster.g","threadId":"57369","inReplyTo":"patch-1.1-e80e8f64eae-20220205T000809Z-avarab@gmail.com","subject":"Re: [PATCH] ls-remote & transport API: release \"struct transport_ls_refs_options\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-07T02:11:39Z","receivedAt":"2022-02-07T02:11:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n\n> Fix a memory leak in codepaths that use the \"struct\n> transport_ls_refs_options\" API. Since the introduction of the struct\n> in 39835409d10 (connect, transport: encapsulate arg in struct,\n> 2021-02-05) the caller has been responsible for freeing it.\n>\n> That commit in turn migrated code originally added in\n> 402c47d9391 (clone: send ref-prefixes when using protocol v2,\n> 2018-07-20) and b4be74105fe (ls-remote: pass ref prefixes when\n> requesting a remote's refs, 2018-03-15). Only some of those codepaths\n> were releasing the allocated resources of the struct, now all of them\n> will.\n>\n> Mark the \"t/t5511-refspec.sh\" test as passing when git is compiled\n> with SANITIZE=leak. They'll now be listed as running under the\n> \"GIT_TEST_PASSING_SANITIZE_LEAK=true\" test mode (the \"linux-leaks\" CI\n> target). Previously 24/47 tests would fail.\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n> ---\n>  builtin/clone.c     | 13 ++++++-------\n>  builtin/fetch.c     |  2 +-\n>  builtin/ls-remote.c |  3 ++-\n>  connect.c           |  4 ++--\n>  t/t5511-refspec.sh  |  1 +\n>  transport.c         |  8 +++++++-\n>  transport.h         | 10 +++++++---\n>  7 files changed, 26 insertions(+), 15 deletions(-)\n\nThis ...\n\n> +void transport_ls_refs_options_release(struct transport_ls_refs_options *opts)\n> +{\n> +\tstrvec_clear(&opts->ref_prefixes);\n> +\tfree((char *)opts->unborn_head_target);\n> +}\n> +\n\n... addition is very much welcomed.  And instead of different code\npaths doing \"we used this member, so clear only that\" ad-hoc, making\nthem all call it makes it very much pleasant read.\n\n> diff --git a/builtin/clone.c b/builtin/clone.c\n> index 727e16e0aea..8564e5f603f 100644\n> --- a/builtin/clone.c\n> +++ b/builtin/clone.c\n> @@ -1233,7 +1233,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n>  \t}\n>  \telse {\n>  \t\tconst char *branch;\n> -\t\tchar *ref;\n> +\t\tconst char *ref;\n> +\t\tchar *ref_free = NULL;\n>  \n>  \t\tif (option_branch)\n>  \t\t\tdie(_(\"Remote branch %s not found in upstream %s\"),\n> @@ -1250,17 +1251,16 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n>  \t\t    skip_prefix(transport_ls_refs_options.unborn_head_target,\n>  \t\t\t\t\"refs/heads/\", &branch)) {\n>  \t\t\tref = transport_ls_refs_options.unborn_head_target;\n> -\t\t\ttransport_ls_refs_options.unborn_head_target = NULL;\n>  \t\t\tcreate_symref(\"HEAD\", ref, reflog_msg.buf);\n>  \t\t} else {\n>  \t\t\tbranch = git_default_branch_name(0);\n> -\t\t\tref = xstrfmt(\"refs/heads/%s\", branch);\n> +\t\t\tref_free = xstrfmt(\"refs/heads/%s\", branch);\n> +\t\t\tref = ref_free;\n>  \t\t}\n>  \n>  \t\tif (!option_bare)\n>  \t\t\tinstall_branch_config(0, branch, remote_name, ref);\n> -\n> -\t\tfree(ref);\n> +\t\tfree(ref_free);\n>  \t}\n\nIt is a bit unfortunate that \"ref\" has to be sometimes a borrowed\npointer and some other times own the storage, only to allow us write\nthe call that uses the variable only once.  But under the\nconstraints of the current code, I think this is the best we could\ndo.\n\nIn our code base, we would usually call the auxiliary variable\n\"to_free\", because its \"ref\"-ness does not matter and its sole\nreason to exist is to be the \"other owner\" of the piece of memory,\nto relieve the \"ref\" variable from the responsibility of releasing\nresources.  With it, \"ref\" consistently borrows from somebody else,\neither \"to_free\", or the .unborn_head_target member, and does not\nhave to be (and should not be) freed itself.\n"}]}