[PATCH 0/7] Fix all leaks in t0001

[PATCH 0/7] Fix all leaks in t0001

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

Note: this series does not guarantee that there are no leaks within
t0000-t0001, it only fixes those leaks which cause test failures. There is
at least one test case in t0000 where git is invoked in a subshell, and the
return value is ignored - meaning that a memory leak that is occuring during
that invocation does not cause tests to fail (I'm still trying to figure out
if that's something that's worth fixing - but that's probably a topic for a
separate thread):
https://git.kernel.org/pub/scm/git/git.git/tree/t/t0000-basic.sh#n1285

In case anyone is interested: I have been using the following workflow to
find leaks and verify fixes - I'm running into crashes when using LSAN
standalone, therefore I'm using full ASAN instead (I'm not particularly
concerned about this: LSAN standalone mode is known to be less-well tested
than leak-checking within ASAN [1], and the crashes are occurring within the
leak-checker itself):

make GIT_TEST_OPTS="-i -v" DEFAULT_TEST_TARGET="t0000-basic.sh"
ASAN_OPTIONS="detect_leaks=1:abort_on_error=1" SANITIZE=address DEVELOPER=1
CFLAGS="-DSUPPRESS_ANNOTATED_LEAKS -g -fno-optimize-sibling-calls -O1
-fno-omit-frame-pointer" test

(I then rerun the entire test suite with ASAN but with leak-checking
disabled in order to gain some confidence that my fixes aren't inadvertently
introducing memory safety issues.)

[1]
https://github.com/google/sanitizers/wiki/AddressSanitizerLeakSanitizer#stand-alone-mode

Andrzej Hunt (7):
  symbolic-ref: don't leak shortened refname in check_symref()
  reset: free instead of leaking unneeded ref
  clone: free or UNLEAK further pointers when finished
  worktree: fix leak in dwim_branch()
  init: remove git_init_db_config() while fixing leaks
  init-db: silence template_dir leak when converting to absolute path
  parse-options: don't leak alias help messages

 builtin/clone.c        | 13 ++++++++++---
 builtin/init-db.c      | 32 ++++++++++----------------------
 builtin/reset.c        |  2 +-
 builtin/symbolic-ref.c | 12 +++++++++---
 builtin/worktree.c     | 12 ++++++------
 parse-options.c        | 29 ++++++++++++++++++++++++-----
 6 files changed, 60 insertions(+), 40 deletions(-)


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

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

[Andrzej Hunt via GitGitGadget]

From: Andrzej Hunt <ajrhunt@google.com>

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 <ajrhunt@google.com>
---
 builtin/symbolic-ref.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/builtin/symbolic-ref.c b/builtin/symbolic-ref.c
index 80237f0df10f..8cf52599693a 100644
--- a/builtin/symbolic-ref.c
+++ b/builtin/symbolic-ref.c
@@ -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;
 }
-- 
gitgitgadget

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

[Jeff King]

On Mon, Mar 08, 2021 at 06:36:14PM +0000, Andrzej Hunt via GitGitGadget wrote:

> From: Andrzej Hunt <ajrhunt@google.com>
> 
> This leak has existed since:
> 9ab55daa55 (git symbolic-ref --delete $symref, 2012-10-21)
> 
> This leak was found when running t0001 with LSAN, see also LSAN output
> below:
> 
> Direct leak of 19 byte(s) in 1 object(s) allocated from:
>     #0 0x486514 in strdup /home/abuild/rpmbuild/BUILD/llvm-11.0.0.src/build/../projects/compiler-rt/lib/asan/asan_interceptors.cpp:452:3
>     #1 0x9ab048 in xstrdup /home/ahunt/oss-fuzz/git/wrapper.c:29:14
>     #2 0x8b452f in refs_shorten_unambiguous_ref /home/ahunt/oss-fuzz/git/refs.c
>     #3 0x8b47e8 in shorten_unambiguous_ref /home/ahunt/oss-fuzz/git/refs.c:1287:9
>     #4 0x679fce in check_symref /home/ahunt/oss-fuzz/git/builtin/symbolic-ref.c:28:14
>     #5 0x679ad8 in cmd_symbolic_ref /home/ahunt/oss-fuzz/git/builtin/symbolic-ref.c:70:9
>     #6 0x4cd60d in run_builtin /home/ahunt/oss-fuzz/git/git.c:453:11
>     #7 0x4cb2da in handle_builtin /home/ahunt/oss-fuzz/git/git.c:704:3
>     #8 0x4ccc37 in run_argv /home/ahunt/oss-fuzz/git/git.c:771:4
>     #9 0x4cac29 in cmd_main /home/ahunt/oss-fuzz/git/git.c:902:19
>     #10 0x69cc6e in main /home/ahunt/oss-fuzz/git/common-main.c:52:11
>     #11 0x7f98388a4349 in __libc_start_main (/lib64/libc.so.6+0x24349)

As a general template for fixing leaks, this information seems pretty
good. You might want to give a brief reason for why it's a leak (like
you do already in the second patch). Here it just would be something
like:

  shorten_unambiguous_ref() returns an allocated string. We have to
  track it separately from the const refname.

Or whatever. It doesn't need to be a novel, but just give an overview of
what's going that makes the diff obvious.

> diff --git a/builtin/symbolic-ref.c b/builtin/symbolic-ref.c
> index 80237f0df10f..8cf52599693a 100644
> --- a/builtin/symbolic-ref.c
> +++ b/builtin/symbolic-ref.c
> @@ -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);
> +		}

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

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

[Andrzej Hunt]

On 08/03/2021 20:01, Jeff King wrote:
> On Mon, Mar 08, 2021 at 06:36:14PM +0000, Andrzej Hunt via GitGitGadget wrote:
> 
>> From: Andrzej Hunt <ajrhunt@google.com>
>>
>> This leak has existed since:
>> 9ab55daa55 (git symbolic-ref --delete $symref, 2012-10-21)
>>
>> This leak was found when running t0001 with LSAN, see also LSAN output
>> below:
>>
>> Direct leak of 19 byte(s) in 1 object(s) allocated from:
>>      #0 0x486514 in strdup /home/abuild/rpmbuild/BUILD/llvm-11.0.0.src/build/../projects/compiler-rt/lib/asan/asan_interceptors.cpp:452:3
>>      #1 0x9ab048 in xstrdup /home/ahunt/oss-fuzz/git/wrapper.c:29:14
>>      #2 0x8b452f in refs_shorten_unambiguous_ref /home/ahunt/oss-fuzz/git/refs.c
>>      #3 0x8b47e8 in shorten_unambiguous_ref /home/ahunt/oss-fuzz/git/refs.c:1287:9
>>      #4 0x679fce in check_symref /home/ahunt/oss-fuzz/git/builtin/symbolic-ref.c:28:14
>>      #5 0x679ad8 in cmd_symbolic_ref /home/ahunt/oss-fuzz/git/builtin/symbolic-ref.c:70:9
>>      #6 0x4cd60d in run_builtin /home/ahunt/oss-fuzz/git/git.c:453:11
>>      #7 0x4cb2da in handle_builtin /home/ahunt/oss-fuzz/git/git.c:704:3
>>      #8 0x4ccc37 in run_argv /home/ahunt/oss-fuzz/git/git.c:771:4
>>      #9 0x4cac29 in cmd_main /home/ahunt/oss-fuzz/git/git.c:902:19
>>      #10 0x69cc6e in main /home/ahunt/oss-fuzz/git/common-main.c:52:11
>>      #11 0x7f98388a4349 in __libc_start_main (/lib64/libc.so.6+0x24349)
> 
> As a general template for fixing leaks, this information seems pretty
> good. You might want to give a brief reason for why it's a leak (like
> you do already in the second patch). Here it just would be something
> like:
> 
>    shorten_unambiguous_ref() returns an allocated string. We have to
>    track it separately from the const refname.
> 
> Or whatever. It doesn't need to be a novel, but just give an overview of
> what's going that makes the diff obvious.

