Fix all leaks in t0001

PATCH Superseded Andrzej Hunt via GitGitGadget 6 participants 52 messages Mar 8, 2021 – Mar 21, 2021

Share & tools

Andrzej Hunt via GitGitGadget

author

2021-03-08 18:37 UTC #0 lore raw

cover Fix all leaks in t0001

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

Signature · 1 line

gitgitgadget

Andrzej Hunt via GitGitGadget

author

2021-03-08 18:37 UTC #1 lore raw

PATCH 1/7 symbolic-ref: don't leak shortened refname in check_symref()

From: Andrzej Hunt <[email protected]>

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 <[email protected]>

---
 builtin/symbolic-ref.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

builtin/symbolic-ref.c +9 -3

index 80237f0df10f..8cf52599693a 100644
@@ -24,9 +24,15 @@ static int check_symref(const char *HEAD, int quiet, int shorten, int print)
 			return 1;
 	}
 	if (print) {
-		if (shorten)
-			refname = shorten_unambiguous_ref(refname, 0);
-		puts(refname);
+		if (shorten) {
+			const char *shortened_refname;
+
+			shortened_refname = shorten_unambiguous_ref(refname, 0);
+			puts(shortened_refname);
+			free((void *)shortened_refname);
+		} else {
+			puts(refname);
+		}
 	}
 	return 0;
 }

Signature · 1 line

gitgitgadget

Jeff King

2021-03-08 19:02 UTC #2 lore raw

Re: [PATCH 1/7] symbolic-ref: don't leak shortened refname in check_symref()

Quoted 9 lines · Andrzej Hunt via GitGitGadget

From: Andrzej Hunt <[email protected]>
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:

Quoted 12 lines

    #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.

Quoted 10 lines

builtin/symbolic-ref.c +0 -2

index 80237f0df10f..8cf52599693a 100644
@@ -24,9 +24,15 @@ static int check_symref(const char *HEAD, int quiet, int shorten, int print)
 			return 1;
 	}
 	if (print) {
-		if (shorten)
-			refname = shorten_unambiguous_ref(refname, 0);

builtin/symbolic-ref.c static int check_symref(const char *HEAD, int quiet, int shorten, int print) · line 35

Quoted 10 lines

builtin/symbolic-ref.c +9 -1

-		puts(refname);
+		if (shorten) {
+			const char *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

Andrzej Hunt

2021-03-14 18:08 UTC #3 lore raw

Re: [PATCH 1/7] symbolic-ref: don't leak shortened refname in check_symref()

Quoted 29 lines · Jeff King

Quoted 9 lines · Andrzej Hunt via GitGitGadget
From: Andrzej Hunt <[email protected]>
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:
Quoted 12 lines
     #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:

Quoted 5 lines

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!

Quoted 12 lines

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);
   }

Quoted 3 lines

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!

Andrzej Hunt via GitGitGadget

author

2021-03-08 18:37 UTC #4 lore raw

PATCH 2/7 reset: free instead of leaking unneeded ref

In reply to Andrzej Hunt via GitGitGadget

From: Andrzej Hunt <[email protected]>

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 <[email protected]>

---
 builtin/reset.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

builtin/reset.c +1 -1

index c635b062c3a7..43e855cb8876 100644
@@ -425,7 +425,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 
 			dwim_ref(rev, strlen(rev), &dummy, &ref, 0);
 			if (ref && !starts_with(ref, "refs/"))
-				ref = NULL;
+				FREE_AND_NULL(ref);
 
 			err = reset_index(ref, &oid, reset_type, quiet);
 			if (reset_type == KEEP && !err)

Signature · 1 line

gitgitgadget

Jeff King

2021-03-08 19:04 UTC #5 lore raw

Re: [PATCH 2/7] reset: free instead of leaking unneeded ref

builtin/reset.c int cmd_reset(int argc, const char **argv, const char *prefix) · line 428

Quoted 7 lines · Andrzej Hunt via GitGitGadget

dwim_ref() allocs a new string into ref. Instead of setting to NULL to discard it, we can FREE_AND_NULL.
[...]
builtin/reset.c +1 -1
 			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

Andrzej Hunt via GitGitGadget

author

2021-03-08 18:37 UTC #6 lore raw

PATCH 3/7 clone: free or UNLEAK further pointers when finished

In reply to Andrzej Hunt via GitGitGadget

From: Andrzej Hunt <[email protected]>

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 <[email protected]>

---
 builtin/clone.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

builtin/clone.c +10 -3

index 51e844a2de0a..7f76fdceceb7 100644
@@ -967,7 +967,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	char *path, *dir, *display_repo = NULL;
 	int dest_exists, real_dest_exists = 0;
 	const struct ref *refs, *remote_head;
-	const struct ref *remote_head_points_at;
+	const struct ref *remote_head_points_at = NULL;
 	const struct ref *our_head_points_at;
 	struct ref *mapped_refs;
 	const struct ref *ref;

@@ -1017,9 +1017,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	repo_name = argv[0];
 
 	path = get_repo_path(repo_name, &is_bundle);
-	if (path)
+	if (path) {
+		free(path);
 		repo = absolute_pathdup(repo_name);
-	else if (strchr(repo_name, ':')) {
+	} else if (strchr(repo_name, ':')) {
 		repo = repo_name;
 		display_repo = transport_anonymize_url(repo);
 	} else

@@ -1393,6 +1394,12 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	strbuf_release(&reflog_msg);
 	strbuf_release(&branch_top);
 	strbuf_release(&key);
+	free_refs(mapped_refs);
+	free_refs((void *)remote_head_points_at);
+	free_refs((void *)refs);
+	free(dir);
+	free(path);
+	UNLEAK(repo);
 	junk_mode = JUNK_LEAVE_ALL;
 
 	strvec_clear(&transport_ls_refs_options.ref_prefixes);

Signature · 1 line

gitgitgadget

Jeff King

2021-03-08 19:13 UTC #7 lore raw

Re: [PATCH 3/7] clone: free or UNLEAK further pointers when finished

builtin/clone.c int cmd_clone(int argc, const char **argv, const char *prefix) · line 1022

Quoted 8 lines · Andrzej Hunt via GitGitGadget

builtin/clone.c +2 -1

@@ -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?

builtin/clone.c int cmd_clone(int argc, const char **argv, const char *prefix) · line 1398

Quoted 6 lines

builtin/clone.c +2 -0

@@ -1393,6 +1394,12 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	strbuf_release(&reflog_msg);
 	strbuf_release(&branch_top);
 	strbuf_release(&key);
+	free_refs(mapped_refs);
+	free_refs((void *)remote_head_points_at);

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?

builtin/clone.c int cmd_clone(int argc, const char **argv, const char *prefix) · line 1399

Quoted 1 line

builtin/clone.c +1 -0
+	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).

builtin/clone.c int cmd_clone(int argc, const char **argv, const char *prefix) · line 1402

Quoted 1 line

builtin/clone.c +1 -0
+	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

Andrzej Hunt

2021-03-14 16:57 UTC #8 lore raw

Re: [PATCH 3/7] clone: free or UNLEAK further pointers when finished

Quoted 5 lines · Jeff King

Quoted 3 lines · Andrzej Hunt via GitGitGadget
builtin/clone.c +0 -0
@@ -1017,9 +1017,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
  	repo_name = argv[0];
 

Quoted 9 lines

builtin/clone.c int cmd_clone(int argc, const char **argv, const char *prefix) · line 1022
Quoted 5 lines
builtin/clone.c +2 -1
  	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.

Quoted 13 lines

builtin/clone.c int cmd_clone(int argc, const char **argv, const char *prefix) · line 1398
Quoted 7 lines
builtin/clone.c +2 -0
@@ -1393,6 +1394,12 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
  	strbuf_release(&reflog_msg);
  	strbuf_release(&branch_top);
  	strbuf_release(&key);
+	free_refs(mapped_refs);
+	free_refs((void *)remote_head_points_at);
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 10 lines

builtin/clone.c int cmd_clone(int argc, const char **argv, const char *prefix) · line 1399
Quoted 1 line
builtin/clone.c +1 -0
+	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#n953
https://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().)

Andrzej Hunt via GitGitGadget

author

2021-03-08 18:37 UTC #9 lore raw

PATCH 4/7 worktree: fix leak in dwim_branch()

In reply to Andrzej Hunt via GitGitGadget

From: Andrzej Hunt <[email protected]>

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 <[email protected]>

---
 builtin/worktree.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

builtin/worktree.c +6 -6

