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 <redacted>
---
config.c | 4 +++-
refs.c | 5 ++++-
2 files changed, 7 insertions(+), 2 deletions(-)
From: Andrzej Hunt <hidden> Date: 2021-10-21 15:33:47
On 21 Oct 2021, at 13:42, Ævar Arnfjörð Bjarmason [off-list ref] 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.)
quoted hunk
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 <redacted>
---
config.c | 4 +++-
refs.c | 5 ++++-
2 files changed, 7 insertions(+), 2 deletions(-)
From: Martin Ågren <hidden> Date: 2021-10-21 16:13:17
On Thu, 21 Oct 2021 at 13:43, Ævar Arnfjörð Bjarmason [off-list ref] 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.
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
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 [off-list ref]
## 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 [off-list ref]
## 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
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 <redacted>
---
config.c | 7 +++++--
t/t1305-config-include.sh | 1 +
2 files changed, 6 insertions(+), 2 deletions(-)
@@ -148,8 +148,10 @@ static int handle_path_include(const char *path, struct config_include_data *incif(!is_absolute_path(path)){char*slash;-if(!cf||!cf->path)-returnerror(_("relative config includes must come from files"));+if(!cf||!cf->path){+ret=error(_("relative config includes must come from files"));+gotocleanup;+}slash=find_last_dir_sep(cf->path);if(slash)
@@ -1,6 +1,7 @@#!/bin/shtest_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
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 <redacted>
---
refs.c | 8 +++-----
refs.h | 1 -
2 files changed, 3 insertions(+), 6 deletions(-)
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 <redacted>
---
config.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
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 [off-list ref]
@@ 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 [off-list ref]
## 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 [off-list ref]
## 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
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 <redacted>
---
git-compat-util.h | 3 +++
usage.c | 46 ++++++++++++++++++++++++++++++++++------------
2 files changed, 37 insertions(+), 12 deletions(-)
@@ -157,14 +173,23 @@ void NORETURN usage(const char *err)usagef("%s",err);}+#undef die_message+intdie_message(constchar*err,...)+{+va_listparams;++va_start(params,err);+die_message_routine(err,params);+va_end(params);+return128;+}+voidNORETURNdie(constchar*err,...){va_listparams;-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, ...)charbuf[1024];va_listparams;-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);
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 <redacted>
---
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(-)
@@ -201,11 +201,12 @@ static void prepare_note_data(const struct object_id *object, struct note_data *staticvoidwrite_note_data(structnote_data*d,structobject_id*oid){if(write_object_file(d->buf.buf,d->buf.len,blob_type,oid)){-error(_("unable to write note object"));+intstatus=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);}}
@@ -340,15 +340,6 @@ static void child_close_pair(int fd[2])child_close(fd[1]);}-/*-*parentwillmakeitlooklikethechildspewedafatalerroranddied-*thisisneededtopreventchangestot0061.-*/-staticvoidfake_fatal(constchar*err,va_listparams)-{-vreportf("fatal: ",err,params);-}-staticvoidchild_error_fn(constchar*err,va_listparams){constcharmsg[]="error() should not be called in child\n";
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 <redacted>
---
builtin/gc.c | 21 ++++++++++++---------
git-compat-util.h | 1 +
usage.c | 12 ++++++++++++
3 files changed, 25 insertions(+), 9 deletions(-)
@@ -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){-intret=report_last_gc_error();-if(ret<0)-/* an I/O error occurred, already reported */-exit(128);-if(ret==1)+intskip;+intret=report_last_gc_error(&skip);++if(skip)/* Last gc --auto failed. Skip this one. */return0;+if(ret)+/* an error occurred, already reported */+exit(ret);if(lock_repo_for_gc(force,&pid))return0;
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 <redacted>
---
config.c | 7 +++++--
t/t1305-config-include.sh | 1 +
2 files changed, 6 insertions(+), 2 deletions(-)
@@ -148,8 +148,10 @@ static int handle_path_include(const char *path, struct config_include_data *incif(!is_absolute_path(path)){char*slash;-if(!cf||!cf->path)-returnerror(_("relative config includes must come from files"));+if(!cf||!cf->path){+ret=error(_("relative config includes must come from files"));+gotocleanup;+}slash=find_last_dir_sep(cf->path);if(slash)
@@ -1,6 +1,7 @@#!/bin/shtest_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
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 <redacted>
---
config.c | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
From: Jeff King <hidden> Date: 2021-10-26 08:53:55
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
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 <redacted>
---
refs.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
From: Jonathan Tan <hidden> Date: 2021-10-27 21:50:58
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.)