Good point - I've copied this one verbatim - but it's also a good thing 
to remember if/when I fix further leaks!

> 
> There's also an idiom in Git's codebase when a const pointer may alias
> unowned storage, or a buffer that needs to be freed. Something like:
> 
>    if (print) {
>            char *to_free = NULL;
> 	  if (shorten)
> 	          refname = to_free = shorten_unambiguous_ref(refname, 0);
> 	  puts(refname);
> 	  free(to_free);
>    }
> 
> That avoids duplicating the part of the code that handles the variable.
> In this case it is only a single line, but IMHO it's still easier to
> read, as it makes clear that we call puts() in either case.

That's a nice pattern, and will probably be useful for future leak fixes 
too - I've made this change too!

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

[Andrzej Hunt via GitGitGadget]

From: Andrzej Hunt <ajrhunt@google.com>

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 <ajrhunt@google.com>
---
 builtin/reset.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/reset.c b/builtin/reset.c
index c635b062c3a7..43e855cb8876 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -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)
-- 
gitgitgadget

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

[Jeff King]

On Mon, Mar 08, 2021 at 06:36:15PM +0000, Andrzej Hunt via GitGitGadget wrote:

> dwim_ref() allocs a new string into ref. Instead of setting to NULL to discard
> it, we can FREE_AND_NULL.
> [...]
>  			dwim_ref(rev, strlen(rev), &dummy, &ref, 0);
>  			if (ref && !starts_with(ref, "refs/"))
> -				ref = NULL;
> +				FREE_AND_NULL(ref);

Yeah, this seems like a perfect solution for this case.

-Peff

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

[Andrzej Hunt via GitGitGadget]

From: Andrzej Hunt <ajrhunt@google.com>

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

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

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

Direct leak of 178 byte(s) in 1 object(s) allocated from:
    #0 0x49a53d in malloc /home/abuild/rpmbuild/BUILD/llvm-11.0.0.src/build/../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3
    #1 0x9a6ff4 in do_xmalloc /home/ahunt/oss-fuzz/git/wrapper.c:41:8
    #2 0x9a6fca in xmalloc /home/ahunt/oss-fuzz/git/wrapper.c:62:9
    #3 0x8ce296 in copy_ref /home/ahunt/oss-fuzz/git/remote.c:885:8
    #4 0x8d2ebd in guess_remote_head /home/ahunt/oss-fuzz/git/remote.c:2215:10
    #5 0x51d0c5 in cmd_clone /home/ahunt/oss-fuzz/git/builtin/clone.c:1308:4
    #6 0x4cd60d in run_builtin /home/ahunt/oss-fuzz/git/git.c:453:11
    #7 0x4cb2da in handle_builtin /home/ahunt/oss-fuzz/git/git.c:704:3
    #8 0x4ccc37 in run_argv /home/ahunt/oss-fuzz/git/git.c:771:4
    #9 0x4cac29 in cmd_main /home/ahunt/oss-fuzz/git/git.c:902:19
    #10 0x69c45e in main /home/ahunt/oss-fuzz/git/common-main.c:52:11
    #11 0x7f6a459d5349 in __libc_start_main (/lib64/libc.so.6+0x24349)

Direct leak of 165 byte(s) in 1 object(s) allocated from:
    #0 0x49a53d in malloc /home/abuild/rpmbuild/BUILD/llvm-11.0.0.src/build/../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3
    #1 0x9a6fc4 in do_xmalloc /home/ahunt/oss-fuzz/git/wrapper.c:41:8
    #2 0x9a6f9a in xmalloc /home/ahunt/oss-fuzz/git/wrapper.c:62:9
    #3 0x8ce266 in copy_ref /home/ahunt/oss-fuzz/git/remote.c:885:8
    #4 0x51e9bd in wanted_peer_refs /home/ahunt/oss-fuzz/git/builtin/clone.c:574:21
    #5 0x51cfe1 in cmd_clone /home/ahunt/oss-fuzz/git/builtin/clone.c:1284:17
    #6 0x4cd60d in run_builtin /home/ahunt/oss-fuzz/git/git.c:453:11
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 <ajrhunt@google.com>
---
 builtin/clone.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index 51e844a2de0a..7f76fdceceb7 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -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);
-- 
gitgitgadget

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

[Jeff King]

On Mon, Mar 08, 2021 at 06:36:16PM +0000, Andrzej Hunt via GitGitGadget wrote:

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

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

> +	free_refs((void *)refs);

This one is more questionable to me. It comes from
transport_get_remote_refs(), which does return a const pointer. And it
looks like that memory is owned by the transport struct. So presumably
we need to tell the transport code to clean itself up (or mark it with
UNLEAK). Or perhaps there's a bug in the transport code (e.g., should it
be freeing transport->remote_refs in transport_disconnect()? You'd want
to make sure that no other callers expect the ref list to live on past
the disconnect).

> +	UNLEAK(repo);

This one could be done with the idiom I mentioned earlier:

  repo = absolute_repo = absolute_pathdup(repo_name);
  ...
  free(absolute_repo);

but I think this is a perfect use of UNLEAK().

-Peff

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

[Andrzej Hunt]

On 08/03/2021 20:12, Jeff King wrote:
> On Mon, Mar 08, 2021 at 06:36:16PM +0000, Andrzej Hunt via GitGitGadget wrote:
> 
>> @@ -1017,9 +1017,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>>   	repo_name = argv[0];
>>   
>>   	path = get_repo_path(repo_name, &is_bundle);
>> -	if (path)
>> +	if (path) {
>> +		free(path);
>>   		repo = absolute_pathdup(repo_name);
> 
> You mentioned that "path" gets reused again later. Should we use
> FREE_AND_NULL() to make sure that nobody tries to look at it in the
> meantime?

That sounds sensible - I wasn't too sure at first because we are 
unconditionally setting path later... but I can't see an reason not to 
make this safer.

But that makes me wonder if the definition of path should set it to NULL 
too. Setting it to NULL after freeing it guarantees that we can't read 
the old value anymore. However later code has no idea if path will be 
NULL or merely initialised (at least until we overwrite it with the new 
path).

I've provisionally updated my patch to also set path = NULL at point of 
definition, but I don't know if that's idiomatic in this scenario.

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

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

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

[Andrzej Hunt via GitGitGadget]

From: Andrzej Hunt <ajrhunt@google.com>

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 <ajrhunt@google.com>
---
 builtin/worktree.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index 1cd5c2016e3f..44d3f058d065 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -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) {
-- 
gitgitgadget

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

[Jeff King]

On Mon, Mar 08, 2021 at 06:36:17PM +0000, Andrzej Hunt via GitGitGadget wrote:

> Make sure that we release the temporary strbuf during dwim_branch() for
> all codepaths (and not just for the early return).

Makes sense. Two style nits:

>  static const char *dwim_branch(const char *path, const char **new_branch)
>  {
> -	int n;
> +	int n, branch_exists;

If two variables aren't strictly related, but just happen to share the
same type, IMHO it's better to declare them separately. (There are
numerous spots in Git's code that don't follow this rule, but I think we
should be moving in that direction).

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

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

[Andrzej Hunt]

On 08/03/2021 20:16, Jeff King wrote:
> On Mon, Mar 08, 2021 at 06:36:17PM +0000, Andrzej Hunt via GitGitGadget wrote:
> 
>> Make sure that we release the temporary strbuf during dwim_branch() for
>> all codepaths (and not just for the early return).
> 
> Makes sense. Two style nits:

Thank, I'll fix both.
> 
>> -	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);

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.

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

