[PATCH] leak tests: free() before die for two API functions

[PATCH] leak tests: free() before die for two API functions

[Ævar Arnfjörð Bjarmason]

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.

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

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 config.c | 4 +++-
 refs.c   | 5 ++++-
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/config.c b/config.c
index 2dcbe901b6b..93979d39b21 100644
--- a/config.c
+++ b/config.c
@@ -159,11 +159,13 @@ 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)
+		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");
+		}
 		ret = git_config_from_file(git_config_include, path, inc);
 		inc->depth--;
 	}
diff --git a/refs.c b/refs.c
index 7f019c2377e..52929286032 100644
--- a/refs.c
+++ b/refs.c
@@ -590,8 +590,11 @@ 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);
 
 	return ret;
-- 
2.33.1.1486.gb2bc4955b90

Re: [PATCH] leak tests: free() before die for two API functions

[Andrzej Hunt]

> On 21 Oct 2021, at 13:42, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> 
> 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.

I’m curious - to me this seems like a compiler/sanitiser bug, can it also be reproduced with clang, or even newer versions of gcc? Similarly, can it be reproduced with your gcc version, using ASAN+LSAN (as opposed to LSAN by itself)? I remember seeing some false positives in the past for some permutations of compilers and sanitisers, but I’ve lost track of the details.

These kinds of fixes seem noisy if it’s just to work around what appears to be a bug (and to be philosophical: we wouldn’t want to do the same for all “leaks” up the call stack if a specific compiler complained about them after a die() - after all there will be many more allocations that didn’t get free’d floating around - so why is it OK for these “leaks”?)

If it this is a gcc-specific or LSAN-only-specific bug, I would suggest giving up on that combination for leak checking instead of adding such workarounds. After all the code seems correct - and while such compiler-specific workarounds are probably justified for user-visible bugs, these fixes seem to just be silencing a non-issue that only happens with what is probably a  “broken” setup?

(From what I can remember, I never saw these when running t00* using clang 11 or 12, always using LSAN+ASAN, but that was a while back. I’ve not spent much time using gcc.)

> 
> 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").
> 
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> config.c | 4 +++-
> refs.c   | 5 ++++-
> 2 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/config.c b/config.c
> index 2dcbe901b6b..93979d39b21 100644
> --- a/config.c
> +++ b/config.c
> @@ -159,11 +159,13 @@ 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)
> +        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");
> +        }
>        ret = git_config_from_file(git_config_include, path, inc);
>        inc->depth--;
>    }
> diff --git a/refs.c b/refs.c
> index 7f019c2377e..52929286032 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -590,8 +590,11 @@ 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);
> 
>    return ret;
> -- 
> 2.33.1.1486.gb2bc4955b90
> 

Re: [PATCH] leak tests: free() before die for two API functions

[Martin Ågren]

On Thu, 21 Oct 2021 at 13:43, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>
> 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.

Seems a bit unfortunate. I have to wonder why these in particular
trigger this compiler bug or whatever it is, but oh well.

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

This looks like use-after-free. Rather than complicating this by, e.g.,
first formatting the string, then freeing `ret`, then dying, could we --
if we really want this workaround -- make the workaround be `UNLEAK`
instead?

Also, if we do something like this patch, I think we should try to avoid
this free-before-die then being cargo-culted all across the codebase.
How about

  UNLEAK(ret); /* work around compiler bug */
  UNLEAK(full_ref); /* work around compiler bug */

or something?

Martin

Re: [PATCH] leak tests: free() before die for two API functions

[Junio C Hamano]

Andrzej Hunt <andrzej@ahunt.org> writes:

>> On 21 Oct 2021, at 13:42, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>> 
>> 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.
>
> I’m curious - to me this seems like a compiler/sanitiser bug, can
> it also be reproduced with clang, or even newer versions of gcc?
> Similarly, can it be reproduced with your gcc version, using
> ASAN+LSAN (as opposed to LSAN by itself)? I remember seeing some
> false positives in the past for some permutations of compilers and
> sanitisers, but I’ve lost track of the details.
>
> These kinds of fixes seem noisy if it’s just to work around what
> appears to be a bug (and to be philosophical: we wouldn’t want to
> do the same for all “leaks” up the call stack if a specific
> compiler complained about them after a die() - after all there
> will be many more allocations that didn’t get free’d floating
> around - so why is it OK for these “leaks”?)

Exactly my feeling.  I'll leave this patch hanging on the list
without picking it up until we know this is a reasonable "fix" on
our side and not adding noize only to work around the bug in the
tools.

Thanks.

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

[Ævar Arnfjörð Bjarmason]

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

[PATCH v2 1/3] refs.c: make "repo_default_branch_name" static, remove xstrfmt()

[Ævar Arnfjörð Bjarmason]

The repo_default_branch_name() function introduced in
8747ebb7cde (init: allow setting the default for the initial branch
name via the config, 2020-06-24) has never been used outside of this
file, so let's make it static, its sibling function
git_default_branch_name() is what external callers use.

In addition the xstrfmt() to get the "full_ref" in the same commit
isn't needed, we can use the "REFNAME_ALLOW_ONELEVEL" flag to
check_refname_format() instead.

This also happens to fix an issue with c150064dbe2 (leak tests: run
various built-in tests in t00*.sh SANITIZE=leak, 2021-10-12) in "next"
when combined with SANITIZE=leak and higher optimization flags on at
least some GCC versions. See [1].

1. https://lore.kernel.org/git/patch-1.1-5a47bf2e9c9-20211021T114223Z-avarab@gmail.com/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 refs.c | 8 +++-----
 refs.h | 1 -
 2 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/refs.c b/refs.c
index 7f019c2377e..ccb09acbf1d 100644
--- a/refs.c
+++ b/refs.c
@@ -571,11 +571,11 @@ static const char default_branch_name_advice[] = N_(
 "\tgit branch -m <name>\n"
 );
 
-char *repo_default_branch_name(struct repository *r, int quiet)
+static char *repo_default_branch_name(struct repository *r, int quiet)
 {
 	const char *config_key = "init.defaultbranch";
 	const char *config_display_key = "init.defaultBranch";
-	char *ret = NULL, *full_ref;
+	char *ret = NULL;
 	const char *env = getenv("GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME");
 
 	if (env && *env)
@@ -589,10 +589,8 @@ char *repo_default_branch_name(struct repository *r, int quiet)
 			advise(_(default_branch_name_advice), ret);
 	}
 
-	full_ref = xstrfmt("refs/heads/%s", ret);
-	if (check_refname_format(full_ref, 0))
+	if (check_refname_format(ret, REFNAME_ALLOW_ONELEVEL))
 		die(_("invalid branch name: %s = %s"), config_display_key, ret);
