git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH v3 3/9] clone: free or UNLEAK further pointers when finished

From
Andrzej Hunt via GitGitGadget <gitgitgadget@gmail.com>
Date
Mar 21, 2021, 16:58 UTC
Message-ID
<40c5c915fc1e818661fc272cdc8c37678e8f3fcf.1616345918.git.gitgitgadget@gmail.com>
In-Reply-To
<pull.899.v3.git.1616345918.gitgitgadget@gmail.com>
From: Andrzej Hunt <ajrhunt@google.com>

Most of these pointers can safely be freed when cmd_clone() completes, therefore we make sure to free them. The one exception is that we have to UNLEAK(repo) because it can point either to argv[0], or a malloc'd string returned by absolute_pathdup().

We also have to free(path) in the middle of cmd_clone(): later during
cmd_clone(), path is unconditionally overwritten with a different path,
triggering a leak. Freeing the first path immediately after use (but
only in the case where it contains data) seems like the cleanest
solution, as opposed to freeing it unconditionally before path is reused
for another path. This leak appears to have been introduced in:
  f38aa83f9a (use local cloning if insteadOf makes a local URL, 2014-07-17)

These leaks were found when running t0001 with LSAN, see also an excerpt of the LSAN output below (the full list is omitted because it's far too long, and mostly consists of indirect leakage of members of the refs we are freeing).

Direct leak of 178 byte(s) in 1 object(s) allocated from:
    #0 0x49a53d in malloc /home/abuild/rpmbuild/BUILD/llvm-11.0.0.src/build/../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3
    #1 0x9a6ff4 in do_xmalloc /home/ahunt/oss-fuzz/git/wrapper.c:41:8
    #2 0x9a6fca in xmalloc /home/ahunt/oss-fuzz/git/wrapper.c:62:9
    #3 0x8ce296 in copy_ref /home/ahunt/oss-fuzz/git/remote.c:885:8
    #4 0x8d2ebd in guess_remote_head /home/ahunt/oss-fuzz/git/remote.c:2215:10
    #5 0x51d0c5 in cmd_clone /home/ahunt/oss-fuzz/git/builtin/clone.c:1308:4
    #6 0x4cd60d in run_builtin /home/ahunt/oss-fuzz/git/git.c:453:11
    #7 0x4cb2da in handle_builtin /home/ahunt/oss-fuzz/git/git.c:704:3
    #8 0x4ccc37 in run_argv /home/ahunt/oss-fuzz/git/git.c:771:4
    #9 0x4cac29 in cmd_main /home/ahunt/oss-fuzz/git/git.c:902:19
    #10 0x69c45e in main /home/ahunt/oss-fuzz/git/common-main.c:52:11
    #11 0x7f6a459d5349 in __libc_start_main (/lib64/libc.so.6+0x24349)
Direct leak of 165 byte(s) in 1 object(s) allocated from:
    #0 0x49a53d in malloc /home/abuild/rpmbuild/BUILD/llvm-11.0.0.src/build/../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3
    #1 0x9a6fc4 in do_xmalloc /home/ahunt/oss-fuzz/git/wrapper.c:41:8
    #2 0x9a6f9a in xmalloc /home/ahunt/oss-fuzz/git/wrapper.c:62:9
    #3 0x8ce266 in copy_ref /home/ahunt/oss-fuzz/git/remote.c:885:8
    #4 0x51e9bd in wanted_peer_refs /home/ahunt/oss-fuzz/git/builtin/clone.c:574:21
    #5 0x51cfe1 in cmd_clone /home/ahunt/oss-fuzz/git/builtin/clone.c:1284:17
    #6 0x4cd60d in run_builtin /home/ahunt/oss-fuzz/git/git.c:453:11
    #7 0x4cb2da in handle_builtin /home/ahunt/oss-fuzz/git/git.c:704:3
    #8 0x4ccc37 in run_argv /home/ahunt/oss-fuzz/git/git.c:771:4
    #9 0x4cac29 in cmd_main /home/ahunt/oss-fuzz/git/git.c:902:19
    #10 0x69c42e in main /home/ahunt/oss-fuzz/git/common-main.c:52:11
    #11 0x7f8fef0c2349 in __libc_start_main (/lib64/libc.so.6+0x24349)
Direct leak of 178 byte(s) in 1 object(s) allocated from:
    #0 0x49a53d in malloc /home/abuild/rpmbuild/BUILD/llvm-11.0.0.src/build/../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3
    #1 0x9a6ff4 in do_xmalloc /home/ahunt/oss-fuzz/git/wrapper.c:41:8
    #2 0x9a6fca in xmalloc /home/ahunt/oss-fuzz/git/wrapper.c:62:9
    #3 0x8ce296 in copy_ref /home/ahunt/oss-fuzz/git/remote.c:885:8
    #4 0x8d2ebd in guess_remote_head /home/ahunt/oss-fuzz/git/remote.c:2215:10
    #5 0x51d0c5 in cmd_clone /home/ahunt/oss-fuzz/git/builtin/clone.c:1308:4
    #6 0x4cd60d in run_builtin /home/ahunt/oss-fuzz/git/git.c:453:11
    #7 0x4cb2da in handle_builtin /home/ahunt/oss-fuzz/git/git.c:704:3
    #8 0x4ccc37 in run_argv /home/ahunt/oss-fuzz/git/git.c:771:4
    #9 0x4cac29 in cmd_main /home/ahunt/oss-fuzz/git/git.c:902:19
    #10 0x69c45e in main /home/ahunt/oss-fuzz/git/common-main.c:52:11
    #11 0x7f6a459d5349 in __libc_start_main (/lib64/libc.so.6+0x24349)
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)
Direct leak of 105 byte(s) in 1 object(s) allocated from:
    #0 0x49a859 in realloc /home/abuild/rpmbuild/BUILD/llvm-11.0.0.src/build/../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3
    #1 0x9a71f6 in xrealloc /home/ahunt/oss-fuzz/git/wrapper.c:126:8
    #2 0x93622d in strbuf_grow /home/ahunt/oss-fuzz/git/strbuf.c:98:2
    #3 0x937a73 in strbuf_addch /home/ahunt/oss-fuzz/git/./strbuf.h:231:3
    #4 0x939fcd in strbuf_add_absolute_path /home/ahunt/oss-fuzz/git/strbuf.c:911:4
    #5 0x69d3ce in absolute_pathdup /home/ahunt/oss-fuzz/git/abspath.c:261:2
    #6 0x51c688 in cmd_clone /home/ahunt/oss-fuzz/git/builtin/clone.c:1021:10
    #7 0x4cd60d in run_builtin /home/ahunt/oss-fuzz/git/git.c:453:11
    #8 0x4cb2da in handle_builtin /home/ahunt/oss-fuzz/git/git.c:704:3
    #9 0x4ccc37 in run_argv /home/ahunt/oss-fuzz/git/git.c:771:4
    #10 0x4cac29 in cmd_main /home/ahunt/oss-fuzz/git/git.c:902:19
    #11 0x69c45e in main /home/ahunt/oss-fuzz/git/common-main.c:52:11
    #12 0x7f6a459d5349 in __libc_start_main (/lib64/libc.so.6+0x24349)