[Andrzej Hunt via GitGitGadget]

From: Andrzej Hunt <ajrhunt@google.com>

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 <ajrhunt@google.com>
---
 builtin/init-db.c | 28 +++++++---------------------
 1 file changed, 7 insertions(+), 21 deletions(-)

diff --git a/builtin/init-db.c b/builtin/init-db.c
index dcc45bef5148..d31dbc883746 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -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);
 
-- 
gitgitgadget

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

[Jeff King]

On Mon, Mar 08, 2021 at 06:36:18PM +0000, Andrzej Hunt via GitGitGadget wrote:

> @@ -212,10 +200,7 @@ static int create_default_files(const char *template_path,
>  	int reinit;
>  	int filemode;
>  	struct strbuf err = STRBUF_INIT;
> -
> -	/* Just look for `init.templatedir` */
> -	init_db_template_dir = NULL; /* re-set in case it was set before */
> -	git_config(git_init_db_config, NULL);
> +	const char *init_template_dir = NULL;
>  
>  	/*
>  	 * First copy the templates -- we might have the default
> @@ -226,7 +211,8 @@ static int create_default_files(const char *template_path,
>  	 * values (since we've just potentially changed what's available on
>  	 * disk).
>  	 */
> -	copy_templates(template_path);
> +	git_config_get_value("init.templatedir", &init_template_dir);
> +	copy_templates(template_path, init_template_dir);
>  	git_config_clear();
>  	reset_shared_repository();
>  	git_config(git_default_config, NULL);
> @@ -422,8 +408,8 @@ int init_db(const char *git_dir, const char *real_git_dir,
>  	}
>  	startup_info->have_repository = 1;
>  
> -	/* Just look for `core.hidedotfiles` */
> -	git_config(git_init_db_config, NULL);
> +	/* Ensure `core.hidedotfiles` is processed */
> +	git_config(platform_core_config, NULL);

There are some subtle ordering dependencies in init_db(), because it may
start in one git repository, but then create and shift into another.

It's hard to see the ordering just from the diff. I think this change is
OK. The platform_core_config bits are loaded at the same moment, and
it's only the extra git_config() call in create_default_files() that
goes away. That _could_ be overwriting the platform bits with something
else, but I think that was not the intent of the code. And I think it's
impossible, because the intervening calls are not moving from one repo
to the other.

-Peff

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

[Andrzej Hunt via GitGitGadget]

From: Andrzej Hunt <ajrhunt@google.com>

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 <ajrhunt@google.com>
---
 builtin/init-db.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/builtin/init-db.c b/builtin/init-db.c
index d31dbc883746..efc66523e22c 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -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;
-- 
gitgitgadget

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

[Jeff King]

On Mon, Mar 08, 2021 at 06:36:19PM +0000, Andrzej Hunt via GitGitGadget wrote:

> template_dir starts off pointing to either argv or nothing. However if
> the value supplied in argv is a relative path, absolute_pathdup() is
> used to turn it into an absolute path. absolute_pathdup() allocates
> a new string, and we then "leak" it when cmd_init_db() completes.
> 
> We don't bother to actually free the return value (instead we UNLEAK
> it), because there's no significant advantage to doing so here.
> Correctly freeing it would require more significant changes to code flow
> which would be more noisy than beneficial.

Makes sense. The UNLEAK() could be conditional at the end of the
function (since it is OK to unleak a non-allocated variable, too), but I
like keeping it here with the allocation for clarity.

-Peff

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

[Andrzej Hunt via GitGitGadget]

From: Andrzej Hunt <ajrhunt@google.com>

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 <ajrhunt@google.com>
---
 parse-options.c | 29 ++++++++++++++++++++++++-----
 1 file changed, 24 insertions(+), 5 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index fbea16eaf5c2..3fe1dacc08cb 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -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);
 }
-- 
gitgitgadget

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

[Jeff King]

On Mon, Mar 08, 2021 at 06:36:20PM +0000, Andrzej Hunt via GitGitGadget wrote:

> preprocess_options() allocates new strings for help messages for
> OPTION_ALIAS. Therefore we also need to clean those help messages up
> when freeing the returned options.

Yep, makes sense.

> The preprocessed options themselves no longer contain any indication
> that a given option is/was an alias: the easiest and fastest way to
> figure it out is to look back at the original options. Alternatively we
> could iterate over the alias_groups list - but that would require nested
> looping and is likely to be a (little) less efficient.

Yeah, this is a bit ugly. We could probably set a bit in the aliased
struct's flags field to indicate that it's an alias, though.

> As far as I can tell, parse_options() is only ever used once per
> command, and the help messages are small - hence this leak has very
> little impact.

We _could_ UNLEAK() it, but I prefer this actual cleanup. We're getting
far enough away from main() that we aren't actually sure that we'll only
be called once per process.

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

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

> +	for (i = 0; original_options[i].type != OPTION_END; i++) {
> +		if (original_options[i].type == OPTION_ALIAS) {
> +			free((void *)(*preprocessed_options)[i].help);
> +		}
> +	}

OK, so we look through the original options to find ones that became an
alias, and then free them. Makes sense.

Do the indexes always correspond between the original and the
preprocessed arrays? I _think_ so, but preprocess_options() is a little
hard to follow.

If the preprocess code set a flag in the resulting option, though, we
could make it much more obviously correct. And avoid having to pass
original_options at all.

> +	free((void *)*preprocessed_options);

With the interface suggestions above, this becomes just:

  free(preprocessed_options);

> @@ -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;
> +	}

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:

diff --git a/parse-options.c b/parse-options.c
index fbea16eaf5..43431b96b1 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -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;
 		}
 
diff --git a/parse-options.h b/parse-options.h
index ff6506a504..32b0b49a2d 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -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

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

[Andrzej Hunt]

On 08/03/2021 20:46, Jeff King wrote:
> On Mon, Mar 08, 2021 at 06:36:20PM +0000, Andrzej Hunt via GitGitGadget wrote:
> 
>> +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)

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.

> 
>> +	for (i = 0; original_options[i].type != OPTION_END; i++) {
>> +		if (original_options[i].type == OPTION_ALIAS) {
>> +			free((void *)(*preprocessed_options)[i].help);
>> +		}
>> +	}
> 
> OK, so we look through the original options to find ones that became an
> alias, and then free them. Makes sense.
> 
> Do the indexes always correspond between the original and the
> preprocessed arrays? I _think_ so, but preprocess_options() is a little
> hard to follow.
> 
> If the preprocess code set a flag in the resulting option, though, we
> could make it much more obviously correct. And avoid having to pass
> original_options at all.

_At this time_, indexes always correspond between the original and 
preprocessed options, but in the back of my mind I was still a little 
bit uncomfortable depending on that. Your suggestion is much better - so 
I've gone ahead and implemented it.

> 
>> +	free((void *)*preprocessed_options);
> 
> With the interface suggestions above, this becomes just:
> 
>    free(preprocessed_options);
> 
>> @@ -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;
>> +	}
> 
> 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.

> 
> To spell it out, I mean something like on the writing side:
> 
> diff --git a/parse-options.c b/parse-options.c
> index fbea16eaf5..43431b96b1 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -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;
>   		}
>   
> diff --git a/parse-options.h b/parse-options.h
> index ff6506a504..32b0b49a2d 100644
> --- a/parse-options.h
> +++ b/parse-options.h
> @@ -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).

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!

Re: [PATCH 0/7] Fix all leaks in t0001

