* [PATCH] leak tests: free() before die for two API functions @ 2021-10-21 11:42 Ævar Arnfjörð Bjarmason 2021-10-21 15:33 ` Andrzej Hunt ` (2 more replies) 0 siblings, 3 replies; 26+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-10-21 11:42 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Æ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 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH] leak tests: free() before die for two API functions 2021-10-21 11:42 [PATCH] leak tests: free() before die for two API functions Ævar Arnfjörð Bjarmason @ 2021-10-21 15:33 ` Andrzej Hunt 2021-10-21 18:51 ` Junio C Hamano 2021-10-21 16:13 ` Martin Ågren 2021-10-21 19:54 ` [PATCH v2 0/3] refs.c + config.c: plug memory leaks Ævar Arnfjörð Bjarmason 2 siblings, 1 reply; 26+ messages in thread From: Andrzej Hunt @ 2021-10-21 15:33 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason; +Cc: git, Junio C Hamano > 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 > ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] leak tests: free() before die for two API functions 2021-10-21 15:33 ` Andrzej Hunt @ 2021-10-21 18:51 ` Junio C Hamano 0 siblings, 0 replies; 26+ messages in thread From: Junio C Hamano @ 2021-10-21 18:51 UTC (permalink / raw) To: Andrzej Hunt; +Cc: Ævar Arnfjörð Bjarmason, git 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. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] leak tests: free() before die for two API functions 2021-10-21 11:42 [PATCH] leak tests: free() before die for two API functions Ævar Arnfjörð Bjarmason 2021-10-21 15:33 ` Andrzej Hunt @ 2021-10-21 16:13 ` Martin Ågren 2021-10-21 19:54 ` [PATCH v2 0/3] refs.c + config.c: plug memory leaks Ævar Arnfjörð Bjarmason 2 siblings, 0 replies; 26+ messages in thread From: Martin Ågren @ 2021-10-21 16:13 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason; +Cc: Git Mailing List, Junio C Hamano 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v2 0/3] refs.c + config.c: plug memory leaks 2021-10-21 11:42 [PATCH] leak tests: free() before die for two API functions Ævar Arnfjörð Bjarmason 2021-10-21 15:33 ` Andrzej Hunt 2021-10-21 16:13 ` Martin Ågren @ 2021-10-21 19:54 ` Ævar Arnfjörð Bjarmason 2021-10-21 19:54 ` [PATCH v2 1/3] refs.c: make "repo_default_branch_name" static, remove xstrfmt() Ævar Arnfjörð Bjarmason ` (3 more replies) 2 siblings, 4 replies; 26+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-10-21 19:54 UTC (permalink / raw) To: git Cc: Junio C Hamano, Andrzej Hunt, Martin Ågren, Æ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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v2 1/3] refs.c: make "repo_default_branch_name" static, remove xstrfmt() 2021-10-21 19:54 ` [PATCH v2 0/3] refs.c + config.c: plug memory leaks Ævar Arnfjörð Bjarmason @ 2021-10-21 19:54 ` Ævar Arnfjörð Bjarmason 2021-10-21 23:26 ` Junio C Hamano 2021-10-21 19:54 ` [PATCH v2 2/3] config.c: don't leak memory in handle_path_include() Ævar Arnfjörð Bjarmason ` (2 subsequent siblings) 3 siblings, 1 reply; 26+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-10-21 19:54 UTC (permalink / raw) To: git Cc: Junio C Hamano, Andrzej Hunt, Martin Ågren, Æ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 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH v2 1/3] refs.c: make "repo_default_branch_name" static, remove xstrfmt() 2021-10-21 19:54 ` [PATCH v2 1/3] refs.c: make "repo_default_branch_name" static, remove xstrfmt() Ævar Arnfjörð Bjarmason @ 2021-10-21 23:26 ` Junio C Hamano 0 siblings, 0 replies; 26+ messages in thread From: Junio C Hamano @ 2021-10-21 23:26 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: git, Andrzej Hunt, Martin Ågren Æ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. ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v2 2/3] config.c: don't leak memory in handle_path_include() 2021-10-21 19:54 ` [PATCH v2 0/3] refs.c + config.c: plug memory leaks Ævar Arnfjörð Bjarmason 2021-10-21 19:54 ` [PATCH v2 1/3] refs.c: make "repo_default_branch_name" static, remove xstrfmt() Ævar Arnfjörð Bjarmason @ 2021-10-21 19:54 ` Ævar Arnfjörð Bjarmason 2021-10-21 23:30 ` Junio C Hamano 2021-10-21 19:54 ` [PATCH v2 3/3] config.c: free(expanded) before die(), work around GCC oddity Ævar Arnfjörð Bjarmason 2021-10-22 18:19 ` [PATCH v3 0/6] usage.c: add die_message() & plug memory leaks in refs.c & config.c Ævar Arnfjörð Bjarmason 3 siblings, 1 reply; 26+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-10-21 19:54 UTC (permalink / raw) To: git Cc: Junio C Hamano, Andrzej Hunt, Martin Ågren, Æ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 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH v2 2/3] config.c: don't leak memory in handle_path_include() 2021-10-21 19:54 ` [PATCH v2 2/3] config.c: don't leak memory in handle_path_include() Ævar Arnfjörð Bjarmason @ 2021-10-21 23:30 ` Junio C Hamano 2021-10-22 17:19 ` Ævar Arnfjörð Bjarmason 0 siblings, 1 reply; 26+ messages in thread From: Junio C Hamano @ 2021-10-21 23:30 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: git, Andrzej Hunt, Martin Ågren Æ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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 2/3] config.c: don't leak memory in handle_path_include() 2021-10-21 23:30 ` Junio C Hamano @ 2021-10-22 17:19 ` Ævar Arnfjörð Bjarmason 2021-10-22 21:21 ` Junio C Hamano 0 siblings, 1 reply; 26+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-10-22 17:19 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Andrzej Hunt, Martin Ågren 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. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 2/3] config.c: don't leak memory in handle_path_include() 2021-10-22 17:19 ` Ævar Arnfjörð Bjarmason @ 2021-10-22 21:21 ` Junio C Hamano 2021-10-22 22:30 ` Ævar Arnfjörð Bjarmason 0 siblings, 1 reply; 26+ messages in thread From: Junio C Hamano @ 2021-10-22 21:21 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: git, Andrzej Hunt, Martin Ågren Æ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. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 2/3] config.c: don't leak memory in handle_path_include() 2021-10-22 21:21 ` Junio C Hamano @ 2021-10-22 22:30 ` Ævar Arnfjörð Bjarmason 0 siblings, 0 replies; 26+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-10-22 22:30 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Andrzej Hunt, Martin Ågren, René Scharfe 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/ ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v2 3/3] config.c: free(expanded) before die(), work around GCC oddity 2021-10-21 19:54 ` [PATCH v2 0/3] refs.c + config.c: plug memory leaks Ævar Arnfjörð Bjarmason 2021-10-21 19:54 ` [PATCH v2 1/3] refs.c: make "repo_default_branch_name" static, remove xstrfmt() Ævar Arnfjörð Bjarmason 2021-10-21 19:54 ` [PATCH v2 2/3] config.c: don't leak memory in handle_path_include() Ævar Arnfjörð Bjarmason @ 2021-10-21 19:54 ` Ævar Arnfjörð Bjarmason 2021-10-21 23:32 ` Junio C Hamano 2021-10-22 18:19 ` [PATCH v3 0/6] usage.c: add die_message() & plug memory leaks in refs.c & config.c Ævar Arnfjörð Bjarmason 3 siblings, 1 reply; 26+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-10-21 19:54 UTC (permalink / raw) To: git Cc: Junio C Hamano, Andrzej Hunt, Martin Ågren, Æ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 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH v2 3/3] config.c: free(expanded) before die(), work around GCC oddity 2021-10-21 19:54 ` [PATCH v2 3/3] config.c: free(expanded) before die(), work around GCC oddity Ævar Arnfjörð Bjarmason @ 2021-10-21 23:32 ` Junio C Hamano 0 siblings, 0 replies; 26+ messages in thread From: Junio C Hamano @ 2021-10-21 23:32 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: git, Andrzej Hunt, Martin Ågren Æ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? ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v3 0/6] usage.c: add die_message() & plug memory leaks in refs.c & config.c 2021-10-21 19:54 ` [PATCH v2 0/3] refs.c + config.c: plug memory leaks Ævar Arnfjörð Bjarmason ` (2 preceding siblings ...) 2021-10-21 19:54 ` [PATCH v2 3/3] config.c: free(expanded) before die(), work around GCC oddity Ævar Arnfjörð Bjarmason @ 2021-10-22 18:19 ` Ævar Arnfjörð Bjarmason 2021-10-22 18:19 ` [PATCH v3 1/6] usage.c: add a die_message() routine Ævar Arnfjörð Bjarmason ` (6 more replies) 3 siblings, 7 replies; 26+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-10-22 18:19 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, Andrzej Hunt, Martin Ågren, Æ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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v3 1/6] usage.c: add a die_message() routine 2021-10-22 18:19 ` [PATCH v3 0/6] usage.c: add die_message() & plug memory leaks in refs.c & config.c Ævar Arnfjörð Bjarmason @ 2021-10-22 18:19 ` Ævar Arnfjörð Bjarmason 2021-10-24 5:49 ` Junio C Hamano 2021-10-22 18:19 ` [PATCH v3 2/6] usage.c API users: use die_message() where appropriate Ævar Arnfjörð Bjarmason ` (5 subsequent siblings) 6 siblings, 1 reply; 26+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-10-22 18:19 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, Andrzej Hunt, Martin Ågren, Æ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 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH v3 1/6] usage.c: add a die_message() routine 2021-10-22 18:19 ` [PATCH v3 1/6] usage.c: add a die_message() routine Ævar Arnfjörð Bjarmason @ 2021-10-24 5:49 ` Junio C Hamano 0 siblings, 0 replies; 26+ messages in thread From: Junio C Hamano @ 2021-10-24 5:49 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: git, Jeff King, Andrzej Hunt, Martin Ågren Æ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); ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v3 2/6] usage.c API users: use die_message() where appropriate 2021-10-22 18:19 ` [PATCH v3 0/6] usage.c: add die_message() & plug memory leaks in refs.c & config.c Ævar Arnfjörð Bjarmason 2021-10-22 18:19 ` [PATCH v3 1/6] usage.c: add a die_message() routine Ævar Arnfjörð Bjarmason @ 2021-10-22 18:19 ` Ævar Arnfjörð Bjarmason 2021-10-22 18:19 ` [PATCH v3 3/6] usage.c + gc: add and use a die_message_errno() Ævar Arnfjörð Bjarmason ` (4 subsequent siblings) 6 siblings, 0 replies; 26+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-10-22 18:19 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, Andrzej Hunt, Martin Ågren, Æ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 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v3 3/6] usage.c + gc: add and use a die_message_errno() 2021-10-22 18:19 ` [PATCH v3 0/6] usage.c: add die_message() & plug memory leaks in refs.c & config.c Ævar Arnfjörð Bjarmason 2021-10-22 18:19 ` [PATCH v3 1/6] usage.c: add a die_message() routine Ævar Arnfjörð Bjarmason 2021-10-22 18:19 ` [PATCH v3 2/6] usage.c API users: use die_message() where appropriate Ævar Arnfjörð Bjarmason @ 2021-10-22 18:19 ` Ævar Arnfjörð Bjarmason 2021-10-24 5:52 ` Junio C Hamano 2021-10-22 18:19 ` [PATCH v3 4/6] config.c: don't leak memory in handle_path_include() Ævar Arnfjörð Bjarmason ` (3 subsequent siblings) 6 siblings, 1 reply; 26+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-10-22 18:19 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, Andrzej Hunt, Martin Ågren, Æ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 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH v3 3/6] usage.c + gc: add and use a die_message_errno() 2021-10-22 18:19 ` [PATCH v3 3/6] usage.c + gc: add and use a die_message_errno() Ævar Arnfjörð Bjarmason @ 2021-10-24 5:52 ` Junio C Hamano 0 siblings, 0 replies; 26+ messages in thread From: Junio C Hamano @ 2021-10-24 5:52 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: git, Jeff King, Andrzej Hunt, Martin Ågren Æ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, ...) > { ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v3 4/6] config.c: don't leak memory in handle_path_include() 2021-10-22 18:19 ` [PATCH v3 0/6] usage.c: add die_message() & plug memory leaks in refs.c & config.c Ævar Arnfjörð Bjarmason ` (2 preceding siblings ...) 2021-10-22 18:19 ` [PATCH v3 3/6] usage.c + gc: add and use a die_message_errno() Ævar Arnfjörð Bjarmason @ 2021-10-22 18:19 ` Ævar Arnfjörð Bjarmason 2021-10-24 5:53 ` Junio C Hamano 2021-10-22 18:19 ` [PATCH v3 5/6] config.c: free(expanded) before die(), work around GCC oddity Ævar Arnfjörð Bjarmason ` (2 subsequent siblings) 6 siblings, 1 reply; 26+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-10-22 18:19 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, Andrzej Hunt, Martin Ågren, Æ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 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH v3 4/6] config.c: don't leak memory in handle_path_include() 2021-10-22 18:19 ` [PATCH v3 4/6] config.c: don't leak memory in handle_path_include() Ævar Arnfjörð Bjarmason @ 2021-10-24 5:53 ` Junio C Hamano 0 siblings, 0 replies; 26+ messages in thread From: Junio C Hamano @ 2021-10-24 5:53 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: git, Jeff King, Andrzej Hunt, Martin Ågren Æ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'. ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v3 5/6] config.c: free(expanded) before die(), work around GCC oddity 2021-10-22 18:19 ` [PATCH v3 0/6] usage.c: add die_message() & plug memory leaks in refs.c & config.c Ævar Arnfjörð Bjarmason ` (3 preceding siblings ...) 2021-10-22 18:19 ` [PATCH v3 4/6] config.c: don't leak memory in handle_path_include() Ævar Arnfjörð Bjarmason @ 2021-10-22 18:19 ` Ævar Arnfjörð Bjarmason 2021-10-26 8:53 ` Jeff King 2021-10-22 18:19 ` [PATCH v3 6/6] refs: plug memory leak in repo_default_branch_name() Ævar Arnfjörð Bjarmason 2021-10-27 21:50 ` [PATCH v3 0/6] usage.c: add die_message() & plug memory leaks in refs.c & config.c Jonathan Tan 6 siblings, 1 reply; 26+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-10-22 18:19 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, Andrzej Hunt, Martin Ågren, Æ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 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH v3 5/6] config.c: free(expanded) before die(), work around GCC oddity 2021-10-22 18:19 ` [PATCH v3 5/6] config.c: free(expanded) before die(), work around GCC oddity Ævar Arnfjörð Bjarmason @ 2021-10-26 8:53 ` Jeff King 0 siblings, 0 replies; 26+ messages in thread From: Jeff King @ 2021-10-26 8:53 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: git, Junio C Hamano, Andrzej Hunt, Martin Ågren 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v3 6/6] refs: plug memory leak in repo_default_branch_name() 2021-10-22 18:19 ` [PATCH v3 0/6] usage.c: add die_message() & plug memory leaks in refs.c & config.c Ævar Arnfjörð Bjarmason ` (4 preceding siblings ...) 2021-10-22 18:19 ` [PATCH v3 5/6] config.c: free(expanded) before die(), work around GCC oddity Ævar Arnfjörð Bjarmason @ 2021-10-22 18:19 ` Ævar Arnfjörð Bjarmason 2021-10-27 21:50 ` [PATCH v3 0/6] usage.c: add die_message() & plug memory leaks in refs.c & config.c Jonathan Tan 6 siblings, 0 replies; 26+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-10-22 18:19 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, Andrzej Hunt, Martin Ågren, Æ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 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH v3 0/6] usage.c: add die_message() & plug memory leaks in refs.c & config.c 2021-10-22 18:19 ` [PATCH v3 0/6] usage.c: add die_message() & plug memory leaks in refs.c & config.c Ævar Arnfjörð Bjarmason ` (5 preceding siblings ...) 2021-10-22 18:19 ` [PATCH v3 6/6] refs: plug memory leak in repo_default_branch_name() Ævar Arnfjörð Bjarmason @ 2021-10-27 21:50 ` Jonathan Tan 6 siblings, 0 replies; 26+ messages in thread From: Jonathan Tan @ 2021-10-27 21:50 UTC (permalink / raw) To: avarab; +Cc: git, gitster, peff, andrzej, martin.agren, 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.) ^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2021-10-27 21:50 UTC | newest] Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-10-21 11:42 [PATCH] leak tests: free() before die for two API functions Ævar Arnfjörð Bjarmason 2021-10-21 15:33 ` Andrzej Hunt 2021-10-21 18:51 ` Junio C Hamano 2021-10-21 16:13 ` Martin Ågren 2021-10-21 19:54 ` [PATCH v2 0/3] refs.c + config.c: plug memory leaks Ævar Arnfjörð Bjarmason 2021-10-21 19:54 ` [PATCH v2 1/3] refs.c: make "repo_default_branch_name" static, remove xstrfmt() Ævar Arnfjörð Bjarmason 2021-10-21 23:26 ` Junio C Hamano 2021-10-21 19:54 ` [PATCH v2 2/3] config.c: don't leak memory in handle_path_include() Ævar Arnfjörð Bjarmason 2021-10-21 23:30 ` Junio C Hamano 2021-10-22 17:19 ` Ævar Arnfjörð Bjarmason 2021-10-22 21:21 ` Junio C Hamano 2021-10-22 22:30 ` Ævar Arnfjörð Bjarmason 2021-10-21 19:54 ` [PATCH v2 3/3] config.c: free(expanded) before die(), work around GCC oddity Ævar Arnfjörð Bjarmason 2021-10-21 23:32 ` Junio C Hamano 2021-10-22 18:19 ` [PATCH v3 0/6] usage.c: add die_message() & plug memory leaks in refs.c & config.c Ævar Arnfjörð Bjarmason 2021-10-22 18:19 ` [PATCH v3 1/6] usage.c: add a die_message() routine Ævar Arnfjörð Bjarmason 2021-10-24 5:49 ` Junio C Hamano 2021-10-22 18:19 ` [PATCH v3 2/6] usage.c API users: use die_message() where appropriate Ævar Arnfjörð Bjarmason 2021-10-22 18:19 ` [PATCH v3 3/6] usage.c + gc: add and use a die_message_errno() Ævar Arnfjörð Bjarmason 2021-10-24 5:52 ` Junio C Hamano 2021-10-22 18:19 ` [PATCH v3 4/6] config.c: don't leak memory in handle_path_include() Ævar Arnfjörð Bjarmason 2021-10-24 5:53 ` Junio C Hamano 2021-10-22 18:19 ` [PATCH v3 5/6] config.c: free(expanded) before die(), work around GCC oddity Ævar Arnfjörð Bjarmason 2021-10-26 8:53 ` Jeff King 2021-10-22 18:19 ` [PATCH v3 6/6] refs: plug memory leak in repo_default_branch_name() Ævar Arnfjörð Bjarmason 2021-10-27 21:50 ` [PATCH v3 0/6] usage.c: add die_message() & plug memory leaks in refs.c & config.c Jonathan Tan
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.