| Message ID | patch-1.1-5a47bf2e9c9-20211021T114223Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | leak tests: free() before die for two API functions | expand |
> 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 >
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
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.
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;
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(-)