[Jeff King]

On Mon, Mar 08, 2021 at 06:36:13PM +0000, Andrzej Hunt via GitGitGadget wrote:

> This series fixes (or annotates) all the memory leaks that can cause t0001
> to fail when run with LeakSanitizer (t0000 already passes without failures).
> 
> I suspect that none of these leaks had any user impact, and I'm aware that
> every change does cause some noise - I would have no objections to
> abandoning this series if it's not deemed valuable enough. On the other
> hand: fixing or suppressing these leaks should make it easier to spot leaks
> that have more significant user impact (it's entirely plausible that no real
> impactful leaks exist).

I think it's worth doing. The reason t0000 passes is because it was my
reference script when adding UNLEAK() back in:

  https://lore.kernel.org/git/20170905130149.agc3zp3s6i6e5aki@sigill.intra.peff.net/

(which might be of historical interest if you haven't read it). I knew
that the next step would be tediously going through the test suite
looking at the tool results, and I somehow stalled on that part. ;)

But I think it's nice to move the goal forward incrementally. I agree
that a lot of these leaks aren't that important, but it's generally as
easy to fix or annotate them as it is to argue that they shouldn't be
dealt with.

> Note: this series does not guarantee that there are no leaks within
> t0000-t0001, it only fixes those leaks which cause test failures. There is
> at least one test case in t0000 where git is invoked in a subshell, and the
> return value is ignored - meaning that a memory leak that is occuring during
> that invocation does not cause tests to fail (I'm still trying to figure out
> if that's something that's worth fixing - but that's probably a topic for a
> separate thread):
> https://git.kernel.org/pub/scm/git/git.git/tree/t/t0000-basic.sh#n1285

It's not the subshell there, but rather that git is on the left-hand
side of a pipe (and so its exit code is discarded). We've been slowly
fixing such cases (the usual technique is to use a tempfile).

> In case anyone is interested: I have been using the following workflow to
> find leaks and verify fixes - I'm running into crashes when using LSAN
> standalone, therefore I'm using full ASAN instead (I'm not particularly
> concerned about this: LSAN standalone mode is known to be less-well tested
> than leak-checking within ASAN [1], and the crashes are occurring within the
> leak-checker itself):

Yeah, I think using ASAN is just fine. I found that LSAN is faster, but
if you are running a single script the difference probably doesn't
matter. I also found that clang's support was much more mature than
gcc's (I don't know how different the state is these days, though).

Regardless, if you can get it to run cleanly with _any_ leak checker,
I'll be quite happy. :)

> make GIT_TEST_OPTS="-i -v" DEFAULT_TEST_TARGET="t0000-basic.sh"
> ASAN_OPTIONS="detect_leaks=1:abort_on_error=1" SANITIZE=address DEVELOPER=1
> CFLAGS="-DSUPPRESS_ANNOTATED_LEAKS -g -fno-optimize-sibling-calls -O1
> -fno-omit-frame-pointer" test

There's some magic in the Makefile for detecting SANITIZE=leak and
setting -DSUPPRESS_ANNOTATED_LEAKS. It might be worth that extending
that to SANITIZE=address, but I guess we wouldn't want to do so for most
builds (which also are setting detect_leaks=0 in the test suite). Maybe
we should have some other name to trigger asan-as-a-leak-detector. Or
maybe that just gets complicated, because we pass the results of
SANITIZE on to the compiler directly.

> (I then rerun the entire test suite with ASAN but with leak-checking
> disabled in order to gain some confidence that my fixes aren't inadvertently
> introducing memory safety issues.)

Definitely a good idea.

> Andrzej Hunt (7):
>   symbolic-ref: don't leak shortened refname in check_symref()
>   reset: free instead of leaking unneeded ref
>   clone: free or UNLEAK further pointers when finished
>   worktree: fix leak in dwim_branch()
>   init: remove git_init_db_config() while fixing leaks
>   init-db: silence template_dir leak when converting to absolute path
>   parse-options: don't leak alias help messages

I haven't looked at the individual patches yet. I'll respond to them
individually.

-Peff

Re: [PATCH 0/7] Fix all leaks in t0001

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I think it's worth doing. The reason t0000 passes is because it was my
> reference script when adding UNLEAK() back in:
>
>   https://lore.kernel.org/git/20170905130149.agc3zp3s6i6e5aki@sigill.intra.peff.net/
>
> (which might be of historical interest if you haven't read it). I knew
> that the next step would be tediously going through the test suite
> looking at the tool results, and I somehow stalled on that part. ;)
> ...
> 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.

Re: [PATCH 0/7] Fix all leaks in t0001

[Andrzej Hunt]

On 08/03/2021 19:55, Jeff King wrote:
> I think it's worth doing. The reason t0000 passes is because it was my
> reference script when adding UNLEAK() back in:
> 
>    https://lore.kernel.org/git/20170905130149.agc3zp3s6i6e5aki@sigill.intra.peff.net/
> 
> (which might be of historical interest if you haven't read it). I knew
> that the next step would be tediously going through the test suite
> looking at the tool results, and I somehow stalled on that part. ;)
> 
> But I think it's nice to move the goal forward incrementally. I agree
> that a lot of these leaks aren't that important, but it's generally as
> easy to fix or annotate them as it is to argue that they shouldn't be > dealt with.

Thanks for the confirmation! I'd seen your post, but wasn't sure if 
there'd been a change of plan or just lack of time :).

>> Note: this series does not guarantee that there are no leaks within
>> t0000-t0001, it only fixes those leaks which cause test failures. There is
>> at least one test case in t0000 where git is invoked in a subshell, and the
>> return value is ignored - meaning that a memory leak that is occuring during
>> that invocation does not cause tests to fail (I'm still trying to figure out
>> if that's something that's worth fixing - but that's probably a topic for a
>> separate thread):
>> https://git.kernel.org/pub/scm/git/git.git/tree/t/t0000-basic.sh#n1285
> 
> It's not the subshell there, but rather that git is on the left-hand
> side of a pipe (and so its exit code is discarded). We've been slowly
> fixing such cases (the usual technique is to use a tempfile).

Thanks for the tip! I've started on another series to fix t0000-basic 
along with the leaks that that uncovers. In future I suspect it's best 
to start by removing pipes _before_ running leak-checking against a 
given test. (Fortunately t0001 doesn't contain any such cases, so this 
series is valid as is.)

>> In case anyone is interested: I have been using the following workflow to
>> find leaks and verify fixes - I'm running into crashes when using LSAN
>> standalone, therefore I'm using full ASAN instead (I'm not particularly
>> concerned about this: LSAN standalone mode is known to be less-well tested
>> than leak-checking within ASAN [1], and the crashes are occurring within the
>> leak-checker itself):
> 
> Yeah, I think using ASAN is just fine. I found that LSAN is faster, but
> if you are running a single script the difference probably doesn't
> matter. I also found that clang's support was much more mature than
> gcc's (I don't know how different the state is these days, though).
> 
> Regardless, if you can get it to run cleanly with _any_ leak checker,
> I'll be quite happy. :)

I was wrong when it comes to LSAN being broken. What was actually 
happening is: we default to running ASAN and LSAN with abort_on_error=1, 
and I had overridden that setting when running with ASAN. When I 
switched to LSAN, abort_on_error was enabled again - and I was just 
misinterpreting the intentional abortion as opposed to seeing an 
unexplained crashes. [As far as I can tell, abort_on_error is needed to 
detect leaks during a test_must_fail and similar scenarios.]

I've briefly tested all the various combinations of gcc or clang, with 
LSAN or ASAN or both - and they all seem to work as expected, with one 
exception: gcc with LSAN-only finds what seems to be a false positive, 
in a method which mallocs, followed by a die().