index 1cd5c2016e3f..44d3f058d065 100644
@@ -445,17 +445,17 @@ static void print_preparing_worktree_line(int detach,
 
 static const char *dwim_branch(const char *path, const char **new_branch)
 {
-	int n;
+	int n, branch_exists;
 	const char *s = worktree_basename(path, &n);
 	const char *branchname = xstrndup(s, n);
 	struct strbuf ref = STRBUF_INIT;
-
 	UNLEAK(branchname);
-	if (!strbuf_check_branch_ref(&ref, branchname) &&
-	    ref_exists(ref.buf)) {
-		strbuf_release(&ref);
+
+	branch_exists = (!strbuf_check_branch_ref(&ref, branchname) &&
+			 ref_exists(ref.buf));
+	strbuf_release(&ref);
+	if (branch_exists)
 		return branchname;
-	}
 
 	*new_branch = branchname;
 	if (guess_remote) {

Signature · 1 line

gitgitgadget

Jeff King

2021-03-08 19:17 UTC #10 lore raw

Re: [PATCH 4/7] worktree: fix leak in dwim_branch()

Quoted 2 lines · Andrzej Hunt via GitGitGadget

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:

builtin/worktree.c static void print_preparing_worktree_line(int detach, · line 448

Quoted 4 lines

static const char *dwim_branch(const char *path, const char **new_branch) {
builtin/worktree.c +1 -1
-	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).

builtin/worktree.c static void print_preparing_worktree_line(int detach, · line 455

Quoted 6 lines

builtin/worktree.c +3 -3

-	if (!strbuf_check_branch_ref(&ref, branchname) &&
-	    ref_exists(ref.buf)) {
-		strbuf_release(&ref);
+
+	branch_exists = (!strbuf_check_branch_ref(&ref, branchname) &&
+			 ref_exists(ref.buf));

We'd usually omit the extra parentheses here. I.e.,:

  branch_exists = !strbuf_check_branch_ref(&ref, branchname) &&
                  ref_exists(ref.buf);

-Peff

Andrzej Hunt

2021-03-14 18:02 UTC #11 lore raw

Re: [PATCH 4/7] worktree: fix leak in dwim_branch()

Quoted 6 lines · Jeff King

Quoted 2 lines · Andrzej Hunt via GitGitGadget
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:

Thank, I'll fix both.

Quoted 10 lines

builtin/worktree.c static void print_preparing_worktree_line(int detach, · line 455
Quoted 7 lines
builtin/worktree.c +3 -3
-	if (!strbuf_check_branch_ref(&ref, branchname) &&
-	    ref_exists(ref.buf)) {
-		strbuf_release(&ref);
+
+	branch_exists = (!strbuf_check_branch_ref(&ref, branchname) &&
+			 ref_exists(ref.buf));
We'd usually omit the extra parentheses here. I.e.,:

Quoted 2 lines

   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.

Andrzej Hunt via GitGitGadget

author

2021-03-08 18:37 UTC #12 lore raw

PATCH 5/7 init: remove git_init_db_config() while fixing leaks

In reply to Andrzej Hunt via GitGitGadget

From: Andrzej Hunt <[email protected]>

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 <[email protected]>

---
 builtin/init-db.c | 28 +++++++---------------------
 1 file changed, 7 insertions(+), 21 deletions(-)

builtin/init-db.c +7 ~~-21~~

index dcc45bef5148..d31dbc883746 100644
@@ -25,7 +25,6 @@
 
 static int init_is_bare_repository = 0;
 static int init_shared_repository = -1;
-static const char *init_db_template_dir;
 
 static void copy_templates_1(struct strbuf *path, struct strbuf *template_path,
 			     DIR *dir)

@@ -94,7 +93,7 @@ static void copy_templates_1(struct strbuf *path, struct strbuf *template_path,
 	}
 }
 
-static void copy_templates(const char *template_dir)
+static void copy_templates(const char *template_dir, const char *init_template_dir)
 {
 	struct strbuf path = STRBUF_INIT;
 	struct strbuf template_path = STRBUF_INIT;

@@ -107,7 +106,7 @@ static void copy_templates(const char *template_dir)
 	if (!template_dir)
 		template_dir = getenv(TEMPLATE_DIR_ENVIRONMENT);
 	if (!template_dir)
-		template_dir = init_db_template_dir;
+		template_dir = init_template_dir;
 	if (!template_dir)
 		template_dir = to_free = system_path(DEFAULT_GIT_TEMPLATE_DIR);
 	if (!template_dir[0]) {

@@ -154,17 +153,6 @@ static void copy_templates(const char *template_dir)
 	clear_repository_format(&template_format);
 }
 
-static int git_init_db_config(const char *k, const char *v, void *cb)
-{
-	if (!strcmp(k, "init.templatedir"))
-		return git_config_pathname(&init_db_template_dir, k, v);
-
-	if (starts_with(k, "core."))
-		return platform_core_config(k, v, cb);
-
-	return 0;
-}
-
 /*
  * If the git_dir is not directly inside the working tree, then git will not
  * find it by default, and we need to set the worktree explicitly.

@@ -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);
 
 	safe_create_dir(git_dir, 0);

Signature · 1 line

gitgitgadget

Jeff King

2021-03-08 19:30 UTC #13 lore raw

Re: [PATCH 5/7] init: remove git_init_db_config() while fixing leaks

Quoted 23 lines · Andrzej Hunt via GitGitGadget

builtin/init-db.c +3 -5

@@ -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);

builtin/init-db.c int init_db(const char *git_dir, const char *real_git_dir, · line 412

Quoted 7 lines

builtin/init-db.c +2 -2

@@ -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

Andrzej Hunt via GitGitGadget

author

2021-03-08 18:37 UTC #14 lore raw

PATCH 6/7 init-db: silence template_dir leak when converting to absolute path

In reply to Andrzej Hunt via GitGitGadget

From: Andrzej Hunt <[email protected]>

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 <[email protected]>

---
 builtin/init-db.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

builtin/init-db.c +3 -1

index d31dbc883746..efc66523e22c 100644
@@ -561,8 +561,10 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)
 	if (real_git_dir && !is_absolute_path(real_git_dir))
 		real_git_dir = real_pathdup(real_git_dir, 1);
 
-	if (template_dir && *template_dir && !is_absolute_path(template_dir))
+	if (template_dir && *template_dir && !is_absolute_path(template_dir)) {
 		template_dir = absolute_pathdup(template_dir);
+		UNLEAK(template_dir);
+	}
 
 	if (argc == 1) {
 		int mkdir_tried = 0;

Signature · 1 line

gitgitgadget

Jeff King

2021-03-08 19:31 UTC #15 lore raw

Re: [PATCH 6/7] init-db: silence template_dir leak when converting to absolute path

Quoted 9 lines · Andrzej Hunt via GitGitGadget

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

Andrzej Hunt via GitGitGadget

author

2021-03-08 18:37 UTC #16 lore raw

PATCH 7/7 parse-options: don't leak alias help messages

In reply to Andrzej Hunt via GitGitGadget

From: Andrzej Hunt <[email protected]>

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 <[email protected]>

---
 parse-options.c | 29 ++++++++++++++++++++++++-----
 1 file changed, 24 insertions(+), 5 deletions(-)

parse-options.c +24 -5

index fbea16eaf5c2..3fe1dacc08cb 100644
@@ -625,6 +625,8 @@ static int show_gitcomp(const struct option *opts, int show_all)
  *
  * Right now this is only used to preprocess and substitute
  * OPTION_ALIAS.
+ *
+ * The returned options should be freed using free_preprocessed_options.
  */
 static struct option *preprocess_options(struct parse_opt_ctx_t *ctx,
 					 const struct option *options)

@@ -693,6 +695,21 @@ static struct option *preprocess_options(struct parse_opt_ctx_t *ctx,
 	return newopt;
 }
 
+static void free_preprocessed_options(const struct option ** preprocessed_options, const struct option *original_options)
+{
+	int i;
+
+	if (!*preprocessed_options) {
+		return;
+	}
+	for (i = 0; original_options[i].type != OPTION_END; i++) {
+		if (original_options[i].type == OPTION_ALIAS) {
+			free((void *)(*preprocessed_options)[i].help);
+		}
+	}
+	free((void *)*preprocessed_options);
+}
+
 static int usage_with_options_internal(struct parse_opt_ctx_t *,
 				       const char * const *,
 				       const struct option *, int, int);

@@ -838,15 +855,17 @@ int parse_options(int argc, const char **argv, const char *prefix,
 		  int flags)
 {
 	struct parse_opt_ctx_t ctx;
-	struct option *real_options;
+	const struct option *preprocessed_options, *original_options = NULL;
 
 	disallow_abbreviated_options =
 		git_env_bool("GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS", 0);
 
 	memset(&ctx, 0, sizeof(ctx));
-	real_options = preprocess_options(&ctx, options);
-	if (real_options)
-		options = real_options;
+	preprocessed_options = preprocess_options(&ctx, options);
+	if (preprocessed_options) {
+		original_options = options;
+		options = preprocessed_options;
+	}
 	parse_options_start_1(&ctx, argc, argv, prefix, options, flags);
 	switch (parse_options_step(&ctx, options, usagestr)) {
 	case PARSE_OPT_HELP:

@@ -870,7 +889,7 @@ int parse_options(int argc, const char **argv, const char *prefix,
 	}
 
 	precompose_argv_prefix(argc, argv, NULL);
-	free(real_options);
+	free_preprocessed_options(&preprocessed_options, original_options);
 	free(ctx.alias_groups);
 	return parse_options_end(&ctx);
 }

Signature · 1 line

gitgitgadget

Jeff King

2021-03-08 19:46 UTC #17 lore raw

Re: [PATCH 7/7] parse-options: don't leak alias help messages

Quoted 3 lines · Andrzej Hunt via GitGitGadget

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.

Quoted 5 lines

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.

Quoted 3 lines

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.

Quoted 1 line

+static void free_preprocessed_options(const struct option ** preprocessed_options, const struct option *original_options)

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)

Review

Quoted 6 lines

(quoted diff) +6 -0

+{
+	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.

Review

Quoted 5 lines

(quoted diff) +5 -0

+	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.

Review

Quoted 1 line

(quoted diff) +1 -0

+	free((void *)*preprocessed_options);

With the interface suggestions above, this becomes just:

  free(preprocessed_options);

Quoted 9 lines

(quoted diff) +1 -1

@@ -838,15 +855,17 @@ int parse_options(int argc, const char **argv, const char *prefix,
 		  int flags)
 {
 	struct parse_opt_ctx_t ctx;
-	struct option *real_options;
+	const struct option *preprocessed_options, *original_options = NULL;
 
 	disallow_abbreviated_options =
 		git_env_bool("GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS", 0);

Review int parse_options(int argc, const char **argv, const char *prefix, · line 868

Quoted 10 lines

(quoted diff) +5 -3

 
 	memset(&ctx, 0, sizeof(ctx));
-	real_options = preprocess_options(&ctx, options);
-	if (real_options)
-		options = real_options;
+	preprocessed_options = preprocess_options(&ctx, options);
+	if (preprocessed_options) {
+		original_options = options;
+		options = 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:

parse-options.c +1 -0

index fbea16eaf5..43431b96b1 100644
@@ -678,6 +678,7 @@ static struct option *preprocess_options(struct parse_opt_ctx_t *ctx,
 			newopt[i].short_name = short_name;
 			newopt[i].long_name = long_name;
 			newopt[i].help = strbuf_detach(&help, NULL);
+			newopt[i].flags |= PARSE_OPT_FROM_ALIAS;
 			break;
 		}

parse-options.h +2 -1

index ff6506a504..32b0b49a2d 100644
@@ -47,7 +47,8 @@ enum parse_opt_option_flags {
 	PARSE_OPT_SHELL_EVAL = 256,
 	PARSE_OPT_NOCOMPLETE = 512,
 	PARSE_OPT_COMP_ARG = 1024,
-	PARSE_OPT_CMDMODE = 2048
+	PARSE_OPT_CMDMODE = 2048,
+	PARSE_OPT_FROM_ALIAS = 4096,
 };
 
 enum parse_opt_result {

(as an aside, these manual bitfield values are tedious; I wouldn't be sad to see them converted to "1 << 0", "1 << 1", and so on).

-Peff

Andrzej Hunt

2021-03-14 17:04 UTC #18 lore raw

Re: [PATCH 7/7] parse-options: don't leak alias help messages

Quoted 8 lines · Jeff King

Quoted 1 line · Andrzej Hunt via GitGitGadget
+static void free_preprocessed_options(const struct option ** preprocessed_options, const struct option *original_options)
A few style nits:
- omit the space between "**" and preprocessed_options

Quoted 8 lines

- 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 10 lines

Review
Quoted 6 lines
(quoted diff) +5 -0
+	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.

Quoted 7 lines

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 17 lines

Review

Quoted 2 lines

(quoted diff) +1 -0

+	free((void *)*preprocessed_options);

With the interface suggestions above, this becomes just:

   free(preprocessed_options);

Quoted 10 lines

(quoted diff) +1 -1

@@ -838,15 +855,17 @@ int parse_options(int argc, const char **argv, const char *prefix,
  		  int flags)
  {
  	struct parse_opt_ctx_t ctx;
-	struct option *real_options;
+	const struct option *preprocessed_options, *original_options = NULL;
 
  	disallow_abbreviated_options =
  		git_env_bool("GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS", 0);

Quoted 13 lines

Review int parse_options(int argc, const char **argv, const char *prefix, · line 868
Quoted 9 lines
(quoted diff) +5 -3
  	memset(&ctx, 0, sizeof(ctx));
-	real_options = preprocess_options(&ctx, options);
-	if (real_options)
-		options = real_options;
+	preprocessed_options = preprocess_options(&ctx, options);
+	if (preprocessed_options) {
+		original_options = options;
+		options = 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 30 lines

To spell it out, I mean something like on the writing side:

parse-options.c +1 -0

index fbea16eaf5..43431b96b1 100644
@@ -678,6 +678,7 @@ static struct option *preprocess_options(struct parse_opt_ctx_t *ctx,
  			newopt[i].short_name = short_name;
  			newopt[i].long_name = long_name;
  			newopt[i].help = strbuf_detach(&help, NULL);
+			newopt[i].flags |= PARSE_OPT_FROM_ALIAS;
  			break;
  		}

parse-options.h +2 -1

index ff6506a504..32b0b49a2d 100644
@@ -47,7 +47,8 @@ enum parse_opt_option_flags {
  	PARSE_OPT_SHELL_EVAL = 256,
  	PARSE_OPT_NOCOMPLETE = 512,
  	PARSE_OPT_COMP_ARG = 1024,
-	PARSE_OPT_CMDMODE = 2048
+	PARSE_OPT_CMDMODE = 2048,
+	PARSE_OPT_FROM_ALIAS = 4096,
  };
 
  enum parse_opt_result {

Quoted 2 lines

(as an aside, these manual bitfield values are tedious; I wouldn't be sad to see them converted to "1 << 0", "1 << 1", and so on).

I've added a separate patch to take care of the bitfield improvements - and have incorporated the PARSE_OPT_FROM_ALIAS change into the original patch!

Jeff King

2021-03-08 18:56 UTC #19 lore raw

In reply to Andrzej Hunt via GitGitGadget

Quoted 9 lines · Andrzej Hunt via GitGitGadget

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/[email protected]/

(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.

Quoted 8 lines

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).

Quoted 6 lines

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. :)

Quoted 4 lines

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.

Quoted 3 lines

(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.

Quoted 8 lines

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

Junio C Hamano

2021-03-12 23:43 UTC #20 lore raw

Quoted 11 lines · Jeff King

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/[email protected]/
(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. ;)
...
I haven't looked at the individual patches yet. I'll respond to them individually.

Thanks for working on this, and reviewing the result.

I agree with Peff's review comments and will look forward to seeing a new iteration that updates the issues pointed out, which range from coding styles to constness---IIUC, none of them were rocket science to correct.

Andrzej Hunt

2021-03-14 16:56 UTC #21 lore raw

In reply to Jeff King

Quoted 5 lines · Jeff King

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/[email protected]/

Quoted 7 lines

(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 9 lines

Quoted 8 lines
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

Quoted 3 lines

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 7 lines

Quoted 6 lines
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):

Quoted 7 lines

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 5 lines

Quoted 4 lines
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

Quoted 7 lines

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.

Quoted 4 lines

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 :).

Andrzej Hunt

2021-03-15 16:24 UTC #22 lore raw

Quoted 2 lines · Andrzej Hunt

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 ...

Quoted 2 lines

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).

Junio C Hamano

2021-03-08 18:59 UTC #23 lore raw

In reply to Andrzej Hunt via GitGitGadget

Quoted 3 lines · Andrzej Hunt via GitGitGadget

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).

Even if there is no leaks that matter exist now, to help maintain that state, suppressing false positives would be useful, provided if such checkers are run sufficiently often.

Andrzej Hunt

2021-03-14 16:56 UTC #24 lore raw

Quoted 9 lines · Junio C Hamano

Quoted 3 lines · Andrzej Hunt via GitGitGadget
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).
Even if there is no leaks that matter exist now, to help maintain that state, suppressing false positives would be useful, provided if such checkers are run sufficiently often.

Expanding on my thoughts in response to Peff's comments regarding running leak checking under ASAN: I'm wondering whether it would be acceptable to piggyback leak-checking on top of the existing ASAN test runs:

- It looks like people are already running tests with ASAN from time to time - enabling leak checking there would add leak-checking coverage without having to run LSAN separately (this would have to be restricted to those tests that are leak-free - more on that below).
- I also saw some discussion around enabling ASAN in CI, although I don't think that went anywhere yet [1]. I'd be interested in trying to pick that up - ASAN seems quite valuable by itself, and running it in CI would be a simple way to also get leak-checking in CI. (Again, this would have to be limited to known leak-free tests.)

(I'm planning to run some benchmarks to see how much enabling leak-checking with ASAN actually costs - I'm assuming it's fairly cheap if you're already running ASAN, but I'd like to verify that.)

As to the mechanics of enabling leak-checking with ASAN: test-lib.sh currently completely disables ASAN's leak-checking. Instead we could add a simple allowlist, to enable leak-checking for those tests that don't - Fix LSAN crashleak - this needn't be much more complex than:

$TEST_NUMBER <= highest_leak_free_test

I.e. something like this:

t/test-lib.sh +7 -1

index d3f6af6a65..cf9f1ad827 100644
@@ -41,7 +41,13 @@ GIT_BUILD_DIR="$TEST_DIRECTORY"/..
 # the noise level. This needs to happen at the start of the script,
 # before we even do our "did we build git yet" check (since we don't
 # want that one to complain to stderr).
-: ${ASAN_OPTIONS=detect_leaks=0:abort_on_error=1}
+if test $TEST_NUMBER -le 10
+then
+    DETECT_LEAKS=1
+else
+    DETECT_LEAKS=0
+fi
+: ${ASAN_OPTIONS=detect_leaks=$DETECT_LEAKS:abort_on_error=1}
 export ASAN_OPTIONS
 
 # If LSAN is in effect we _do_ want leak checking, but we still

(This would also require moving TEST_NUMBER a bit earlier in
test-lib.sh.)

[1] https://public-inbox.org/git/[email protected]/

Andrzej Hunt via GitGitGadget

author

2021-03-14 18:48 UTC #25 lore raw

v2 cover Fix all leaks in t0001

In reply to Andrzej Hunt via GitGitGadget

V2 of this series:

* Addresses all of Peff's comments - many thanks for the review.
* Adds a patch converting enum bitfields in parse-options.h to use binary shifts instead of numerical constants.
* Adds a patch to free remote_refs in transport_disconnect() instead of freeing those refs in the callers of transport_get_remote_refs().

Andrzej Hunt (9):
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: convert bitfield values to use binary shift
parse-options: don't leak alias help messages
transport: also free remote_refs in transport_disconnect()

 builtin/clone.c        | 14 ++++++++++----
 builtin/init-db.c      | 32 ++++++++++----------------------
 builtin/ls-remote.c    |  4 ++--
 builtin/remote.c       |  8 ++++----
 builtin/reset.c        |  2 +-
 builtin/symbolic-ref.c |  4 +++-
 builtin/worktree.c     | 10 ++++++----
 parse-options.c        | 20 +++++++++++++++++++-
 parse-options.h        | 35 ++++++++++++++++++-----------------
 transport.c            |  2 ++
 10 files changed, 75 insertions(+), 56 deletions(-)

base-commit: 13d7ab6b5d7929825b626f050b62a11241ea4945
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-899%2Fahunt%2Fleaksan-t0001-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-899/ahunt/leaksan-t0001-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/899

Range-diff vs v1:

1: ff0f7c167fa5 ! 1: c7bb403ae381 symbolic-ref: don't leak shortened refname in check_symref()

     @@ Metadata
       ## Commit message ##
          symbolic-ref: don't leak shortened refname in check_symref()
      
     +    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)
      
     @@ builtin/symbolic-ref.c: static int check_symref(const char *HEAD, int quiet, int
       			return 1;
       	}
       	if (print) {
     --		if (shorten)
     ++		char *to_free = NULL;
     + 		if (shorten)
      -			refname = shorten_unambiguous_ref(refname, 0);
     --		puts(refname);
     -+		if (shorten) {
     -+			const char *shortened_refname;
     -+
     -+			shortened_refname = shorten_unambiguous_ref(refname, 0);
     -+			puts(shortened_refname);
     -+			free((void *)shortened_refname);
     -+		} else {
     -+			puts(refname);
     -+		}
     ++			refname = to_free = shorten_unambiguous_ref(refname, 0);
     + 		puts(refname);
     ++		free(to_free);
       	}
       	return 0;
       }

2: a7b6b873460f ! 2: da05fc72b77a reset: free instead of leaking unneeded ref
@@ Metadata
## Commit message ##
reset: free instead of leaking unneeded ref

- dwim_ref() allocs a new string into ref. Instead of setting to NULL to discard
- it, we can FREE_AND_NULL.
+ 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)
3: 107e98d00e16 ! 3: a74bbcae7363 clone: free or UNLEAK further pointers when finished

     @@ Commit message
              #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
     +        #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
     @@ Commit message
      
       ## builtin/clone.c ##
      @@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)
     - 	char *path, *dir, *display_repo = NULL;
     + {
     + 	int is_bundle = 0, is_local;
     + 	const char *repo_name, *repo, *work_tree, *git_dir;
     +-	char *path, *dir, *display_repo = NULL;
     ++	char *path = NULL, *dir, *display_repo = NULL;
       	int dest_exists, real_dest_exists = 0;
       	const struct ref *refs, *remote_head;
      -	const struct ref *remote_head_points_at;
     -+	const struct ref *remote_head_points_at = NULL;
     ++	struct ref *remote_head_points_at = NULL;
       	const struct ref *our_head_points_at;
       	struct ref *mapped_refs;
       	const struct ref *ref;
     @@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)
       	path = get_repo_path(repo_name, &is_bundle);
      -	if (path)
      +	if (path) {
     -+		free(path);
     ++		FREE_AND_NULL(path);
       		repo = absolute_pathdup(repo_name);
      -	else if (strchr(repo_name, ':')) {
      +	} else if (strchr(repo_name, ':')) {
     @@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)
       	strbuf_release(&branch_top);
       	strbuf_release(&key);
      +	free_refs(mapped_refs);
     -+	free_refs((void *)remote_head_points_at);
     -+	free_refs((void *)refs);
     ++	free_refs(remote_head_points_at);
      +	free(dir);
      +	free(path);
      +	UNLEAK(repo);
  4:  d46a4e701620 !  4:  a10ab9e68809 worktree: fix leak in dwim_branch()
     @@ Commit message
      
       ## builtin/worktree.c ##
      @@ builtin/worktree.c: static void print_preparing_worktree_line(int detach,
     - 
       static const char *dwim_branch(const char *path, const char **new_branch)
       {
     --	int n;
     -+	int n, branch_exists;
     + 	int n;
     ++	int branch_exists;
       	const char *s = worktree_basename(path, &n);
       	const char *branchname = xstrndup(s, n);
       	struct strbuf ref = STRBUF_INIT;
     --
     + 
       	UNLEAK(branchname);
      -	if (!strbuf_check_branch_ref(&ref, branchname) &&
      -	    ref_exists(ref.buf)) {
      -		strbuf_release(&ref);
      +
     -+	branch_exists = (!strbuf_check_branch_ref(&ref, branchname) &&
     -+			 ref_exists(ref.buf));
     ++	branch_exists = !strbuf_check_branch_ref(&ref, branchname) &&
     ++	                ref_exists(ref.buf);
      +	strbuf_release(&ref);
      +	if (branch_exists)
       		return branchname;
  5:  d30365d96765 =  5:  206a82200ca1 init: remove git_init_db_config() while fixing leaks
  6:  6f81f3b2ab28 =  6:  aa345e50782f init-db: silence template_dir leak when converting to absolute path
  -:  ------------ >  7:  2b03785bd4cb parse-options: convert bitfield values to use binary shift
  7:  fb456bee0f69 !  8:  4397c1fd8020 parse-options: don't leak alias help messages
     @@ Commit message
            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.
     +    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
     @@ Commit message
      
          Signed-off-by: Andrzej Hunt <[email protected]>
      
     +    fold
     +
       ## parse-options.c ##
      @@ parse-options.c: static int show_gitcomp(const struct option *opts, int show_all)
        *
     @@ parse-options.c: static int show_gitcomp(const struct option *opts, int show_all
        */
       static struct option *preprocess_options(struct parse_opt_ctx_t *ctx,
       					 const struct option *options)
     +@@ parse-options.c: static struct option *preprocess_options(struct parse_opt_ctx_t *ctx,
     + 			newopt[i].short_name = short_name;
     + 			newopt[i].long_name = long_name;
     + 			newopt[i].help = strbuf_detach(&help, NULL);
     ++			newopt[i].flags |= PARSE_OPT_FROM_ALIAS;
     + 			break;
     + 		}
     + 
      @@ parse-options.c: static struct option *preprocess_options(struct parse_opt_ctx_t *ctx,
       	return newopt;
       }
       
     -+static void free_preprocessed_options(const struct option ** preprocessed_options, const struct option *original_options)
     ++static void free_preprocessed_options(struct option *options)
      +{
      +	int i;
      +
     -+	if (!*preprocessed_options) {
     ++	if (!options)
      +		return;
     -+	}
     -+	for (i = 0; original_options[i].type != OPTION_END; i++) {
     -+		if (original_options[i].type == OPTION_ALIAS) {
     -+			free((void *)(*preprocessed_options)[i].help);
     ++
     ++	for (i = 0; options[i].type != OPTION_END; i++) {
     ++		if (options[i].flags & PARSE_OPT_FROM_ALIAS) {
     ++			free((void *)options[i].help);
      +		}
      +	}
     -+	free((void *)*preprocessed_options);
     ++	free(options);
      +}
      +
       static int usage_with_options_internal(struct parse_opt_ctx_t *,
       				       const char * const *,
       				       const struct option *, int, int);
     -@@ parse-options.c: int parse_options(int argc, const char **argv, const char *prefix,
     - 		  int flags)
     - {
     - 	struct parse_opt_ctx_t ctx;
     --	struct option *real_options;
     -+	const struct option *preprocessed_options, *original_options = NULL;
     - 
     - 	disallow_abbreviated_options =
     - 		git_env_bool("GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS", 0);
     - 
     - 	memset(&ctx, 0, sizeof(ctx));
     --	real_options = preprocess_options(&ctx, options);
     --	if (real_options)
     --		options = real_options;
     -+	preprocessed_options = preprocess_options(&ctx, options);
     -+	if (preprocessed_options) {
     -+		original_options = options;
     -+		options = preprocessed_options;
     -+	}
     - 	parse_options_start_1(&ctx, argc, argv, prefix, options, flags);
     - 	switch (parse_options_step(&ctx, options, usagestr)) {
     - 	case PARSE_OPT_HELP:
      @@ parse-options.c: int parse_options(int argc, const char **argv, const char *prefix,
       	}
       
       	precompose_argv_prefix(argc, argv, NULL);
      -	free(real_options);
     -+	free_preprocessed_options(&preprocessed_options, original_options);
     ++	free_preprocessed_options(real_options);
       	free(ctx.alias_groups);
       	return parse_options_end(&ctx);
       }
     +
     + ## parse-options.h ##
     +@@ parse-options.h: enum parse_opt_option_flags {
     + 	PARSE_OPT_NOCOMPLETE = 1 << 8,
     + 	PARSE_OPT_COMP_ARG = 1 << 9,
     + 	PARSE_OPT_CMDMODE = 1 << 10,
     ++	PARSE_OPT_FROM_ALIAS = 1 << 11,
     + };
     + 
     + enum parse_opt_result {

-: ------------ > 9: a907f2460d42 transport: also free remote_refs in transport_disconnect()

Signature · 1 line

gitgitgadget

Andrzej Hunt via GitGitGadget

author

2021-03-14 18:48 UTC #26 lore raw

PATCH v2 1/9 symbolic-ref: don't leak shortened refname in check_symref()

From: Andrzej Hunt <[email protected]>

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 <[email protected]>

---
 builtin/symbolic-ref.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

builtin/symbolic-ref.c +3 -1

index 80237f0df10f..e547a08d6c7c 100644
@@ -24,9 +24,11 @@ static int check_symref(const char *HEAD, int quiet, int shorten, int print)
 			return 1;
 	}
 	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);
 	}
 	return 0;
 }

Signature · 1 line

gitgitgadget

Andrzej Hunt via GitGitGadget

author

2021-03-14 18:48 UTC #27 lore raw

PATCH v2 2/9 reset: free instead of leaking unneeded ref

In reply to Andrzej Hunt via GitGitGadget

From: Andrzej Hunt <[email protected]>

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 <[email protected]>

---
 builtin/reset.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

builtin/reset.c +1 -1

index c635b062c3a7..43e855cb8876 100644
@@ -425,7 +425,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 
 			dwim_ref(rev, strlen(rev), &dummy, &ref, 0);
 			if (ref && !starts_with(ref, "refs/"))
-				ref = NULL;
+				FREE_AND_NULL(ref);
 
 			err = reset_index(ref, &oid, reset_type, quiet);
 			if (reset_type == KEEP && !err)

Signature · 1 line

gitgitgadget

Andrzej Hunt via GitGitGadget

author

2021-03-14 18:48 UTC #28 lore raw

PATCH v2 3/9 clone: free or UNLEAK further pointers when finished

In reply to Andrzej Hunt via GitGitGadget

From: Andrzej Hunt <[email protected]>

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 <[email protected]>

---
 builtin/clone.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

builtin/clone.c +10 -4

index 51e844a2de0a..952fe3d8fc88 100644
@@ -964,10 +964,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 {
 	int is_bundle = 0, is_local;
 	const char *repo_name, *repo, *work_tree, *git_dir;
-	char *path, *dir, *display_repo = NULL;
+	char *path = NULL, *dir, *display_repo = NULL;
 	int dest_exists, real_dest_exists = 0;
 	const struct ref *refs, *remote_head;
-	const struct ref *remote_head_points_at;
+	struct ref *remote_head_points_at = NULL;
 	const struct ref *our_head_points_at;
 	struct ref *mapped_refs;
 	const struct ref *ref;

@@ -1017,9 +1017,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	repo_name = argv[0];
 
 	path = get_repo_path(repo_name, &is_bundle);
-	if (path)
+	if (path) {
+		FREE_AND_NULL(path);
 		repo = absolute_pathdup(repo_name);
-	else if (strchr(repo_name, ':')) {
+	} else if (strchr(repo_name, ':')) {
 		repo = repo_name;
 		display_repo = transport_anonymize_url(repo);
 	} else

@@ -1393,6 +1394,11 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	strbuf_release(&reflog_msg);
 	strbuf_release(&branch_top);
 	strbuf_release(&key);
+	free_refs(mapped_refs);
+	free_refs(remote_head_points_at);
+	free(dir);
+	free(path);
+	UNLEAK(repo);
 	junk_mode = JUNK_LEAVE_ALL;
 
 	strvec_clear(&transport_ls_refs_options.ref_prefixes);

Signature · 1 line

gitgitgadget

Andrzej Hunt via GitGitGadget

author

2021-03-14 18:48 UTC #29 lore raw

PATCH v2 4/9 worktree: fix leak in dwim_branch()

In reply to Andrzej Hunt via GitGitGadget

From: Andrzej Hunt <[email protected]>

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)

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 <[email protected]>

---
 builtin/worktree.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

builtin/worktree.c +6 -4

index 1cd5c2016e3f..b0563aef685f 100644
@@ -446,16 +446,18 @@ static void print_preparing_worktree_line(int detach,
 static const char *dwim_branch(const char *path, const char **new_branch)
 {
 	int n;
+	int branch_exists;
 	const char *s = worktree_basename(path, &n);
 	const char *branchname = xstrndup(s, n);
 	struct strbuf ref = STRBUF_INIT;
 
 	UNLEAK(branchname);
-	if (!strbuf_check_branch_ref(&ref, branchname) &&
-	    ref_exists(ref.buf)) {
-		strbuf_release(&ref);
+
+	branch_exists = !strbuf_check_branch_ref(&ref, branchname) &&
+	                ref_exists(ref.buf);
+	strbuf_release(&ref);
+	if (branch_exists)
 		return branchname;
-	}
 
 	*new_branch = branchname;
 	if (guess_remote) {

Signature · 1 line

gitgitgadget

Andrzej Hunt via GitGitGadget

author

2021-03-14 18:48 UTC #30 lore raw

PATCH v2 5/9 init: remove git_init_db_config() while fixing leaks

In reply to Andrzej Hunt via GitGitGadget

From: Andrzej Hunt <[email protected]>

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 <[email protected]>

---
 builtin/init-db.c | 28 +++++++---------------------
 1 file changed, 7 insertions(+), 21 deletions(-)

builtin/init-db.c +7 ~~-21~~

index dcc45bef5148..d31dbc883746 100644
@@ -25,7 +25,6 @@
 
 static int init_is_bare_repository = 0;
 static int init_shared_repository = -1;
-static const char *init_db_template_dir;
 
 static void copy_templates_1(struct strbuf *path, struct strbuf *template_path,
 			     DIR *dir)

@@ -94,7 +93,7 @@ static void copy_templates_1(struct strbuf *path, struct strbuf *template_path,
 	}
 }
 
-static void copy_templates(const char *template_dir)
+static void copy_templates(const char *template_dir, const char *init_template_dir)
 {
 	struct strbuf path = STRBUF_INIT;
 	struct strbuf template_path = STRBUF_INIT;

@@ -107,7 +106,7 @@ static void copy_templates(const char *template_dir)
 	if (!template_dir)
 		template_dir = getenv(TEMPLATE_DIR_ENVIRONMENT);
 	if (!template_dir)
-		template_dir = init_db_template_dir;
+		template_dir = init_template_dir;
 	if (!template_dir)
 		template_dir = to_free = system_path(DEFAULT_GIT_TEMPLATE_DIR);
 	if (!template_dir[0]) {

@@ -154,17 +153,6 @@ static void copy_templates(const char *template_dir)
 	clear_repository_format(&template_format);
 }
 
-static int git_init_db_config(const char *k, const char *v, void *cb)
-{
-	if (!strcmp(k, "init.templatedir"))
-		return git_config_pathname(&init_db_template_dir, k, v);
-
-	if (starts_with(k, "core."))
-		return platform_core_config(k, v, cb);
-
-	return 0;
-}
-
 /*
  * If the git_dir is not directly inside the working tree, then git will not
  * find it by default, and we need to set the worktree explicitly.

@@ -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);
 
 	safe_create_dir(git_dir, 0);

Signature · 1 line

gitgitgadget

Andrzej Hunt via GitGitGadget

author

2021-03-14 18:48 UTC #31 lore raw

PATCH v2 6/9 init-db: silence template_dir leak when converting to absolute path

In reply to Andrzej Hunt via GitGitGadget

From: Andrzej Hunt <[email protected]>

Signed-off-by Andrzej Hunt <[email protected]>

---
 builtin/init-db.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

builtin/init-db.c +3 -1

index d31dbc883746..efc66523e22c 100644
@@ -561,8 +561,10 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)
 	if (real_git_dir && !is_absolute_path(real_git_dir))
 		real_git_dir = real_pathdup(real_git_dir, 1);
 
-	if (template_dir && *template_dir && !is_absolute_path(template_dir))
+	if (template_dir && *template_dir && !is_absolute_path(template_dir)) {
 		template_dir = absolute_pathdup(template_dir);
+		UNLEAK(template_dir);
+	}
 
 	if (argc == 1) {
 		int mkdir_tried = 0;

Signature · 1 line

gitgitgadget

Andrzej Hunt via GitGitGadget

author

2021-03-14 18:48 UTC #32 lore raw

PATCH v2 7/9 parse-options: convert bitfield values to use binary shift

In reply to Andrzej Hunt via GitGitGadget

From: Andrzej Hunt <[email protected]>

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%[email protected]/

Signed-off-by Andrzej Hunt <[email protected]>

---
 parse-options.h | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

parse-options.h +17 ~~-17~~

index ff6506a50470..36ce0a44b2e9 100644
@@ -28,26 +28,26 @@ enum parse_opt_type {
 };
 
 enum parse_opt_flags {
-	PARSE_OPT_KEEP_DASHDASH = 1,
-	PARSE_OPT_STOP_AT_NON_OPTION = 2,
-	PARSE_OPT_KEEP_ARGV0 = 4,
-	PARSE_OPT_KEEP_UNKNOWN = 8,
-	PARSE_OPT_NO_INTERNAL_HELP = 16,
-	PARSE_OPT_ONE_SHOT = 32
+	PARSE_OPT_KEEP_DASHDASH = 1 << 0,
+	PARSE_OPT_STOP_AT_NON_OPTION = 1 << 1,
+	PARSE_OPT_KEEP_ARGV0 = 1 << 2,
+	PARSE_OPT_KEEP_UNKNOWN = 1 << 3,
+	PARSE_OPT_NO_INTERNAL_HELP = 1 << 4,
+	PARSE_OPT_ONE_SHOT = 1 << 5,
 };
 
 enum parse_opt_option_flags {
-	PARSE_OPT_OPTARG  = 1,
-	PARSE_OPT_NOARG   = 2,
-	PARSE_OPT_NONEG   = 4,
-	PARSE_OPT_HIDDEN  = 8,
-	PARSE_OPT_LASTARG_DEFAULT = 16,
-	PARSE_OPT_NODASH = 32,
-	PARSE_OPT_LITERAL_ARGHELP = 64,
-	PARSE_OPT_SHELL_EVAL = 256,
-	PARSE_OPT_NOCOMPLETE = 512,
-	PARSE_OPT_COMP_ARG = 1024,
-	PARSE_OPT_CMDMODE = 2048
+	PARSE_OPT_OPTARG  = 1 << 0,
+	PARSE_OPT_NOARG   = 1 << 1,
+	PARSE_OPT_NONEG   = 1 << 2,
+	PARSE_OPT_HIDDEN  = 1 << 3,
+	PARSE_OPT_LASTARG_DEFAULT = 1 << 4,
+	PARSE_OPT_NODASH = 1 << 5,
+	PARSE_OPT_LITERAL_ARGHELP = 1 << 6,
+	PARSE_OPT_SHELL_EVAL = 1 << 7,
+	PARSE_OPT_NOCOMPLETE = 1 << 8,
+	PARSE_OPT_COMP_ARG = 1 << 9,
+	PARSE_OPT_CMDMODE = 1 << 10,
 };
 
 enum parse_opt_result {

Signature · 1 line

gitgitgadget

Martin Ågren

2021-03-14 20:26 UTC #33 lore raw

Re: [PATCH v2 7/9] parse-options: convert bitfield values to use binary shift

Quoted 6 lines · Andrzej Hunt via GitGitGadget

From: Andrzej Hunt <[email protected]>
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 2 lines

Also add a trailing comma to the last enum entry to simplify addition of new flags.

Makes sense.

Quoted 1 line

This changee was originally suggested by Peff in:

s/changee/change/

Quoted 6 lines

https://public-inbox.org/git/YEZ%[email protected]/
enum parse_opt_flags {
parse-options.h +0 -3
-       PARSE_OPT_KEEP_DASHDASH = 1,
-       PARSE_OPT_STOP_AT_NON_OPTION = 2,
-       PARSE_OPT_KEEP_ARGV0 = 4,

parse-options.h enum parse_opt_type { · line 37

Quoted 10 lines

parse-options.h +6 -3

-       PARSE_OPT_KEEP_UNKNOWN = 8,
-       PARSE_OPT_NO_INTERNAL_HELP = 16,
-       PARSE_OPT_ONE_SHOT = 32
+       PARSE_OPT_KEEP_DASHDASH = 1 << 0,
+       PARSE_OPT_STOP_AT_NON_OPTION = 1 << 1,
+       PARSE_OPT_KEEP_ARGV0 = 1 << 2,
+       PARSE_OPT_KEEP_UNKNOWN = 1 << 3,
+       PARSE_OPT_NO_INTERNAL_HELP = 1 << 4,
+       PARSE_OPT_ONE_SHOT = 1 << 5,
 };

Straightforward.

parse-options.h enum parse_opt_type { · line 46

Quoted 8 lines

enum parse_opt_option_flags {

parse-options.h +0 -7

-       PARSE_OPT_OPTARG  = 1,
-       PARSE_OPT_NOARG   = 2,
-       PARSE_OPT_NONEG   = 4,
-       PARSE_OPT_HIDDEN  = 8,
-       PARSE_OPT_LASTARG_DEFAULT = 16,
-       PARSE_OPT_NODASH = 32,
-       PARSE_OPT_LITERAL_ARGHELP = 64,

PARSE_OPT_NEGHELP is gone since acbb08c2e0b ("parse-options: remove PARSE_OPT_NEGHELP", 2012-02-28), which explains the jump here.

Quoted 6 lines

parse-options.h +2 -4

-       PARSE_OPT_SHELL_EVAL = 256,
-       PARSE_OPT_NOCOMPLETE = 512,
-       PARSE_OPT_COMP_ARG = 1024,
-       PARSE_OPT_CMDMODE = 2048
+       PARSE_OPT_OPTARG  = 1 << 0,
+       PARSE_OPT_NOARG   = 1 << 1,

parse-options.h enum parse_opt_type { · line 51

Quoted 10 lines

parse-options.h +9 -0

+       PARSE_OPT_NONEG   = 1 << 2,
+       PARSE_OPT_HIDDEN  = 1 << 3,
+       PARSE_OPT_LASTARG_DEFAULT = 1 << 4,
+       PARSE_OPT_NODASH = 1 << 5,
+       PARSE_OPT_LITERAL_ARGHELP = 1 << 6,
+       PARSE_OPT_SHELL_EVAL = 1 << 7,
+       PARSE_OPT_NOCOMPLETE = 1 << 8,
+       PARSE_OPT_COMP_ARG = 1 << 9,
+       PARSE_OPT_CMDMODE = 1 << 10,
 };

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

Junio C Hamano

2021-03-14 22:58 UTC #34 lore raw

Re: [PATCH v2 7/9] parse-options: convert bitfield values to use binary shift

Quoted 29 lines · Martin Ågren

parse-options.h enum parse_opt_type { · line 46

Quoted 8 lines

enum parse_opt_option_flags {

parse-options.h +0 -7

-       PARSE_OPT_OPTARG  = 1,
-       PARSE_OPT_NOARG   = 2,
-       PARSE_OPT_NONEG   = 4,
-       PARSE_OPT_HIDDEN  = 8,
-       PARSE_OPT_LASTARG_DEFAULT = 16,
-       PARSE_OPT_NODASH = 32,
-       PARSE_OPT_LITERAL_ARGHELP = 64,

PARSE_OPT_NEGHELP is gone since acbb08c2e0b ("parse-options: remove PARSE_OPT_NEGHELP", 2012-02-28), which explains the jump here.

Quoted 6 lines

parse-options.h +2 -4

-       PARSE_OPT_SHELL_EVAL = 256,
-       PARSE_OPT_NOCOMPLETE = 512,
-       PARSE_OPT_COMP_ARG = 1024,
-       PARSE_OPT_CMDMODE = 2048
+       PARSE_OPT_OPTARG  = 1 << 0,
+       PARSE_OPT_NOARG   = 1 << 1,

parse-options.h enum parse_opt_type { · line 51

Quoted 10 lines

parse-options.h +9 -0

+       PARSE_OPT_NONEG   = 1 << 2,
+       PARSE_OPT_HIDDEN  = 1 << 3,
+       PARSE_OPT_LASTARG_DEFAULT = 1 << 4,
+       PARSE_OPT_NODASH = 1 << 5,
+       PARSE_OPT_LITERAL_ARGHELP = 1 << 6,
+       PARSE_OPT_SHELL_EVAL = 1 << 7,
+       PARSE_OPT_NOCOMPLETE = 1 << 8,
+       PARSE_OPT_COMP_ARG = 1 << 9,
+       PARSE_OPT_CMDMODE = 1 << 10,
 };

Quoted 7 lines

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.)

I think 8 vs 1<<3 is bikeshedding, but a patch that claims to "convert bitfield flags to use binary shift" is supposed to be a no-op, so it would highly be problematic to see such a hidden renumbering happening at the same time. Even if the existing code or any code in flight only relies on the fact that these bits are distinct and non overlapping.

Thanks for a sharp set of eyes.

Andrzej Hunt

2021-03-15 16:21 UTC #35 lore raw

Re: [PATCH v2 7/9] parse-options: convert bitfield values to use binary shift

In reply to Martin Ågren

Quoted 68 lines · Martin Ågren

Quoted 6 lines · Andrzej Hunt via GitGitGadget
From: Andrzej Hunt <[email protected]>
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 2 lines
Also add a trailing comma to the last enum entry to simplify addition of new flags.
Makes sense.
Quoted 1 line
This changee was originally suggested by Peff in:
s/changee/change/
Quoted 6 lines
https://public-inbox.org/git/YEZ%[email protected]/
  enum parse_opt_flags {
parse-options.h +0 -3
-       PARSE_OPT_KEEP_DASHDASH = 1,
-       PARSE_OPT_STOP_AT_NON_OPTION = 2,
-       PARSE_OPT_KEEP_ARGV0 = 4,
parse-options.h enum parse_opt_type { · line 37
Quoted 10 lines
parse-options.h +6 -3
-       PARSE_OPT_KEEP_UNKNOWN = 8,
-       PARSE_OPT_NO_INTERNAL_HELP = 16,
-       PARSE_OPT_ONE_SHOT = 32
+       PARSE_OPT_KEEP_DASHDASH = 1 << 0,
+       PARSE_OPT_STOP_AT_NON_OPTION = 1 << 1,
+       PARSE_OPT_KEEP_ARGV0 = 1 << 2,
+       PARSE_OPT_KEEP_UNKNOWN = 1 << 3,
+       PARSE_OPT_NO_INTERNAL_HELP = 1 << 4,
+       PARSE_OPT_ONE_SHOT = 1 << 5,
  };
Straightforward.
parse-options.h enum parse_opt_type { · line 46
Quoted 8 lines
  enum parse_opt_option_flags {
parse-options.h +0 -7
-       PARSE_OPT_OPTARG  = 1,
-       PARSE_OPT_NOARG   = 2,
-       PARSE_OPT_NONEG   = 4,
-       PARSE_OPT_HIDDEN  = 8,
-       PARSE_OPT_LASTARG_DEFAULT = 16,
-       PARSE_OPT_NODASH = 32,
-       PARSE_OPT_LITERAL_ARGHELP = 64,
PARSE_OPT_NEGHELP is gone since acbb08c2e0b ("parse-options: remove PARSE_OPT_NEGHELP", 2012-02-28), which explains the jump here.
Quoted 6 lines
parse-options.h +2 -4
-       PARSE_OPT_SHELL_EVAL = 256,
-       PARSE_OPT_NOCOMPLETE = 512,
-       PARSE_OPT_COMP_ARG = 1024,
-       PARSE_OPT_CMDMODE = 2048
+       PARSE_OPT_OPTARG  = 1 << 0,
+       PARSE_OPT_NOARG   = 1 << 1,
parse-options.h enum parse_opt_type { · line 51
Quoted 10 lines
parse-options.h +9 -0
+       PARSE_OPT_NONEG   = 1 << 2,
+       PARSE_OPT_HIDDEN  = 1 << 3,
+       PARSE_OPT_LASTARG_DEFAULT = 1 << 4,
+       PARSE_OPT_NODASH = 1 << 5,
+       PARSE_OPT_LITERAL_ARGHELP = 1 << 6,
+       PARSE_OPT_SHELL_EVAL = 1 << 7,
+       PARSE_OPT_NOCOMPLETE = 1 << 8,
+       PARSE_OPT_COMP_ARG = 1 << 9,
+       PARSE_OPT_CMDMODE = 1 << 10,
  };

Quoted 7 lines

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

Andrzej Hunt via GitGitGadget

author

2021-03-14 18:48 UTC #36 lore raw

PATCH v2 8/9 parse-options: don't leak alias help messages

In reply to Andrzej Hunt via GitGitGadget

From: Andrzej Hunt <[email protected]>

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 <[email protected]>

fold

---
 parse-options.c | 20 +++++++++++++++++++-
 parse-options.h |  1 +
 2 files changed, 20 insertions(+), 1 deletion(-)

parse-options.c +19 -1

index fbea16eaf5c2..6c7f927240b6 100644
@@ -625,6 +625,8 @@ static int show_gitcomp(const struct option *opts, int show_all)
  *
  * Right now this is only used to preprocess and substitute
  * OPTION_ALIAS.
+ *
+ * The returned options should be freed using free_preprocessed_options.
  */
 static struct option *preprocess_options(struct parse_opt_ctx_t *ctx,
 					 const struct option *options)

@@ -678,6 +680,7 @@ static struct option *preprocess_options(struct parse_opt_ctx_t *ctx,
 			newopt[i].short_name = short_name;
 			newopt[i].long_name = long_name;
 			newopt[i].help = strbuf_detach(&help, NULL);
+			newopt[i].flags |= PARSE_OPT_FROM_ALIAS;
 			break;
 		}

@@ -693,6 +696,21 @@ static struct option *preprocess_options(struct parse_opt_ctx_t *ctx,
 	return newopt;
 }
 
+static void free_preprocessed_options(struct option *options)
+{
+	int i;
+
+	if (!options)
+		return;
+
+	for (i = 0; options[i].type != OPTION_END; i++) {
+		if (options[i].flags & PARSE_OPT_FROM_ALIAS) {
+			free((void *)options[i].help);
+		}
+	}
+	free(options);
+}
+
 static int usage_with_options_internal(struct parse_opt_ctx_t *,
 				       const char * const *,
 				       const struct option *, int, int);

@@ -870,7 +888,7 @@ int parse_options(int argc, const char **argv, const char *prefix,
 	}
 
 	precompose_argv_prefix(argc, argv, NULL);
-	free(real_options);
+	free_preprocessed_options(real_options);
 	free(ctx.alias_groups);
 	return parse_options_end(&ctx);
 }

parse-options.h +1 -0

index 36ce0a44b2e9..331c6c67812c 100644
@@ -48,6 +48,7 @@ enum parse_opt_option_flags {
 	PARSE_OPT_NOCOMPLETE = 1 << 8,
 	PARSE_OPT_COMP_ARG = 1 << 9,
 	PARSE_OPT_CMDMODE = 1 << 10,
+	PARSE_OPT_FROM_ALIAS = 1 << 11,
 };
 
 enum parse_opt_result {

Signature · 1 line

gitgitgadget

Eric Sunshine

2021-03-14 19:49 UTC #37 lore raw

Re: [PATCH v2 8/9] parse-options: don't leak alias help messages

Quoted 8 lines · Andrzej Hunt via GitGitGadget

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 <[email protected]>
fold
---

Unwanted "fold".

Andrzej Hunt

2021-03-15 16:21 UTC #38 lore raw

Re: [PATCH v2 8/9] parse-options: don't leak alias help messages

Quoted 8 lines · Eric Sunshine

Quoted 6 lines · Andrzej Hunt via GitGitGadget
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 <[email protected]>

Quoted 5 lines

Quoted 2 lines
fold
---
Unwanted "fold".

Thank you for catching this - I'll remove it!

Andrzej Hunt

2021-03-14 20:01 UTC #39 lore raw

Re: [PATCH v2 8/9] parse-options: don't leak alias help messages

In reply to Andrzej Hunt via GitGitGadget

parse-options.c static struct option *preprocess_options(struct parse_opt_ctx_t *ctx, · line 709

Quoted 6 lines · Andrzej Hunt via GitGitGadget

parse-options.c +4 -0

+    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.

Andrzej Hunt via GitGitGadget

author

2021-03-14 18:48 UTC #40 lore raw

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

In reply to Andrzej Hunt via GitGitGadget

From: Andrzej Hunt <[email protected]>

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 <[email protected]>

---
 builtin/ls-remote.c | 4 ++--
 builtin/remote.c    | 8 ++++----
 transport.c         | 2 ++
 3 files changed, 8 insertions(+), 6 deletions(-)

builtin/ls-remote.c +2 -2

index ef604752a044..5432d239a681 100644
@@ -124,8 +124,6 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 		int hash_algo = hash_algo_by_ptr(transport_get_hash_algo(transport));
 		repo_set_hash_algo(the_repository, hash_algo);
 	}
-	if (transport_disconnect(transport))
-		return 1;
 
 	if (!dest && !quiet)
 		fprintf(stderr, "From %s\n", *remote->url);

@@ -151,5 +149,7 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 	}
 
 	ref_array_clear(&ref_array);
+	if (transport_disconnect(transport))
+		return 1;
 	return status;
 }

builtin/remote.c +4 -4

index d11a5589e49d..e31d9c99470e 100644
@@ -938,9 +938,6 @@ static int get_remote_ref_states(const char *name,
 				 struct ref_states *states,
 				 int query)
 {
-	struct transport *transport;
-	const struct ref *remote_refs;
-
 	states->remote = remote_get(name);
 	if (!states->remote)
 		return error(_("No such remote: '%s'"), name);

@@ -948,10 +945,12 @@ static int get_remote_ref_states(const char *name,
 	read_branches();
 
 	if (query) {
+		struct transport *transport;
+		const struct ref *remote_refs;
+
 		transport = transport_get(states->remote, states->remote->url_nr > 0 ?
 			states->remote->url[0] : NULL);
 		remote_refs = transport_get_remote_refs(transport, NULL);
-		transport_disconnect(transport);
 
 		states->queried = 1;
 		if (query & GET_REF_STATES)

@@ -960,6 +959,7 @@ static int get_remote_ref_states(const char *name,
 			get_head_names(remote_refs, states);
 		if (query & GET_PUSH_REF_STATES)
 			get_push_ref_states(remote_refs, states);
+		transport_disconnect(transport);
 	} else {
 		for_each_ref(append_ref_to_tracked_list, states);
 		string_list_sort(&states->tracked);

transport.c +2 -0

index b13fab5dc3b1..62362d79dd87 100644
@@ -1452,6 +1452,8 @@ int transport_disconnect(struct transport *transport)
 	int ret = 0;
 	if (transport->vtable->disconnect)
 		ret = transport->vtable->disconnect(transport);
+	if (transport->got_remote_refs)
+		free_refs((void *)transport->remote_refs);
 	free(transport);
 	return ret;
 }

Signature · 1 line

gitgitgadget

Andrzej Hunt via GitGitGadget

author

2021-03-21 16:59 UTC #41 lore raw

v3 cover Fix all leaks in t0001

In reply to Andrzej Hunt via GitGitGadget

V3:

* Ensures we don't silently change enum values in "parse-options: convert bitfield values to use binary shift". Instead we retain the original values in that patch, and reuse the unused value in a later patch.
* Fixes some silly commit message typos.

Thanks as always for the sharp-eyed reviews.

ATB, Andrzej

 builtin/clone.c        | 14 ++++++++++----
 builtin/init-db.c      | 32 ++++++++++----------------------
 builtin/ls-remote.c    |  4 ++--
 builtin/remote.c       |  8 ++++----
 builtin/reset.c        |  2 +-
 builtin/symbolic-ref.c |  4 +++-
 builtin/worktree.c     | 10 ++++++----
 parse-options.c        | 19 ++++++++++++++++++-
 parse-options.h        | 35 ++++++++++++++++++-----------------
 transport.c            |  2 ++
 10 files changed, 74 insertions(+), 56 deletions(-)

base-commit: 98164e9585e02e31dcf1377a553efe076c15f8c6
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-899%2Fahunt%2Fleaksan-t0001-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-899/ahunt/leaksan-t0001-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/899

Range-diff vs v2:

  1:  c7bb403ae381 =  1:  6af157dfed79 symbolic-ref: don't leak shortened refname in check_symref()
  2:  da05fc72b77a =  2:  add6931b2138 reset: free instead of leaking unneeded ref
  3:  a74bbcae7363 =  3:  40c5c915fc1e clone: free or UNLEAK further pointers when finished
  4:  a10ab9e68809 =  4:  963f291d5344 worktree: fix leak in dwim_branch()
  5:  206a82200ca1 =  5:  b615fda790f0 init: remove git_init_db_config() while fixing leaks
  6:  aa345e50782f =  6:  953cc8f29885 init-db: silence template_dir leak when converting to absolute path
  7:  2b03785bd4cb !  7:  c2220434ab2c parse-options: convert bitfield values to use binary shift
     @@ Commit message
          Also add a trailing comma to the last enum entry to simplify addition of
          new flags.
      
     -    This changee was originally suggested by Peff in:
     +    This change was originally suggested by Peff in:
          https://public-inbox.org/git/YEZ%[email protected]/
      
          Signed-off-by: Andrzej Hunt <[email protected]>
     @@ parse-options.h: enum parse_opt_type {
      +	PARSE_OPT_LASTARG_DEFAULT = 1 << 4,
      +	PARSE_OPT_NODASH = 1 << 5,
      +	PARSE_OPT_LITERAL_ARGHELP = 1 << 6,
     -+	PARSE_OPT_SHELL_EVAL = 1 << 7,
     -+	PARSE_OPT_NOCOMPLETE = 1 << 8,
     -+	PARSE_OPT_COMP_ARG = 1 << 9,
     -+	PARSE_OPT_CMDMODE = 1 << 10,
     ++	PARSE_OPT_SHELL_EVAL = 1 << 8,
     ++	PARSE_OPT_NOCOMPLETE = 1 << 9,
     ++	PARSE_OPT_COMP_ARG = 1 << 10,
     ++	PARSE_OPT_CMDMODE = 1 << 11,
       };
       
       enum parse_opt_result {
  8:  4397c1fd8020 !  8:  6e46cd332023 parse-options: don't leak alias help messages
     @@ Commit message
      
          Signed-off-by: Andrzej Hunt <[email protected]>
      
     -    fold
     -
       ## parse-options.c ##
      @@ parse-options.c: static int show_gitcomp(const struct option *opts, int show_all)
        *
     @@ parse-options.c: static struct option *preprocess_options(struct parse_opt_ctx_t
      +		return;
      +
      +	for (i = 0; options[i].type != OPTION_END; i++) {
     -+		if (options[i].flags & PARSE_OPT_FROM_ALIAS) {
     ++		if (options[i].flags & PARSE_OPT_FROM_ALIAS)
      +			free((void *)options[i].help);
     -+		}
      +	}
      +	free(options);
      +}
     @@ parse-options.c: int parse_options(int argc, const char **argv, const char *pref
      
       ## parse-options.h ##
      @@ parse-options.h: enum parse_opt_option_flags {
     - 	PARSE_OPT_NOCOMPLETE = 1 << 8,
     - 	PARSE_OPT_COMP_ARG = 1 << 9,
     - 	PARSE_OPT_CMDMODE = 1 << 10,
     -+	PARSE_OPT_FROM_ALIAS = 1 << 11,
     - };
     - 
     - enum parse_opt_result {
     + 	PARSE_OPT_LASTARG_DEFAULT = 1 << 4,
     + 	PARSE_OPT_NODASH = 1 << 5,
     + 	PARSE_OPT_LITERAL_ARGHELP = 1 << 6,
     ++	PARSE_OPT_FROM_ALIAS = 1 << 7,
     + 	PARSE_OPT_SHELL_EVAL = 1 << 8,
     + 	PARSE_OPT_NOCOMPLETE = 1 << 9,
     + 	PARSE_OPT_COMP_ARG = 1 << 10,

9: a907f2460d42 = 9: 50a2b9693aa3 transport: also free remote_refs in transport_disconnect()

Signature · 1 line

gitgitgadget

Andrzej Hunt via GitGitGadget

author

2021-03-21 16:59 UTC #42 lore raw

PATCH v3 1/9 symbolic-ref: don't leak shortened refname in check_symref()

From: Andrzej Hunt <[email protected]>

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 <[email protected]>

---
 builtin/symbolic-ref.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

builtin/symbolic-ref.c +3 -1

index 80237f0df10f..e547a08d6c7c 100644
@@ -24,9 +24,11 @@ static int check_symref(const char *HEAD, int quiet, int shorten, int print)
 			return 1;
 	}
 	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);
 	}
 	return 0;
 }

Signature · 1 line

gitgitgadget

Andrzej Hunt via GitGitGadget

author

2021-03-21 16:59 UTC #43 lore raw

PATCH v3 2/9 reset: free instead of leaking unneeded ref

In reply to Andrzej Hunt via GitGitGadget

From: Andrzej Hunt <[email protected]>

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 <[email protected]>

---
 builtin/reset.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

builtin/reset.c +1 -1

index c635b062c3a7..43e855cb8876 100644
@@ -425,7 +425,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 
 			dwim_ref(rev, strlen(rev), &dummy, &ref, 0);
 			if (ref && !starts_with(ref, "refs/"))
-				ref = NULL;
+				FREE_AND_NULL(ref);
 
 			err = reset_index(ref, &oid, reset_type, quiet);
 			if (reset_type == KEEP && !err)

Signature · 1 line

gitgitgadget

Andrzej Hunt via GitGitGadget

author

2021-03-21 16:59 UTC #44 lore raw

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

In reply to Andrzej Hunt via GitGitGadget

From: Andrzej Hunt <[email protected]>

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 <[email protected]>

---
 builtin/clone.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

builtin/clone.c +10 -4

index 51e844a2de0a..952fe3d8fc88 100644
@@ -964,10 +964,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 {
 	int is_bundle = 0, is_local;
 	const char *repo_name, *repo, *work_tree, *git_dir;
-	char *path, *dir, *display_repo = NULL;
+	char *path = NULL, *dir, *display_repo = NULL;
 	int dest_exists, real_dest_exists = 0;
 	const struct ref *refs, *remote_head;
-	const struct ref *remote_head_points_at;
+	struct ref *remote_head_points_at = NULL;
 	const struct ref *our_head_points_at;
 	struct ref *mapped_refs;
 	const struct ref *ref;

@@ -1017,9 +1017,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	repo_name = argv[0];
 
 	path = get_repo_path(repo_name, &is_bundle);
-	if (path)
+	if (path) {
+		FREE_AND_NULL(path);
 		repo = absolute_pathdup(repo_name);
-	else if (strchr(repo_name, ':')) {
+	} else if (strchr(repo_name, ':')) {
 		repo = repo_name;
 		display_repo = transport_anonymize_url(repo);
 	} else

@@ -1393,6 +1394,11 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	strbuf_release(&reflog_msg);
 	strbuf_release(&branch_top);
 	strbuf_release(&key);
+	free_refs(mapped_refs);
+	free_refs(remote_head_points_at);
+	free(dir);
+	free(path);
+	UNLEAK(repo);
 	junk_mode = JUNK_LEAVE_ALL;
 
 	strvec_clear(&transport_ls_refs_options.ref_prefixes);

Signature · 1 line

gitgitgadget

Andrzej Hunt via GitGitGadget

author

2021-03-21 16:59 UTC #45 lore raw

PATCH v3 4/9 worktree: fix leak in dwim_branch()

In reply to Andrzej Hunt via GitGitGadget

From: Andrzej Hunt <[email protected]>

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)

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 <[email protected]>

---
 builtin/worktree.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

builtin/worktree.c +6 -4

index 1cd5c2016e3f..b0563aef685f 100644
@@ -446,16 +446,18 @@ static void print_preparing_worktree_line(int detach,
 static const char *dwim_branch(const char *path, const char **new_branch)
 {
 	int n;
+	int branch_exists;
 	const char *s = worktree_basename(path, &n);
 	const char *branchname = xstrndup(s, n);
 	struct strbuf ref = STRBUF_INIT;
 
 	UNLEAK(branchname);
-	if (!strbuf_check_branch_ref(&ref, branchname) &&
-	    ref_exists(ref.buf)) {
-		strbuf_release(&ref);
+
+	branch_exists = !strbuf_check_branch_ref(&ref, branchname) &&
+	                ref_exists(ref.buf);
+	strbuf_release(&ref);
+	if (branch_exists)
 		return branchname;
-	}
 
 	*new_branch = branchname;
 	if (guess_remote) {

Signature · 1 line

gitgitgadget

Andrzej Hunt via GitGitGadget

author

2021-03-21 16:59 UTC #46 lore raw

PATCH v3 5/9 init: remove git_init_db_config() while fixing leaks

In reply to Andrzej Hunt via GitGitGadget

From: Andrzej Hunt <[email protected]>

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 <[email protected]>

---
 builtin/init-db.c | 28 +++++++---------------------
 1 file changed, 7 insertions(+), 21 deletions(-)

builtin/init-db.c +7 ~~-21~~

index dcc45bef5148..d31dbc883746 100644
@@ -25,7 +25,6 @@
 
 static int init_is_bare_repository = 0;
 static int init_shared_repository = -1;
-static const char *init_db_template_dir;
 
 static void copy_templates_1(struct strbuf *path, struct strbuf *template_path,
 			     DIR *dir)

@@ -94,7 +93,7 @@ static void copy_templates_1(struct strbuf *path, struct strbuf *template_path,
 	}
 }
 
-static void copy_templates(const char *template_dir)
+static void copy_templates(const char *template_dir, const char *init_template_dir)
 {
 	struct strbuf path = STRBUF_INIT;
 	struct strbuf template_path = STRBUF_INIT;

@@ -107,7 +106,7 @@ static void copy_templates(const char *template_dir)
 	if (!template_dir)
 		template_dir = getenv(TEMPLATE_DIR_ENVIRONMENT);
 	if (!template_dir)
-		template_dir = init_db_template_dir;
+		template_dir = init_template_dir;
 	if (!template_dir)
 		template_dir = to_free = system_path(DEFAULT_GIT_TEMPLATE_DIR);
 	if (!template_dir[0]) {

@@ -154,17 +153,6 @@ static void copy_templates(const char *template_dir)
 	clear_repository_format(&template_format);
 }
 
-static int git_init_db_config(const char *k, const char *v, void *cb)
-{
-	if (!strcmp(k, "init.templatedir"))
-		return git_config_pathname(&init_db_template_dir, k, v);
-
-	if (starts_with(k, "core."))
-		return platform_core_config(k, v, cb);
-
-	return 0;
-}
-
 /*
  * If the git_dir is not directly inside the working tree, then git will not
  * find it by default, and we need to set the worktree explicitly.

@@ -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);
 
 	safe_create_dir(git_dir, 0);

Signature · 1 line

gitgitgadget

Andrzej Hunt via GitGitGadget

author

2021-03-21 16:59 UTC #47 lore raw

PATCH v3 6/9 init-db: silence template_dir leak when converting to absolute path

In reply to Andrzej Hunt via GitGitGadget

From: Andrzej Hunt <[email protected]>

Signed-off-by Andrzej Hunt <[email protected]>

---
 builtin/init-db.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

builtin/init-db.c +3 -1

index d31dbc883746..efc66523e22c 100644
@@ -561,8 +561,10 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)
 	if (real_git_dir && !is_absolute_path(real_git_dir))
 		real_git_dir = real_pathdup(real_git_dir, 1);
 
-	if (template_dir && *template_dir && !is_absolute_path(template_dir))
+	if (template_dir && *template_dir && !is_absolute_path(template_dir)) {
 		template_dir = absolute_pathdup(template_dir);
+		UNLEAK(template_dir);
+	}
 
 	if (argc == 1) {
 		int mkdir_tried = 0;

Signature · 1 line

gitgitgadget

Andrzej Hunt via GitGitGadget

author

2021-03-21 16:59 UTC #48 lore raw

PATCH v3 7/9 parse-options: convert bitfield values to use binary shift

In reply to Andrzej Hunt via GitGitGadget

From: Andrzej Hunt <[email protected]>

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%[email protected]/

Signed-off-by Andrzej Hunt <[email protected]>

---
 parse-options.h | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

parse-options.h +17 ~~-17~~

index ff6506a50470..f2ddef18f7b0 100644
@@ -28,26 +28,26 @@ enum parse_opt_type {
 };
 
 enum parse_opt_flags {
-	PARSE_OPT_KEEP_DASHDASH = 1,
-	PARSE_OPT_STOP_AT_NON_OPTION = 2,
-	PARSE_OPT_KEEP_ARGV0 = 4,
-	PARSE_OPT_KEEP_UNKNOWN = 8,
-	PARSE_OPT_NO_INTERNAL_HELP = 16,
-	PARSE_OPT_ONE_SHOT = 32
+	PARSE_OPT_KEEP_DASHDASH = 1 << 0,
+	PARSE_OPT_STOP_AT_NON_OPTION = 1 << 1,
+	PARSE_OPT_KEEP_ARGV0 = 1 << 2,
+	PARSE_OPT_KEEP_UNKNOWN = 1 << 3,
+	PARSE_OPT_NO_INTERNAL_HELP = 1 << 4,
+	PARSE_OPT_ONE_SHOT = 1 << 5,
 };
 
 enum parse_opt_option_flags {
-	PARSE_OPT_OPTARG  = 1,
-	PARSE_OPT_NOARG   = 2,
-	PARSE_OPT_NONEG   = 4,
-	PARSE_OPT_HIDDEN  = 8,
-	PARSE_OPT_LASTARG_DEFAULT = 16,
-	PARSE_OPT_NODASH = 32,
-	PARSE_OPT_LITERAL_ARGHELP = 64,
-	PARSE_OPT_SHELL_EVAL = 256,
-	PARSE_OPT_NOCOMPLETE = 512,
-	PARSE_OPT_COMP_ARG = 1024,
-	PARSE_OPT_CMDMODE = 2048
+	PARSE_OPT_OPTARG  = 1 << 0,
+	PARSE_OPT_NOARG   = 1 << 1,
+	PARSE_OPT_NONEG   = 1 << 2,
+	PARSE_OPT_HIDDEN  = 1 << 3,
+	PARSE_OPT_LASTARG_DEFAULT = 1 << 4,
+	PARSE_OPT_NODASH = 1 << 5,
+	PARSE_OPT_LITERAL_ARGHELP = 1 << 6,
+	PARSE_OPT_SHELL_EVAL = 1 << 8,
+	PARSE_OPT_NOCOMPLETE = 1 << 9,
+	PARSE_OPT_COMP_ARG = 1 << 10,
+	PARSE_OPT_CMDMODE = 1 << 11,
 };
 
 enum parse_opt_result {

Signature · 1 line

gitgitgadget

Andrzej Hunt via GitGitGadget

author

2021-03-21 16:59 UTC #49 lore raw

PATCH v3 8/9 parse-options: don't leak alias help messages

In reply to Andrzej Hunt via GitGitGadget

From: Andrzej Hunt <[email protected]>

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)

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 <[email protected]>

---
 parse-options.c | 19 ++++++++++++++++++-
 parse-options.h |  1 +
 2 files changed, 19 insertions(+), 1 deletion(-)

parse-options.c +18 -1

index fbea16eaf5c2..e6f56768ca5d 100644
@@ -625,6 +625,8 @@ static int show_gitcomp(const struct option *opts, int show_all)
  *
  * Right now this is only used to preprocess and substitute
  * OPTION_ALIAS.
+ *
+ * The returned options should be freed using free_preprocessed_options.
  */
 static struct option *preprocess_options(struct parse_opt_ctx_t *ctx,
 					 const struct option *options)

@@ -678,6 +680,7 @@ static struct option *preprocess_options(struct parse_opt_ctx_t *ctx,
 			newopt[i].short_name = short_name;
 			newopt[i].long_name = long_name;
 			newopt[i].help = strbuf_detach(&help, NULL);
+			newopt[i].flags |= PARSE_OPT_FROM_ALIAS;
 			break;
 		}

@@ -693,6 +696,20 @@ static struct option *preprocess_options(struct parse_opt_ctx_t *ctx,
 	return newopt;
 }
 
+static void free_preprocessed_options(struct option *options)
+{
+	int i;
+
+	if (!options)
+		return;
+
+	for (i = 0; options[i].type != OPTION_END; i++) {
+		if (options[i].flags & PARSE_OPT_FROM_ALIAS)
+			free((void *)options[i].help);
+	}
+	free(options);
+}
+
 static int usage_with_options_internal(struct parse_opt_ctx_t *,
 				       const char * const *,
 				       const struct option *, int, int);

@@ -870,7 +887,7 @@ int parse_options(int argc, const char **argv, const char *prefix,
 	}
 
 	precompose_argv_prefix(argc, argv, NULL);
-	free(real_options);
+	free_preprocessed_options(real_options);
 	free(ctx.alias_groups);
 	return parse_options_end(&ctx);
 }

parse-options.h +1 -0

index f2ddef18f7b0..a845a9d95274 100644
@@ -44,6 +44,7 @@ enum parse_opt_option_flags {
 	PARSE_OPT_LASTARG_DEFAULT = 1 << 4,
 	PARSE_OPT_NODASH = 1 << 5,
 	PARSE_OPT_LITERAL_ARGHELP = 1 << 6,
+	PARSE_OPT_FROM_ALIAS = 1 << 7,
 	PARSE_OPT_SHELL_EVAL = 1 << 8,
 	PARSE_OPT_NOCOMPLETE = 1 << 9,
 	PARSE_OPT_COMP_ARG = 1 << 10,

Signature · 1 line

gitgitgadget

Andrzej Hunt via GitGitGadget

author

2021-03-21 16:59 UTC #50 lore raw

PATCH v3 9/9 transport: also free remote_refs in transport_disconnect()

In reply to Andrzej Hunt via GitGitGadget

From: Andrzej Hunt <[email protected]>

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 <[email protected]>

---
 builtin/ls-remote.c | 4 ++--
 builtin/remote.c    | 8 ++++----
 transport.c         | 2 ++
 3 files changed, 8 insertions(+), 6 deletions(-)

builtin/ls-remote.c +2 -2

index abfa9847374f..1794548c7117 100644
@@ -124,8 +124,6 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 		int hash_algo = hash_algo_by_ptr(transport_get_hash_algo(transport));
 		repo_set_hash_algo(the_repository, hash_algo);
 	}
-	if (transport_disconnect(transport))
-		return 1;
 
 	if (!dest && !quiet)
 		fprintf(stderr, "From %s\n", *remote->url);

@@ -151,5 +149,7 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 	}
 
 	ref_array_clear(&ref_array);
+	if (transport_disconnect(transport))
+		return 1;
 	return status;
 }

builtin/remote.c +4 -4

index d11a5589e49d..e31d9c99470e 100644
@@ -938,9 +938,6 @@ static int get_remote_ref_states(const char *name,
 				 struct ref_states *states,
 				 int query)
 {
-	struct transport *transport;
-	const struct ref *remote_refs;
-
 	states->remote = remote_get(name);
 	if (!states->remote)
 		return error(_("No such remote: '%s'"), name);

@@ -948,10 +945,12 @@ static int get_remote_ref_states(const char *name,
 	read_branches();
 
 	if (query) {
+		struct transport *transport;
+		const struct ref *remote_refs;
+
 		transport = transport_get(states->remote, states->remote->url_nr > 0 ?
 			states->remote->url[0] : NULL);
 		remote_refs = transport_get_remote_refs(transport, NULL);
-		transport_disconnect(transport);
 
 		states->queried = 1;
 		if (query & GET_REF_STATES)

@@ -960,6 +959,7 @@ static int get_remote_ref_states(const char *name,
 			get_head_names(remote_refs, states);
 		if (query & GET_PUSH_REF_STATES)
 			get_push_ref_states(remote_refs, states);
+		transport_disconnect(transport);
 	} else {
 		for_each_ref(append_ref_to_tracked_list, states);
 		string_list_sort(&states->tracked);

transport.c +2 -0

index 1c4ab676d1b1..eb4b2d4e475f 100644
@@ -1452,6 +1452,8 @@ int transport_disconnect(struct transport *transport)
 	int ret = 0;
 	if (transport->vtable->disconnect)
 		ret = transport->vtable->disconnect(transport);
+	if (transport->got_remote_refs)
+		free_refs((void *)transport->remote_refs);
 	free(transport);
 	return ret;
 }

Signature · 1 line

gitgitgadget

Junio C Hamano

2021-03-21 21:41 UTC #51 lore raw

In reply to Andrzej Hunt via GitGitGadget

Quoted 8 lines · Andrzej Hunt via GitGitGadget

V3:
* Ensures we don't silently change enum values in "parse-options: convert bitfield values to use binary shift". Instead we retain the original values in that patch, and reuse the unused value in a later patch.
* Fixes some silly commit message typos.
Thanks as always for the sharp-eyed reviews.

Thanks, all. Will replace.

I think this one is ready for 'next'. Let's merge it immediately after we rewind the tip of 'next' and rebuild on top of the latest release.