[PATCH v2 0/3] refs.c + config.c: plug memory leaks

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

In response to the feedback on v1 on this (in particular the
use-after-free, thanks Martin!) here's a v2 which I think is a good
thing to do with our without that particular GCC behavior I ran into.

As noted in 3/3 I think this is a known caveat of those SANITIZE=
modes, e.g. valgrind reports a memory leak regardless of optimization
level.

The only pure workaround for the issue is now 3/3, which I think is a
worthwhile to carry to avoid developer potentially wasting time on it.

Ævar Arnfjörð Bjarmason (3):
  refs.c: make "repo_default_branch_name" static, remove xstrfmt()
  config.c: don't leak memory in handle_path_include()
  config.c: free(expanded) before die(), work around GCC oddity

 config.c                  | 22 ++++++++++++++--------
 refs.c                    |  8 +++-----
 refs.h                    |  1 -
 t/t1305-config-include.sh |  1 +
 4 files changed, 18 insertions(+), 14 deletions(-)

Range-diff against v1:
-:  ----------- > 1:  4f8554bb02e refs.c: make "repo_default_branch_name" static, remove xstrfmt()
-:  ----------- > 2:  d6d04da1d9d config.c: don't leak memory in handle_path_include()
1:  5a47bf2e9c9 ! 3:  d812358e331 leak tests: free() before die for two API functions
    @@ Metadata
     Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## Commit message ##
    -    leak tests: free() before die for two API functions
    +    config.c: free(expanded) before die(), work around GCC oddity
     
    -    Call free() just before die() in two API functions whose tests are
    -    asserted under SANITIZE=leak. Normally this would not be needed due to
    -    how SANITIZE=leak works, but in these cases my GCC version (10.2.1-6)
    -    will fail tests t0001 and t0017 under SANITIZE=leak depending on the
    -    optimization level.
    +    On my GCC version (10.2.1-6), but not the clang I have available t0017
    +    will fail under SANITIZE=leak on optimization levels higher than -O0,
    +    which is annoying when combined with the change in 956d2e4639b (tests:
    +    add a test mode for SANITIZE=leak, run it in CI, 2021-09-23).
     
    -    See 956d2e4639b (tests: add a test mode for SANITIZE=leak, run it in
    -    CI, 2021-09-23) for the commit that marked t0017 for testing with
    -    SANITIZE=leak, and c150064dbe2 (leak tests: run various built-in tests
    -    in t00*.sh SANITIZE=leak, 2021-10-12) for t0001 (currently in "next").
    +    We really do have a memory leak here in either case, as e.g. running
    +    the pre-image under valgrind(1) will reveal. It's documented
    +    SANITIZE=leak (and "address", which exhibits the same behavior) might
    +    interact with compiler optimization in this way in some cases, and
    +    since this function is called recursively it's going to be especially
    +    interesting as an optimization target.
    +
    +    Let's work around this issue by freeing the "expanded" memory before
    +    we call die(), using the "goto cleanup" pattern introduced in the
    +    preceding commit.
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## config.c ##
    +@@ config.c: static int handle_path_include(const char *path, struct config_include_data *inc
    + 	int ret = 0;
    + 	struct strbuf buf = STRBUF_INIT;
    + 	char *expanded;
    ++	int die_depth = 0;
    + 
    + 	if (!path)
    + 		return config_error_nonbool("include.path");
     @@ config.c: static int handle_path_include(const char *path, struct config_include_data *inc
      	}
      
      	if (!access_or_die(path, R_OK, 0)) {
     -		if (++inc->depth > MAX_INCLUDE_DEPTH)
    +-			die(_(include_depth_advice), MAX_INCLUDE_DEPTH, path,
    +-			    !cf ? "<unknown>" :
    +-			    cf->name ? cf->name :
    +-			    "the command line");
     +		if (++inc->depth > MAX_INCLUDE_DEPTH) {
    -+			free(expanded);
    - 			die(_(include_depth_advice), MAX_INCLUDE_DEPTH, path,
    - 			    !cf ? "<unknown>" :
    - 			    cf->name ? cf->name :
    - 			    "the command line");
    ++			die_depth = 1;
    ++			goto cleanup;
     +		}
      		ret = git_config_from_file(git_config_include, path, inc);
      		inc->depth--;
      	}
    -
    - ## refs.c ##
    -@@ refs.c: char *repo_default_branch_name(struct repository *r, int quiet)
    - 	}
    - 
    - 	full_ref = xstrfmt("refs/heads/%s", ret);
    --	if (check_refname_format(full_ref, 0))
    -+	if (check_refname_format(full_ref, 0)) {
    -+		free(ret);
    -+		free(full_ref);
    - 		die(_("invalid branch name: %s = %s"), config_display_key, ret);
    -+	}
    - 	free(full_ref);
    + cleanup:
    + 	strbuf_release(&buf);
    + 	free(expanded);
    +-	return ret;
    ++	if (!die_depth)
    ++		return ret;
    ++	die(_(include_depth_advice), MAX_INCLUDE_DEPTH, path,
    ++	    !cf ? "<unknown>" : cf->name ? cf->name : "the command line");
    + }
      
    - 	return ret;
    + static void add_trailing_starstar_for_dir(struct strbuf *pat)
-- 
2.33.1.1494.g88b39a443e1