To make it trickier, that new "leak" happens inside a test_must_fail - 
the LSAN output is swallowed, making it hard to diagnose. I'll try to 
prepare a separate patch to not discard stderr in that scenario.

Regardless of the LSAN/ASAN differences - I'm wondering whether 
piggybacking on the existing ASAN validation might be the best way to 
get leak checking run more often (limited to a subset of leak-free tests 
of course). I'll expand on these thoughts in my reply to Junio.

>> make GIT_TEST_OPTS="-i -v" DEFAULT_TEST_TARGET="t0000-basic.sh"
>> ASAN_OPTIONS="detect_leaks=1:abort_on_error=1" SANITIZE=address DEVELOPER=1
>> CFLAGS="-DSUPPRESS_ANNOTATED_LEAKS -g -fno-optimize-sibling-calls -O1
>> -fno-omit-frame-pointer" test
> 
> There's some magic in the Makefile for detecting SANITIZE=leak and
> setting -DSUPPRESS_ANNOTATED_LEAKS. It might be worth that extending
> that to SANITIZE=address, but I guess we wouldn't want to do so for most
> builds (which also are setting detect_leaks=0 in the test suite). Maybe
> we should have some other name to trigger asan-as-a-leak-detector. Or
> maybe that just gets complicated, because we pass the results of
> SANITIZE on to the compiler directly.

I've realised it's enough to set SANITIZE=leak,address. That gives us 
the benefits of ASAN, but still adds -DSUPPRESS_ANNOTATED_LEAKS. Given 
that LSAN seems stable enough with clang, I suppose this is only really 
useful for gcc users.

> I haven't looked at the individual patches yet. I'll respond to them
> individually.
> 
> -Peff

Thank you for the reviews! I'm still a bit new to the git codebase, 
thank you for being patient with my (deficits of) style :).

Re: [PATCH 0/7] Fix all leaks in t0001

[Andrzej Hunt]

On 14/03/2021 17:54, Andrzej Hunt wrote:
> 
> I was wrong when it comes to LSAN being broken. What was actually 
> happening is: we default to running ASAN and LSAN with abort_on_error=1, 
> and I had overridden that setting when running with ASAN. When I 
> switched to LSAN, abort_on_error was enabled again - and I was just 
> misinterpreting the intentional abortion as opposed to seeing an 
> unexplained crashes. [As far as I can tell, abort_on_error is needed to 
> detect leaks during a test_must_fail and similar scenarios.]

Actually I might have been unintentionally right the first time round: 
running clang+LSAN against t0000 (with pipe-related fixes applied) 
results in reports as follows, which are effectively useless:

Direct leak of 41 byte(s) in 1 object(s) allocated from:
     #0 0x44fc98 in malloc 
/home/abuild/rpmbuild/BUILD/llvm-11.0.0.src/build/../projects/compiler-rt/lib/lsan/lsan_interceptors.cpp:56:3
     #1 0x7fcdbb3dc749 in strdup (/lib64/libc.so.6+0x8b749)

Switching back to ASAN shows me the real stack instead:

Direct leak of 41 byte(s) in 1 object(s) allocated from:
     #0 0x486834 in strdup 
/home/abuild/rpmbuild/BUILD/llvm-11.0.0.src/build/../projects/compiler-rt/lib/asan/asan_interceptors.cpp:452:3
     #1 0x9ab1b8 in xstrdup /home/ahunt/oss-fuzz/git/wrapper.c:29:14
     ... rest of stack here ...
> 
> To make it trickier, that new "leak" happens inside a test_must_fail - 
> the LSAN output is swallowed, making it hard to diagnose. I'll try to 
> prepare a separate patch to not discard stderr in that scenario.

The issue here is that:

  - test_must_fail does actually keep git's stderr on stderr (there is
    some redirection within test_must_fail itself - but eventually
    what was git's stderr becomes test_must_fail's stderr).
  - many uses of test_must_fail redirect stderr into a temporary file.
  - when git fails as expected, test_must_fail succeeds, and the
    next command in the test validates that temporary file (that
    validation step prints that error output on failure).
  - however if git aborted, or didn't fail as expected: test_must_fail
    returns an error code. git's error messages remain in the temporary
    file and aren't printed.

My potentially naive idea is that within test_must_fail, git's output 
should be sent to both test_must_fail's stderr, and also to fd-4 (which 
seems to be where verbose output should be sent).

However hat might also result in errors being printed twice (I believe 
that will happen for test_must_fail uses that don't redirect stderr into 
a temporary file).

Re: [PATCH 0/7] Fix all leaks in t0001

[Junio C Hamano]

"Andrzej Hunt via GitGitGadget" <gitgitgadget@gmail.com> writes:

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

Re: [PATCH 0/7] Fix all leaks in t0001

[Andrzej Hunt]

On 08/03/2021 19:57, Junio C Hamano wrote:
> "Andrzej Hunt via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> 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:

diff --git a/t/test-lib.sh b/t/test-lib.sh
index d3f6af6a65..cf9f1ad827 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -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/20170710155831.3zxijp7bvbquvlau@sigill.intra.peff.net/

[PATCH v2 0/9] Fix all leaks in t0001

[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 <ajrhunt@google.com>
      
     +    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()

-- 
gitgitgadget

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

[Andrzej Hunt via GitGitGadget]

From: Andrzej Hunt <ajrhunt@google.com>

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 <ajrhunt@google.com>
---
 builtin/symbolic-ref.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/builtin/symbolic-ref.c b/builtin/symbolic-ref.c
index 80237f0df10f..e547a08d6c7c 100644
--- a/builtin/symbolic-ref.c
+++ b/builtin/symbolic-ref.c
@@ -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;
 }
-- 
gitgitgadget

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

[Andrzej Hunt via GitGitGadget]

From: Andrzej Hunt <ajrhunt@google.com>

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 <ajrhunt@google.com>
---
 builtin/reset.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/reset.c b/builtin/reset.c
index c635b062c3a7..43e855cb8876 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -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)
-- 
gitgitgadget

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

[Andrzej Hunt via GitGitGadget]

From: Andrzej Hunt <ajrhunt@google.com>

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

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

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

Direct leak of 178 byte(s) in 1 object(s) allocated from:
    #0 0x49a53d in malloc /home/abuild/rpmbuild/BUILD/llvm-11.0.0.src/build/../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3
    #1 0x9a6ff4 in do_xmalloc /home/ahunt/oss-fuzz/git/wrapper.c:41:8
    #2 0x9a6fca in xmalloc /home/ahunt/oss-fuzz/git/wrapper.c:62:9
    #3 0x8ce296 in copy_ref /home/ahunt/oss-fuzz/git/remote.c:885:8
    #4 0x8d2ebd in guess_remote_head /home/ahunt/oss-fuzz/git/remote.c:2215:10
    #5 0x51d0c5 in cmd_clone /home/ahunt/oss-fuzz/git/builtin/clone.c:1308:4
    #6 0x4cd60d in run_builtin /home/ahunt/oss-fuzz/git/git.c:453:11
    #7 0x4cb2da in handle_builtin /home/ahunt/oss-fuzz/git/git.c:704:3
    #8 0x4ccc37 in run_argv /home/ahunt/oss-fuzz/git/git.c:771:4
    #9 0x4cac29 in cmd_main /home/ahunt/oss-fuzz/git/git.c:902:19
    #10 0x69c45e in main /home/ahunt/oss-fuzz/git/common-main.c:52:11
    #11 0x7f6a459d5349 in __libc_start_main (/lib64/libc.so.6+0x24349)