Signed-off-by: Andrzej Hunt <ajrhunt@google.com>
---
 builtin/clone.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/builtin/clone.c b/builtin/clone.c
index 51e844a2de0a..952fe3d8fc88 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -964,10 +964,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 {
 	int is_bundle = 0, is_local;
 	const char *repo_name, *repo, *work_tree, *git_dir;
-	char *path, *dir, *display_repo = NULL;
+	char *path = NULL, *dir, *display_repo = NULL;
 	int dest_exists, real_dest_exists = 0;
 	const struct ref *refs, *remote_head;
-	const struct ref *remote_head_points_at;
+	struct ref *remote_head_points_at = NULL;
 	const struct ref *our_head_points_at;
 	struct ref *mapped_refs;
 	const struct ref *ref;
@@ -1017,9 +1017,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	repo_name = argv[0];
 
 	path = get_repo_path(repo_name, &is_bundle);
-	if (path)
+	if (path) {
+		FREE_AND_NULL(path);
 		repo = absolute_pathdup(repo_name);
-	else if (strchr(repo_name, ':')) {
+	} else if (strchr(repo_name, ':')) {
 		repo = repo_name;
 		display_repo = transport_anonymize_url(repo);
 	} else
@@ -1393,6 +1394,11 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	strbuf_release(&reflog_msg);
 	strbuf_release(&branch_top);
 	strbuf_release(&key);
+	free_refs(mapped_refs);
+	free_refs(remote_head_points_at);
+	free(dir);
+	free(path);
+	UNLEAK(repo);
 	junk_mode = JUNK_LEAVE_ALL;
 
 	strvec_clear(&transport_ls_refs_options.ref_prefixes);

-- 
gitgitgadget
Previous: Andrzej Hunt via GitGitGadgetNext: Andrzej Hunt via GitGitGadget
Message 46 of 52 in “Fix all leaks in t0001”
  1. 0/7 Fix all leaks in t0001Andrzej Hunt via GitGitGadget, Mar 8, 2021
  2. 1/7 symbolic-ref: don't leak shortened refname in check_symref()Andrzej Hunt via GitGitGadget, Mar 8, 2021
  3. Jeff KingMar 8, 2021
  4. Andrzej HuntMar 14, 2021
  5. 2/7 reset: free instead of leaking unneeded refAndrzej Hunt via GitGitGadget, Mar 8, 2021
  6. Jeff KingMar 8, 2021
  7. 3/7 clone: free or UNLEAK further pointers when finishedAndrzej Hunt via GitGitGadget, Mar 8, 2021
  8. Jeff KingMar 8, 2021
  9. Andrzej HuntMar 14, 2021
  10. 6/7 init-db: silence template_dir leak when converting to absolute pathAndrzej Hunt via GitGitGadget, Mar 8, 2021
  11. Jeff KingMar 8, 2021
  12. 4/7 worktree: fix leak in dwim_branch()Andrzej Hunt via GitGitGadget, Mar 8, 2021
  13. Jeff KingMar 8, 2021
  14. Andrzej HuntMar 14, 2021
  15. 7/7 parse-options: don't leak alias help messagesAndrzej Hunt via GitGitGadget, Mar 8, 2021
  16. Jeff KingMar 8, 2021
  17. Andrzej HuntMar 14, 2021
  18. 5/7 init: remove git_init_db_config() while fixing leaksAndrzej Hunt via GitGitGadget, Mar 8, 2021
  19. Jeff KingMar 8, 2021
  20. Jeff KingMar 8, 2021
  21. Junio C HamanoMar 12, 2021
  22. Andrzej HuntMar 14, 2021
  23. Andrzej HuntMar 15, 2021
  24. Junio C HamanoMar 8, 2021
  25. Andrzej HuntMar 14, 2021
  26. 0/9 Fix all leaks in t0001Andrzej Hunt via GitGitGadget, Mar 14, 2021
  27. 8/9 parse-options: don't leak alias help messagesAndrzej Hunt via GitGitGadget, Mar 14, 2021
  28. Eric SunshineMar 14, 2021
  29. Andrzej HuntMar 15, 2021
  30. Andrzej HuntMar 14, 2021
  31. 1/9 symbolic-ref: don't leak shortened refname in check_symref()Andrzej Hunt via GitGitGadget, Mar 14, 2021
  32. 7/9 parse-options: convert bitfield values to use binary shiftAndrzej Hunt via GitGitGadget, Mar 14, 2021
  33. Martin ÅgrenMar 14, 2021
  34. Junio C HamanoMar 14, 2021
  35. Andrzej HuntMar 15, 2021
  36. 4/9 worktree: fix leak in dwim_branch()Andrzej Hunt via GitGitGadget, Mar 14, 2021
  37. 5/9 init: remove git_init_db_config() while fixing leaksAndrzej Hunt via GitGitGadget, Mar 14, 2021
  38. 3/9 clone: free or UNLEAK further pointers when finishedAndrzej Hunt via GitGitGadget, Mar 14, 2021
  39. 2/9 reset: free instead of leaking unneeded refAndrzej Hunt via GitGitGadget, Mar 14, 2021
  40. 9/9 transport: also free remote_refs in transport_disconnect()Andrzej Hunt via GitGitGadget, Mar 14, 2021
  41. 6/9 init-db: silence template_dir leak when converting to absolute pathAndrzej Hunt via GitGitGadget, Mar 14, 2021
  42. 0/9 Fix all leaks in t0001Andrzej Hunt via GitGitGadget, Mar 21, 2021
  43. 1/9 symbolic-ref: don't leak shortened refname in check_symref()Andrzej Hunt via GitGitGadget, Mar 21, 2021
  44. 2/9 reset: free instead of leaking unneeded refAndrzej Hunt via GitGitGadget, Mar 21, 2021
  45. 4/9 worktree: fix leak in dwim_branch()Andrzej Hunt via GitGitGadget, Mar 21, 2021
  46. 3/9 clone: free or UNLEAK further pointers when finishedAndrzej Hunt via GitGitGadget, Mar 21, 2021
  47. 7/9 parse-options: convert bitfield values to use binary shiftAndrzej Hunt via GitGitGadget, Mar 21, 2021
  48. 6/9 init-db: silence template_dir leak when converting to absolute pathAndrzej Hunt via GitGitGadget, Mar 21, 2021
  49. 5/9 init: remove git_init_db_config() while fixing leaksAndrzej Hunt via GitGitGadget, Mar 21, 2021
  50. 9/9 transport: also free remote_refs in transport_disconnect()Andrzej Hunt via GitGitGadget, Mar 21, 2021
  51. 8/9 parse-options: don't leak alias help messagesAndrzej Hunt via GitGitGadget, Mar 21, 2021
  52. Junio C HamanoMar 21, 2021

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.