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

["Andrzej Hunt via GitGitGadget" <gitgitgadget@gmail.com>]

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