Direct leak of 165 byte(s) in 1 object(s) allocated from:
    #0 0x49a53d in malloc /home/abuild/rpmbuild/BUILD/llvm-11.0.0.src/build/../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3
    #1 0x9a6fc4 in do_xmalloc /home/ahunt/oss-fuzz/git/wrapper.c:41:8
    #2 0x9a6f9a in xmalloc /home/ahunt/oss-fuzz/git/wrapper.c:62:9
    #3 0x8ce266 in copy_ref /home/ahunt/oss-fuzz/git/remote.c:885:8
    #4 0x51e9bd in wanted_peer_refs /home/ahunt/oss-fuzz/git/builtin/clone.c:574:21
    #5 0x51cfe1 in cmd_clone /home/ahunt/oss-fuzz/git/builtin/clone.c:1284:17
    #6 0x4cd60d in run_builtin /home/ahunt/oss-fuzz/git/git.c:453:11
    #7 0x4cb2da in handle_builtin /home/ahunt/oss-fuzz/git/git.c:704:3
    #8 0x4ccc37 in run_argv /home/ahunt/oss-fuzz/git/git.c:771:4
    #9 0x4cac29 in cmd_main /home/ahunt/oss-fuzz/git/git.c:902:19
    #10 0x69c42e in main /home/ahunt/oss-fuzz/git/common-main.c:52:11
    #11 0x7f8fef0c2349 in __libc_start_main (/lib64/libc.so.6+0x24349)

Direct leak of 178 byte(s) in 1 object(s) allocated from:
    #0 0x49a53d in malloc /home/abuild/rpmbuild/BUILD/llvm-11.0.0.src/build/../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3
    #1 0x9a6ff4 in do_xmalloc /home/ahunt/oss-fuzz/git/wrapper.c:41:8
    #2 0x9a6fca in xmalloc /home/ahunt/oss-fuzz/git/wrapper.c:62:9
    #3 0x8ce296 in copy_ref /home/ahunt/oss-fuzz/git/remote.c:885:8
    #4 0x8d2ebd in guess_remote_head /home/ahunt/oss-fuzz/git/remote.c:2215:10
    #5 0x51d0c5 in cmd_clone /home/ahunt/oss-fuzz/git/builtin/clone.c:1308:4
    #6 0x4cd60d in run_builtin /home/ahunt/oss-fuzz/git/git.c:453:11
    #7 0x4cb2da in handle_builtin /home/ahunt/oss-fuzz/git/git.c:704:3
    #8 0x4ccc37 in run_argv /home/ahunt/oss-fuzz/git/git.c:771:4
    #9 0x4cac29 in cmd_main /home/ahunt/oss-fuzz/git/git.c:902:19
    #10 0x69c45e in main /home/ahunt/oss-fuzz/git/common-main.c:52:11
    #11 0x7f6a459d5349 in __libc_start_main (/lib64/libc.so.6+0x24349)

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

Direct leak of 105 byte(s) in 1 object(s) allocated from:
    #0 0x49a859 in realloc /home/abuild/rpmbuild/BUILD/llvm-11.0.0.src/build/../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3
    #1 0x9a71f6 in xrealloc /home/ahunt/oss-fuzz/git/wrapper.c:126:8
    #2 0x93622d in strbuf_grow /home/ahunt/oss-fuzz/git/strbuf.c:98:2
    #3 0x937a73 in strbuf_addch /home/ahunt/oss-fuzz/git/./strbuf.h:231:3
    #4 0x939fcd in strbuf_add_absolute_path /home/ahunt/oss-fuzz/git/strbuf.c:911:4
    #5 0x69d3ce in absolute_pathdup /home/ahunt/oss-fuzz/git/abspath.c:261:2
    #6 0x51c688 in cmd_clone /home/ahunt/oss-fuzz/git/builtin/clone.c:1021:10
    #7 0x4cd60d in run_builtin /home/ahunt/oss-fuzz/git/git.c:453:11
    #8 0x4cb2da in handle_builtin /home/ahunt/oss-fuzz/git/git.c:704:3
    #9 0x4ccc37 in run_argv /home/ahunt/oss-fuzz/git/git.c:771:4
    #10 0x4cac29 in cmd_main /home/ahunt/oss-fuzz/git/git.c:902:19
    #11 0x69c45e in main /home/ahunt/oss-fuzz/git/common-main.c:52:11
    #12 0x7f6a459d5349 in __libc_start_main (/lib64/libc.so.6+0x24349)