-	free(full_ref);
 
 	return ret;
 }
diff --git a/refs.h b/refs.h
index d5099d4984e..77f899da6ef 100644
--- a/refs.h
+++ b/refs.h
@@ -171,7 +171,6 @@ int dwim_log(const char *str, int len, struct object_id *oid, char **ref);
  * return value of `git_default_branch_name()` is a singleton.
  */
 const char *git_default_branch_name(int quiet);
-char *repo_default_branch_name(struct repository *r, int quiet);
 
 /*
  * A ref_transaction represents a collection of reference updates that
-- 
2.33.1.1494.g88b39a443e1

[PATCH v2 2/3] config.c: don't leak memory in handle_path_include()

[Ævar Arnfjörð Bjarmason]

Fix a memory leak in the error() path in handle_path_include(), this
allows us to run t1305-config-include.sh under SANITIZE=leak,
previously 4 tests there would fail. This fixes up a leak in
9b25a0b52e0 (config: add include directive, 2012-02-06).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 config.c                  | 7 +++++--
 t/t1305-config-include.sh | 1 +
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/config.c b/config.c
index 2dcbe901b6b..c5873f3a706 100644
--- a/config.c
+++ b/config.c
@@ -148,8 +148,10 @@ static int handle_path_include(const char *path, struct config_include_data *inc
 	if (!is_absolute_path(path)) {
 		char *slash;
 
-		if (!cf || !cf->path)
-			return error(_("relative config includes must come from files"));
+		if (!cf || !cf->path) {
+			ret = error(_("relative config includes must come from files"));
+			goto cleanup;
+		}
 
 		slash = find_last_dir_sep(cf->path);
 		if (slash)
@@ -167,6 +169,7 @@ static int handle_path_include(const char *path, struct config_include_data *inc
 		ret = git_config_from_file(git_config_include, path, inc);
 		inc->depth--;
 	}
+cleanup:
 	strbuf_release(&buf);
 	free(expanded);
 	return ret;
diff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh
index ccbb116c016..5cde79ef8c4 100755
--- a/t/t1305-config-include.sh
+++ b/t/t1305-config-include.sh
@@ -1,6 +1,7 @@
 #!/bin/sh
 
 test_description='test config file include directives'
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 # Force setup_explicit_git_dir() to run until the end. This is needed
-- 
2.33.1.1494.g88b39a443e1

[PATCH v2 3/3] config.c: free(expanded) before die(), work around GCC oddity

[Ævar Arnfjörð Bjarmason]

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

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 | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/config.c b/config.c
index c5873f3a706..ab40decaeba 100644
--- a/config.c
+++ b/config.c
@@ -132,6 +132,7 @@ 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");
@@ -161,18 +162,20 @@ 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) {
+			die_depth = 1;
+			goto cleanup;
+		}
 		ret = git_config_from_file(git_config_include, path, inc);
 		inc->depth--;
 	}
 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");
 }
 
 static void add_trailing_starstar_for_dir(struct strbuf *pat)
-- 
2.33.1.1494.g88b39a443e1

Re: [PATCH v2 1/3] refs.c: make "repo_default_branch_name" static, remove xstrfmt()

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> In addition the xstrfmt() to get the "full_ref" in the same commit
> isn't needed, we can use the "REFNAME_ALLOW_ONELEVEL" flag to
> check_refname_format() instead.

Reading the code of check_refname_format(), I do not think one-level
is the only thing that the prefixing of refs/heads/ is defeating,
and more importantly, I'd expect that this will block later changes
like enforcing "HEAD might be OK in onelevel because we want to keep
.git/HEAD working, but we do not like refs/heads/HEAD" at this level
to enhance usability from happening.

Making the function file-local static is a good thing to do, though.

Re: [PATCH v2 2/3] config.c: don't leak memory in handle_path_include()

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Fix a memory leak in the error() path in handle_path_include(), this
> allows us to run t1305-config-include.sh under SANITIZE=leak,
> previously 4 tests there would fail. This fixes up a leak in
> 9b25a0b52e0 (config: add include directive, 2012-02-06).
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  config.c                  | 7 +++++--
>  t/t1305-config-include.sh | 1 +
>  2 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/config.c b/config.c
> index 2dcbe901b6b..c5873f3a706 100644
> --- a/config.c
> +++ b/config.c
> @@ -148,8 +148,10 @@ static int handle_path_include(const char *path, struct config_include_data *inc

Not a problem introduced by this function, but if you look at this
change with "git show -W", we'd notice that the function name on the
hunk header looks strange.  I think we should add a blank line
before the beginning of the function.

>  	if (!is_absolute_path(path)) {
>  		char *slash;
>  
> -		if (!cf || !cf->path)
> -			return error(_("relative config includes must come from files"));
> +		if (!cf || !cf->path) {
> +			ret = error(_("relative config includes must come from files"));
> +			goto cleanup;
> +		}
>  
>  		slash = find_last_dir_sep(cf->path);
>  		if (slash)
> @@ -167,6 +169,7 @@ static int handle_path_include(const char *path, struct config_include_data *inc
>  		ret = git_config_from_file(git_config_include, path, inc);
>  		inc->depth--;
>  	}
> +cleanup:
>  	strbuf_release(&buf);
>  	free(expanded);
>  	return ret;

Quite straight-forward.  At the point of the new "goto cleanup", the
expanded pointer has already been established, so these two calls
will release the right resource.

Thanks.

> diff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh
> index ccbb116c016..5cde79ef8c4 100755
> --- a/t/t1305-config-include.sh
> +++ b/t/t1305-config-include.sh
> @@ -1,6 +1,7 @@
>  #!/bin/sh
>  
>  test_description='test config file include directives'
> +TEST_PASSES_SANITIZE_LEAK=true
>  . ./test-lib.sh
>  
>  # Force setup_explicit_git_dir() to run until the end. This is needed

Re: [PATCH v2 3/3] config.c: free(expanded) before die(), work around GCC oddity

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

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

Yuck.  With or without compiler bugs, this code is too ugly to live,
isn't it?

Re: [PATCH v2 2/3] config.c: don't leak memory in handle_path_include()

[Ævar Arnfjörð Bjarmason]

On Thu, Oct 21 2021, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> Fix a memory leak in the error() path in handle_path_include(), this
>> allows us to run t1305-config-include.sh under SANITIZE=leak,
>> previously 4 tests there would fail. This fixes up a leak in
>> 9b25a0b52e0 (config: add include directive, 2012-02-06).
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>>  config.c                  | 7 +++++--
>>  t/t1305-config-include.sh | 1 +
>>  2 files changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/config.c b/config.c
>> index 2dcbe901b6b..c5873f3a706 100644
>> --- a/config.c
>> +++ b/config.c
>> @@ -148,8 +148,10 @@ static int handle_path_include(const char *path, struct config_include_data *inc
>
> Not a problem introduced by this function, but if you look at this
> change with "git show -W", we'd notice that the function name on the
> hunk header looks strange.  I think we should add a blank line
> before the beginning of the function.

I think this is a bug in -W, after all if without it we we show the
function context line, but with it we advance further, then that means
that -W didn't find the correct function boundary.

[PATCH v3 0/6] usage.c: add die_message() & plug memory leaks in refs.c & config.c

[Ævar Arnfjörð Bjarmason]

What started as a set of small memory leak fixes is now adding and
usinge a die_message() function. This non-fatal-die() is useful to
various callers that want to print "fatal: " before exiting, but don't
want to call die() for whatever reason.

I wasn't planning to submit that now, but these were incomplete
patches I had lying around, and make the 5th and 6th patch much nicer,
in response to comments on v1 and v2 to the effect that managing
free()-ing around die() functions was rather nasty.

This doesn't conflict with anything in-flight, and the changes
themselves are rather simple.

Ævar Arnfjörð Bjarmason (6):
  usage.c: add a die_message() routine
  usage.c API users: use die_message() where appropriate
  usage.c + gc: add and use a die_message_errno()
  config.c: don't leak memory in handle_path_include()
  config.c: free(expanded) before die(), work around GCC oddity
  refs: plug memory leak in repo_default_branch_name()

 builtin/fast-import.c     | 13 +++++----
 builtin/gc.c              | 21 ++++++++------
 builtin/notes.c           |  9 +++---
 config.c                  | 22 ++++++++++-----
 git-compat-util.h         |  4 +++
 http-backend.c            |  3 +-
 parse-options.c           |  2 +-
 refs.c                    |  8 +++++-
 run-command.c             | 16 ++++-------
 t/t1305-config-include.sh |  1 +
 usage.c                   | 58 +++++++++++++++++++++++++++++++--------
 11 files changed, 106 insertions(+), 51 deletions(-)

Range-diff against v2:
-:  ----------- > 1:  fe8763337ed usage.c: add a die_message() routine
-:  ----------- > 2:  dfc3a8fbccb usage.c API users: use die_message() where appropriate
-:  ----------- > 3:  6b33e394b2f usage.c + gc: add and use a die_message_errno()
2:  d6d04da1d9d = 4:  3607b905627 config.c: don't leak memory in handle_path_include()
3:  d812358e331 ! 5:  9a44204c4c9 config.c: free(expanded) before die(), work around GCC oddity
    @@ Commit message
         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
    +    interact with compiler optimization in this way in some cases. 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.
    +    we call die(), using a combination of the "goto cleanup" pattern
    +    introduced in a preceding commit, and the newly introduced
    +    die_message() function.
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
    @@ config.c: static int handle_path_include(const char *path, struct config_include
      	int ret = 0;
      	struct strbuf buf = STRBUF_INIT;
      	char *expanded;
    -+	int die_depth = 0;
    ++	int exit_with = 0;
      
      	if (!path)
      		return config_error_nonbool("include.path");
    @@ config.c: static int handle_path_include(const char *path, struct config_include
     -			    cf->name ? cf->name :
     -			    "the command line");
     +		if (++inc->depth > MAX_INCLUDE_DEPTH) {
    -+			die_depth = 1;
    ++			exit_with = die_message(_(include_depth_advice),
    ++						MAX_INCLUDE_DEPTH, path,
    ++						!cf ? "<unknown>" : cf->name ?
    ++						cf->name : "the command line");
     +			goto cleanup;
     +		}
      		ret = git_config_from_file(git_config_include, path, inc);
    @@ config.c: static int handle_path_include(const char *path, struct config_include
      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");
    ++	if (exit_with)
    ++		exit(exit_with);
    + 	return ret;
      }
      
    - static void add_trailing_starstar_for_dir(struct strbuf *pat)
1:  4f8554bb02e ! 6:  d2f639b53cd refs.c: make "repo_default_branch_name" static, remove xstrfmt()
    @@ Metadata
     Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## Commit message ##
    -    refs.c: make "repo_default_branch_name" static, remove xstrfmt()
    +    refs: plug memory leak in repo_default_branch_name()
     
    -    The repo_default_branch_name() function introduced in
    -    8747ebb7cde (init: allow setting the default for the initial branch
    -    name via the config, 2020-06-24) has never been used outside of this
    -    file, so let's make it static, its sibling function
    -    git_default_branch_name() is what external callers use.
    +    Fix a memory leak in repo_default_branch_name(), we'll leak memory
    +    before exit(128) here.
     
    -    In addition the xstrfmt() to get the "full_ref" in the same commit
    -    isn't needed, we can use the "REFNAME_ALLOW_ONELEVEL" flag to
    -    check_refname_format() instead.
    +    Normally we would not care much about such leaks, we do leak the
    +    memory, as e.g. valgrind(1) will report. But the more commonly used
    +    SANITIZE=leak mode will use GCC and Clang's LSAN mode will not
    +    normally report such leaks.
     
    -    This also happens to fix an issue with c150064dbe2 (leak tests: run
    -    various built-in tests in t00*.sh SANITIZE=leak, 2021-10-12) in "next"
    -    when combined with SANITIZE=leak and higher optimization flags on at
    -    least some GCC versions. See [1].
    +    At least one GCC version does that in this case, and having the tests
    +    fail under -O3 would be annoying, so let's free() the allocated memory
    +    here.
     
    -    1. https://lore.kernel.org/git/patch-1.1-5a47bf2e9c9-20211021T114223Z-avarab@gmail.com/
    +    This uses a new die_message() function introduced in a preceding
    +    commit. That new function makes the flow around such code easier to
    +    manage. In this case we can't free(ret) before the die().
    +
    +    In this case only the "free(full_ref)" appears to be needed, but since
    +    we're freeing one let's free both, some other compiler or version
    +    might arrange this code in such a way as to complain about "ret" too
    +    with SANITIZE=leak, and valgrind(1) will do so in any case.
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## refs.c ##
    -@@ refs.c: static const char default_branch_name_advice[] = N_(
    - "\tgit branch -m <name>\n"
    - );
    - 
    --char *repo_default_branch_name(struct repository *r, int quiet)
    -+static char *repo_default_branch_name(struct repository *r, int quiet)
    - {
    - 	const char *config_key = "init.defaultbranch";
    +@@ refs.c: char *repo_default_branch_name(struct repository *r, int quiet)
      	const char *config_display_key = "init.defaultBranch";
    --	char *ret = NULL, *full_ref;
    -+	char *ret = NULL;
    + 	char *ret = NULL, *full_ref;
      	const char *env = getenv("GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME");
    ++	int exit_with = 0;
      
      	if (env && *env)
    + 		ret = xstrdup(env);
     @@ refs.c: char *repo_default_branch_name(struct repository *r, int quiet)
    - 			advise(_(default_branch_name_advice), ret);
    - 	}
      
    --	full_ref = xstrfmt("refs/heads/%s", ret);
    --	if (check_refname_format(full_ref, 0))
    -+	if (check_refname_format(ret, REFNAME_ALLOW_ONELEVEL))
    - 		die(_("invalid branch name: %s = %s"), config_display_key, ret);
    --	free(full_ref);
    + 	full_ref = xstrfmt("refs/heads/%s", ret);
    + 	if (check_refname_format(full_ref, 0))
    +-		die(_("invalid branch name: %s = %s"), config_display_key, ret);
    ++		exit_with = die_message(_("invalid branch name: %s = %s"),
    ++					config_display_key, ret);
    + 	free(full_ref);
    ++	if (exit_with) {
    ++		free(ret);
    ++		exit(exit_with);
    ++	}
      
      	return ret;
      }
    -
    - ## refs.h ##
    -@@ refs.h: int dwim_log(const char *str, int len, struct object_id *oid, char **ref);
    -  * return value of `git_default_branch_name()` is a singleton.
    -  */
    - const char *git_default_branch_name(int quiet);
    --char *repo_default_branch_name(struct repository *r, int quiet);
    - 
    - /*
    -  * A ref_transaction represents a collection of reference updates that
-- 
2.33.1.1494.g88b39a443e1

[PATCH v3 1/6] usage.c: add a die_message() routine

[Ævar Arnfjörð Bjarmason]

We have code in various places that would like to call die(), but
wants to defer the exit(128) it would invoke, e.g. to print an
additional message, or adjust the exit code. Add a die_message()
helper routine to bridge this gap in the API.

Functionally this behaves just like the error() routine, except it'll
print a "fatal: " prefix, and it will exit with 128 instead of -1,
this is so that caller can pas the return value to exit(128), instead
of having to hardcode "128".

A subsequent commit will migrate various callers that benefit from
this function over to it, for now we're just migrating trivial users
in usage.c itself.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 git-compat-util.h |  3 +++
 usage.c           | 46 ++++++++++++++++++++++++++++++++++------------
 2 files changed, 37 insertions(+), 12 deletions(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index 141bb86351e..c1bb32460b6 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -471,6 +471,7 @@ NORETURN void usage(const char *err);
 NORETURN void usagef(const char *err, ...) __attribute__((format (printf, 1, 2)));
 NORETURN void die(const char *err, ...) __attribute__((format (printf, 1, 2)));
 NORETURN void die_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));
+int die_message(const char *err, ...) __attribute__((format (printf, 1, 2)));
 int error(const char *err, ...) __attribute__((format (printf, 1, 2)));
 int error_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));
 void warning(const char *err, ...) __attribute__((format (printf, 1, 2)));
@@ -505,6 +506,8 @@ static inline int const_error(void)
 typedef void (*report_fn)(const char *, va_list params);
 
 void set_die_routine(NORETURN_PTR report_fn routine);
+void set_die_message_routine(report_fn routine);
+report_fn get_die_message_routine(void);
 void set_error_routine(report_fn routine);
 report_fn get_error_routine(void);
 void set_warn_routine(report_fn routine);
diff --git a/usage.c b/usage.c
index c7d233b0de9..3d4b90bce1f 100644
--- a/usage.c
+++ b/usage.c
@@ -55,6 +55,12 @@ static NORETURN void usage_builtin(const char *err, va_list params)
 	exit(129);
 }
 
+static void die_message_builtin(const char *err, va_list params)
+{
+	trace2_cmd_error_va(err, params);
+	vreportf("fatal: ", err, params);
+}
+
 /*
  * We call trace2_cmd_error_va() in the below functions first and
  * expect it to va_copy 'params' before using it (because an 'ap' can
@@ -62,10 +68,9 @@ static NORETURN void usage_builtin(const char *err, va_list params)
  */
 static NORETURN void die_builtin(const char *err, va_list params)
 {
-	trace2_cmd_error_va(err, params);
-
-	vreportf("fatal: ", err, params);
+	report_fn die_message_fn = get_die_message_routine();
 
+	die_message_fn(err, params);
 	exit(128);
 }
 
