[PATCH v2 9/9] transport: also free remote_refs in transport_disconnect()

From: Andrzej Hunt via GitGitGadget <gitgitgadget@gmail.com>
Date: Mar 14, 2021, 18:47 UTC
Message-ID: <a907f2460d42dbc959e6059f96ba0448e458ca51.1615747662.git.gitgitgadget@gmail.com>
In-Reply-To: <pull.899.v2.git.1615747662.gitgitgadget@gmail.com>

From: Andrzej Hunt <ajrhunt@google.com>

transport_get_remote_refs() can populate the transport struct's remote_refs. transport_disconnect() is already responsible for most of transport's cleanup - therefore we also take care of freeing remote_refs there.

There are 2 locations where transport_disconnect() is called before we're done using the returned remote_refs. This patch changes those callsites to only call transport_disconnect() after the returned refs are no longer being used - which is necessary to safely be able to free remote_refs during transport_disconnect().

This commit fixes the following leak which was found while running t0000, but is expected to also fix the same pattern of leak in all locations that use transport_get_remote_refs():

Direct leak of 165 byte(s) in 1 object(s) allocated from:
    #0 0x49a6b2 in calloc /home/abuild/rpmbuild/BUILD/llvm-11.0.0.src/build/../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:154:3
    #1 0x9a72f2 in xcalloc /home/ahunt/oss-fuzz/git/wrapper.c:140:8
    #2 0x8ce203 in alloc_ref_with_prefix /home/ahunt/oss-fuzz/git/remote.c:867:20
    #3 0x8ce1a2 in alloc_ref /home/ahunt/oss-fuzz/git/remote.c:875:9
    #4 0x72f63e in process_ref_v2 /home/ahunt/oss-fuzz/git/connect.c:426:8
    #5 0x72f21a in get_remote_refs /home/ahunt/oss-fuzz/git/connect.c:525:8
    #6 0x979ab7 in handshake /home/ahunt/oss-fuzz/git/transport.c:305:4
    #7 0x97872d in get_refs_via_connect /home/ahunt/oss-fuzz/git/transport.c:339:9
    #8 0x9774b5 in transport_get_remote_refs /home/ahunt/oss-fuzz/git/transport.c:1388:4
    #9 0x51cf80 in cmd_clone /home/ahunt/oss-fuzz/git/builtin/clone.c:1271:9
    #10 0x4cd60d in run_builtin /home/ahunt/oss-fuzz/git/git.c:453:11
    #11 0x4cb2da in handle_builtin /home/ahunt/oss-fuzz/git/git.c:704:3
    #12 0x4ccc37 in run_argv /home/ahunt/oss-fuzz/git/git.c:771:4
    #13 0x4cac29 in cmd_main /home/ahunt/oss-fuzz/git/git.c:902:19
    #14 0x69c45e in main /home/ahunt/oss-fuzz/git/common-main.c:52:11
    #15 0x7f6a459d5349 in __libc_start_main (/lib64/libc.so.6+0x24349)

Signed-off-by: Andrzej Hunt <ajrhunt@google.com>
---
 builtin/ls-remote.c | 4 ++--
 builtin/remote.c    | 8 ++++----
 transport.c         | 2 ++
 3 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
index ef604752a044..5432d239a681 100644
--- a/builtin/ls-remote.c
+++ b/builtin/ls-remote.c
@@ -124,8 +124,6 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 		int hash_algo = hash_algo_by_ptr(transport_get_hash_algo(transport));
 		repo_set_hash_algo(the_repository, hash_algo);
 	}
-	if (transport_disconnect(transport))
-		return 1;
 
 	if (!dest && !quiet)
 		fprintf(stderr, "From %s\n", *remote->url);
@@ -151,5 +149,7 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 	}
 
 	ref_array_clear(&ref_array);
+	if (transport_disconnect(transport))
+		return 1;
 	return status;
 }
diff --git a/builtin/remote.c b/builtin/remote.c
index d11a5589e49d..e31d9c99470e 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -938,9 +938,6 @@ static int get_remote_ref_states(const char *name,
 				 struct ref_states *states,
 				 int query)
 {
-	struct transport *transport;
-	const struct ref *remote_refs;
-
 	states->remote = remote_get(name);
 	if (!states->remote)
 		return error(_("No such remote: '%s'"), name);
@@ -948,10 +945,12 @@ static int get_remote_ref_states(const char *name,
 	read_branches();
 
 	if (query) {
+		struct transport *transport;
+		const struct ref *remote_refs;
+
 		transport = transport_get(states->remote, states->remote->url_nr > 0 ?
 			states->remote->url[0] : NULL);
 		remote_refs = transport_get_remote_refs(transport, NULL);
-		transport_disconnect(transport);
 
 		states->queried = 1;
 		if (query & GET_REF_STATES)
@@ -960,6 +959,7 @@ static int get_remote_ref_states(const char *name,
 			get_head_names(remote_refs, states);
 		if (query & GET_PUSH_REF_STATES)
 			get_push_ref_states(remote_refs, states);
+		transport_disconnect(transport);
 	} else {
 		for_each_ref(append_ref_to_tracked_list, states);
 		string_list_sort(&states->tracked);
diff --git a/transport.c b/transport.c
index b13fab5dc3b1..62362d79dd87 100644
--- a/transport.c
+++ b/transport.c
@@ -1452,6 +1452,8 @@ int transport_disconnect(struct transport *transport)
 	int ret = 0;
 	if (transport->vtable->disconnect)
 		ret = transport->vtable->disconnect(transport);
+	if (transport->got_remote_refs)
+		free_refs((void *)transport->remote_refs);
 	free(transport);
 	return ret;
 }

-- 
gitgitgadget