From: Andrzej Hunt via GitGitGadget <hidden> Date: 2021-03-08 18:37:01
This series fixes (or annotates) all the memory leaks that can cause t0001
to fail when run with LeakSanitizer (t0000 already passes without failures).
I suspect that none of these leaks had any user impact, and I'm aware that
every change does cause some noise - I would have no objections to
abandoning this series if it's not deemed valuable enough. On the other
hand: fixing or suppressing these leaks should make it easier to spot leaks
that have more significant user impact (it's entirely plausible that no real
impactful leaks exist).
Note: this series does not guarantee that there are no leaks within
t0000-t0001, it only fixes those leaks which cause test failures. There is
at least one test case in t0000 where git is invoked in a subshell, and the
return value is ignored - meaning that a memory leak that is occuring during
that invocation does not cause tests to fail (I'm still trying to figure out
if that's something that's worth fixing - but that's probably a topic for a
separate thread):
https://git.kernel.org/pub/scm/git/git.git/tree/t/t0000-basic.sh#n1285
In case anyone is interested: I have been using the following workflow to
find leaks and verify fixes - I'm running into crashes when using LSAN
standalone, therefore I'm using full ASAN instead (I'm not particularly
concerned about this: LSAN standalone mode is known to be less-well tested
than leak-checking within ASAN [1], and the crashes are occurring within the
leak-checker itself):
make GIT_TEST_OPTS="-i -v" DEFAULT_TEST_TARGET="t0000-basic.sh"
ASAN_OPTIONS="detect_leaks=1:abort_on_error=1" SANITIZE=address DEVELOPER=1
CFLAGS="-DSUPPRESS_ANNOTATED_LEAKS -g -fno-optimize-sibling-calls -O1
-fno-omit-frame-pointer" test
(I then rerun the entire test suite with ASAN but with leak-checking
disabled in order to gain some confidence that my fixes aren't inadvertently
introducing memory safety issues.)
[1]
https://github.com/google/sanitizers/wiki/AddressSanitizerLeakSanitizer#stand-alone-mode
Andrzej Hunt (7):
symbolic-ref: don't leak shortened refname in check_symref()
reset: free instead of leaking unneeded ref
clone: free or UNLEAK further pointers when finished
worktree: fix leak in dwim_branch()
init: remove git_init_db_config() while fixing leaks
init-db: silence template_dir leak when converting to absolute path
parse-options: don't leak alias help messages
builtin/clone.c | 13 ++++++++++---
builtin/init-db.c | 32 ++++++++++----------------------
builtin/reset.c | 2 +-
builtin/symbolic-ref.c | 12 +++++++++---
builtin/worktree.c | 12 ++++++------
parse-options.c | 29 ++++++++++++++++++++++++-----
6 files changed, 60 insertions(+), 40 deletions(-)
base-commit: be7935ed8bff19f481b033d0d242c5d5f239ed50
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-899%2Fahunt%2Fleaksan-t0001-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-899/ahunt/leaksan-t0001-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/899
--
gitgitgadget
From: Andrzej Hunt via GitGitGadget <hidden> Date: 2021-03-08 18:37:01
From: Andrzej Hunt <redacted>
This leak has existed since:
9ab55daa55 (git symbolic-ref --delete $symref, 2012-10-21)
This leak was found when running t0001 with LSAN, see also LSAN output
below:
Direct leak of 19 byte(s) in 1 object(s) allocated from:
#0 0x486514 in strdup /home/abuild/rpmbuild/BUILD/llvm-11.0.0.src/build/../projects/compiler-rt/lib/asan/asan_interceptors.cpp:452:3
#1 0x9ab048 in xstrdup /home/ahunt/oss-fuzz/git/wrapper.c:29:14
#2 0x8b452f in refs_shorten_unambiguous_ref /home/ahunt/oss-fuzz/git/refs.c
#3 0x8b47e8 in shorten_unambiguous_ref /home/ahunt/oss-fuzz/git/refs.c:1287:9
#4 0x679fce in check_symref /home/ahunt/oss-fuzz/git/builtin/symbolic-ref.c:28:14
#5 0x679ad8 in cmd_symbolic_ref /home/ahunt/oss-fuzz/git/builtin/symbolic-ref.c:70:9
#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 0x69cc6e in main /home/ahunt/oss-fuzz/git/common-main.c:52:11
#11 0x7f98388a4349 in __libc_start_main (/lib64/libc.so.6+0x24349)
Signed-off-by: Andrzej Hunt <redacted>
---
builtin/symbolic-ref.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
@@ -24,9 +24,15 @@ static int check_symref(const char *HEAD, int quiet, int shorten, int print)return1;}if(print){-if(shorten)-refname=shorten_unambiguous_ref(refname,0);-puts(refname);+if(shorten){+constchar*shortened_refname;++shortened_refname=shorten_unambiguous_ref(refname,0);+puts(shortened_refname);+free((void*)shortened_refname);+}else{+puts(refname);+}}return0;}
From: Andrzej Hunt via GitGitGadget <hidden> Date: 2021-03-08 18:37:01
From: Andrzej Hunt <redacted>
dwim_ref() allocs a new string into ref. Instead of setting to NULL to discard
it, we can FREE_AND_NULL.
This leak appears to have been introduced in:
4cf76f6bbf (builtin/reset: compute checkout metadata for reset, 2020-03-16)
This leak was found when running t0001 with LSAN, see also LSAN output below:
Direct leak of 5 byte(s) in 1 object(s) allocated from:
#0 0x486514 in strdup /home/abuild/rpmbuild/BUILD/llvm-11.0.0.src/build/../projects/compiler-rt/lib/asan/asan_interceptors.cpp:452:3
#1 0x9a7108 in xstrdup /home/ahunt/oss-fuzz/git/wrapper.c:29:14
#2 0x8add6b in expand_ref /home/ahunt/oss-fuzz/git/refs.c:670:12
#3 0x8ad777 in repo_dwim_ref /home/ahunt/oss-fuzz/git/refs.c:644:22
#4 0x6394af in dwim_ref /home/ahunt/oss-fuzz/git/./refs.h:162:9
#5 0x637e5c in cmd_reset /home/ahunt/oss-fuzz/git/builtin/reset.c:426: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 0x69c5ce in main /home/ahunt/oss-fuzz/git/common-main.c:52:11
#11 0x7f57ebb9d349 in __libc_start_main (/lib64/libc.so.6+0x24349)
Signed-off-by: Andrzej Hunt <redacted>
---
builtin/reset.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Andrzej Hunt via GitGitGadget <hidden> Date: 2021-03-08 18:37:02
From: Andrzej Hunt <redacted>
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
vv #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 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 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 <redacted>
---
builtin/clone.c | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
From: Andrzej Hunt via GitGitGadget <hidden> Date: 2021-03-08 18:37:02
From: Andrzej Hunt <redacted>
template_dir starts off pointing to either argv or nothing. However if
the value supplied in argv is a relative path, absolute_pathdup() is
used to turn it into an absolute path. absolute_pathdup() allocates
a new string, and we then "leak" it when cmd_init_db() completes.
We don't bother to actually free the return value (instead we UNLEAK
it), because there's no significant advantage to doing so here.
Correctly freeing it would require more significant changes to code flow
which would be more noisy than beneficial.
Signed-off-by: Andrzej Hunt <redacted>
---
builtin/init-db.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
From: Andrzej Hunt via GitGitGadget <hidden> Date: 2021-03-08 18:37:02
From: Andrzej Hunt <redacted>
Make sure that we release the temporary strbuf during dwim_branch() for
all codepaths (and not just for the early return).
This leak appears to have been introduced in:
f60a7b763f (worktree: teach "add" to check out existing branches, 2018-04-24)
Note that UNLEAK(branchname) is still needed: the returned result is
used in add(), and is stored in a pointer which is used to point at one
of:
- a string literal ("HEAD")
- member of argv (whatever the user specified in their invocation)
- or our newly allocated string returned from dwim_branch()
Fixing the branchname leak isn't impossible, but does not seem
worthwhile given that add() is called directly from cmd_main(), and
cmd_main() returns immediately thereafter - UNLEAK is good enough.
This leak was found when running t0001 with LSAN, see also LSAN output
below:
Direct leak of 60 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 0x9ab076 in xrealloc /home/ahunt/oss-fuzz/git/wrapper.c:126:8
#2 0x939fcd in strbuf_grow /home/ahunt/oss-fuzz/git/strbuf.c:98:2
#3 0x93af53 in strbuf_splice /home/ahunt/oss-fuzz/git/strbuf.c:239:3
#4 0x83559a in strbuf_check_branch_ref /home/ahunt/oss-fuzz/git/object-name.c:1593:2
#5 0x6988b9 in dwim_branch /home/ahunt/oss-fuzz/git/builtin/worktree.c:454:20
#6 0x695f8f in add /home/ahunt/oss-fuzz/git/builtin/worktree.c:525:19
#7 0x694a04 in cmd_worktree /home/ahunt/oss-fuzz/git/builtin/worktree.c:1036:10
#8 0x4cd60d in run_builtin /home/ahunt/oss-fuzz/git/git.c:453:11
#9 0x4cb2da in handle_builtin /home/ahunt/oss-fuzz/git/git.c:704:3
#10 0x4ccc37 in run_argv /home/ahunt/oss-fuzz/git/git.c:771:4
#11 0x4cac29 in cmd_main /home/ahunt/oss-fuzz/git/git.c:902:19
#12 0x69caee in main /home/ahunt/oss-fuzz/git/common-main.c:52:11
#13 0x7f7b7dd10349 in __libc_start_main (/lib64/libc.so.6+0x24349)
Signed-off-by: Andrzej Hunt <redacted>
---
builtin/worktree.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
From: Andrzej Hunt via GitGitGadget <hidden> Date: 2021-03-08 18:37:02
From: Andrzej Hunt <redacted>
preprocess_options() allocates new strings for help messages for
OPTION_ALIAS. Therefore we also need to clean those help messages up
when freeing the returned options.
First introduced in:
7c280589cf (parse-options: teach "git cmd -h" to show alias as alias, 2020-03-16)
The preprocessed options themselves no longer contain any indication
that a given option is/was an alias: the easiest and fastest way to
figure it out is to look back at the original options. Alternatively we
could iterate over the alias_groups list - but that would require nested
looping and is likely to be a (little) less efficient.
As far as I can tell, parse_options() is only ever used once per
command, and the help messages are small - hence this leak has very
little impact.
This leak was found while running t0001. LSAN output can be found below:
Direct leak of 65 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 0x9aae36 in xrealloc /home/ahunt/oss-fuzz/git/wrapper.c:126:8
#2 0x939d8d in strbuf_grow /home/ahunt/oss-fuzz/git/strbuf.c:98:2
#3 0x93b936 in strbuf_vaddf /home/ahunt/oss-fuzz/git/strbuf.c:392:3
#4 0x93b7ff in strbuf_addf /home/ahunt/oss-fuzz/git/strbuf.c:333:2
#5 0x86747e in preprocess_options /home/ahunt/oss-fuzz/git/parse-options.c:666:3
#6 0x866ed2 in parse_options /home/ahunt/oss-fuzz/git/parse-options.c:847:17
#7 0x51c4a7 in cmd_clone /home/ahunt/oss-fuzz/git/builtin/clone.c:989:9
#8 0x4cd60d in run_builtin /home/ahunt/oss-fuzz/git/git.c:453:11
#9 0x4cb2da in handle_builtin /home/ahunt/oss-fuzz/git/git.c:704:3
#10 0x4ccc37 in run_argv /home/ahunt/oss-fuzz/git/git.c:771:4
#11 0x4cac29 in cmd_main /home/ahunt/oss-fuzz/git/git.c:902:19
#12 0x69c9fe in main /home/ahunt/oss-fuzz/git/common-main.c:52:11
#13 0x7fdac42d4349 in __libc_start_main (/lib64/libc.so.6+0x24349)
Signed-off-by: Andrzej Hunt <redacted>
---
parse-options.c | 29 ++++++++++++++++++++++++-----
1 file changed, 24 insertions(+), 5 deletions(-)
From: Andrzej Hunt via GitGitGadget <hidden> Date: 2021-03-08 18:37:02
From: Andrzej Hunt <redacted>
The primary goal of this change is to stop leaking init_db_template_dir.
This leak can happen because:
1. git_init_db_config() allocates new memory into init_db_template_dir
without first freeing the existing value.
2. init_db_template_dir might already contain data, either because:
2.1 git_config() can be invoked twice with this callback in a single
process - at least 2 allocations are likely.
2.2 A single git_config() allocation can invoke the callback multiple
times for a given key (see further explanation in the function
docs) - each of those calls will trigger another leak.
The simplest fix for the leak would be to free(init_db_template_dir)
before overwriting it. Instead we choose to convert to fetching
init.templatedir via git_config_get_value() as that is more explicit,
more efficient, and avoids allocations (the returned result is owned by
the config cache, so we aren't responsible for freeing it).
If we remove init_db_template_dir, git_init_db_config() ends up being
responsible only for forwarding core.* config values to
platform_core_config(). However platform_core_config() already ignores
non-core.* config values, so we can safely remove git_init_db_config()
and invoke git_config() directly with platform_core_config() as the
callback.
The platform_core_config forwarding was originally added in:
287853392a (mingw: respect core.hidedotfiles = false in git-init again, 2019-03-11
And I suspect the potential for a leak existed since the original
implementation of git_init_db_config in:
90b45187ba (Add `init.templatedir` configuration variable., 2010-02-17)
LSAN output from t0001:
Direct leak of 73 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 0x9a7276 in xrealloc /home/ahunt/oss-fuzz/git/wrapper.c:126:8
#2 0x9362ad in strbuf_grow /home/ahunt/oss-fuzz/git/strbuf.c:98:2
#3 0x936eaa in strbuf_add /home/ahunt/oss-fuzz/git/strbuf.c:295:2
#4 0x868112 in strbuf_addstr /home/ahunt/oss-fuzz/git/./strbuf.h:304:2
#5 0x86a8ad in expand_user_path /home/ahunt/oss-fuzz/git/path.c:758:2
#6 0x720bb1 in git_config_pathname /home/ahunt/oss-fuzz/git/config.c:1287:10
#7 0x5960e2 in git_init_db_config /home/ahunt/oss-fuzz/git/builtin/init-db.c:161:11
#8 0x7255b8 in configset_iter /home/ahunt/oss-fuzz/git/config.c:1982:7
#9 0x7253fc in repo_config /home/ahunt/oss-fuzz/git/config.c:2311:2
#10 0x725ca7 in git_config /home/ahunt/oss-fuzz/git/config.c:2399:2
#11 0x593e8d in create_default_files /home/ahunt/oss-fuzz/git/builtin/init-db.c:225:2
#12 0x5935c6 in init_db /home/ahunt/oss-fuzz/git/builtin/init-db.c:449:11
#13 0x59588e in cmd_init_db /home/ahunt/oss-fuzz/git/builtin/init-db.c:714:9
#14 0x4cd60d in run_builtin /home/ahunt/oss-fuzz/git/git.c:453:11
#15 0x4cb2da in handle_builtin /home/ahunt/oss-fuzz/git/git.c:704:3
#16 0x4ccc37 in run_argv /home/ahunt/oss-fuzz/git/git.c:771:4
#17 0x4cac29 in cmd_main /home/ahunt/oss-fuzz/git/git.c:902:19
#18 0x69c4de in main /home/ahunt/oss-fuzz/git/common-main.c:52:11
#19 0x7f23552d6349 in __libc_start_main (/lib64/libc.so.6+0x24349)
Signed-off-by: Andrzej Hunt <redacted>
---
builtin/init-db.c | 28 +++++++---------------------
1 file changed, 7 insertions(+), 21 deletions(-)
@@ -212,10 +200,7 @@ static int create_default_files(const char *template_path,intreinit;intfilemode;structstrbuferr=STRBUF_INIT;--/* Just look for `init.templatedir` */-init_db_template_dir=NULL;/* re-set in case it was set before */-git_config(git_init_db_config,NULL);+constchar*init_template_dir=NULL;/**Firstcopythetemplates--wemighthavethedefault
@@ -226,7 +211,8 @@ static int create_default_files(const char *template_path,*values(sincewe'vejustpotentiallychangedwhat'savailableon*disk).*/-copy_templates(template_path);+git_config_get_value("init.templatedir",&init_template_dir);+copy_templates(template_path,init_template_dir);git_config_clear();reset_shared_repository();git_config(git_default_config,NULL);
@@ -422,8 +408,8 @@ int init_db(const char *git_dir, const char *real_git_dir,}startup_info->have_repository=1;-/* Just look for `core.hidedotfiles` */-git_config(git_init_db_config,NULL);+/* Ensure `core.hidedotfiles` is processed */+git_config(platform_core_config,NULL);safe_create_dir(git_dir,0);
From: Jeff King <hidden> Date: 2021-03-08 18:56:20
On Mon, Mar 08, 2021 at 06:36:13PM +0000, Andrzej Hunt via GitGitGadget wrote:
This series fixes (or annotates) all the memory leaks that can cause t0001
to fail when run with LeakSanitizer (t0000 already passes without failures).
I suspect that none of these leaks had any user impact, and I'm aware that
every change does cause some noise - I would have no objections to
abandoning this series if it's not deemed valuable enough. On the other
hand: fixing or suppressing these leaks should make it easier to spot leaks
that have more significant user impact (it's entirely plausible that no real
impactful leaks exist).
I think it's worth doing. The reason t0000 passes is because it was my
reference script when adding UNLEAK() back in:
https://lore.kernel.org/git/20170905130149.agc3zp3s6i6e5aki@sigill.intra.peff.net/
(which might be of historical interest if you haven't read it). I knew
that the next step would be tediously going through the test suite
looking at the tool results, and I somehow stalled on that part. ;)
But I think it's nice to move the goal forward incrementally. I agree
that a lot of these leaks aren't that important, but it's generally as
easy to fix or annotate them as it is to argue that they shouldn't be
dealt with.
Note: this series does not guarantee that there are no leaks within
t0000-t0001, it only fixes those leaks which cause test failures. There is
at least one test case in t0000 where git is invoked in a subshell, and the
return value is ignored - meaning that a memory leak that is occuring during
that invocation does not cause tests to fail (I'm still trying to figure out
if that's something that's worth fixing - but that's probably a topic for a
separate thread):
https://git.kernel.org/pub/scm/git/git.git/tree/t/t0000-basic.sh#n1285
It's not the subshell there, but rather that git is on the left-hand
side of a pipe (and so its exit code is discarded). We've been slowly
fixing such cases (the usual technique is to use a tempfile).
In case anyone is interested: I have been using the following workflow to
find leaks and verify fixes - I'm running into crashes when using LSAN
standalone, therefore I'm using full ASAN instead (I'm not particularly
concerned about this: LSAN standalone mode is known to be less-well tested
than leak-checking within ASAN [1], and the crashes are occurring within the
leak-checker itself):
Yeah, I think using ASAN is just fine. I found that LSAN is faster, but
if you are running a single script the difference probably doesn't
matter. I also found that clang's support was much more mature than
gcc's (I don't know how different the state is these days, though).
Regardless, if you can get it to run cleanly with _any_ leak checker,
I'll be quite happy. :)
make GIT_TEST_OPTS="-i -v" DEFAULT_TEST_TARGET="t0000-basic.sh"
ASAN_OPTIONS="detect_leaks=1:abort_on_error=1" SANITIZE=address DEVELOPER=1
CFLAGS="-DSUPPRESS_ANNOTATED_LEAKS -g -fno-optimize-sibling-calls -O1
-fno-omit-frame-pointer" test
There's some magic in the Makefile for detecting SANITIZE=leak and
setting -DSUPPRESS_ANNOTATED_LEAKS. It might be worth that extending
that to SANITIZE=address, but I guess we wouldn't want to do so for most
builds (which also are setting detect_leaks=0 in the test suite). Maybe
we should have some other name to trigger asan-as-a-leak-detector. Or
maybe that just gets complicated, because we pass the results of
SANITIZE on to the compiler directly.
(I then rerun the entire test suite with ASAN but with leak-checking
disabled in order to gain some confidence that my fixes aren't inadvertently
introducing memory safety issues.)
Definitely a good idea.
Andrzej Hunt (7):
symbolic-ref: don't leak shortened refname in check_symref()
reset: free instead of leaking unneeded ref
clone: free or UNLEAK further pointers when finished
worktree: fix leak in dwim_branch()
init: remove git_init_db_config() while fixing leaks
init-db: silence template_dir leak when converting to absolute path
parse-options: don't leak alias help messages
I haven't looked at the individual patches yet. I'll respond to them
individually.
-Peff
From: Jeff King <hidden> Date: 2021-03-08 19:02:14
On Mon, Mar 08, 2021 at 06:36:14PM +0000, Andrzej Hunt via GitGitGadget wrote:
From: Andrzej Hunt <redacted>
This leak has existed since:
9ab55daa55 (git symbolic-ref --delete $symref, 2012-10-21)
This leak was found when running t0001 with LSAN, see also LSAN output
below:
Direct leak of 19 byte(s) in 1 object(s) allocated from:
#0 0x486514 in strdup /home/abuild/rpmbuild/BUILD/llvm-11.0.0.src/build/../projects/compiler-rt/lib/asan/asan_interceptors.cpp:452:3
#1 0x9ab048 in xstrdup /home/ahunt/oss-fuzz/git/wrapper.c:29:14
#2 0x8b452f in refs_shorten_unambiguous_ref /home/ahunt/oss-fuzz/git/refs.c
#3 0x8b47e8 in shorten_unambiguous_ref /home/ahunt/oss-fuzz/git/refs.c:1287:9
#4 0x679fce in check_symref /home/ahunt/oss-fuzz/git/builtin/symbolic-ref.c:28:14
#5 0x679ad8 in cmd_symbolic_ref /home/ahunt/oss-fuzz/git/builtin/symbolic-ref.c:70:9
#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 0x69cc6e in main /home/ahunt/oss-fuzz/git/common-main.c:52:11
#11 0x7f98388a4349 in __libc_start_main (/lib64/libc.so.6+0x24349)
As a general template for fixing leaks, this information seems pretty
good. You might want to give a brief reason for why it's a leak (like
you do already in the second patch). Here it just would be something
like:
shorten_unambiguous_ref() returns an allocated string. We have to
track it separately from the const refname.
Or whatever. It doesn't need to be a novel, but just give an overview of
what's going that makes the diff obvious.
@@ -24,9 +24,15 @@ static int check_symref(const char *HEAD, int quiet, int shorten, int print)return1;}if(print){-if(shorten)-refname=shorten_unambiguous_ref(refname,0);-puts(refname);+if(shorten){+constchar*shortened_refname;++shortened_refname=shorten_unambiguous_ref(refname,0);+puts(shortened_refname);+free((void*)shortened_refname);+}else{+puts(refname);+}
If a variable is meant to take ownership of memory, our usual convention
is to not declare it as "const". And then you don't need to cast to pass
it to free().
There's also an idiom in Git's codebase when a const pointer may alias
unowned storage, or a buffer that needs to be freed. Something like:
if (print) {
char *to_free = NULL;
if (shorten)
refname = to_free = shorten_unambiguous_ref(refname, 0);
puts(refname);
free(to_free);
}
That avoids duplicating the part of the code that handles the variable.
In this case it is only a single line, but IMHO it's still easier to
read, as it makes clear that we call puts() in either case.
-Peff
From: Jeff King <hidden> Date: 2021-03-08 19:04:28
On Mon, Mar 08, 2021 at 06:36:15PM +0000, Andrzej Hunt via GitGitGadget wrote:
dwim_ref() allocs a new string into ref. Instead of setting to NULL to discard
it, we can FREE_AND_NULL.
[...]
dwim_ref(rev, strlen(rev), &dummy, &ref, 0);
if (ref && !starts_with(ref, "refs/"))
- ref = NULL;
+ FREE_AND_NULL(ref);
Yeah, this seems like a perfect solution for this case.
-Peff
We should avoid casting away constness when possible (because it is
often a sign that sometimes the variable _isn't_ pointing to owned
memory). In this case, I think freeing is the right thing; our
guess_remote_head() returns a copy of the struct (which is non-const).
Should remote_head_points_at just be declared without const?
+ free_refs((void *)refs);
This one is more questionable to me. It comes from
transport_get_remote_refs(), which does return a const pointer. And it
looks like that memory is owned by the transport struct. So presumably
we need to tell the transport code to clean itself up (or mark it with
UNLEAK). Or perhaps there's a bug in the transport code (e.g., should it
be freeing transport->remote_refs in transport_disconnect()? You'd want
to make sure that no other callers expect the ref list to live on past
the disconnect).
+ UNLEAK(repo);
This one could be done with the idiom I mentioned earlier:
repo = absolute_repo = absolute_pathdup(repo_name);
...
free(absolute_repo);
but I think this is a perfect use of UNLEAK().
-Peff
From: Jeff King <hidden> Date: 2021-03-08 19:17:06
On Mon, Mar 08, 2021 at 06:36:17PM +0000, Andrzej Hunt via GitGitGadget wrote:
Make sure that we release the temporary strbuf during dwim_branch() for
all codepaths (and not just for the early return).
Makes sense. Two style nits:
quoted hunk
static const char *dwim_branch(const char *path, const char **new_branch) {- int n;+ int n, branch_exists;
If two variables aren't strictly related, but just happen to share the
same type, IMHO it's better to declare them separately. (There are
numerous spots in Git's code that don't follow this rule, but I think we
should be moving in that direction).
From: Jeff King <hidden> Date: 2021-03-08 19:30:19
On Mon, Mar 08, 2021 at 06:36:18PM +0000, Andrzej Hunt via GitGitGadget wrote:
quoted hunk
@@ -212,10 +200,7 @@ static int create_default_files(const char *template_path, int reinit; int filemode; struct strbuf err = STRBUF_INIT;-- /* Just look for `init.templatedir` */- init_db_template_dir = NULL; /* re-set in case it was set before */- git_config(git_init_db_config, NULL);+ const char *init_template_dir = NULL; /* * First copy the templates -- we might have the default
@@ -226,7 +211,8 @@ static int create_default_files(const char *template_path, * values (since we've just potentially changed what's available on * disk). */- copy_templates(template_path);+ git_config_get_value("init.templatedir", &init_template_dir);+ copy_templates(template_path, init_template_dir); git_config_clear(); reset_shared_repository(); git_config(git_default_config, NULL);
@@ -422,8 +408,8 @@ int init_db(const char *git_dir, const char *real_git_dir, } startup_info->have_repository = 1;- /* Just look for `core.hidedotfiles` */- git_config(git_init_db_config, NULL);+ /* Ensure `core.hidedotfiles` is processed */+ git_config(platform_core_config, NULL);
There are some subtle ordering dependencies in init_db(), because it may
start in one git repository, but then create and shift into another.
It's hard to see the ordering just from the diff. I think this change is
OK. The platform_core_config bits are loaded at the same moment, and
it's only the extra git_config() call in create_default_files() that
goes away. That _could_ be overwriting the platform bits with something
else, but I think that was not the intent of the code. And I think it's
impossible, because the intervening calls are not moving from one repo
to the other.
-Peff
From: Jeff King <hidden> Date: 2021-03-08 19:31:24
On Mon, Mar 08, 2021 at 06:36:19PM +0000, Andrzej Hunt via GitGitGadget wrote:
template_dir starts off pointing to either argv or nothing. However if
the value supplied in argv is a relative path, absolute_pathdup() is
used to turn it into an absolute path. absolute_pathdup() allocates
a new string, and we then "leak" it when cmd_init_db() completes.
We don't bother to actually free the return value (instead we UNLEAK
it), because there's no significant advantage to doing so here.
Correctly freeing it would require more significant changes to code flow
which would be more noisy than beneficial.
Makes sense. The UNLEAK() could be conditional at the end of the
function (since it is OK to unleak a non-allocated variable, too), but I
like keeping it here with the allocation for clarity.
-Peff
From: Jeff King <hidden> Date: 2021-03-08 19:46:57
On Mon, Mar 08, 2021 at 06:36:20PM +0000, Andrzej Hunt via GitGitGadget wrote:
preprocess_options() allocates new strings for help messages for
OPTION_ALIAS. Therefore we also need to clean those help messages up
when freeing the returned options.
Yep, makes sense.
The preprocessed options themselves no longer contain any indication
that a given option is/was an alias: the easiest and fastest way to
figure it out is to look back at the original options. Alternatively we
could iterate over the alias_groups list - but that would require nested
looping and is likely to be a (little) less efficient.
Yeah, this is a bit ugly. We could probably set a bit in the aliased
struct's flags field to indicate that it's an alias, though.
As far as I can tell, parse_options() is only ever used once per
command, and the help messages are small - hence this leak has very
little impact.
We _could_ UNLEAK() it, but I prefer this actual cleanup. We're getting
far enough away from main() that we aren't actually sure that we'll only
be called once per process.
A few style nits:
- omit the space between "**" and preprocessed_options
- we'd usually break a long line (after the first parameter comma)
I think preprocessed_options shouldn't be const here. After all, our aim
is to free it! I'm also not sure why it's a pointer-to-pointer. If we
were setting it to NULL after freeing, that would be valuable, but we
don't. So all together:
static void free_preprocessed_options(struct option *preprocessed_options,
const struct option *original_options)
quoted hunk
+{+ int i;++ if (!*preprocessed_options) {+ return;+ }
Style: we'd generally omit the curly braces for a single-liner like
this.
Without the double-pointer, we can omit the extra "*" here, too.
quoted hunk
+ for (i = 0; original_options[i].type != OPTION_END; i++) {+ if (original_options[i].type == OPTION_ALIAS) {+ free((void *)(*preprocessed_options)[i].help);+ }+ }
OK, so we look through the original options to find ones that became an
alias, and then free them. Makes sense.
Do the indexes always correspond between the original and the
preprocessed arrays? I _think_ so, but preprocess_options() is a little
hard to follow.
If the preprocess code set a flag in the resulting option, though, we
could make it much more obviously correct. And avoid having to pass
original_options at all.
+ free((void *)*preprocessed_options);
With the interface suggestions above, this becomes just:
free(preprocessed_options);
OK, we have to keep two variables now rather than aliasing "options",
because we need the original for feeding to the free function (but this
hunk too would go away if we set a flag).
To spell it out, I mean something like on the writing side:
From: Andrzej Hunt <hidden> Date: 2021-03-14 16:56:11
On 08/03/2021 19:55, Jeff King wrote:
I think it's worth doing. The reason t0000 passes is because it was my
reference script when adding UNLEAK() back in:
https://lore.kernel.org/git/20170905130149.agc3zp3s6i6e5aki@sigill.intra.peff.net/
(which might be of historical interest if you haven't read it). I knew
that the next step would be tediously going through the test suite
looking at the tool results, and I somehow stalled on that part. ;)
But I think it's nice to move the goal forward incrementally. I agree
that a lot of these leaks aren't that important, but it's generally as
easy to fix or annotate them as it is to argue that they shouldn't be > dealt with.
Thanks for the confirmation! I'd seen your post, but wasn't sure if there'd been a change of plan or just lack of time :).
quoted
Note: this series does not guarantee that there are no leaks within
t0000-t0001, it only fixes those leaks which cause test failures. There is
at least one test case in t0000 where git is invoked in a subshell, and the
return value is ignored - meaning that a memory leak that is occuring during
that invocation does not cause tests to fail (I'm still trying to figure out
if that's something that's worth fixing - but that's probably a topic for a
separate thread):
https://git.kernel.org/pub/scm/git/git.git/tree/t/t0000-basic.sh#n1285
It's not the subshell there, but rather that git is on the left-hand
side of a pipe (and so its exit code is discarded). We've been slowly
fixing such cases (the usual technique is to use a tempfile).
Thanks for the tip! I've started on another series to fix t0000-basic along with the leaks that that uncovers. In future I suspect it's best to start by removing pipes _before_ running leak-checking against a given test. (Fortunately t0001 doesn't contain any such cases, so this series is valid as is.)
quoted
In case anyone is interested: I have been using the following workflow to
find leaks and verify fixes - I'm running into crashes when using LSAN
standalone, therefore I'm using full ASAN instead (I'm not particularly
concerned about this: LSAN standalone mode is known to be less-well tested
than leak-checking within ASAN [1], and the crashes are occurring within the
leak-checker itself):
Yeah, I think using ASAN is just fine. I found that LSAN is faster, but
if you are running a single script the difference probably doesn't
matter. I also found that clang's support was much more mature than
gcc's (I don't know how different the state is these days, though).
Regardless, if you can get it to run cleanly with _any_ leak checker,
I'll be quite happy. :)
I was wrong when it comes to LSAN being broken. What was actually happening is: we default to running ASAN and LSAN with abort_on_error=1, and I had overridden that setting when running with ASAN. When I switched to LSAN, abort_on_error was enabled again - and I was just misinterpreting the intentional abortion as opposed to seeing an unexplained crashes. [As far as I can tell, abort_on_error is needed to detect leaks during a test_must_fail and similar scenarios.]
I've briefly tested all the various combinations of gcc or clang, with LSAN or ASAN or both - and they all seem to work as expected, with one exception: gcc with LSAN-only finds what seems to be a false positive, in a method which mallocs, followed by a die().
To make it trickier, that new "leak" happens inside a test_must_fail - the LSAN output is swallowed, making it hard to diagnose. I'll try to prepare a separate patch to not discard stderr in that scenario.
Regardless of the LSAN/ASAN differences - I'm wondering whether piggybacking on the existing ASAN validation might be the best way to get leak checking run more often (limited to a subset of leak-free tests of course). I'll expand on these thoughts in my reply to Junio.
quoted
make GIT_TEST_OPTS="-i -v" DEFAULT_TEST_TARGET="t0000-basic.sh"
ASAN_OPTIONS="detect_leaks=1:abort_on_error=1" SANITIZE=address DEVELOPER=1
CFLAGS="-DSUPPRESS_ANNOTATED_LEAKS -g -fno-optimize-sibling-calls -O1
-fno-omit-frame-pointer" test
There's some magic in the Makefile for detecting SANITIZE=leak and
setting -DSUPPRESS_ANNOTATED_LEAKS. It might be worth that extending
that to SANITIZE=address, but I guess we wouldn't want to do so for most
builds (which also are setting detect_leaks=0 in the test suite). Maybe
we should have some other name to trigger asan-as-a-leak-detector. Or
maybe that just gets complicated, because we pass the results of
SANITIZE on to the compiler directly.
I've realised it's enough to set SANITIZE=leak,address. That gives us the benefits of ASAN, but still adds -DSUPPRESS_ANNOTATED_LEAKS. Given that LSAN seems stable enough with clang, I suppose this is only really useful for gcc users.
I haven't looked at the individual patches yet. I'll respond to them
individually.
-Peff
Thank you for the reviews! I'm still a bit new to the git codebase, thank you for being patient with my (deficits of) style :).
From: Andrzej Hunt <hidden> Date: 2021-03-14 16:57:47
On 08/03/2021 20:12, Jeff King wrote:
On Mon, Mar 08, 2021 at 06:36:16PM +0000, Andrzej Hunt via GitGitGadget wrote:
quoted
@@ -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(path); repo = absolute_pathdup(repo_name);
You mentioned that "path" gets reused again later. Should we use
FREE_AND_NULL() to make sure that nobody tries to look at it in the
meantime?
That sounds sensible - I wasn't too sure at first because we are unconditionally setting path later... but I can't see an reason not to make this safer.
But that makes me wonder if the definition of path should set it to NULL too. Setting it to NULL after freeing it guarantees that we can't read the old value anymore. However later code has no idea if path will be NULL or merely initialised (at least until we overwrite it with the new path).
I've provisionally updated my patch to also set path = NULL at point of definition, but I don't know if that's idiomatic in this scenario.
We should avoid casting away constness when possible (because it is
often a sign that sometimes the variable _isn't_ pointing to owned
memory). In this case, I think freeing is the right thing; our
guess_remote_head() returns a copy of the struct (which is non-const).
Should remote_head_points_at just be declared without const?
I think so - I'll change remote_head_points_at to non-const.
quoted
+ free_refs((void *)refs);
This one is more questionable to me. It comes from
transport_get_remote_refs(), which does return a const pointer. And it
looks like that memory is owned by the transport struct. So presumably
we need to tell the transport code to clean itself up (or mark it with
UNLEAK). Or perhaps there's a bug in the transport code (e.g., should it
be freeing transport->remote_refs in transport_disconnect()? You'd want
to make sure that no other callers expect the ref list to live on past
the disconnect).
There are indeed multiple locations where we store the fetched refs, followed by transport_disconnect(), followed by trying to use the refs (that are nomimanlly owned by the now disconnected transport):
https://git.kernel.org/pub/scm/git/git.git/tree/builtin/remote.c?h=next#n953https://git.kernel.org/pub/scm/git/git.git/tree/builtin/ls-remote.c?h=next#n122
However all other locations could handle free()'ing during transport_disconnect - and the 2 I've linked to above are easy enough to fix. So I'll give up on free_refs() from cmd_clone(), and will move it into transport_disconnect() as suggested. I've added a new patch to the end of the series to take care of this change.
(Which ultimately means we've now solved the same pattern of leak for all 5 users of transport_get_remote_refs().)
A few style nits:
- omit the space between "**" and preprocessed_options
- we'd usually break a long line (after the first parameter comma)
I think preprocessed_options shouldn't be const here. After all, our aim
is to free it! I'm also not sure why it's a pointer-to-pointer. If we
were setting it to NULL after freeing, that would be valuable, but we
don't. So all together >
static void free_preprocessed_options(struct option *preprocessed_options,
const struct option *original_options)
I'm not sure what I was originally thinking when I used the pointer-to-pointer - I've incorporated your suggestions, they do make everything easier to read. Moreover we'll remove original_options as per your later suggestions anyway.
quoted
+ for (i = 0; original_options[i].type != OPTION_END; i++) {+ if (original_options[i].type == OPTION_ALIAS) {+ free((void *)(*preprocessed_options)[i].help);+ }+ }
OK, so we look through the original options to find ones that became an
alias, and then free them. Makes sense.
Do the indexes always correspond between the original and the
preprocessed arrays? I _think_ so, but preprocess_options() is a little
hard to follow.
If the preprocess code set a flag in the resulting option, though, we
could make it much more obviously correct. And avoid having to pass
original_options at all.
_At this time_, indexes always correspond between the original and preprocessed options, but in the back of my mind I was still a little bit uncomfortable depending on that. Your suggestion is much better - so I've gone ahead and implemented it.
quoted
+ free((void *)*preprocessed_options);
With the interface suggestions above, this becomes just:
free(preprocessed_options);
OK, we have to keep two variables now rather than aliasing "options",
because we need the original for feeding to the free function (but this
hunk too would go away if we set a flag).
Indeed - after adding the flag as suggested, the changes to parse_options() are reduced down to calling free_preprocessed_options() instead of free() - which is quite a nice simplification.
quoted hunk
To spell it out, I mean something like on the writing side:
We'd usually omit the extra parentheses here. I.e.,:
branch_exists = !strbuf_check_branch_ref(&ref, branchname) &&
ref_exists(ref.buf);
I've made this change - but I have a question about the formatting:
- In the few examples that I could find, the second line is aligned using spaces (i.e. same number of tabs as the previous line, followed by spaces to align correctly). However that appears to violate the indent-with-non-tab style check - should I switch to 100% tabs instead?
That check has been around since:
e2f6331a14 (.gitattributes: CR at the end of the line is an error, 2009-06-19)
So maybe it's being intentionally ignored in the cases that I've seen (I only noticed it for my series because of the automated checks on Github) - but I thought I should ask to sure.
From: Andrzej Hunt <hidden> Date: 2021-03-14 18:08:19
On 08/03/2021 20:01, Jeff King wrote:
On Mon, Mar 08, 2021 at 06:36:14PM +0000, Andrzej Hunt via GitGitGadget wrote:
quoted
From: Andrzej Hunt <redacted>
This leak has existed since:
9ab55daa55 (git symbolic-ref --delete $symref, 2012-10-21)
This leak was found when running t0001 with LSAN, see also LSAN output
below:
Direct leak of 19 byte(s) in 1 object(s) allocated from:
#0 0x486514 in strdup /home/abuild/rpmbuild/BUILD/llvm-11.0.0.src/build/../projects/compiler-rt/lib/asan/asan_interceptors.cpp:452:3
#1 0x9ab048 in xstrdup /home/ahunt/oss-fuzz/git/wrapper.c:29:14
#2 0x8b452f in refs_shorten_unambiguous_ref /home/ahunt/oss-fuzz/git/refs.c
#3 0x8b47e8 in shorten_unambiguous_ref /home/ahunt/oss-fuzz/git/refs.c:1287:9
#4 0x679fce in check_symref /home/ahunt/oss-fuzz/git/builtin/symbolic-ref.c:28:14
#5 0x679ad8 in cmd_symbolic_ref /home/ahunt/oss-fuzz/git/builtin/symbolic-ref.c:70:9
#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 0x69cc6e in main /home/ahunt/oss-fuzz/git/common-main.c:52:11
#11 0x7f98388a4349 in __libc_start_main (/lib64/libc.so.6+0x24349)
As a general template for fixing leaks, this information seems pretty
good. You might want to give a brief reason for why it's a leak (like
you do already in the second patch). Here it just would be something
like:
shorten_unambiguous_ref() returns an allocated string. We have to
track it separately from the const refname.
Or whatever. It doesn't need to be a novel, but just give an overview of
what's going that makes the diff obvious.
Good point - I've copied this one verbatim - but it's also a good thing to remember if/when I fix further leaks!
There's also an idiom in Git's codebase when a const pointer may alias
unowned storage, or a buffer that needs to be freed. Something like:
if (print) {
char *to_free = NULL;
if (shorten)
refname = to_free = shorten_unambiguous_ref(refname, 0);
puts(refname);
free(to_free);
}
That avoids duplicating the part of the code that handles the variable.
In this case it is only a single line, but IMHO it's still easier to
read, as it makes clear that we call puts() in either case.
That's a nice pattern, and will probably be useful for future leak fixes too - I've made this change too!
From: Andrzej Hunt via GitGitGadget <hidden> Date: 2021-03-14 18:48:46
From: Andrzej Hunt <redacted>
preprocess_options() allocates new strings for help messages for
OPTION_ALIAS. Therefore we also need to clean those help messages up
when freeing the returned options.
First introduced in:
7c280589cf (parse-options: teach "git cmd -h" to show alias as alias, 2020-03-16)
The preprocessed options themselves no longer contain any indication
that a given option is/was an alias - therefore we add a new flag to
indicate former aliases. (An alternative approach would be to look back
at the original options to determine which options are aliases - but
that seems like a fragile approach. Or we could even look at the
alias_groups list - which might be less fragile, but would be slower
as it requires nested looping.)
As far as I can tell, parse_options() is only ever used once per
command, and the help messages are small - hence this leak has very
little impact.
This leak was found while running t0001. LSAN output can be found below:
Direct leak of 65 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 0x9aae36 in xrealloc /home/ahunt/oss-fuzz/git/wrapper.c:126:8
#2 0x939d8d in strbuf_grow /home/ahunt/oss-fuzz/git/strbuf.c:98:2
#3 0x93b936 in strbuf_vaddf /home/ahunt/oss-fuzz/git/strbuf.c:392:3
#4 0x93b7ff in strbuf_addf /home/ahunt/oss-fuzz/git/strbuf.c:333:2
#5 0x86747e in preprocess_options /home/ahunt/oss-fuzz/git/parse-options.c:666:3
#6 0x866ed2 in parse_options /home/ahunt/oss-fuzz/git/parse-options.c:847:17
#7 0x51c4a7 in cmd_clone /home/ahunt/oss-fuzz/git/builtin/clone.c:989:9
#8 0x4cd60d in run_builtin /home/ahunt/oss-fuzz/git/git.c:453:11
#9 0x4cb2da in handle_builtin /home/ahunt/oss-fuzz/git/git.c:704:3
#10 0x4ccc37 in run_argv /home/ahunt/oss-fuzz/git/git.c:771:4
#11 0x4cac29 in cmd_main /home/ahunt/oss-fuzz/git/git.c:902:19
#12 0x69c9fe in main /home/ahunt/oss-fuzz/git/common-main.c:52:11
#13 0x7fdac42d4349 in __libc_start_main (/lib64/libc.so.6+0x24349)
Signed-off-by: Andrzej Hunt <redacted>
fold
---
parse-options.c | 20 +++++++++++++++++++-
parse-options.h | 1 +
2 files changed, 20 insertions(+), 1 deletion(-)
From: Andrzej Hunt via GitGitGadget <hidden> Date: 2021-03-14 18:48:46
From: Andrzej Hunt <redacted>
shorten_unambiguous_ref() returns an allocated string. We have to
track it separately from the const refname.
This leak has existed since:
9ab55daa55 (git symbolic-ref --delete $symref, 2012-10-21)
This leak was found when running t0001 with LSAN, see also LSAN output
below:
Direct leak of 19 byte(s) in 1 object(s) allocated from:
#0 0x486514 in strdup /home/abuild/rpmbuild/BUILD/llvm-11.0.0.src/build/../projects/compiler-rt/lib/asan/asan_interceptors.cpp:452:3
#1 0x9ab048 in xstrdup /home/ahunt/oss-fuzz/git/wrapper.c:29:14
#2 0x8b452f in refs_shorten_unambiguous_ref /home/ahunt/oss-fuzz/git/refs.c
#3 0x8b47e8 in shorten_unambiguous_ref /home/ahunt/oss-fuzz/git/refs.c:1287:9
#4 0x679fce in check_symref /home/ahunt/oss-fuzz/git/builtin/symbolic-ref.c:28:14
#5 0x679ad8 in cmd_symbolic_ref /home/ahunt/oss-fuzz/git/builtin/symbolic-ref.c:70:9
#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 0x69cc6e in main /home/ahunt/oss-fuzz/git/common-main.c:52:11
#11 0x7f98388a4349 in __libc_start_main (/lib64/libc.so.6+0x24349)
Signed-off-by: Andrzej Hunt <redacted>
---
builtin/symbolic-ref.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
@@ -24,9 +24,11 @@ static int check_symref(const char *HEAD, int quiet, int shorten, int print)return1;}if(print){+char*to_free=NULL;if(shorten)-refname=shorten_unambiguous_ref(refname,0);+refname=to_free=shorten_unambiguous_ref(refname,0);puts(refname);+free(to_free);}return0;}
From: Andrzej Hunt via GitGitGadget <hidden> Date: 2021-03-14 18:48:46
From: Andrzej Hunt <redacted>
Because it's easier to read, but also likely to be easier to maintain.
I am making this change because I need to add a new flag in a later
commit.
Also add a trailing comma to the last enum entry to simplify addition of
new flags.
This changee was originally suggested by Peff in:
https://public-inbox.org/git/YEZ%2FBWWbpfVwl6nO@coredump.intra.peff.net/
Signed-off-by: Andrzej Hunt <redacted>
---
parse-options.h | 34 +++++++++++++++++-----------------
1 file changed, 17 insertions(+), 17 deletions(-)
From: Andrzej Hunt via GitGitGadget <hidden> Date: 2021-03-14 18:48:47
From: Andrzej Hunt <redacted>
Make sure that we release the temporary strbuf during dwim_branch() for
all codepaths (and not just for the early return).
This leak appears to have been introduced in:
f60a7b763f (worktree: teach "add" to check out existing branches, 2018-04-24)
Note that UNLEAK(branchname) is still needed: the returned result is
used in add(), and is stored in a pointer which is used to point at one
of:
- a string literal ("HEAD")
- member of argv (whatever the user specified in their invocation)
- or our newly allocated string returned from dwim_branch()
Fixing the branchname leak isn't impossible, but does not seem
worthwhile given that add() is called directly from cmd_main(), and
cmd_main() returns immediately thereafter - UNLEAK is good enough.
This leak was found when running t0001 with LSAN, see also LSAN output
below:
Direct leak of 60 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 0x9ab076 in xrealloc /home/ahunt/oss-fuzz/git/wrapper.c:126:8
#2 0x939fcd in strbuf_grow /home/ahunt/oss-fuzz/git/strbuf.c:98:2
#3 0x93af53 in strbuf_splice /home/ahunt/oss-fuzz/git/strbuf.c:239:3
#4 0x83559a in strbuf_check_branch_ref /home/ahunt/oss-fuzz/git/object-name.c:1593:2
#5 0x6988b9 in dwim_branch /home/ahunt/oss-fuzz/git/builtin/worktree.c:454:20
#6 0x695f8f in add /home/ahunt/oss-fuzz/git/builtin/worktree.c:525:19
#7 0x694a04 in cmd_worktree /home/ahunt/oss-fuzz/git/builtin/worktree.c:1036:10
#8 0x4cd60d in run_builtin /home/ahunt/oss-fuzz/git/git.c:453:11
#9 0x4cb2da in handle_builtin /home/ahunt/oss-fuzz/git/git.c:704:3
#10 0x4ccc37 in run_argv /home/ahunt/oss-fuzz/git/git.c:771:4
#11 0x4cac29 in cmd_main /home/ahunt/oss-fuzz/git/git.c:902:19
#12 0x69caee in main /home/ahunt/oss-fuzz/git/common-main.c:52:11
#13 0x7f7b7dd10349 in __libc_start_main (/lib64/libc.so.6+0x24349)
Signed-off-by: Andrzej Hunt <redacted>
---
builtin/worktree.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
From: Andrzej Hunt via GitGitGadget <hidden> Date: 2021-03-14 18:48:47
From: Andrzej Hunt <redacted>
The primary goal of this change is to stop leaking init_db_template_dir.
This leak can happen because:
1. git_init_db_config() allocates new memory into init_db_template_dir
without first freeing the existing value.
2. init_db_template_dir might already contain data, either because:
2.1 git_config() can be invoked twice with this callback in a single
process - at least 2 allocations are likely.
2.2 A single git_config() allocation can invoke the callback multiple
times for a given key (see further explanation in the function
docs) - each of those calls will trigger another leak.
The simplest fix for the leak would be to free(init_db_template_dir)
before overwriting it. Instead we choose to convert to fetching
init.templatedir via git_config_get_value() as that is more explicit,
more efficient, and avoids allocations (the returned result is owned by
the config cache, so we aren't responsible for freeing it).
If we remove init_db_template_dir, git_init_db_config() ends up being
responsible only for forwarding core.* config values to
platform_core_config(). However platform_core_config() already ignores
non-core.* config values, so we can safely remove git_init_db_config()
and invoke git_config() directly with platform_core_config() as the
callback.
The platform_core_config forwarding was originally added in:
287853392a (mingw: respect core.hidedotfiles = false in git-init again, 2019-03-11
And I suspect the potential for a leak existed since the original
implementation of git_init_db_config in:
90b45187ba (Add `init.templatedir` configuration variable., 2010-02-17)
LSAN output from t0001:
Direct leak of 73 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 0x9a7276 in xrealloc /home/ahunt/oss-fuzz/git/wrapper.c:126:8
#2 0x9362ad in strbuf_grow /home/ahunt/oss-fuzz/git/strbuf.c:98:2
#3 0x936eaa in strbuf_add /home/ahunt/oss-fuzz/git/strbuf.c:295:2
#4 0x868112 in strbuf_addstr /home/ahunt/oss-fuzz/git/./strbuf.h:304:2
#5 0x86a8ad in expand_user_path /home/ahunt/oss-fuzz/git/path.c:758:2
#6 0x720bb1 in git_config_pathname /home/ahunt/oss-fuzz/git/config.c:1287:10
#7 0x5960e2 in git_init_db_config /home/ahunt/oss-fuzz/git/builtin/init-db.c:161:11
#8 0x7255b8 in configset_iter /home/ahunt/oss-fuzz/git/config.c:1982:7
#9 0x7253fc in repo_config /home/ahunt/oss-fuzz/git/config.c:2311:2
#10 0x725ca7 in git_config /home/ahunt/oss-fuzz/git/config.c:2399:2
#11 0x593e8d in create_default_files /home/ahunt/oss-fuzz/git/builtin/init-db.c:225:2
#12 0x5935c6 in init_db /home/ahunt/oss-fuzz/git/builtin/init-db.c:449:11
#13 0x59588e in cmd_init_db /home/ahunt/oss-fuzz/git/builtin/init-db.c:714:9
#14 0x4cd60d in run_builtin /home/ahunt/oss-fuzz/git/git.c:453:11
#15 0x4cb2da in handle_builtin /home/ahunt/oss-fuzz/git/git.c:704:3
#16 0x4ccc37 in run_argv /home/ahunt/oss-fuzz/git/git.c:771:4
#17 0x4cac29 in cmd_main /home/ahunt/oss-fuzz/git/git.c:902:19
#18 0x69c4de in main /home/ahunt/oss-fuzz/git/common-main.c:52:11
#19 0x7f23552d6349 in __libc_start_main (/lib64/libc.so.6+0x24349)
Signed-off-by: Andrzej Hunt <redacted>
---
builtin/init-db.c | 28 +++++++---------------------
1 file changed, 7 insertions(+), 21 deletions(-)
@@ -212,10 +200,7 @@ static int create_default_files(const char *template_path,intreinit;intfilemode;structstrbuferr=STRBUF_INIT;--/* Just look for `init.templatedir` */-init_db_template_dir=NULL;/* re-set in case it was set before */-git_config(git_init_db_config,NULL);+constchar*init_template_dir=NULL;/**Firstcopythetemplates--wemighthavethedefault
@@ -226,7 +211,8 @@ static int create_default_files(const char *template_path,*values(sincewe'vejustpotentiallychangedwhat'savailableon*disk).*/-copy_templates(template_path);+git_config_get_value("init.templatedir",&init_template_dir);+copy_templates(template_path,init_template_dir);git_config_clear();reset_shared_repository();git_config(git_default_config,NULL);
@@ -422,8 +408,8 @@ int init_db(const char *git_dir, const char *real_git_dir,}startup_info->have_repository=1;-/* Just look for `core.hidedotfiles` */-git_config(git_init_db_config,NULL);+/* Ensure `core.hidedotfiles` is processed */+git_config(platform_core_config,NULL);safe_create_dir(git_dir,0);
From: Andrzej Hunt via GitGitGadget <hidden> Date: 2021-03-14 18:48:47
From: Andrzej Hunt <redacted>
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 <redacted>
---
builtin/clone.c | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)
From: Andrzej Hunt via GitGitGadget <hidden> Date: 2021-03-14 18:48:47
From: Andrzej Hunt <redacted>
dwim_ref() allocs a new string into ref. Instead of setting to NULL to
discard it, we can FREE_AND_NULL.
This leak appears to have been introduced in:
4cf76f6bbf (builtin/reset: compute checkout metadata for reset, 2020-03-16)
This leak was found when running t0001 with LSAN, see also LSAN output below:
Direct leak of 5 byte(s) in 1 object(s) allocated from:
#0 0x486514 in strdup /home/abuild/rpmbuild/BUILD/llvm-11.0.0.src/build/../projects/compiler-rt/lib/asan/asan_interceptors.cpp:452:3
#1 0x9a7108 in xstrdup /home/ahunt/oss-fuzz/git/wrapper.c:29:14
#2 0x8add6b in expand_ref /home/ahunt/oss-fuzz/git/refs.c:670:12
#3 0x8ad777 in repo_dwim_ref /home/ahunt/oss-fuzz/git/refs.c:644:22
#4 0x6394af in dwim_ref /home/ahunt/oss-fuzz/git/./refs.h:162:9
#5 0x637e5c in cmd_reset /home/ahunt/oss-fuzz/git/builtin/reset.c:426: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 0x69c5ce in main /home/ahunt/oss-fuzz/git/common-main.c:52:11
#11 0x7f57ebb9d349 in __libc_start_main (/lib64/libc.so.6+0x24349)
Signed-off-by: Andrzej Hunt <redacted>
---
builtin/reset.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Andrzej Hunt via GitGitGadget <hidden> Date: 2021-03-14 18:48:47
From: Andrzej Hunt <redacted>
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 <redacted>
---
builtin/ls-remote.c | 4 ++--
builtin/remote.c | 8 ++++----
transport.c | 2 ++
3 files changed, 8 insertions(+), 6 deletions(-)
@@ -1452,6 +1452,8 @@ int transport_disconnect(struct transport *transport)intret=0;if(transport->vtable->disconnect)ret=transport->vtable->disconnect(transport);+if(transport->got_remote_refs)+free_refs((void*)transport->remote_refs);free(transport);returnret;}
From: Andrzej Hunt via GitGitGadget <hidden> Date: 2021-03-14 18:48:47
From: Andrzej Hunt <redacted>
template_dir starts off pointing to either argv or nothing. However if
the value supplied in argv is a relative path, absolute_pathdup() is
used to turn it into an absolute path. absolute_pathdup() allocates
a new string, and we then "leak" it when cmd_init_db() completes.
We don't bother to actually free the return value (instead we UNLEAK
it), because there's no significant advantage to doing so here.
Correctly freeing it would require more significant changes to code flow
which would be more noisy than beneficial.
Signed-off-by: Andrzej Hunt <redacted>
---
builtin/init-db.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
From: Eric Sunshine <hidden> Date: 2021-03-14 19:49:24
On Sun, Mar 14, 2021 at 2:49 PM Andrzej Hunt via GitGitGadget
[off-list ref] wrote:
preprocess_options() allocates new strings for help messages for
OPTION_ALIAS. Therefore we also need to clean those help messages up
when freeing the returned options.
[...]
Signed-off-by: Andrzej Hunt <redacted>
fold
---
From: Andrzej Hunt <hidden> Date: 2021-03-14 20:01:29
On 14 Mar 2021, at 19:47, Andrzej Hunt via GitGitGadget [off-list ref] wrote:
+ for (i = 0; options[i].type != OPTION_END; i++) {
+ if (options[i].flags & PARSE_OPT_FROM_ALIAS) {
+ free((void *)options[i].help);
+ }
Oops, I did it again: I’ll remove these unnecessary braces in V3.
From: Martin Ågren <hidden> Date: 2021-03-14 20:26:58
On Sun, 14 Mar 2021 at 20:05, Andrzej Hunt via GitGitGadget
[off-list ref] wrote:
From: Andrzej Hunt <redacted>
Because it's easier to read, but also likely to be easier to maintain.
I am making this change because I need to add a new flag in a later
commit.
Makes sense.
Also add a trailing comma to the last enum entry to simplify addition of
new flags.
Those last few conversions close the gap and we end with 1024 rather
than 2048. That "should" be ok, unless some piece of code relies on
exact values here, or even their relations(!). Hopefully not? Might be
worth calling out in the commit message that you're changing some
values, if you're rerolling anyway. (Or you could leave 1<<8 unused to
make this a true no-op, then use that value in the next patch. Anyway, I
think this is safely in bikeshedding land.)
Martin
From: Andrzej Hunt <hidden> Date: 2021-03-15 16:21:54
On 14/03/2021 20:48, Eric Sunshine wrote:
On Sun, Mar 14, 2021 at 2:49 PM Andrzej Hunt via GitGitGadget
[off-list ref] wrote:
quoted
preprocess_options() allocates new strings for help messages for
OPTION_ALIAS. Therefore we also need to clean those help messages up
when freeing the returned options.
[...]
Signed-off-by: Andrzej Hunt <redacted>
fold
---
From: Andrzej Hunt <hidden> Date: 2021-03-15 16:21:54
On 14/03/2021 21:25, Martin Ågren wrote:
On Sun, 14 Mar 2021 at 20:05, Andrzej Hunt via GitGitGadget
[off-list ref] wrote:
quoted
From: Andrzej Hunt <redacted>
Because it's easier to read, but also likely to be easier to maintain.
I am making this change because I need to add a new flag in a later
commit.
Makes sense.
quoted
Also add a trailing comma to the last enum entry to simplify addition of
new flags.
Those last few conversions close the gap and we end with 1024 rather
than 2048. That "should" be ok, unless some piece of code relies on
exact values here, or even their relations(!). Hopefully not? Might be
worth calling out in the commit message that you're changing some
values, if you're rerolling anyway. (Or you could leave 1<<8 unused to
make this a true no-op, then use that value in the next patch. Anyway, I
think this is safely in bikeshedding land.)
Thanks for catching this. Indeed I don't think any code relies on the specific values - but I don't feel comfortable making these changes silently or accidentally.
I will follow your suggested approach: I'll update this patch to retain the original values - and later on I'll grab the unused value for my new flag. That way *if* the new value were to cause a regression, it will be much more obvious where it came from.
ATB,
Andrzej
From: Andrzej Hunt <hidden> Date: 2021-03-15 16:24:35
On 14/03/2021 17:54, Andrzej Hunt wrote:
I was wrong when it comes to LSAN being broken. What was actually happening is: we default to running ASAN and LSAN with abort_on_error=1, and I had overridden that setting when running with ASAN. When I switched to LSAN, abort_on_error was enabled again - and I was just misinterpreting the intentional abortion as opposed to seeing an unexplained crashes. [As far as I can tell, abort_on_error is needed to detect leaks during a test_must_fail and similar scenarios.]
Actually I might have been unintentionally right the first time round: running clang+LSAN against t0000 (with pipe-related fixes applied) results in reports as follows, which are effectively useless:
Direct leak of 41 byte(s) in 1 object(s) allocated from:
#0 0x44fc98 in malloc /home/abuild/rpmbuild/BUILD/llvm-11.0.0.src/build/../projects/compiler-rt/lib/lsan/lsan_interceptors.cpp:56:3
#1 0x7fcdbb3dc749 in strdup (/lib64/libc.so.6+0x8b749)
Switching back to ASAN shows me the real stack instead:
Direct leak of 41 byte(s) in 1 object(s) allocated from:
#0 0x486834 in strdup /home/abuild/rpmbuild/BUILD/llvm-11.0.0.src/build/../projects/compiler-rt/lib/asan/asan_interceptors.cpp:452:3
#1 0x9ab1b8 in xstrdup /home/ahunt/oss-fuzz/git/wrapper.c:29:14
... rest of stack here ...
To make it trickier, that new "leak" happens inside a test_must_fail - the LSAN output is swallowed, making it hard to diagnose. I'll try to prepare a separate patch to not discard stderr in that scenario.
The issue here is that:
- test_must_fail does actually keep git's stderr on stderr (there is
some redirection within test_must_fail itself - but eventually
what was git's stderr becomes test_must_fail's stderr).
- many uses of test_must_fail redirect stderr into a temporary file.
- when git fails as expected, test_must_fail succeeds, and the
next command in the test validates that temporary file (that
validation step prints that error output on failure).
- however if git aborted, or didn't fail as expected: test_must_fail
returns an error code. git's error messages remain in the temporary
file and aren't printed.
My potentially naive idea is that within test_must_fail, git's output should be sent to both test_must_fail's stderr, and also to fd-4 (which seems to be where verbose output should be sent).
However hat might also result in errors being printed twice (I believe that will happen for test_must_fail uses that don't redirect stderr into a temporary file).
From: Andrzej Hunt via GitGitGadget <hidden> Date: 2021-03-21 16:59:57
From: Andrzej Hunt <redacted>
shorten_unambiguous_ref() returns an allocated string. We have to
track it separately from the const refname.
This leak has existed since:
9ab55daa55 (git symbolic-ref --delete $symref, 2012-10-21)
This leak was found when running t0001 with LSAN, see also LSAN output
below:
Direct leak of 19 byte(s) in 1 object(s) allocated from:
#0 0x486514 in strdup /home/abuild/rpmbuild/BUILD/llvm-11.0.0.src/build/../projects/compiler-rt/lib/asan/asan_interceptors.cpp:452:3
#1 0x9ab048 in xstrdup /home/ahunt/oss-fuzz/git/wrapper.c:29:14
#2 0x8b452f in refs_shorten_unambiguous_ref /home/ahunt/oss-fuzz/git/refs.c
#3 0x8b47e8 in shorten_unambiguous_ref /home/ahunt/oss-fuzz/git/refs.c:1287:9
#4 0x679fce in check_symref /home/ahunt/oss-fuzz/git/builtin/symbolic-ref.c:28:14
#5 0x679ad8 in cmd_symbolic_ref /home/ahunt/oss-fuzz/git/builtin/symbolic-ref.c:70:9
#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 0x69cc6e in main /home/ahunt/oss-fuzz/git/common-main.c:52:11
#11 0x7f98388a4349 in __libc_start_main (/lib64/libc.so.6+0x24349)
Signed-off-by: Andrzej Hunt <redacted>
---
builtin/symbolic-ref.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
@@ -24,9 +24,11 @@ static int check_symref(const char *HEAD, int quiet, int shorten, int print)return1;}if(print){+char*to_free=NULL;if(shorten)-refname=shorten_unambiguous_ref(refname,0);+refname=to_free=shorten_unambiguous_ref(refname,0);puts(refname);+free(to_free);}return0;}
From: Andrzej Hunt via GitGitGadget <hidden> Date: 2021-03-21 16:59:57
From: Andrzej Hunt <redacted>
dwim_ref() allocs a new string into ref. Instead of setting to NULL to
discard it, we can FREE_AND_NULL.
This leak appears to have been introduced in:
4cf76f6bbf (builtin/reset: compute checkout metadata for reset, 2020-03-16)
This leak was found when running t0001 with LSAN, see also LSAN output below:
Direct leak of 5 byte(s) in 1 object(s) allocated from:
#0 0x486514 in strdup /home/abuild/rpmbuild/BUILD/llvm-11.0.0.src/build/../projects/compiler-rt/lib/asan/asan_interceptors.cpp:452:3
#1 0x9a7108 in xstrdup /home/ahunt/oss-fuzz/git/wrapper.c:29:14
#2 0x8add6b in expand_ref /home/ahunt/oss-fuzz/git/refs.c:670:12
#3 0x8ad777 in repo_dwim_ref /home/ahunt/oss-fuzz/git/refs.c:644:22
#4 0x6394af in dwim_ref /home/ahunt/oss-fuzz/git/./refs.h:162:9
#5 0x637e5c in cmd_reset /home/ahunt/oss-fuzz/git/builtin/reset.c:426: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 0x69c5ce in main /home/ahunt/oss-fuzz/git/common-main.c:52:11
#11 0x7f57ebb9d349 in __libc_start_main (/lib64/libc.so.6+0x24349)
Signed-off-by: Andrzej Hunt <redacted>
---
builtin/reset.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Andrzej Hunt via GitGitGadget <hidden> Date: 2021-03-21 16:59:57
From: Andrzej Hunt <redacted>
Make sure that we release the temporary strbuf during dwim_branch() for
all codepaths (and not just for the early return).
This leak appears to have been introduced in:
f60a7b763f (worktree: teach "add" to check out existing branches, 2018-04-24)
Note that UNLEAK(branchname) is still needed: the returned result is
used in add(), and is stored in a pointer which is used to point at one
of:
- a string literal ("HEAD")
- member of argv (whatever the user specified in their invocation)
- or our newly allocated string returned from dwim_branch()
Fixing the branchname leak isn't impossible, but does not seem
worthwhile given that add() is called directly from cmd_main(), and
cmd_main() returns immediately thereafter - UNLEAK is good enough.
This leak was found when running t0001 with LSAN, see also LSAN output
below:
Direct leak of 60 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 0x9ab076 in xrealloc /home/ahunt/oss-fuzz/git/wrapper.c:126:8
#2 0x939fcd in strbuf_grow /home/ahunt/oss-fuzz/git/strbuf.c:98:2
#3 0x93af53 in strbuf_splice /home/ahunt/oss-fuzz/git/strbuf.c:239:3
#4 0x83559a in strbuf_check_branch_ref /home/ahunt/oss-fuzz/git/object-name.c:1593:2
#5 0x6988b9 in dwim_branch /home/ahunt/oss-fuzz/git/builtin/worktree.c:454:20
#6 0x695f8f in add /home/ahunt/oss-fuzz/git/builtin/worktree.c:525:19
#7 0x694a04 in cmd_worktree /home/ahunt/oss-fuzz/git/builtin/worktree.c:1036:10
#8 0x4cd60d in run_builtin /home/ahunt/oss-fuzz/git/git.c:453:11
#9 0x4cb2da in handle_builtin /home/ahunt/oss-fuzz/git/git.c:704:3
#10 0x4ccc37 in run_argv /home/ahunt/oss-fuzz/git/git.c:771:4
#11 0x4cac29 in cmd_main /home/ahunt/oss-fuzz/git/git.c:902:19
#12 0x69caee in main /home/ahunt/oss-fuzz/git/common-main.c:52:11
#13 0x7f7b7dd10349 in __libc_start_main (/lib64/libc.so.6+0x24349)
Signed-off-by: Andrzej Hunt <redacted>
---
builtin/worktree.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
From: Andrzej Hunt via GitGitGadget <hidden> Date: 2021-03-21 16:59:57
From: Andrzej Hunt <redacted>
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 <redacted>
---
builtin/clone.c | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)
From: Andrzej Hunt via GitGitGadget <hidden> Date: 2021-03-21 16:59:58
From: Andrzej Hunt <redacted>
Because it's easier to read, but also likely to be easier to maintain.
I am making this change because I need to add a new flag in a later
commit.
Also add a trailing comma to the last enum entry to simplify addition of
new flags.
This change was originally suggested by Peff in:
https://public-inbox.org/git/YEZ%2FBWWbpfVwl6nO@coredump.intra.peff.net/
Signed-off-by: Andrzej Hunt <redacted>
---
parse-options.h | 34 +++++++++++++++++-----------------
1 file changed, 17 insertions(+), 17 deletions(-)
From: Andrzej Hunt via GitGitGadget <hidden> Date: 2021-03-21 16:59:58
From: Andrzej Hunt <redacted>
template_dir starts off pointing to either argv or nothing. However if
the value supplied in argv is a relative path, absolute_pathdup() is
used to turn it into an absolute path. absolute_pathdup() allocates
a new string, and we then "leak" it when cmd_init_db() completes.
We don't bother to actually free the return value (instead we UNLEAK
it), because there's no significant advantage to doing so here.
Correctly freeing it would require more significant changes to code flow
which would be more noisy than beneficial.
Signed-off-by: Andrzej Hunt <redacted>
---
builtin/init-db.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
From: Andrzej Hunt via GitGitGadget <hidden> Date: 2021-03-21 16:59:58
From: Andrzej Hunt <redacted>
The primary goal of this change is to stop leaking init_db_template_dir.
This leak can happen because:
1. git_init_db_config() allocates new memory into init_db_template_dir
without first freeing the existing value.
2. init_db_template_dir might already contain data, either because:
2.1 git_config() can be invoked twice with this callback in a single
process - at least 2 allocations are likely.
2.2 A single git_config() allocation can invoke the callback multiple
times for a given key (see further explanation in the function
docs) - each of those calls will trigger another leak.
The simplest fix for the leak would be to free(init_db_template_dir)
before overwriting it. Instead we choose to convert to fetching
init.templatedir via git_config_get_value() as that is more explicit,
more efficient, and avoids allocations (the returned result is owned by
the config cache, so we aren't responsible for freeing it).
If we remove init_db_template_dir, git_init_db_config() ends up being
responsible only for forwarding core.* config values to
platform_core_config(). However platform_core_config() already ignores
non-core.* config values, so we can safely remove git_init_db_config()
and invoke git_config() directly with platform_core_config() as the
callback.
The platform_core_config forwarding was originally added in:
287853392a (mingw: respect core.hidedotfiles = false in git-init again, 2019-03-11
And I suspect the potential for a leak existed since the original
implementation of git_init_db_config in:
90b45187ba (Add `init.templatedir` configuration variable., 2010-02-17)
LSAN output from t0001:
Direct leak of 73 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 0x9a7276 in xrealloc /home/ahunt/oss-fuzz/git/wrapper.c:126:8
#2 0x9362ad in strbuf_grow /home/ahunt/oss-fuzz/git/strbuf.c:98:2
#3 0x936eaa in strbuf_add /home/ahunt/oss-fuzz/git/strbuf.c:295:2
#4 0x868112 in strbuf_addstr /home/ahunt/oss-fuzz/git/./strbuf.h:304:2
#5 0x86a8ad in expand_user_path /home/ahunt/oss-fuzz/git/path.c:758:2
#6 0x720bb1 in git_config_pathname /home/ahunt/oss-fuzz/git/config.c:1287:10
#7 0x5960e2 in git_init_db_config /home/ahunt/oss-fuzz/git/builtin/init-db.c:161:11
#8 0x7255b8 in configset_iter /home/ahunt/oss-fuzz/git/config.c:1982:7
#9 0x7253fc in repo_config /home/ahunt/oss-fuzz/git/config.c:2311:2
#10 0x725ca7 in git_config /home/ahunt/oss-fuzz/git/config.c:2399:2
#11 0x593e8d in create_default_files /home/ahunt/oss-fuzz/git/builtin/init-db.c:225:2
#12 0x5935c6 in init_db /home/ahunt/oss-fuzz/git/builtin/init-db.c:449:11
#13 0x59588e in cmd_init_db /home/ahunt/oss-fuzz/git/builtin/init-db.c:714:9
#14 0x4cd60d in run_builtin /home/ahunt/oss-fuzz/git/git.c:453:11
#15 0x4cb2da in handle_builtin /home/ahunt/oss-fuzz/git/git.c:704:3
#16 0x4ccc37 in run_argv /home/ahunt/oss-fuzz/git/git.c:771:4
#17 0x4cac29 in cmd_main /home/ahunt/oss-fuzz/git/git.c:902:19
#18 0x69c4de in main /home/ahunt/oss-fuzz/git/common-main.c:52:11
#19 0x7f23552d6349 in __libc_start_main (/lib64/libc.so.6+0x24349)
Signed-off-by: Andrzej Hunt <redacted>
---
builtin/init-db.c | 28 +++++++---------------------
1 file changed, 7 insertions(+), 21 deletions(-)
@@ -212,10 +200,7 @@ static int create_default_files(const char *template_path,intreinit;intfilemode;structstrbuferr=STRBUF_INIT;--/* Just look for `init.templatedir` */-init_db_template_dir=NULL;/* re-set in case it was set before */-git_config(git_init_db_config,NULL);+constchar*init_template_dir=NULL;/**Firstcopythetemplates--wemighthavethedefault
@@ -226,7 +211,8 @@ static int create_default_files(const char *template_path,*values(sincewe'vejustpotentiallychangedwhat'savailableon*disk).*/-copy_templates(template_path);+git_config_get_value("init.templatedir",&init_template_dir);+copy_templates(template_path,init_template_dir);git_config_clear();reset_shared_repository();git_config(git_default_config,NULL);
@@ -422,8 +408,8 @@ int init_db(const char *git_dir, const char *real_git_dir,}startup_info->have_repository=1;-/* Just look for `core.hidedotfiles` */-git_config(git_init_db_config,NULL);+/* Ensure `core.hidedotfiles` is processed */+git_config(platform_core_config,NULL);safe_create_dir(git_dir,0);
From: Andrzej Hunt via GitGitGadget <hidden> Date: 2021-03-21 16:59:58
From: Andrzej Hunt <redacted>
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 <redacted>
---
builtin/ls-remote.c | 4 ++--
builtin/remote.c | 8 ++++----
transport.c | 2 ++
3 files changed, 8 insertions(+), 6 deletions(-)
@@ -1452,6 +1452,8 @@ int transport_disconnect(struct transport *transport)intret=0;if(transport->vtable->disconnect)ret=transport->vtable->disconnect(transport);+if(transport->got_remote_refs)+free_refs((void*)transport->remote_refs);free(transport);returnret;}
From: Andrzej Hunt via GitGitGadget <hidden> Date: 2021-03-21 16:59:58
From: Andrzej Hunt <redacted>
preprocess_options() allocates new strings for help messages for
OPTION_ALIAS. Therefore we also need to clean those help messages up
when freeing the returned options.
First introduced in:
7c280589cf (parse-options: teach "git cmd -h" to show alias as alias, 2020-03-16)
The preprocessed options themselves no longer contain any indication
that a given option is/was an alias - therefore we add a new flag to
indicate former aliases. (An alternative approach would be to look back
at the original options to determine which options are aliases - but
that seems like a fragile approach. Or we could even look at the
alias_groups list - which might be less fragile, but would be slower
as it requires nested looping.)
As far as I can tell, parse_options() is only ever used once per
command, and the help messages are small - hence this leak has very
little impact.
This leak was found while running t0001. LSAN output can be found below:
Direct leak of 65 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 0x9aae36 in xrealloc /home/ahunt/oss-fuzz/git/wrapper.c:126:8
#2 0x939d8d in strbuf_grow /home/ahunt/oss-fuzz/git/strbuf.c:98:2
#3 0x93b936 in strbuf_vaddf /home/ahunt/oss-fuzz/git/strbuf.c:392:3
#4 0x93b7ff in strbuf_addf /home/ahunt/oss-fuzz/git/strbuf.c:333:2
#5 0x86747e in preprocess_options /home/ahunt/oss-fuzz/git/parse-options.c:666:3
#6 0x866ed2 in parse_options /home/ahunt/oss-fuzz/git/parse-options.c:847:17
#7 0x51c4a7 in cmd_clone /home/ahunt/oss-fuzz/git/builtin/clone.c:989:9
#8 0x4cd60d in run_builtin /home/ahunt/oss-fuzz/git/git.c:453:11
#9 0x4cb2da in handle_builtin /home/ahunt/oss-fuzz/git/git.c:704:3
#10 0x4ccc37 in run_argv /home/ahunt/oss-fuzz/git/git.c:771:4
#11 0x4cac29 in cmd_main /home/ahunt/oss-fuzz/git/git.c:902:19
#12 0x69c9fe in main /home/ahunt/oss-fuzz/git/common-main.c:52:11
#13 0x7fdac42d4349 in __libc_start_main (/lib64/libc.so.6+0x24349)
Signed-off-by: Andrzej Hunt <redacted>
---
parse-options.c | 19 ++++++++++++++++++-
parse-options.h | 1 +
2 files changed, 19 insertions(+), 1 deletion(-)