Signed-off-by: Andrzej Hunt <ajrhunt@google.com>
---
 builtin/clone.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index 51e844a2de0a..952fe3d8fc88 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -964,10 +964,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 {
 	int is_bundle = 0, is_local;
 	const char *repo_name, *repo, *work_tree, *git_dir;
-	char *path, *dir, *display_repo = NULL;
+	char *path = NULL, *dir, *display_repo = NULL;
 	int dest_exists, real_dest_exists = 0;
 	const struct ref *refs, *remote_head;
-	const struct ref *remote_head_points_at;
+	struct ref *remote_head_points_at = NULL;
 	const struct ref *our_head_points_at;
 	struct ref *mapped_refs;
 	const struct ref *ref;
@@ -1017,9 +1017,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	repo_name = argv[0];
 
 	path = get_repo_path(repo_name, &is_bundle);
-	if (path)
+	if (path) {
+		FREE_AND_NULL(path);
 		repo = absolute_pathdup(repo_name);
-	else if (strchr(repo_name, ':')) {
+	} else if (strchr(repo_name, ':')) {
 		repo = repo_name;
 		display_repo = transport_anonymize_url(repo);
 	} else
@@ -1393,6 +1394,11 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	strbuf_release(&reflog_msg);
 	strbuf_release(&branch_top);
 	strbuf_release(&key);
+	free_refs(mapped_refs);
+	free_refs(remote_head_points_at);
+	free(dir);
+	free(path);
+	UNLEAK(repo);
 	junk_mode = JUNK_LEAVE_ALL;
 
 	strvec_clear(&transport_ls_refs_options.ref_prefixes);
-- 
gitgitgadget

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

[Andrzej Hunt via GitGitGadget]

From: Andrzej Hunt <ajrhunt@google.com>

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 <ajrhunt@google.com>
---
 builtin/worktree.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index 1cd5c2016e3f..b0563aef685f 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -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) {
-- 
gitgitgadget

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

[Andrzej Hunt via GitGitGadget]

From: Andrzej Hunt <ajrhunt@google.com>

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 <ajrhunt@google.com>
---
 builtin/init-db.c | 28 +++++++---------------------
 1 file changed, 7 insertions(+), 21 deletions(-)

diff --git a/builtin/init-db.c b/builtin/init-db.c
index dcc45bef5148..d31dbc883746 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -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);
 
-- 
gitgitgadget

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

[Andrzej Hunt via GitGitGadget]

From: Andrzej Hunt <ajrhunt@google.com>

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 <ajrhunt@google.com>
---
 builtin/init-db.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/builtin/init-db.c b/builtin/init-db.c
index d31dbc883746..efc66523e22c 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -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;
-- 
gitgitgadget

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

[Andrzej Hunt via GitGitGadget]

From: Andrzej Hunt <ajrhunt@google.com>

Because it's easier to read, but also likely to be easier to maintain.
I am making this change because I need to add a new flag in a later
commit.

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

This changee was originally suggested by Peff in:
https://public-inbox.org/git/YEZ%2FBWWbpfVwl6nO@coredump.intra.peff.net/

Signed-off-by: Andrzej Hunt <ajrhunt@google.com>
---
 parse-options.h | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/parse-options.h b/parse-options.h
index ff6506a50470..36ce0a44b2e9 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -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 {
-- 
gitgitgadget

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

[Martin Ågren]

On Sun, 14 Mar 2021 at 20:05, Andrzej Hunt via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Andrzej Hunt <ajrhunt@google.com>
>
> Because it's easier to read, but also likely to be easier to maintain.
> I am making this change because I need to add a new flag in a later
> commit.

Makes sense.

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

Makes sense.

> This changee was originally suggested by Peff in:

s/changee/change/

> https://public-inbox.org/git/YEZ%2FBWWbpfVwl6nO@coredump.intra.peff.net/

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

Straightforward.

>  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_NEGHELP` is gone since acbb08c2e0b ("parse-options: remove
PARSE_OPT_NEGHELP", 2012-02-28), which explains the jump here.

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

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

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

[Junio C Hamano]

Martin Ågren <martin.agren@gmail.com> writes:

>>  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_NEGHELP` is gone since acbb08c2e0b ("parse-options: remove
> PARSE_OPT_NEGHELP", 2012-02-28), which explains the jump here.
>
>> -       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,
>>  };
>
> 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.

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

[Andrzej Hunt]

On 14/03/2021 21:25, Martin Ågren wrote:
> On Sun, 14 Mar 2021 at 20:05, Andrzej Hunt via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>>
>> From: Andrzej Hunt <ajrhunt@google.com>
>>
>> Because it's easier to read, but also likely to be easier to maintain.
>> I am making this change because I need to add a new flag in a later
>> commit.
> 
> Makes sense.
> 
>> Also add a trailing comma to the last enum entry to simplify addition of
>> new flags.
> 
> Makes sense.
> 
>> This changee was originally suggested by Peff in:
> 
> s/changee/change/
> 
>> https://public-inbox.org/git/YEZ%2FBWWbpfVwl6nO@coredump.intra.peff.net/
> 
>>   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,
>>   };
> 
> Straightforward.
> 
>>   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_NEGHELP` is gone since acbb08c2e0b ("parse-options: remove
> PARSE_OPT_NEGHELP", 2012-02-28), which explains the jump here.
> 
>> -       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,
>>   };
> 
> 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

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

[Andrzej Hunt via GitGitGadget]

From: Andrzej Hunt <ajrhunt@google.com>

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 <ajrhunt@google.com>

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

diff --git a/parse-options.c b/parse-options.c
index fbea16eaf5c2..6c7f927240b6 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -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);
 }
diff --git a/parse-options.h b/parse-options.h
index 36ce0a44b2e9..331c6c67812c 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -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 {
-- 
gitgitgadget

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

[Eric Sunshine]

On Sun, Mar 14, 2021 at 2:49 PM Andrzej Hunt via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> preprocess_options() allocates new strings for help messages for
> OPTION_ALIAS. Therefore we also need to clean those help messages up
> when freeing the returned options.
> [...]
> Signed-off-by: Andrzej Hunt <ajrhunt@google.com>
>
> fold
> ---

Unwanted "fold".

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

[Andrzej Hunt]

On 14/03/2021 20:48, Eric Sunshine wrote:
> On Sun, Mar 14, 2021 at 2:49 PM Andrzej Hunt via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>> preprocess_options() allocates new strings for help messages for
>> OPTION_ALIAS. Therefore we also need to clean those help messages up
>> when freeing the returned options.
>> [...]
>> Signed-off-by: Andrzej Hunt <ajrhunt@google.com>
>>
>> fold
>> ---
> 
> Unwanted "fold".
> 

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

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

[Andrzej Hunt]

> On 14 Mar 2021, at 19:47, Andrzej Hunt via GitGitGadget <gitgitgadget@gmail.com> wrote:
> +    for (i = 0; options[i].type != OPTION_END; i++) {
> +        if (options[i].flags & PARSE_OPT_FROM_ALIAS) {
> +            free((void *)options[i].help);
> +        }

Oops, I did it again: I’ll remove these unnecessary braces in V3.

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

[Andrzej Hunt via GitGitGadget]

From: Andrzej Hunt <ajrhunt@google.com>

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

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

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

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

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

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

[PATCH v3 0/9] Fix all leaks in t0001

[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

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        | 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%2FBWWbpfVwl6nO@coredump.intra.peff.net/
      
          Signed-off-by: Andrzej Hunt <ajrhunt@google.com>
     @@ 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 <ajrhunt@google.com>
      
     -    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()

-- 
gitgitgadget

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

[Andrzej Hunt via GitGitGadget]

From: Andrzej Hunt <ajrhunt@google.com>

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 <ajrhunt@google.com>
---
 builtin/symbolic-ref.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/builtin/symbolic-ref.c b/builtin/symbolic-ref.c
index 80237f0df10f..e547a08d6c7c 100644
--- a/builtin/symbolic-ref.c
+++ b/builtin/symbolic-ref.c
@@ -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;
 }
-- 
gitgitgadget

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

[Andrzej Hunt via GitGitGadget]

From: Andrzej Hunt <ajrhunt@google.com>

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 <ajrhunt@google.com>
---
 builtin/reset.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/reset.c b/builtin/reset.c
index c635b062c3a7..43e855cb8876 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -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)
-- 
gitgitgadget

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

[Andrzej Hunt via GitGitGadget]

From: Andrzej Hunt <ajrhunt@google.com>

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

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

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

Direct leak of 178 byte(s) in 1 object(s) allocated from:
    #0 0x49a53d in malloc /home/abuild/rpmbuild/BUILD/llvm-11.0.0.src/build/../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3
    #1 0x9a6ff4 in do_xmalloc /home/ahunt/oss-fuzz/git/wrapper.c:41:8
    #2 0x9a6fca in xmalloc /home/ahunt/oss-fuzz/git/wrapper.c:62:9
    #3 0x8ce296 in copy_ref /home/ahunt/oss-fuzz/git/remote.c:885:8
    #4 0x8d2ebd in guess_remote_head /home/ahunt/oss-fuzz/git/remote.c:2215:10
    #5 0x51d0c5 in cmd_clone /home/ahunt/oss-fuzz/git/builtin/clone.c:1308:4
    #6 0x4cd60d in run_builtin /home/ahunt/oss-fuzz/git/git.c:453:11
    #7 0x4cb2da in handle_builtin /home/ahunt/oss-fuzz/git/git.c:704:3
    #8 0x4ccc37 in run_argv /home/ahunt/oss-fuzz/git/git.c:771:4
    #9 0x4cac29 in cmd_main /home/ahunt/oss-fuzz/git/git.c:902:19
    #10 0x69c45e in main /home/ahunt/oss-fuzz/git/common-main.c:52:11
    #11 0x7f6a459d5349 in __libc_start_main (/lib64/libc.so.6+0x24349)

Direct leak of 165 byte(s) in 1 object(s) allocated from:
    #0 0x49a53d in malloc /home/abuild/rpmbuild/BUILD/llvm-11.0.0.src/build/../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3
    #1 0x9a6fc4 in do_xmalloc /home/ahunt/oss-fuzz/git/wrapper.c:41:8
    #2 0x9a6f9a in xmalloc /home/ahunt/oss-fuzz/git/wrapper.c:62:9
    #3 0x8ce266 in copy_ref /home/ahunt/oss-fuzz/git/remote.c:885:8
    #4 0x51e9bd in wanted_peer_refs /home/ahunt/oss-fuzz/git/builtin/clone.c:574:21
    #5 0x51cfe1 in cmd_clone /home/ahunt/oss-fuzz/git/builtin/clone.c:1284:17
    #6 0x4cd60d in run_builtin /home/ahunt/oss-fuzz/git/git.c:453:11
    #7 0x4cb2da in handle_builtin /home/ahunt/oss-fuzz/git/git.c:704:3
    #8 0x4ccc37 in run_argv /home/ahunt/oss-fuzz/git/git.c:771:4
    #9 0x4cac29 in cmd_main /home/ahunt/oss-fuzz/git/git.c:902:19
    #10 0x69c42e in main /home/ahunt/oss-fuzz/git/common-main.c:52:11
    #11 0x7f8fef0c2349 in __libc_start_main (/lib64/libc.so.6+0x24349)

Direct leak of 178 byte(s) in 1 object(s) allocated from:
    #0 0x49a53d in malloc /home/abuild/rpmbuild/BUILD/llvm-11.0.0.src/build/../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3
    #1 0x9a6ff4 in do_xmalloc /home/ahunt/oss-fuzz/git/wrapper.c:41:8
    #2 0x9a6fca in xmalloc /home/ahunt/oss-fuzz/git/wrapper.c:62:9
    #3 0x8ce296 in copy_ref /home/ahunt/oss-fuzz/git/remote.c:885:8
    #4 0x8d2ebd in guess_remote_head /home/ahunt/oss-fuzz/git/remote.c:2215:10
    #5 0x51d0c5 in cmd_clone /home/ahunt/oss-fuzz/git/builtin/clone.c:1308:4
    #6 0x4cd60d in run_builtin /home/ahunt/oss-fuzz/git/git.c:453:11
    #7 0x4cb2da in handle_builtin /home/ahunt/oss-fuzz/git/git.c:704:3
    #8 0x4ccc37 in run_argv /home/ahunt/oss-fuzz/git/git.c:771:4
    #9 0x4cac29 in cmd_main /home/ahunt/oss-fuzz/git/git.c:902:19
    #10 0x69c45e in main /home/ahunt/oss-fuzz/git/common-main.c:52:11
    #11 0x7f6a459d5349 in __libc_start_main (/lib64/libc.so.6+0x24349)

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

Direct leak of 105 byte(s) in 1 object(s) allocated from:
    #0 0x49a859 in realloc /home/abuild/rpmbuild/BUILD/llvm-11.0.0.src/build/../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:164:3
    #1 0x9a71f6 in xrealloc /home/ahunt/oss-fuzz/git/wrapper.c:126:8
    #2 0x93622d in strbuf_grow /home/ahunt/oss-fuzz/git/strbuf.c:98:2
    #3 0x937a73 in strbuf_addch /home/ahunt/oss-fuzz/git/./strbuf.h:231:3
    #4 0x939fcd in strbuf_add_absolute_path /home/ahunt/oss-fuzz/git/strbuf.c:911:4
    #5 0x69d3ce in absolute_pathdup /home/ahunt/oss-fuzz/git/abspath.c:261:2
    #6 0x51c688 in cmd_clone /home/ahunt/oss-fuzz/git/builtin/clone.c:1021:10
    #7 0x4cd60d in run_builtin /home/ahunt/oss-fuzz/git/git.c:453:11
    #8 0x4cb2da in handle_builtin /home/ahunt/oss-fuzz/git/git.c:704:3
    #9 0x4ccc37 in run_argv /home/ahunt/oss-fuzz/git/git.c:771:4
    #10 0x4cac29 in cmd_main /home/ahunt/oss-fuzz/git/git.c:902:19
    #11 0x69c45e in main /home/ahunt/oss-fuzz/git/common-main.c:52:11
    #12 0x7f6a459d5349 in __libc_start_main (/lib64/libc.so.6+0x24349)

Signed-off-by: Andrzej Hunt <ajrhunt@google.com>
---
 builtin/clone.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index 51e844a2de0a..952fe3d8fc88 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -964,10 +964,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 {
 	int is_bundle = 0, is_local;
 	const char *repo_name, *repo, *work_tree, *git_dir;
-	char *path, *dir, *display_repo = NULL;
+	char *path = NULL, *dir, *display_repo = NULL;
 	int dest_exists, real_dest_exists = 0;
 	const struct ref *refs, *remote_head;
-	const struct ref *remote_head_points_at;
+	struct ref *remote_head_points_at = NULL;
 	const struct ref *our_head_points_at;
 	struct ref *mapped_refs;
 	const struct ref *ref;
@@ -1017,9 +1017,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	repo_name = argv[0];
 
 	path = get_repo_path(repo_name, &is_bundle);
-	if (path)
+	if (path) {
+		FREE_AND_NULL(path);
 		repo = absolute_pathdup(repo_name);
-	else if (strchr(repo_name, ':')) {
+	} else if (strchr(repo_name, ':')) {
 		repo = repo_name;
 		display_repo = transport_anonymize_url(repo);
 	} else
@@ -1393,6 +1394,11 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	strbuf_release(&reflog_msg);
 	strbuf_release(&branch_top);
 	strbuf_release(&key);
+	free_refs(mapped_refs);
+	free_refs(remote_head_points_at);
+	free(dir);
+	free(path);
+	UNLEAK(repo);
 	junk_mode = JUNK_LEAVE_ALL;
 
 	strvec_clear(&transport_ls_refs_options.ref_prefixes);
-- 
gitgitgadget

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

[Andrzej Hunt via GitGitGadget]

From: Andrzej Hunt <ajrhunt@google.com>

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 <ajrhunt@google.com>
---
 builtin/worktree.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index 1cd5c2016e3f..b0563aef685f 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -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) {
-- 
gitgitgadget

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

[Andrzej Hunt via GitGitGadget]

From: Andrzej Hunt <ajrhunt@google.com>

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 <ajrhunt@google.com>
---
 builtin/init-db.c | 28 +++++++---------------------
 1 file changed, 7 insertions(+), 21 deletions(-)

diff --git a/builtin/init-db.c b/builtin/init-db.c
index dcc45bef5148..d31dbc883746 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -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);
 
-- 
gitgitgadget

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

[Andrzej Hunt via GitGitGadget]

From: Andrzej Hunt <ajrhunt@google.com>

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 <ajrhunt@google.com>
---
 builtin/init-db.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/builtin/init-db.c b/builtin/init-db.c
index d31dbc883746..efc66523e22c 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -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;
-- 
gitgitgadget

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

[Andrzej Hunt via GitGitGadget]

From: Andrzej Hunt <ajrhunt@google.com>

Because it's easier to read, but also likely to be easier to maintain.
I am making this change because I need to add a new flag in a later
commit.

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

This change was originally suggested by Peff in:
https://public-inbox.org/git/YEZ%2FBWWbpfVwl6nO@coredump.intra.peff.net/

Signed-off-by: Andrzej Hunt <ajrhunt@google.com>
---
 parse-options.h | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/parse-options.h b/parse-options.h
index ff6506a50470..f2ddef18f7b0 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -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 {
-- 
gitgitgadget

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

[Andrzej Hunt via GitGitGadget]

From: Andrzej Hunt <ajrhunt@google.com>

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 <ajrhunt@google.com>
---
 parse-options.c | 19 ++++++++++++++++++-
 parse-options.h |  1 +
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/parse-options.c b/parse-options.c
index fbea16eaf5c2..e6f56768ca5d 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -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);
 }
diff --git a/parse-options.h b/parse-options.h
index f2ddef18f7b0..a845a9d95274 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -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,
-- 
gitgitgadget

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

[Andrzej Hunt via GitGitGadget]

From: Andrzej Hunt <ajrhunt@google.com>

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

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

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

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

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

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

Re: [PATCH v3 0/9] Fix all leaks in t0001

[Junio C Hamano]

"Andrzej Hunt via GitGitGadget" <gitgitgadget@gmail.com> writes:

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