@@ -109,6 +114,7 @@ static int die_is_recursing_builtin(void)
  * (ugh), so keep things static. */
 static NORETURN_PTR report_fn usage_routine = usage_builtin;
 static NORETURN_PTR report_fn die_routine = die_builtin;
+static report_fn die_message_routine = die_message_builtin;
 static report_fn error_routine = error_builtin;
 static report_fn warn_routine = warn_builtin;
 static int (*die_is_recursing)(void) = die_is_recursing_builtin;
@@ -118,6 +124,16 @@ void set_die_routine(NORETURN_PTR report_fn routine)
 	die_routine = routine;
 }
 
+void set_die_message_routine(report_fn routine)
+{
+	die_message_routine = routine;
+}
+
+report_fn get_die_message_routine(void)
+{
+	return die_message_routine;
+}
+
 void set_error_routine(report_fn routine)
 {
 	error_routine = routine;
@@ -157,14 +173,23 @@ void NORETURN usage(const char *err)
 	usagef("%s", err);
 }
 
+#undef die_message
+int die_message(const char *err, ...)
+{
+	va_list params;
+
+	va_start(params, err);
+	die_message_routine(err, params);
+	va_end(params);
+	return 128;
+}
+
 void NORETURN die(const char *err, ...)
 {
 	va_list params;
 
-	if (die_is_recursing()) {
-		fputs("fatal: recursion detected in die handler\n", stderr);
-		exit(128);
-	}
+	if (die_is_recursing())
+		exit(die_message("recursion detected in die handler"));
 
 	va_start(params, err);
 	die_routine(err, params);
@@ -200,11 +225,8 @@ void NORETURN die_errno(const char *fmt, ...)
 	char buf[1024];
 	va_list params;
 
-	if (die_is_recursing()) {
-		fputs("fatal: recursion detected in die_errno handler\n",
-			stderr);
-		exit(128);
-	}
+	if (die_is_recursing())
+		exit(die_message("recursion detected in die_errno handler"));
 
 	va_start(params, fmt);
 	die_routine(fmt_with_err(buf, sizeof(buf), fmt), params);
-- 
2.33.1.1494.g88b39a443e1

[PATCH v3 2/6] usage.c API users: use die_message() where appropriate

[Ævar Arnfjörð Bjarmason]

Change code that either called error() and proceeded to exit with 128,
or emitted its own "fatal: " messages to use the die_message()
function added in a preceding commit.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/fast-import.c | 13 ++++++++-----
 builtin/notes.c       |  9 +++++----
 http-backend.c        |  3 ++-
 parse-options.c       |  2 +-
 run-command.c         | 16 +++++-----------
 5 files changed, 21 insertions(+), 22 deletions(-)

diff --git a/builtin/fast-import.c b/builtin/fast-import.c
index 20406f67754..11cd5b0c56c 100644
--- a/builtin/fast-import.c
+++ b/builtin/fast-import.c
@@ -401,16 +401,19 @@ static void dump_marks(void);
 
 static NORETURN void die_nicely(const char *err, va_list params)
 {
+	va_list cp;
 	static int zombie;
-	char message[2 * PATH_MAX];
+	report_fn die_message_fn = get_die_message_routine();
 
-	vsnprintf(message, sizeof(message), err, params);
-	fputs("fatal: ", stderr);
-	fputs(message, stderr);
-	fputc('\n', stderr);
+	if (!zombie)
+		va_copy(cp, params);
+	die_message_fn(err, params);
 
 	if (!zombie) {
+		char message[2 * PATH_MAX];
+
 		zombie = 1;
+		vsnprintf(message, sizeof(message), err, cp);
 		write_crash_report(message);
 		end_packfile();
 		unkeep_all_packs();
diff --git a/builtin/notes.c b/builtin/notes.c
index 71c59583a17..2812d1eac40 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -201,11 +201,12 @@ static void prepare_note_data(const struct object_id *object, struct note_data *
 static void write_note_data(struct note_data *d, struct object_id *oid)
 {
 	if (write_object_file(d->buf.buf, d->buf.len, blob_type, oid)) {
-		error(_("unable to write note object"));
+		int status = die_message(_("unable to write note object"));
+
 		if (d->edit_path)
-			error(_("the note contents have been left in %s"),
-				d->edit_path);
-		exit(128);
+			die_message(_("the note contents have been left in %s"),
+				    d->edit_path);
+		exit(status);
 	}
 }
 
diff --git a/http-backend.c b/http-backend.c
index e7c0eeab230..bc853356e73 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -661,8 +661,9 @@ static NORETURN void die_webcgi(const char *err, va_list params)
 {
 	if (dead <= 1) {
 		struct strbuf hdr = STRBUF_INIT;
+		report_fn die_message_fn = get_die_message_routine();
 
-		vreportf("fatal: ", err, params);
+		die_message_fn(err, params);
 
 		http_status(&hdr, 500, "Internal Server Error");
 		hdr_nocache(&hdr);
diff --git a/parse-options.c b/parse-options.c
index 6e0535bdaad..c892641d9a1 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -1049,7 +1049,7 @@ void NORETURN usage_msg_opt(const char *msg,
 		   const char * const *usagestr,
 		   const struct option *options)
 {
-	fprintf(stderr, "fatal: %s\n\n", msg);
+	die_message("%s\n", msg); /* The extra \n is intentional */
 	usage_with_options(usagestr, options);
 }
 
diff --git a/run-command.c b/run-command.c
index 7ef5cc712a9..220cf53deb4 100644
--- a/run-command.c
+++ b/run-command.c
@@ -340,15 +340,6 @@ static void child_close_pair(int fd[2])
 	child_close(fd[1]);
 }
 
-/*
- * parent will make it look like the child spewed a fatal error and died
- * this is needed to prevent changes to t0061.
- */
-static void fake_fatal(const char *err, va_list params)
-{
-	vreportf("fatal: ", err, params);
-}
-
 static void child_error_fn(const char *err, va_list params)
 {
 	const char msg[] = "error() should not be called in child\n";
@@ -372,9 +363,10 @@ static void NORETURN child_die_fn(const char *err, va_list params)
 static void child_err_spew(struct child_process *cmd, struct child_err *cerr)
 {
 	static void (*old_errfn)(const char *err, va_list params);
+	report_fn die_message_routine = get_die_message_routine();
 
 	old_errfn = get_error_routine();
-	set_error_routine(fake_fatal);
+	set_error_routine(die_message_routine);
 	errno = cerr->syserr;
 
 	switch (cerr->err) {
@@ -1082,7 +1074,9 @@ static void *run_thread(void *data)
 
 static NORETURN void die_async(const char *err, va_list params)
 {
-	vreportf("fatal: ", err, params);
+	report_fn die_message_fn = get_die_message_routine();
+
+	die_message_fn(err, params);
 
 	if (in_async()) {
 		struct async *async = pthread_getspecific(async_key);
-- 
2.33.1.1494.g88b39a443e1

[PATCH v3 3/6] usage.c + gc: add and use a die_message_errno()

[Ævar Arnfjörð Bjarmason]

Change code the "error: " output when we exit with 128 due to gc.log
errors to use a "fatal: " prefix instead. This adds a sibling function
to the die_errno() added in a preceding commit.

Since it returns 128 instead of -1 we'll need to adjust
report_last_gc_error(). Let's adjust it while we're at it to not
conflate the "should skip" and "exit with this non-zero code"
conditions, as the caller is no longer hardcoding "128", but relying
on die_errno() to return a nen-zero exit() status.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/gc.c      | 21 ++++++++++++---------
 git-compat-util.h |  1 +
 usage.c           | 12 ++++++++++++
 3 files changed, 25 insertions(+), 9 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index 6b3de3dd514..f7deef08974 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -472,19 +472,20 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)
  * gc should not proceed due to an error in the last run. Prints a
  * message and returns -1 if an error occurred while reading gc.log
  */
-static int report_last_gc_error(void)
+static int report_last_gc_error(int *skip)
 {
 	struct strbuf sb = STRBUF_INIT;
 	int ret = 0;
 	ssize_t len;
 	struct stat st;
 	char *gc_log_path = git_pathdup("gc.log");
+	*skip = 0;
 
 	if (stat(gc_log_path, &st)) {
 		if (errno == ENOENT)
 			goto done;
 
-		ret = error_errno(_("cannot stat '%s'"), gc_log_path);
+		ret = die_message_errno(_("cannot stat '%s'"), gc_log_path);
 		goto done;
 	}
 
@@ -493,7 +494,7 @@ static int report_last_gc_error(void)
 
 	len = strbuf_read_file(&sb, gc_log_path, 0);
 	if (len < 0)
-		ret = error_errno(_("cannot read '%s'"), gc_log_path);
+		ret = die_message_errno(_("cannot read '%s'"), gc_log_path);
 	else if (len > 0) {
 		/*
 		 * A previous gc failed.  Report the error, and don't
@@ -507,7 +508,7 @@ static int report_last_gc_error(void)
 			       "until the file is removed.\n\n"
 			       "%s"),
 			    gc_log_path, sb.buf);
-		ret = 1;
+		*skip = 1;
 	}
 	strbuf_release(&sb);
 done:
@@ -610,13 +611,15 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 			fprintf(stderr, _("See \"git help gc\" for manual housekeeping.\n"));
 		}
 		if (detach_auto) {
-			int ret = report_last_gc_error();
-			if (ret < 0)
-				/* an I/O error occurred, already reported */
-				exit(128);
-			if (ret == 1)
+			int skip;
+			int ret = report_last_gc_error(&skip);
+
+			if (skip)
 				/* Last gc --auto failed. Skip this one. */
 				return 0;
+			if (ret)
+				/* an error occurred, already reported */
+				exit(ret);
 
 			if (lock_repo_for_gc(force, &pid))
 				return 0;
diff --git a/git-compat-util.h b/git-compat-util.h
index c1bb32460b6..ea0ac80f7db 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -472,6 +472,7 @@ NORETURN void usagef(const char *err, ...) __attribute__((format (printf, 1, 2))
 NORETURN void die(const char *err, ...) __attribute__((format (printf, 1, 2)));
 NORETURN void die_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));
 int die_message(const char *err, ...) __attribute__((format (printf, 1, 2)));
+int die_message_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));
 int error(const char *err, ...) __attribute__((format (printf, 1, 2)));
 int error_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));
 void warning(const char *err, ...) __attribute__((format (printf, 1, 2)));
diff --git a/usage.c b/usage.c
index 3d4b90bce1f..efc2064dde3 100644
--- a/usage.c
+++ b/usage.c
@@ -233,6 +233,18 @@ void NORETURN die_errno(const char *fmt, ...)
 	va_end(params);
 }
 
+#undef die_message_errno
+int die_message_errno(const char *fmt, ...)
+{
+	char buf[1024];
+	va_list params;
+
+	va_start(params, fmt);
+	die_message_routine(fmt_with_err(buf, sizeof(buf), fmt), params);
+	va_end(params);
+	return -1;
+}
+
 #undef error_errno
 int error_errno(const char *fmt, ...)
 {
-- 
2.33.1.1494.g88b39a443e1

[PATCH v3 4/6] config.c: don't leak memory in handle_path_include()

[Ævar Arnfjörð Bjarmason]

Fix a memory leak in the error() path in handle_path_include(), this
allows us to run t1305-config-include.sh under SANITIZE=leak,
previously 4 tests there would fail. This fixes up a leak in
9b25a0b52e0 (config: add include directive, 2012-02-06).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 config.c                  | 7 +++++--
 t/t1305-config-include.sh | 1 +
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/config.c b/config.c
index 2dcbe901b6b..c5873f3a706 100644
--- a/config.c
+++ b/config.c
@@ -148,8 +148,10 @@ static int handle_path_include(const char *path, struct config_include_data *inc
 	if (!is_absolute_path(path)) {
 		char *slash;
 
-		if (!cf || !cf->path)
-			return error(_("relative config includes must come from files"));
+		if (!cf || !cf->path) {
+			ret = error(_("relative config includes must come from files"));
+			goto cleanup;
+		}
 
 		slash = find_last_dir_sep(cf->path);
 		if (slash)
@@ -167,6 +169,7 @@ static int handle_path_include(const char *path, struct config_include_data *inc
 		ret = git_config_from_file(git_config_include, path, inc);
 		inc->depth--;
 	}
+cleanup:
 	strbuf_release(&buf);
 	free(expanded);
 	return ret;
diff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh
index ccbb116c016..5cde79ef8c4 100755
--- a/t/t1305-config-include.sh
+++ b/t/t1305-config-include.sh
@@ -1,6 +1,7 @@
 #!/bin/sh
 
 test_description='test config file include directives'
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 # Force setup_explicit_git_dir() to run until the end. This is needed
-- 
2.33.1.1494.g88b39a443e1

[PATCH v3 5/6] config.c: free(expanded) before die(), work around GCC oddity

[Ævar Arnfjörð Bjarmason]

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

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. 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 a combination of the "goto cleanup" pattern
introduced in a preceding commit, and the newly introduced
die_message() function.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 config.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/config.c b/config.c
index c5873f3a706..c36e85c2077 100644
--- a/config.c
+++ b/config.c
@@ -132,6 +132,7 @@ static int handle_path_include(const char *path, struct config_include_data *inc
 	int ret = 0;
 	struct strbuf buf = STRBUF_INIT;
 	char *expanded;
+	int exit_with = 0;
 
 	if (!path)
 		return config_error_nonbool("include.path");
@@ -161,17 +162,21 @@ 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) {
+			exit_with = die_message(_(include_depth_advice),
+						MAX_INCLUDE_DEPTH, path,
+						!cf ? "<unknown>" : cf->name ?
+						cf->name : "the command line");
+			goto cleanup;
+		}
 		ret = git_config_from_file(git_config_include, path, inc);
 		inc->depth--;
 	}
 cleanup:
 	strbuf_release(&buf);
 	free(expanded);
+	if (exit_with)
+		exit(exit_with);
 	return ret;
 }
 
-- 
2.33.1.1494.g88b39a443e1

[PATCH v3 6/6] refs: plug memory leak in repo_default_branch_name()

[Ævar Arnfjörð Bjarmason]

Fix a memory leak in repo_default_branch_name(), we'll leak memory
before exit(128) here.

Normally we would not care much about such leaks, we do leak the
memory, as e.g. valgrind(1) will report. But the more commonly used
SANITIZE=leak mode will use GCC and Clang's LSAN mode will not
normally report such leaks.

At least one GCC version does that in this case, and having the tests
fail under -O3 would be annoying, so let's free() the allocated memory
here.

This uses a new die_message() function introduced in a preceding
commit. That new function makes the flow around such code easier to
manage. In this case we can't free(ret) before the die().

In this case only the "free(full_ref)" appears to be needed, but since
we're freeing one let's free both, some other compiler or version
might arrange this code in such a way as to complain about "ret" too
with SANITIZE=leak, and valgrind(1) will do so in any case.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 refs.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/refs.c b/refs.c
index 7f019c2377e..2a816c9561d 100644
--- a/refs.c
+++ b/refs.c
@@ -577,6 +577,7 @@ char *repo_default_branch_name(struct repository *r, int quiet)
 	const char *config_display_key = "init.defaultBranch";
 	char *ret = NULL, *full_ref;
 	const char *env = getenv("GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME");
+	int exit_with = 0;
 
 	if (env && *env)
 		ret = xstrdup(env);
@@ -591,8 +592,13 @@ char *repo_default_branch_name(struct repository *r, int quiet)
 
 	full_ref = xstrfmt("refs/heads/%s", ret);
 	if (check_refname_format(full_ref, 0))
-		die(_("invalid branch name: %s = %s"), config_display_key, ret);
+		exit_with = die_message(_("invalid branch name: %s = %s"),
+					config_display_key, ret);
 	free(full_ref);
+	if (exit_with) {
+		free(ret);
+		exit(exit_with);
+	}
 
 	return ret;
 }
-- 
2.33.1.1494.g88b39a443e1

Re: [PATCH v2 2/3] config.c: don't leak memory in handle_path_include()

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

>> Not a problem introduced by this function, but if you look at this
>> change with "git show -W", we'd notice that the function name on the
>> hunk header looks strange.  I think we should add a blank line
>> before the beginning of the function.
>
> I think this is a bug in -W, after all if without it we we show the
> function context line, but with it we advance further, then that means
> that -W didn't find the correct function boundary.

That's a chicken-and-egg argument, and I do not think it is a bug in
"-W" nor the funcname regular expression pattern we use.  We expect
a blank line there and the pattern reflects that expectation, so not
having an expected blank line is what causes this problem.

In any case, we should add a blank linke before the beginning of the
function, and of course that is obviously outside the scope of these
patches.

Re: [PATCH v2 2/3] config.c: don't leak memory in handle_path_include()

[Ævar Arnfjörð Bjarmason]

On Fri, Oct 22 2021, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>>> Not a problem introduced by this function, but if you look at this
>>> change with "git show -W", we'd notice that the function name on the
>>> hunk header looks strange.  I think we should add a blank line
>>> before the beginning of the function.
>>
>> I think this is a bug in -W, after all if without it we we show the
>> function context line, but with it we advance further, then that means
>> that -W didn't find the correct function boundary.
>
> That's a chicken-and-egg argument, and I do not think it is a bug in
> "-W" nor the funcname regular expression pattern we use.  We expect
> a blank line there and the pattern reflects that expectation, so not
> having an expected blank line is what causes this problem.
>
> In any case, we should add a blank linke before the beginning of the
> function, and of course that is obviously outside the scope of these
> patches.

Sort of, if you were running with the patch I posted at [1] you wouldn't
see the bad value at @@, but we still extend upwards with -W, which I
consider a bug.

I.e. both the current context we display and the over-extension there is
ultimately a symptom of the same issue, which is that what we're doing
with -W gets conflated with behavior that makes sense without -W, notice
how if you do "git log -W" on anything that the @@ context we display is
the prototype of the function /above/ the one you're likely looking at
the code change in.

So the blank line is the cause of the over-extension, but we'd still
show the (IMO) incorrect context in either case.

Anyway, as you say a discussion for some other thread. I've been meaning
to get back to those patches at some point, the first problem is that
our test coverage for what function context we should find when is
really lacking, so any changes in that part of the xdiff code are likely
to break things. I had those tests, but they got lost in some
bikeshedding...

1. https://lore.kernel.org/git/20210215155020.2804-2-avarab@gmail.com/

Re: [PATCH v3 1/6] usage.c: add a die_message() routine

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> We have code in various places that would like to call die(), but
> wants to defer the exit(128) it would invoke, e.g. to print an
> additional message, or adjust the exit code. Add a die_message()
> helper routine to bridge this gap in the API.
>
> Functionally this behaves just like the error() routine, except it'll
> print a "fatal: " prefix, and it will exit with 128 instead of -1,

exit with -> return?

> this is so that caller can pas the return value to exit(128), instead
> of having to hardcode "128".

Is it just me or do your patch always have to do about the same
amount of seemingly unnecessary and/or unadvertised changes as the
necessary and/or advertised changes?  I agree that adding
die_message() that returns 128 after giving a message is an
excellent idea, and I can see that it is necessary and sufficient to
achieve the above advertised goal, but I don't see any reason why
set/get_message_routine() must exist, especially as a part of this
step.

IOW, perhaps that half of this patch belongs to and should be
squashed into one of the later steps of this series?

> A subsequent commit will migrate various callers that benefit from
> this function over to it, for now we're just migrating trivial users
> in usage.c itself.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  git-compat-util.h |  3 +++
>  usage.c           | 46 ++++++++++++++++++++++++++++++++++------------
>  2 files changed, 37 insertions(+), 12 deletions(-)
>
> diff --git a/git-compat-util.h b/git-compat-util.h
> index 141bb86351e..c1bb32460b6 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -471,6 +471,7 @@ NORETURN void usage(const char *err);
>  NORETURN void usagef(const char *err, ...) __attribute__((format (printf, 1, 2)));
>  NORETURN void die(const char *err, ...) __attribute__((format (printf, 1, 2)));
>  NORETURN void die_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));
> +int die_message(const char *err, ...) __attribute__((format (printf, 1, 2)));
>  int error(const char *err, ...) __attribute__((format (printf, 1, 2)));
>  int error_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));
>  void warning(const char *err, ...) __attribute__((format (printf, 1, 2)));
> @@ -505,6 +506,8 @@ static inline int const_error(void)
>  typedef void (*report_fn)(const char *, va_list params);
>  
>  void set_die_routine(NORETURN_PTR report_fn routine);
> +void set_die_message_routine(report_fn routine);
> +report_fn get_die_message_routine(void);
>  void set_error_routine(report_fn routine);
>  report_fn get_error_routine(void);
>  void set_warn_routine(report_fn routine);
> diff --git a/usage.c b/usage.c
> index c7d233b0de9..3d4b90bce1f 100644
> --- a/usage.c
> +++ b/usage.c
> @@ -55,6 +55,12 @@ static NORETURN void usage_builtin(const char *err, va_list params)
>  	exit(129);
>  }
>  
> +static void die_message_builtin(const char *err, va_list params)
> +{
> +	trace2_cmd_error_va(err, params);
> +	vreportf("fatal: ", err, params);
> +}
> +
>  /*
>   * We call trace2_cmd_error_va() in the below functions first and
>   * expect it to va_copy 'params' before using it (because an 'ap' can
> @@ -62,10 +68,9 @@ static NORETURN void usage_builtin(const char *err, va_list params)
>   */
>  static NORETURN void die_builtin(const char *err, va_list params)
>  {
> -	trace2_cmd_error_va(err, params);
> -
> -	vreportf("fatal: ", err, params);
> +	report_fn die_message_fn = get_die_message_routine();
>  
> +	die_message_fn(err, params);
>  	exit(128);
>  }
>  
> @@ -109,6 +114,7 @@ static int die_is_recursing_builtin(void)
>   * (ugh), so keep things static. */
>  static NORETURN_PTR report_fn usage_routine = usage_builtin;
>  static NORETURN_PTR report_fn die_routine = die_builtin;
> +static report_fn die_message_routine = die_message_builtin;
>  static report_fn error_routine = error_builtin;
>  static report_fn warn_routine = warn_builtin;
>  static int (*die_is_recursing)(void) = die_is_recursing_builtin;
> @@ -118,6 +124,16 @@ void set_die_routine(NORETURN_PTR report_fn routine)
>  	die_routine = routine;
>  }
>  
> +void set_die_message_routine(report_fn routine)
> +{
> +	die_message_routine = routine;
> +}
> +
> +report_fn get_die_message_routine(void)
> +{
> +	return die_message_routine;
> +}
> +
>  void set_error_routine(report_fn routine)
>  {
>  	error_routine = routine;
> @@ -157,14 +173,23 @@ void NORETURN usage(const char *err)
>  	usagef("%s", err);
>  }
>  
> +#undef die_message
> +int die_message(const char *err, ...)
> +{
> +	va_list params;
> +
> +	va_start(params, err);
> +	die_message_routine(err, params);
> +	va_end(params);
> +	return 128;
> +}
> +
>  void NORETURN die(const char *err, ...)
>  {
>  	va_list params;
>  
> -	if (die_is_recursing()) {
> -		fputs("fatal: recursion detected in die handler\n", stderr);
> -		exit(128);
> -	}
> +	if (die_is_recursing())
> +		exit(die_message("recursion detected in die handler"));
>  
>  	va_start(params, err);
>  	die_routine(err, params);
> @@ -200,11 +225,8 @@ void NORETURN die_errno(const char *fmt, ...)
>  	char buf[1024];
>  	va_list params;
>  
> -	if (die_is_recursing()) {
> -		fputs("fatal: recursion detected in die_errno handler\n",
> -			stderr);
> -		exit(128);
> -	}
> +	if (die_is_recursing())
> +		exit(die_message("recursion detected in die_errno handler"));
>  
>  	va_start(params, fmt);
>  	die_routine(fmt_with_err(buf, sizeof(buf), fmt), params);

Re: [PATCH v3 3/6] usage.c + gc: add and use a die_message_errno()

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Change code the "error: " output when we exit with 128 due to gc.log
> errors to use a "fatal: " prefix instead. This adds a sibling function
> to the die_errno() added in a preceding commit.
>
> Since it returns 128 instead of -1 we'll need to adjust
> report_last_gc_error(). Let's adjust it while we're at it to not
> conflate the "should skip" and "exit with this non-zero code"
> conditions, as the caller is no longer hardcoding "128", but relying
> on die_errno() to return a nen-zero exit() status.

OK, that sort of makes sense, and I am very glad that you didn't add
die_message_errno() to [1/6].  Adding this function to support the
caller that will benefit by using it in this same commit makes quite
a lot of sense.

> diff --git a/usage.c b/usage.c
> index 3d4b90bce1f..efc2064dde3 100644
> --- a/usage.c
> +++ b/usage.c
> @@ -233,6 +233,18 @@ void NORETURN die_errno(const char *fmt, ...)
>  	va_end(params);
>  }
>  
> +#undef die_message_errno
> +int die_message_errno(const char *fmt, ...)
> +{
> +	char buf[1024];
> +	va_list params;
> +
> +	va_start(params, fmt);
> +	die_message_routine(fmt_with_err(buf, sizeof(buf), fmt), params);
> +	va_end(params);
> +	return -1;
> +}
> +
>  #undef error_errno
>  int error_errno(const char *fmt, ...)
>  {

Re: [PATCH v3 4/6] config.c: don't leak memory in handle_path_include()

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Fix a memory leak in the error() path in handle_path_include(), this
> allows us to run t1305-config-include.sh under SANITIZE=leak,
> previously 4 tests there would fail. This fixes up a leak in
> 9b25a0b52e0 (config: add include directive, 2012-02-06).

I think this has been queued already separately and in 'next'.

Re: [PATCH v3 5/6] config.c: free(expanded) before die(), work around GCC oddity

[Jeff King]

On Fri, Oct 22, 2021 at 08:19:38PM +0200, Ævar Arnfjörð Bjarmason wrote:

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

This one really makes me sad. The resulting code is more complicated,
and what guarantee do we have that we won't run into similar problems
with other die() calls?

If we're getting false positives, I'd rather see us work around them
with annotations, or a better compiler (I couldn't reproduce with gcc
10.3.0 or 11.2.0 from Debian, so I doubt there is even much point in
reporting it upstream).

> 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. Since
> this function is called recursively it's going to be especially
> interesting as an optimization target.

I don't see how we have a leak. If we hit this die code-path then we
never exit the function. I can't reproduce the problem, but it sounds
like -O2 is reusing the stack space of "expanded" to prepare for the
die() call? IMHO that is not an actual leak. It is still in scope from
the perspective of C, and anyway we are about to exit from within the
die().

If we were to do anything in the code itself, I'd much prefer to hit it
with an UNLEAK().

-Peff

Re: [PATCH v3 0/6] usage.c: add die_message() & plug memory leaks in refs.c & config.c

[Jonathan Tan]

> What started as a set of small memory leak fixes is now adding and
> usinge a die_message() function. This non-fatal-die() is useful to
> various callers that want to print "fatal: " before exiting, but don't
> want to call die() for whatever reason.
> 
> I wasn't planning to submit that now, but these were incomplete
> patches I had lying around, and make the 5th and 6th patch much nicer,
> in response to comments on v1 and v2 to the effect that managing
> free()-ing around die() functions was rather nasty.
> 
> This doesn't conflict with anything in-flight, and the changes
> themselves are rather simple.

Is this mainly to make a CI work, or just so that a certain set of tools
work? (You mention an old version of GCC in patch 5.) If for CI, I think
that there might be a sufficient version to fix this, but if not, I
would think that something less intrusive would be better (e.g. a
comment that certain versions do not work).

As for patches 5 and 6, I think that any leak detection we use has to
consider any pointers still on the stack as "live" - if not we wouldn't
be able to die without returning back to the topmost function since any
intermediate function could have allocations (unless I'm mistaking
something). (Unless die() is somehow overwriting the stackframe through
some tail call optimization or something - in which case maybe what we
should do is to disable the tail call optimization when we are checking
for leaks.)