{"thread":{"id":"56746","subject":"[PATCH] leak tests: free() before die for two API functions","startedAt":"2021-10-21T11:42:38Z","lastAt":"2021-10-27T21:50:58Z","messageCount":26,"participants":["Ævar Arnfjörð Bjarmason","Andrzej Hunt","Martin Ågren","Junio C Hamano","Jeff King","Jonathan Tan"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"439185","messageId":"patch-1.1-5a47bf2e9c9-20211021T114223Z-avarab@gmail.com","threadId":"56746","inReplyTo":null,"subject":"[PATCH] leak tests: free() before die for two API functions","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-21T11:42:29Z","receivedAt":"2021-10-21T11:42:38Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Call free() just before die() in two API functions whose tests are\nasserted under SANITIZE=leak. Normally this would not be needed due to\nhow SANITIZE=leak works, but in these cases my GCC version (10.2.1-6)\nwill fail tests t0001 and t0017 under SANITIZE=leak depending on the\noptimization level.\n\nSee 956d2e4639b (tests: add a test mode for SANITIZE=leak, run it in\nCI, 2021-09-23) for the commit that marked t0017 for testing with\nSANITIZE=leak, and c150064dbe2 (leak tests: run various built-in tests\nin t00*.sh SANITIZE=leak, 2021-10-12) for t0001 (currently in \"next\").\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n config.c | 4 +++-\n refs.c   | 5 ++++-\n 2 files changed, 7 insertions(+), 2 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex 2dcbe901b6b..93979d39b21 100644\n--- a/config.c\n+++ b/config.c\n@@ -159,11 +159,13 @@ static int handle_path_include(const char *path, struct config_include_data *inc\n \t}\n \n \tif (!access_or_die(path, R_OK, 0)) {\n-\t\tif (++inc->depth > MAX_INCLUDE_DEPTH)\n+\t\tif (++inc->depth > MAX_INCLUDE_DEPTH) {\n+\t\t\tfree(expanded);\n \t\t\tdie(_(include_depth_advice), MAX_INCLUDE_DEPTH, path,\n \t\t\t    !cf ? \"<unknown>\" :\n \t\t\t    cf->name ? cf->name :\n \t\t\t    \"the command line\");\n+\t\t}\n \t\tret = git_config_from_file(git_config_include, path, inc);\n \t\tinc->depth--;\n \t}\ndiff --git a/refs.c b/refs.c\nindex 7f019c2377e..52929286032 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -590,8 +590,11 @@ char *repo_default_branch_name(struct repository *r, int quiet)\n \t}\n \n \tfull_ref = xstrfmt(\"refs/heads/%s\", ret);\n-\tif (check_refname_format(full_ref, 0))\n+\tif (check_refname_format(full_ref, 0)) {\n+\t\tfree(ret);\n+\t\tfree(full_ref);\n \t\tdie(_(\"invalid branch name: %s = %s\"), config_display_key, ret);\n+\t}\n \tfree(full_ref);\n \n \treturn ret;\n-- \n2.33.1.1486.gb2bc4955b90\n\n"},{"id":"439240","messageId":"FD837FF9-E83F-42AA-AC13-EADD161D20BE@ahunt.org","threadId":"56746","inReplyTo":"patch-1.1-5a47bf2e9c9-20211021T114223Z-avarab@gmail.com","subject":"Re: [PATCH] leak tests: free() before die for two API functions","fromName":"Andrzej Hunt","fromEmail":"andrzej@ahunt.org","sentAt":"2021-10-21T15:33:41Z","receivedAt":"2021-10-21T15:33:47Z","isPatch":true,"sender":{"key":"andrzej@ahunt.org","avatar":"https://avatars.githubusercontent.com/u/1546915?v=4"},"body":"\n\n> On 21 Oct 2021, at 13:42, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:\n> \n> Call free() just before die() in two API functions whose tests are\n> asserted under SANITIZE=leak. Normally this would not be needed due to\n> how SANITIZE=leak works, but in these cases my GCC version (10.2.1-6)\n> will fail tests t0001 and t0017 under SANITIZE=leak depending on the\n> optimization level.\n\nI’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.\n\nThese 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”?)\n\nIf 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?\n\n(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.)\n\n> \n> See 956d2e4639b (tests: add a test mode for SANITIZE=leak, run it in\n> CI, 2021-09-23) for the commit that marked t0017 for testing with\n> SANITIZE=leak, and c150064dbe2 (leak tests: run various built-in tests\n> in t00*.sh SANITIZE=leak, 2021-10-12) for t0001 (currently in \"next\").\n> \n> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n> ---\n> config.c | 4 +++-\n> refs.c   | 5 ++++-\n> 2 files changed, 7 insertions(+), 2 deletions(-)\n> \n> diff --git a/config.c b/config.c\n> index 2dcbe901b6b..93979d39b21 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -159,11 +159,13 @@ static int handle_path_include(const char *path, struct config_include_data *inc\n>    }\n> \n>    if (!access_or_die(path, R_OK, 0)) {\n> -        if (++inc->depth > MAX_INCLUDE_DEPTH)\n> +        if (++inc->depth > MAX_INCLUDE_DEPTH) {\n> +            free(expanded);\n>            die(_(include_depth_advice), MAX_INCLUDE_DEPTH, path,\n>                !cf ? \"<unknown>\" :\n>                cf->name ? cf->name :\n>                \"the command line\");\n> +        }\n>        ret = git_config_from_file(git_config_include, path, inc);\n>        inc->depth--;\n>    }\n> diff --git a/refs.c b/refs.c\n> index 7f019c2377e..52929286032 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -590,8 +590,11 @@ char *repo_default_branch_name(struct repository *r, int quiet)\n>    }\n> \n>    full_ref = xstrfmt(\"refs/heads/%s\", ret);\n> -    if (check_refname_format(full_ref, 0))\n> +    if (check_refname_format(full_ref, 0)) {\n> +        free(ret);\n> +        free(full_ref);\n>        die(_(\"invalid branch name: %s = %s\"), config_display_key, ret);\n> +    }\n>    free(full_ref);\n> \n>    return ret;\n> -- \n> 2.33.1.1486.gb2bc4955b90\n> \n\n"},{"id":"439248","messageId":"CAN0heSqA4uqWahWKTa0ZLBCnFLG4jxEx+18aVQUieX0f6dzWMw@mail.gmail.com","threadId":"56746","inReplyTo":"patch-1.1-5a47bf2e9c9-20211021T114223Z-avarab@gmail.com","subject":"Re: [PATCH] leak tests: free() before die for two API functions","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2021-10-21T16:13:00Z","receivedAt":"2021-10-21T16:13:17Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Thu, 21 Oct 2021 at 13:43, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:\n>\n> Call free() just before die() in two API functions whose tests are\n> asserted under SANITIZE=leak. Normally this would not be needed due to\n> how SANITIZE=leak works, but in these cases my GCC version (10.2.1-6)\n> will fail tests t0001 and t0017 under SANITIZE=leak depending on the\n> optimization level.\n\nSeems a bit unfortunate. I have to wonder why these in particular\ntrigger this compiler bug or whatever it is, but oh well.\n\n> -       if (check_refname_format(full_ref, 0))\n> +       if (check_refname_format(full_ref, 0)) {\n> +               free(ret);\n> +               free(full_ref);\n>                 die(_(\"invalid branch name: %s = %s\"), config_display_key, ret);\n> +       }\n>         free(full_ref);\n\nThis looks like use-after-free. Rather than complicating this by, e.g.,\nfirst formatting the string, then freeing `ret`, then dying, could we --\nif we really want this workaround -- make the workaround be `UNLEAK`\ninstead?\n\nAlso, if we do something like this patch, I think we should try to avoid\nthis free-before-die then being cargo-culted all across the codebase.\nHow about\n\n  UNLEAK(ret); /* work around compiler bug */\n  UNLEAK(full_ref); /* work around compiler bug */\n\nor something?\n\nMartin\n"},{"id":"439266","messageId":"xmqqcznyky8b.fsf@gitster.g","threadId":"56746","inReplyTo":"FD837FF9-E83F-42AA-AC13-EADD161D20BE@ahunt.org","subject":"Re: [PATCH] leak tests: free() before die for two API functions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-21T18:51:32Z","receivedAt":"2021-10-21T18:51:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Andrzej Hunt <andrzej@ahunt.org> writes:\n\n>> On 21 Oct 2021, at 13:42, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:\n>> \n>> Call free() just before die() in two API functions whose tests are\n>> asserted under SANITIZE=leak. Normally this would not be needed due to\n>> how SANITIZE=leak works, but in these cases my GCC version (10.2.1-6)\n>> will fail tests t0001 and t0017 under SANITIZE=leak depending on the\n>> optimization level.\n>\n> I’m curious - to me this seems like a compiler/sanitiser bug, can\n> it also be reproduced with clang, or even newer versions of gcc?\n> Similarly, can it be reproduced with your gcc version, using\n> ASAN+LSAN (as opposed to LSAN by itself)? I remember seeing some\n> false positives in the past for some permutations of compilers and\n> sanitisers, but I’ve lost track of the details.\n>\n> These kinds of fixes seem noisy if it’s just to work around what\n> appears to be a bug (and to be philosophical: we wouldn’t want to\n> do the same for all “leaks” up the call stack if a specific\n> compiler complained about them after a die() - after all there\n> will be many more allocations that didn’t get free’d floating\n> around - so why is it OK for these “leaks”?)\n\nExactly my feeling.  I'll leave this patch hanging on the list\nwithout picking it up until we know this is a reasonable \"fix\" on\nour side and not adding noize only to work around the bug in the\ntools.\n\nThanks.\n"},{"id":"439267","messageId":"cover-v2-0.3-00000000000-20211021T195133Z-avarab@gmail.com","threadId":"56746","inReplyTo":"patch-1.1-5a47bf2e9c9-20211021T114223Z-avarab@gmail.com","subject":"[PATCH v2 0/3] refs.c + config.c: plug memory leaks","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-21T19:54:12Z","receivedAt":"2021-10-21T19:54:22Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"In response to the feedback on v1 on this (in particular the\nuse-after-free, thanks Martin!) here's a v2 which I think is a good\nthing to do with our without that particular GCC behavior I ran into.\n\nAs noted in 3/3 I think this is a known caveat of those SANITIZE=\nmodes, e.g. valgrind reports a memory leak regardless of optimization\nlevel.\n\nThe only pure workaround for the issue is now 3/3, which I think is a\nworthwhile to carry to avoid developer potentially wasting time on it.\n\nÆvar Arnfjörð Bjarmason (3):\n  refs.c: make \"repo_default_branch_name\" static, remove xstrfmt()\n  config.c: don't leak memory in handle_path_include()\n  config.c: free(expanded) before die(), work around GCC oddity\n\n config.c                  | 22 ++++++++++++++--------\n refs.c                    |  8 +++-----\n refs.h                    |  1 -\n t/t1305-config-include.sh |  1 +\n 4 files changed, 18 insertions(+), 14 deletions(-)\n\nRange-diff against v1:\n-:  ----------- > 1:  4f8554bb02e refs.c: make \"repo_default_branch_name\" static, remove xstrfmt()\n-:  ----------- > 2:  d6d04da1d9d config.c: don't leak memory in handle_path_include()\n1:  5a47bf2e9c9 ! 3:  d812358e331 leak tests: free() before die for two API functions\n    @@ Metadata\n     Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n     \n      ## Commit message ##\n    -    leak tests: free() before die for two API functions\n    +    config.c: free(expanded) before die(), work around GCC oddity\n     \n    -    Call free() just before die() in two API functions whose tests are\n    -    asserted under SANITIZE=leak. Normally this would not be needed due to\n    -    how SANITIZE=leak works, but in these cases my GCC version (10.2.1-6)\n    -    will fail tests t0001 and t0017 under SANITIZE=leak depending on the\n    -    optimization level.\n    +    On my GCC version (10.2.1-6), but not the clang I have available t0017\n    +    will fail under SANITIZE=leak on optimization levels higher than -O0,\n    +    which is annoying when combined with the change in 956d2e4639b (tests:\n    +    add a test mode for SANITIZE=leak, run it in CI, 2021-09-23).\n     \n    -    See 956d2e4639b (tests: add a test mode for SANITIZE=leak, run it in\n    -    CI, 2021-09-23) for the commit that marked t0017 for testing with\n    -    SANITIZE=leak, and c150064dbe2 (leak tests: run various built-in tests\n    -    in t00*.sh SANITIZE=leak, 2021-10-12) for t0001 (currently in \"next\").\n    +    We really do have a memory leak here in either case, as e.g. running\n    +    the pre-image under valgrind(1) will reveal. It's documented\n    +    SANITIZE=leak (and \"address\", which exhibits the same behavior) might\n    +    interact with compiler optimization in this way in some cases, and\n    +    since this function is called recursively it's going to be especially\n    +    interesting as an optimization target.\n    +\n    +    Let's work around this issue by freeing the \"expanded\" memory before\n    +    we call die(), using the \"goto cleanup\" pattern introduced in the\n    +    preceding commit.\n     \n         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n     \n      ## config.c ##\n    +@@ config.c: static int handle_path_include(const char *path, struct config_include_data *inc\n    + \tint ret = 0;\n    + \tstruct strbuf buf = STRBUF_INIT;\n    + \tchar *expanded;\n    ++\tint die_depth = 0;\n    + \n    + \tif (!path)\n    + \t\treturn config_error_nonbool(\"include.path\");\n     @@ config.c: static int handle_path_include(const char *path, struct config_include_data *inc\n      \t}\n      \n      \tif (!access_or_die(path, R_OK, 0)) {\n     -\t\tif (++inc->depth > MAX_INCLUDE_DEPTH)\n    +-\t\t\tdie(_(include_depth_advice), MAX_INCLUDE_DEPTH, path,\n    +-\t\t\t    !cf ? \"<unknown>\" :\n    +-\t\t\t    cf->name ? cf->name :\n    +-\t\t\t    \"the command line\");\n     +\t\tif (++inc->depth > MAX_INCLUDE_DEPTH) {\n    -+\t\t\tfree(expanded);\n    - \t\t\tdie(_(include_depth_advice), MAX_INCLUDE_DEPTH, path,\n    - \t\t\t    !cf ? \"<unknown>\" :\n    - \t\t\t    cf->name ? cf->name :\n    - \t\t\t    \"the command line\");\n    ++\t\t\tdie_depth = 1;\n    ++\t\t\tgoto cleanup;\n     +\t\t}\n      \t\tret = git_config_from_file(git_config_include, path, inc);\n      \t\tinc->depth--;\n      \t}\n    -\n    - ## refs.c ##\n    -@@ refs.c: char *repo_default_branch_name(struct repository *r, int quiet)\n    - \t}\n    - \n    - \tfull_ref = xstrfmt(\"refs/heads/%s\", ret);\n    --\tif (check_refname_format(full_ref, 0))\n    -+\tif (check_refname_format(full_ref, 0)) {\n    -+\t\tfree(ret);\n    -+\t\tfree(full_ref);\n    - \t\tdie(_(\"invalid branch name: %s = %s\"), config_display_key, ret);\n    -+\t}\n    - \tfree(full_ref);\n    + cleanup:\n    + \tstrbuf_release(&buf);\n    + \tfree(expanded);\n    +-\treturn ret;\n    ++\tif (!die_depth)\n    ++\t\treturn ret;\n    ++\tdie(_(include_depth_advice), MAX_INCLUDE_DEPTH, path,\n    ++\t    !cf ? \"<unknown>\" : cf->name ? cf->name : \"the command line\");\n    + }\n      \n    - \treturn ret;\n    + static void add_trailing_starstar_for_dir(struct strbuf *pat)\n-- \n2.33.1.1494.g88b39a443e1\n\n"},{"id":"439268","messageId":"patch-v2-2.3-d6d04da1d9d-20211021T195133Z-avarab@gmail.com","threadId":"56746","inReplyTo":"cover-v2-0.3-00000000000-20211021T195133Z-avarab@gmail.com","subject":"[PATCH v2 2/3] config.c: don't leak memory in handle_path_include()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-21T19:54:14Z","receivedAt":"2021-10-21T19:54:25Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Fix a memory leak in the error() path in handle_path_include(), this\nallows us to run t1305-config-include.sh under SANITIZE=leak,\npreviously 4 tests there would fail. This fixes up a leak in\n9b25a0b52e0 (config: add include directive, 2012-02-06).\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n config.c                  | 7 +++++--\n t/t1305-config-include.sh | 1 +\n 2 files changed, 6 insertions(+), 2 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex 2dcbe901b6b..c5873f3a706 100644\n--- a/config.c\n+++ b/config.c\n@@ -148,8 +148,10 @@ static int handle_path_include(const char *path, struct config_include_data *inc\n \tif (!is_absolute_path(path)) {\n \t\tchar *slash;\n \n-\t\tif (!cf || !cf->path)\n-\t\t\treturn error(_(\"relative config includes must come from files\"));\n+\t\tif (!cf || !cf->path) {\n+\t\t\tret = error(_(\"relative config includes must come from files\"));\n+\t\t\tgoto cleanup;\n+\t\t}\n \n \t\tslash = find_last_dir_sep(cf->path);\n \t\tif (slash)\n@@ -167,6 +169,7 @@ static int handle_path_include(const char *path, struct config_include_data *inc\n \t\tret = git_config_from_file(git_config_include, path, inc);\n \t\tinc->depth--;\n \t}\n+cleanup:\n \tstrbuf_release(&buf);\n \tfree(expanded);\n \treturn ret;\ndiff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh\nindex ccbb116c016..5cde79ef8c4 100755\n--- a/t/t1305-config-include.sh\n+++ b/t/t1305-config-include.sh\n@@ -1,6 +1,7 @@\n #!/bin/sh\n \n test_description='test config file include directives'\n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n # Force setup_explicit_git_dir() to run until the end. This is needed\n-- \n2.33.1.1494.g88b39a443e1\n\n"},{"id":"439269","messageId":"patch-v2-1.3-4f8554bb02e-20211021T195133Z-avarab@gmail.com","threadId":"56746","inReplyTo":"cover-v2-0.3-00000000000-20211021T195133Z-avarab@gmail.com","subject":"[PATCH v2 1/3] refs.c: make \"repo_default_branch_name\" static, remove xstrfmt()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-21T19:54:13Z","receivedAt":"2021-10-21T19:54:26Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"The repo_default_branch_name() function introduced in\n8747ebb7cde (init: allow setting the default for the initial branch\nname via the config, 2020-06-24) has never been used outside of this\nfile, so let's make it static, its sibling function\ngit_default_branch_name() is what external callers use.\n\nIn addition the xstrfmt() to get the \"full_ref\" in the same commit\nisn't needed, we can use the \"REFNAME_ALLOW_ONELEVEL\" flag to\ncheck_refname_format() instead.\n\nThis also happens to fix an issue with c150064dbe2 (leak tests: run\nvarious built-in tests in t00*.sh SANITIZE=leak, 2021-10-12) in \"next\"\nwhen combined with SANITIZE=leak and higher optimization flags on at\nleast some GCC versions. See [1].\n\n1. https://lore.kernel.org/git/patch-1.1-5a47bf2e9c9-20211021T114223Z-avarab@gmail.com/\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n refs.c | 8 +++-----\n refs.h | 1 -\n 2 files changed, 3 insertions(+), 6 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 7f019c2377e..ccb09acbf1d 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -571,11 +571,11 @@ static const char default_branch_name_advice[] = N_(\n \"\\tgit branch -m <name>\\n\"\n );\n \n-char *repo_default_branch_name(struct repository *r, int quiet)\n+static char *repo_default_branch_name(struct repository *r, int quiet)\n {\n \tconst char *config_key = \"init.defaultbranch\";\n \tconst char *config_display_key = \"init.defaultBranch\";\n-\tchar *ret = NULL, *full_ref;\n+\tchar *ret = NULL;\n \tconst char *env = getenv(\"GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\");\n \n \tif (env && *env)\n@@ -589,10 +589,8 @@ char *repo_default_branch_name(struct repository *r, int quiet)\n \t\t\tadvise(_(default_branch_name_advice), ret);\n \t}\n \n-\tfull_ref = xstrfmt(\"refs/heads/%s\", ret);\n-\tif (check_refname_format(full_ref, 0))\n+\tif (check_refname_format(ret, REFNAME_ALLOW_ONELEVEL))\n \t\tdie(_(\"invalid branch name: %s = %s\"), config_display_key, ret);\n-\tfree(full_ref);\n \n \treturn ret;\n }\ndiff --git a/refs.h b/refs.h\nindex d5099d4984e..77f899da6ef 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -171,7 +171,6 @@ int dwim_log(const char *str, int len, struct object_id *oid, char **ref);\n  * return value of `git_default_branch_name()` is a singleton.\n  */\n const char *git_default_branch_name(int quiet);\n-char *repo_default_branch_name(struct repository *r, int quiet);\n \n /*\n  * A ref_transaction represents a collection of reference updates that\n-- \n2.33.1.1494.g88b39a443e1\n\n"},{"id":"439270","messageId":"patch-v2-3.3-d812358e331-20211021T195133Z-avarab@gmail.com","threadId":"56746","inReplyTo":"cover-v2-0.3-00000000000-20211021T195133Z-avarab@gmail.com","subject":"[PATCH v2 3/3] config.c: free(expanded) before die(), work around GCC oddity","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-21T19:54:15Z","receivedAt":"2021-10-21T19:54:27Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On my GCC version (10.2.1-6), but not the clang I have available t0017\nwill fail under SANITIZE=leak on optimization levels higher than -O0,\nwhich is annoying when combined with the change in 956d2e4639b (tests:\nadd a test mode for SANITIZE=leak, run it in CI, 2021-09-23).\n\nWe really do have a memory leak here in either case, as e.g. running\nthe pre-image under valgrind(1) will reveal. It's documented\nSANITIZE=leak (and \"address\", which exhibits the same behavior) might\ninteract with compiler optimization in this way in some cases, and\nsince this function is called recursively it's going to be especially\ninteresting as an optimization target.\n\nLet's work around this issue by freeing the \"expanded\" memory before\nwe call die(), using the \"goto cleanup\" pattern introduced in the\npreceding commit.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n config.c | 15 +++++++++------\n 1 file changed, 9 insertions(+), 6 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex c5873f3a706..ab40decaeba 100644\n--- a/config.c\n+++ b/config.c\n@@ -132,6 +132,7 @@ static int handle_path_include(const char *path, struct config_include_data *inc\n \tint ret = 0;\n \tstruct strbuf buf = STRBUF_INIT;\n \tchar *expanded;\n+\tint die_depth = 0;\n \n \tif (!path)\n \t\treturn config_error_nonbool(\"include.path\");\n@@ -161,18 +162,20 @@ static int handle_path_include(const char *path, struct config_include_data *inc\n \t}\n \n \tif (!access_or_die(path, R_OK, 0)) {\n-\t\tif (++inc->depth > MAX_INCLUDE_DEPTH)\n-\t\t\tdie(_(include_depth_advice), MAX_INCLUDE_DEPTH, path,\n-\t\t\t    !cf ? \"<unknown>\" :\n-\t\t\t    cf->name ? cf->name :\n-\t\t\t    \"the command line\");\n+\t\tif (++inc->depth > MAX_INCLUDE_DEPTH) {\n+\t\t\tdie_depth = 1;\n+\t\t\tgoto cleanup;\n+\t\t}\n \t\tret = git_config_from_file(git_config_include, path, inc);\n \t\tinc->depth--;\n \t}\n cleanup:\n \tstrbuf_release(&buf);\n \tfree(expanded);\n-\treturn ret;\n+\tif (!die_depth)\n+\t\treturn ret;\n+\tdie(_(include_depth_advice), MAX_INCLUDE_DEPTH, path,\n+\t    !cf ? \"<unknown>\" : cf->name ? cf->name : \"the command line\");\n }\n \n static void add_trailing_starstar_for_dir(struct strbuf *pat)\n-- \n2.33.1.1494.g88b39a443e1\n\n"},{"id":"439305","messageId":"xmqqsfwugdss.fsf@gitster.g","threadId":"56746","inReplyTo":"patch-v2-1.3-4f8554bb02e-20211021T195133Z-avarab@gmail.com","subject":"Re: [PATCH v2 1/3] refs.c: make \"repo_default_branch_name\" static, remove xstrfmt()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-21T23:26:27Z","receivedAt":"2021-10-21T23:26:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n\n> In addition the xstrfmt() to get the \"full_ref\" in the same commit\n> isn't needed, we can use the \"REFNAME_ALLOW_ONELEVEL\" flag to\n> check_refname_format() instead.\n\nReading the code of check_refname_format(), I do not think one-level\nis the only thing that the prefixing of refs/heads/ is defeating,\nand more importantly, I'd expect that this will block later changes\nlike enforcing \"HEAD might be OK in onelevel because we want to keep\n.git/HEAD working, but we do not like refs/heads/HEAD\" at this level\nto enhance usability from happening.\n\nMaking the function file-local static is a good thing to do, though.\n"},{"id":"439306","messageId":"xmqqmtn2gdlv.fsf@gitster.g","threadId":"56746","inReplyTo":"patch-v2-2.3-d6d04da1d9d-20211021T195133Z-avarab@gmail.com","subject":"Re: [PATCH v2 2/3] config.c: don't leak memory in handle_path_include()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-21T23:30:36Z","receivedAt":"2021-10-21T23:30:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n\n> Fix a memory leak in the error() path in handle_path_include(), this\n> allows us to run t1305-config-include.sh under SANITIZE=leak,\n> previously 4 tests there would fail. This fixes up a leak in\n> 9b25a0b52e0 (config: add include directive, 2012-02-06).\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n> ---\n>  config.c                  | 7 +++++--\n>  t/t1305-config-include.sh | 1 +\n>  2 files changed, 6 insertions(+), 2 deletions(-)\n>\n> diff --git a/config.c b/config.c\n> index 2dcbe901b6b..c5873f3a706 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -148,8 +148,10 @@ static int handle_path_include(const char *path, struct config_include_data *inc\n\nNot a problem introduced by this function, but if you look at this\nchange with \"git show -W\", we'd notice that the function name on the\nhunk header looks strange.  I think we should add a blank line\nbefore the beginning of the function.\n\n>  \tif (!is_absolute_path(path)) {\n>  \t\tchar *slash;\n>  \n> -\t\tif (!cf || !cf->path)\n> -\t\t\treturn error(_(\"relative config includes must come from files\"));\n> +\t\tif (!cf || !cf->path) {\n> +\t\t\tret = error(_(\"relative config includes must come from files\"));\n> +\t\t\tgoto cleanup;\n> +\t\t}\n>  \n>  \t\tslash = find_last_dir_sep(cf->path);\n>  \t\tif (slash)\n> @@ -167,6 +169,7 @@ static int handle_path_include(const char *path, struct config_include_data *inc\n>  \t\tret = git_config_from_file(git_config_include, path, inc);\n>  \t\tinc->depth--;\n>  \t}\n> +cleanup:\n>  \tstrbuf_release(&buf);\n>  \tfree(expanded);\n>  \treturn ret;\n\nQuite straight-forward.  At the point of the new \"goto cleanup\", the\nexpanded pointer has already been established, so these two calls\nwill release the right resource.\n\nThanks.\n\n> diff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh\n> index ccbb116c016..5cde79ef8c4 100755\n> --- a/t/t1305-config-include.sh\n> +++ b/t/t1305-config-include.sh\n> @@ -1,6 +1,7 @@\n>  #!/bin/sh\n>  \n>  test_description='test config file include directives'\n> +TEST_PASSES_SANITIZE_LEAK=true\n>  . ./test-lib.sh\n>  \n>  # Force setup_explicit_git_dir() to run until the end. This is needed\n"},{"id":"439307","messageId":"xmqqilxqgdjc.fsf@gitster.g","threadId":"56746","inReplyTo":"patch-v2-3.3-d812358e331-20211021T195133Z-avarab@gmail.com","subject":"Re: [PATCH v2 3/3] config.c: free(expanded) before die(), work around GCC oddity","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-21T23:32:07Z","receivedAt":"2021-10-21T23:32:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n\n>  cleanup:\n>  \tstrbuf_release(&buf);\n>  \tfree(expanded);\n> -\treturn ret;\n> +\tif (!die_depth)\n> +\t\treturn ret;\n> +\tdie(_(include_depth_advice), MAX_INCLUDE_DEPTH, path,\n> +\t    !cf ? \"<unknown>\" : cf->name ? cf->name : \"the command line\");\n>  }\n\nYuck.  With or without compiler bugs, this code is too ugly to live,\nisn't it?\n"},{"id":"439393","messageId":"211022.86ilxpj7si.gmgdl@evledraar.gmail.com","threadId":"56746","inReplyTo":"xmqqmtn2gdlv.fsf@gitster.g","subject":"Re: [PATCH v2 2/3] config.c: don't leak memory in handle_path_include()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-22T17:19:26Z","receivedAt":"2021-10-22T17:20:20Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Oct 21 2021, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n>\n>> Fix a memory leak in the error() path in handle_path_include(), this\n>> allows us to run t1305-config-include.sh under SANITIZE=leak,\n>> previously 4 tests there would fail. This fixes up a leak in\n>> 9b25a0b52e0 (config: add include directive, 2012-02-06).\n>>\n>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n>> ---\n>>  config.c                  | 7 +++++--\n>>  t/t1305-config-include.sh | 1 +\n>>  2 files changed, 6 insertions(+), 2 deletions(-)\n>>\n>> diff --git a/config.c b/config.c\n>> index 2dcbe901b6b..c5873f3a706 100644\n>> --- a/config.c\n>> +++ b/config.c\n>> @@ -148,8 +148,10 @@ static int handle_path_include(const char *path, struct config_include_data *inc\n>\n> Not a problem introduced by this function, but if you look at this\n> change with \"git show -W\", we'd notice that the function name on the\n> hunk header looks strange.  I think we should add a blank line\n> before the beginning of the function.\n\nI think this is a bug in -W, after all if without it we we show the\nfunction context line, but with it we advance further, then that means\nthat -W didn't find the correct function boundary.\n"},{"id":"439398","messageId":"cover-v3-0.6-00000000000-20211022T175227Z-avarab@gmail.com","threadId":"56746","inReplyTo":"cover-v2-0.3-00000000000-20211021T195133Z-avarab@gmail.com","subject":"[PATCH v3 0/6] usage.c: add die_message() & plug memory leaks in refs.c & config.c","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-22T18:19:33Z","receivedAt":"2021-10-22T18:19:44Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"What started as a set of small memory leak fixes is now adding and\nusinge a die_message() function. This non-fatal-die() is useful to\nvarious callers that want to print \"fatal: \" before exiting, but don't\nwant to call die() for whatever reason.\n\nI wasn't planning to submit that now, but these were incomplete\npatches I had lying around, and make the 5th and 6th patch much nicer,\nin response to comments on v1 and v2 to the effect that managing\nfree()-ing around die() functions was rather nasty.\n\nThis doesn't conflict with anything in-flight, and the changes\nthemselves are rather simple.\n\nÆvar Arnfjörð Bjarmason (6):\n  usage.c: add a die_message() routine\n  usage.c API users: use die_message() where appropriate\n  usage.c + gc: add and use a die_message_errno()\n  config.c: don't leak memory in handle_path_include()\n  config.c: free(expanded) before die(), work around GCC oddity\n  refs: plug memory leak in repo_default_branch_name()\n\n builtin/fast-import.c     | 13 +++++----\n builtin/gc.c              | 21 ++++++++------\n builtin/notes.c           |  9 +++---\n config.c                  | 22 ++++++++++-----\n git-compat-util.h         |  4 +++\n http-backend.c            |  3 +-\n parse-options.c           |  2 +-\n refs.c                    |  8 +++++-\n run-command.c             | 16 ++++-------\n t/t1305-config-include.sh |  1 +\n usage.c                   | 58 +++++++++++++++++++++++++++++++--------\n 11 files changed, 106 insertions(+), 51 deletions(-)\n\nRange-diff against v2:\n-:  ----------- > 1:  fe8763337ed usage.c: add a die_message() routine\n-:  ----------- > 2:  dfc3a8fbccb usage.c API users: use die_message() where appropriate\n-:  ----------- > 3:  6b33e394b2f usage.c + gc: add and use a die_message_errno()\n2:  d6d04da1d9d = 4:  3607b905627 config.c: don't leak memory in handle_path_include()\n3:  d812358e331 ! 5:  9a44204c4c9 config.c: free(expanded) before die(), work around GCC oddity\n    @@ Commit message\n         We really do have a memory leak here in either case, as e.g. running\n         the pre-image under valgrind(1) will reveal. It's documented\n         SANITIZE=leak (and \"address\", which exhibits the same behavior) might\n    -    interact with compiler optimization in this way in some cases, and\n    -    since this function is called recursively it's going to be especially\n    +    interact with compiler optimization in this way in some cases. Since\n    +    this function is called recursively it's going to be especially\n         interesting as an optimization target.\n     \n         Let's work around this issue by freeing the \"expanded\" memory before\n    -    we call die(), using the \"goto cleanup\" pattern introduced in the\n    -    preceding commit.\n    +    we call die(), using a combination of the \"goto cleanup\" pattern\n    +    introduced in a preceding commit, and the newly introduced\n    +    die_message() function.\n     \n         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n     \n    @@ config.c: static int handle_path_include(const char *path, struct config_include\n      \tint ret = 0;\n      \tstruct strbuf buf = STRBUF_INIT;\n      \tchar *expanded;\n    -+\tint die_depth = 0;\n    ++\tint exit_with = 0;\n      \n      \tif (!path)\n      \t\treturn config_error_nonbool(\"include.path\");\n    @@ config.c: static int handle_path_include(const char *path, struct config_include\n     -\t\t\t    cf->name ? cf->name :\n     -\t\t\t    \"the command line\");\n     +\t\tif (++inc->depth > MAX_INCLUDE_DEPTH) {\n    -+\t\t\tdie_depth = 1;\n    ++\t\t\texit_with = die_message(_(include_depth_advice),\n    ++\t\t\t\t\t\tMAX_INCLUDE_DEPTH, path,\n    ++\t\t\t\t\t\t!cf ? \"<unknown>\" : cf->name ?\n    ++\t\t\t\t\t\tcf->name : \"the command line\");\n     +\t\t\tgoto cleanup;\n     +\t\t}\n      \t\tret = git_config_from_file(git_config_include, path, inc);\n    @@ config.c: static int handle_path_include(const char *path, struct config_include\n      cleanup:\n      \tstrbuf_release(&buf);\n      \tfree(expanded);\n    --\treturn ret;\n    -+\tif (!die_depth)\n    -+\t\treturn ret;\n    -+\tdie(_(include_depth_advice), MAX_INCLUDE_DEPTH, path,\n    -+\t    !cf ? \"<unknown>\" : cf->name ? cf->name : \"the command line\");\n    ++\tif (exit_with)\n    ++\t\texit(exit_with);\n    + \treturn ret;\n      }\n      \n    - static void add_trailing_starstar_for_dir(struct strbuf *pat)\n1:  4f8554bb02e ! 6:  d2f639b53cd refs.c: make \"repo_default_branch_name\" static, remove xstrfmt()\n    @@ Metadata\n     Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n     \n      ## Commit message ##\n    -    refs.c: make \"repo_default_branch_name\" static, remove xstrfmt()\n    +    refs: plug memory leak in repo_default_branch_name()\n     \n    -    The repo_default_branch_name() function introduced in\n    -    8747ebb7cde (init: allow setting the default for the initial branch\n    -    name via the config, 2020-06-24) has never been used outside of this\n    -    file, so let's make it static, its sibling function\n    -    git_default_branch_name() is what external callers use.\n    +    Fix a memory leak in repo_default_branch_name(), we'll leak memory\n    +    before exit(128) here.\n     \n    -    In addition the xstrfmt() to get the \"full_ref\" in the same commit\n    -    isn't needed, we can use the \"REFNAME_ALLOW_ONELEVEL\" flag to\n    -    check_refname_format() instead.\n    +    Normally we would not care much about such leaks, we do leak the\n    +    memory, as e.g. valgrind(1) will report. But the more commonly used\n    +    SANITIZE=leak mode will use GCC and Clang's LSAN mode will not\n    +    normally report such leaks.\n     \n    -    This also happens to fix an issue with c150064dbe2 (leak tests: run\n    -    various built-in tests in t00*.sh SANITIZE=leak, 2021-10-12) in \"next\"\n    -    when combined with SANITIZE=leak and higher optimization flags on at\n    -    least some GCC versions. See [1].\n    +    At least one GCC version does that in this case, and having the tests\n    +    fail under -O3 would be annoying, so let's free() the allocated memory\n    +    here.\n     \n    -    1. https://lore.kernel.org/git/patch-1.1-5a47bf2e9c9-20211021T114223Z-avarab@gmail.com/\n    +    This uses a new die_message() function introduced in a preceding\n    +    commit. That new function makes the flow around such code easier to\n    +    manage. In this case we can't free(ret) before the die().\n    +\n    +    In this case only the \"free(full_ref)\" appears to be needed, but since\n    +    we're freeing one let's free both, some other compiler or version\n    +    might arrange this code in such a way as to complain about \"ret\" too\n    +    with SANITIZE=leak, and valgrind(1) will do so in any case.\n     \n         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n     \n      ## refs.c ##\n    -@@ refs.c: static const char default_branch_name_advice[] = N_(\n    - \"\\tgit branch -m <name>\\n\"\n    - );\n    - \n    --char *repo_default_branch_name(struct repository *r, int quiet)\n    -+static char *repo_default_branch_name(struct repository *r, int quiet)\n    - {\n    - \tconst char *config_key = \"init.defaultbranch\";\n    +@@ refs.c: char *repo_default_branch_name(struct repository *r, int quiet)\n      \tconst char *config_display_key = \"init.defaultBranch\";\n    --\tchar *ret = NULL, *full_ref;\n    -+\tchar *ret = NULL;\n    + \tchar *ret = NULL, *full_ref;\n      \tconst char *env = getenv(\"GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\");\n    ++\tint exit_with = 0;\n      \n      \tif (env && *env)\n    + \t\tret = xstrdup(env);\n     @@ refs.c: char *repo_default_branch_name(struct repository *r, int quiet)\n    - \t\t\tadvise(_(default_branch_name_advice), ret);\n    - \t}\n      \n    --\tfull_ref = xstrfmt(\"refs/heads/%s\", ret);\n    --\tif (check_refname_format(full_ref, 0))\n    -+\tif (check_refname_format(ret, REFNAME_ALLOW_ONELEVEL))\n    - \t\tdie(_(\"invalid branch name: %s = %s\"), config_display_key, ret);\n    --\tfree(full_ref);\n    + \tfull_ref = xstrfmt(\"refs/heads/%s\", ret);\n    + \tif (check_refname_format(full_ref, 0))\n    +-\t\tdie(_(\"invalid branch name: %s = %s\"), config_display_key, ret);\n    ++\t\texit_with = die_message(_(\"invalid branch name: %s = %s\"),\n    ++\t\t\t\t\tconfig_display_key, ret);\n    + \tfree(full_ref);\n    ++\tif (exit_with) {\n    ++\t\tfree(ret);\n    ++\t\texit(exit_with);\n    ++\t}\n      \n      \treturn ret;\n      }\n    -\n    - ## refs.h ##\n    -@@ refs.h: int dwim_log(const char *str, int len, struct object_id *oid, char **ref);\n    -  * return value of `git_default_branch_name()` is a singleton.\n    -  */\n    - const char *git_default_branch_name(int quiet);\n    --char *repo_default_branch_name(struct repository *r, int quiet);\n    - \n    - /*\n    -  * A ref_transaction represents a collection of reference updates that\n-- \n2.33.1.1494.g88b39a443e1\n\n"},{"id":"439399","messageId":"patch-v3-1.6-fe8763337ed-20211022T175227Z-avarab@gmail.com","threadId":"56746","inReplyTo":"cover-v3-0.6-00000000000-20211022T175227Z-avarab@gmail.com","subject":"[PATCH v3 1/6] usage.c: add a die_message() routine","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-22T18:19:34Z","receivedAt":"2021-10-22T18:19:46Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"We have code in various places that would like to call die(), but\nwants to defer the exit(128) it would invoke, e.g. to print an\nadditional message, or adjust the exit code. Add a die_message()\nhelper routine to bridge this gap in the API.\n\nFunctionally this behaves just like the error() routine, except it'll\nprint a \"fatal: \" prefix, and it will exit with 128 instead of -1,\nthis is so that caller can pas the return value to exit(128), instead\nof having to hardcode \"128\".\n\nA subsequent commit will migrate various callers that benefit from\nthis function over to it, for now we're just migrating trivial users\nin usage.c itself.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n git-compat-util.h |  3 +++\n usage.c           | 46 ++++++++++++++++++++++++++++++++++------------\n 2 files changed, 37 insertions(+), 12 deletions(-)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 141bb86351e..c1bb32460b6 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -471,6 +471,7 @@ NORETURN void usage(const char *err);\n NORETURN void usagef(const char *err, ...) __attribute__((format (printf, 1, 2)));\n NORETURN void die(const char *err, ...) __attribute__((format (printf, 1, 2)));\n NORETURN void die_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));\n+int die_message(const char *err, ...) __attribute__((format (printf, 1, 2)));\n int error(const char *err, ...) __attribute__((format (printf, 1, 2)));\n int error_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));\n void warning(const char *err, ...) __attribute__((format (printf, 1, 2)));\n@@ -505,6 +506,8 @@ static inline int const_error(void)\n typedef void (*report_fn)(const char *, va_list params);\n \n void set_die_routine(NORETURN_PTR report_fn routine);\n+void set_die_message_routine(report_fn routine);\n+report_fn get_die_message_routine(void);\n void set_error_routine(report_fn routine);\n report_fn get_error_routine(void);\n void set_warn_routine(report_fn routine);\ndiff --git a/usage.c b/usage.c\nindex c7d233b0de9..3d4b90bce1f 100644\n--- a/usage.c\n+++ b/usage.c\n@@ -55,6 +55,12 @@ static NORETURN void usage_builtin(const char *err, va_list params)\n \texit(129);\n }\n \n+static void die_message_builtin(const char *err, va_list params)\n+{\n+\ttrace2_cmd_error_va(err, params);\n+\tvreportf(\"fatal: \", err, params);\n+}\n+\n /*\n  * We call trace2_cmd_error_va() in the below functions first and\n  * expect it to va_copy 'params' before using it (because an 'ap' can\n@@ -62,10 +68,9 @@ static NORETURN void usage_builtin(const char *err, va_list params)\n  */\n static NORETURN void die_builtin(const char *err, va_list params)\n {\n-\ttrace2_cmd_error_va(err, params);\n-\n-\tvreportf(\"fatal: \", err, params);\n+\treport_fn die_message_fn = get_die_message_routine();\n \n+\tdie_message_fn(err, params);\n \texit(128);\n }\n \n@@ -109,6 +114,7 @@ static int die_is_recursing_builtin(void)\n  * (ugh), so keep things static. */\n static NORETURN_PTR report_fn usage_routine = usage_builtin;\n static NORETURN_PTR report_fn die_routine = die_builtin;\n+static report_fn die_message_routine = die_message_builtin;\n static report_fn error_routine = error_builtin;\n static report_fn warn_routine = warn_builtin;\n static int (*die_is_recursing)(void) = die_is_recursing_builtin;\n@@ -118,6 +124,16 @@ void set_die_routine(NORETURN_PTR report_fn routine)\n \tdie_routine = routine;\n }\n \n+void set_die_message_routine(report_fn routine)\n+{\n+\tdie_message_routine = routine;\n+}\n+\n+report_fn get_die_message_routine(void)\n+{\n+\treturn die_message_routine;\n+}\n+\n void set_error_routine(report_fn routine)\n {\n \terror_routine = routine;\n@@ -157,14 +173,23 @@ void NORETURN usage(const char *err)\n \tusagef(\"%s\", err);\n }\n \n+#undef die_message\n+int die_message(const char *err, ...)\n+{\n+\tva_list params;\n+\n+\tva_start(params, err);\n+\tdie_message_routine(err, params);\n+\tva_end(params);\n+\treturn 128;\n+}\n+\n void NORETURN die(const char *err, ...)\n {\n \tva_list params;\n \n-\tif (die_is_recursing()) {\n-\t\tfputs(\"fatal: recursion detected in die handler\\n\", stderr);\n-\t\texit(128);\n-\t}\n+\tif (die_is_recursing())\n+\t\texit(die_message(\"recursion detected in die handler\"));\n \n \tva_start(params, err);\n \tdie_routine(err, params);\n@@ -200,11 +225,8 @@ void NORETURN die_errno(const char *fmt, ...)\n \tchar buf[1024];\n \tva_list params;\n \n-\tif (die_is_recursing()) {\n-\t\tfputs(\"fatal: recursion detected in die_errno handler\\n\",\n-\t\t\tstderr);\n-\t\texit(128);\n-\t}\n+\tif (die_is_recursing())\n+\t\texit(die_message(\"recursion detected in die_errno handler\"));\n \n \tva_start(params, fmt);\n \tdie_routine(fmt_with_err(buf, sizeof(buf), fmt), params);\n-- \n2.33.1.1494.g88b39a443e1\n\n"},{"id":"439400","messageId":"patch-v3-2.6-dfc3a8fbccb-20211022T175227Z-avarab@gmail.com","threadId":"56746","inReplyTo":"cover-v3-0.6-00000000000-20211022T175227Z-avarab@gmail.com","subject":"[PATCH v3 2/6] usage.c API users: use die_message() where appropriate","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-22T18:19:35Z","receivedAt":"2021-10-22T18:19:47Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change code that either called error() and proceeded to exit with 128,\nor emitted its own \"fatal: \" messages to use the die_message()\nfunction added in a preceding commit.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n builtin/fast-import.c | 13 ++++++++-----\n builtin/notes.c       |  9 +++++----\n http-backend.c        |  3 ++-\n parse-options.c       |  2 +-\n run-command.c         | 16 +++++-----------\n 5 files changed, 21 insertions(+), 22 deletions(-)\n\ndiff --git a/builtin/fast-import.c b/builtin/fast-import.c\nindex 20406f67754..11cd5b0c56c 100644\n--- a/builtin/fast-import.c\n+++ b/builtin/fast-import.c\n@@ -401,16 +401,19 @@ static void dump_marks(void);\n \n static NORETURN void die_nicely(const char *err, va_list params)\n {\n+\tva_list cp;\n \tstatic int zombie;\n-\tchar message[2 * PATH_MAX];\n+\treport_fn die_message_fn = get_die_message_routine();\n \n-\tvsnprintf(message, sizeof(message), err, params);\n-\tfputs(\"fatal: \", stderr);\n-\tfputs(message, stderr);\n-\tfputc('\\n', stderr);\n+\tif (!zombie)\n+\t\tva_copy(cp, params);\n+\tdie_message_fn(err, params);\n \n \tif (!zombie) {\n+\t\tchar message[2 * PATH_MAX];\n+\n \t\tzombie = 1;\n+\t\tvsnprintf(message, sizeof(message), err, cp);\n \t\twrite_crash_report(message);\n \t\tend_packfile();\n \t\tunkeep_all_packs();\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex 71c59583a17..2812d1eac40 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -201,11 +201,12 @@ static void prepare_note_data(const struct object_id *object, struct note_data *\n static void write_note_data(struct note_data *d, struct object_id *oid)\n {\n \tif (write_object_file(d->buf.buf, d->buf.len, blob_type, oid)) {\n-\t\terror(_(\"unable to write note object\"));\n+\t\tint status = die_message(_(\"unable to write note object\"));\n+\n \t\tif (d->edit_path)\n-\t\t\terror(_(\"the note contents have been left in %s\"),\n-\t\t\t\td->edit_path);\n-\t\texit(128);\n+\t\t\tdie_message(_(\"the note contents have been left in %s\"),\n+\t\t\t\t    d->edit_path);\n+\t\texit(status);\n \t}\n }\n \ndiff --git a/http-backend.c b/http-backend.c\nindex e7c0eeab230..bc853356e73 100644\n--- a/http-backend.c\n+++ b/http-backend.c\n@@ -661,8 +661,9 @@ static NORETURN void die_webcgi(const char *err, va_list params)\n {\n \tif (dead <= 1) {\n \t\tstruct strbuf hdr = STRBUF_INIT;\n+\t\treport_fn die_message_fn = get_die_message_routine();\n \n-\t\tvreportf(\"fatal: \", err, params);\n+\t\tdie_message_fn(err, params);\n \n \t\thttp_status(&hdr, 500, \"Internal Server Error\");\n \t\thdr_nocache(&hdr);\ndiff --git a/parse-options.c b/parse-options.c\nindex 6e0535bdaad..c892641d9a1 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -1049,7 +1049,7 @@ void NORETURN usage_msg_opt(const char *msg,\n \t\t   const char * const *usagestr,\n \t\t   const struct option *options)\n {\n-\tfprintf(stderr, \"fatal: %s\\n\\n\", msg);\n+\tdie_message(\"%s\\n\", msg); /* The extra \\n is intentional */\n \tusage_with_options(usagestr, options);\n }\n \ndiff --git a/run-command.c b/run-command.c\nindex 7ef5cc712a9..220cf53deb4 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -340,15 +340,6 @@ static void child_close_pair(int fd[2])\n \tchild_close(fd[1]);\n }\n \n-/*\n- * parent will make it look like the child spewed a fatal error and died\n- * this is needed to prevent changes to t0061.\n- */\n-static void fake_fatal(const char *err, va_list params)\n-{\n-\tvreportf(\"fatal: \", err, params);\n-}\n-\n static void child_error_fn(const char *err, va_list params)\n {\n \tconst char msg[] = \"error() should not be called in child\\n\";\n@@ -372,9 +363,10 @@ static void NORETURN child_die_fn(const char *err, va_list params)\n static void child_err_spew(struct child_process *cmd, struct child_err *cerr)\n {\n \tstatic void (*old_errfn)(const char *err, va_list params);\n+\treport_fn die_message_routine = get_die_message_routine();\n \n \told_errfn = get_error_routine();\n-\tset_error_routine(fake_fatal);\n+\tset_error_routine(die_message_routine);\n \terrno = cerr->syserr;\n \n \tswitch (cerr->err) {\n@@ -1082,7 +1074,9 @@ static void *run_thread(void *data)\n \n static NORETURN void die_async(const char *err, va_list params)\n {\n-\tvreportf(\"fatal: \", err, params);\n+\treport_fn die_message_fn = get_die_message_routine();\n+\n+\tdie_message_fn(err, params);\n \n \tif (in_async()) {\n \t\tstruct async *async = pthread_getspecific(async_key);\n-- \n2.33.1.1494.g88b39a443e1\n\n"},{"id":"439401","messageId":"patch-v3-3.6-6b33e394b2f-20211022T175227Z-avarab@gmail.com","threadId":"56746","inReplyTo":"cover-v3-0.6-00000000000-20211022T175227Z-avarab@gmail.com","subject":"[PATCH v3 3/6] usage.c + gc: add and use a die_message_errno()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-22T18:19:36Z","receivedAt":"2021-10-22T18:19:48Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change code the \"error: \" output when we exit with 128 due to gc.log\nerrors to use a \"fatal: \" prefix instead. This adds a sibling function\nto the die_errno() added in a preceding commit.\n\nSince it returns 128 instead of -1 we'll need to adjust\nreport_last_gc_error(). Let's adjust it while we're at it to not\nconflate the \"should skip\" and \"exit with this non-zero code\"\nconditions, as the caller is no longer hardcoding \"128\", but relying\non die_errno() to return a nen-zero exit() status.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n builtin/gc.c      | 21 ++++++++++++---------\n git-compat-util.h |  1 +\n usage.c           | 12 ++++++++++++\n 3 files changed, 25 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 6b3de3dd514..f7deef08974 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -472,19 +472,20 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)\n  * gc should not proceed due to an error in the last run. Prints a\n  * message and returns -1 if an error occurred while reading gc.log\n  */\n-static int report_last_gc_error(void)\n+static int report_last_gc_error(int *skip)\n {\n \tstruct strbuf sb = STRBUF_INIT;\n \tint ret = 0;\n \tssize_t len;\n \tstruct stat st;\n \tchar *gc_log_path = git_pathdup(\"gc.log\");\n+\t*skip = 0;\n \n \tif (stat(gc_log_path, &st)) {\n \t\tif (errno == ENOENT)\n \t\t\tgoto done;\n \n-\t\tret = error_errno(_(\"cannot stat '%s'\"), gc_log_path);\n+\t\tret = die_message_errno(_(\"cannot stat '%s'\"), gc_log_path);\n \t\tgoto done;\n \t}\n \n@@ -493,7 +494,7 @@ static int report_last_gc_error(void)\n \n \tlen = strbuf_read_file(&sb, gc_log_path, 0);\n \tif (len < 0)\n-\t\tret = error_errno(_(\"cannot read '%s'\"), gc_log_path);\n+\t\tret = die_message_errno(_(\"cannot read '%s'\"), gc_log_path);\n \telse if (len > 0) {\n \t\t/*\n \t\t * A previous gc failed.  Report the error, and don't\n@@ -507,7 +508,7 @@ static int report_last_gc_error(void)\n \t\t\t       \"until the file is removed.\\n\\n\"\n \t\t\t       \"%s\"),\n \t\t\t    gc_log_path, sb.buf);\n-\t\tret = 1;\n+\t\t*skip = 1;\n \t}\n \tstrbuf_release(&sb);\n done:\n@@ -610,13 +611,15 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \t\t\tfprintf(stderr, _(\"See \\\"git help gc\\\" for manual housekeeping.\\n\"));\n \t\t}\n \t\tif (detach_auto) {\n-\t\t\tint ret = report_last_gc_error();\n-\t\t\tif (ret < 0)\n-\t\t\t\t/* an I/O error occurred, already reported */\n-\t\t\t\texit(128);\n-\t\t\tif (ret == 1)\n+\t\t\tint skip;\n+\t\t\tint ret = report_last_gc_error(&skip);\n+\n+\t\t\tif (skip)\n \t\t\t\t/* Last gc --auto failed. Skip this one. */\n \t\t\t\treturn 0;\n+\t\t\tif (ret)\n+\t\t\t\t/* an error occurred, already reported */\n+\t\t\t\texit(ret);\n \n \t\t\tif (lock_repo_for_gc(force, &pid))\n \t\t\t\treturn 0;\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex c1bb32460b6..ea0ac80f7db 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -472,6 +472,7 @@ NORETURN void usagef(const char *err, ...) __attribute__((format (printf, 1, 2))\n NORETURN void die(const char *err, ...) __attribute__((format (printf, 1, 2)));\n NORETURN void die_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));\n int die_message(const char *err, ...) __attribute__((format (printf, 1, 2)));\n+int die_message_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));\n int error(const char *err, ...) __attribute__((format (printf, 1, 2)));\n int error_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));\n void warning(const char *err, ...) __attribute__((format (printf, 1, 2)));\ndiff --git a/usage.c b/usage.c\nindex 3d4b90bce1f..efc2064dde3 100644\n--- a/usage.c\n+++ b/usage.c\n@@ -233,6 +233,18 @@ void NORETURN die_errno(const char *fmt, ...)\n \tva_end(params);\n }\n \n+#undef die_message_errno\n+int die_message_errno(const char *fmt, ...)\n+{\n+\tchar buf[1024];\n+\tva_list params;\n+\n+\tva_start(params, fmt);\n+\tdie_message_routine(fmt_with_err(buf, sizeof(buf), fmt), params);\n+\tva_end(params);\n+\treturn -1;\n+}\n+\n #undef error_errno\n int error_errno(const char *fmt, ...)\n {\n-- \n2.33.1.1494.g88b39a443e1\n\n"},{"id":"439402","messageId":"patch-v3-4.6-3607b905627-20211022T175227Z-avarab@gmail.com","threadId":"56746","inReplyTo":"cover-v3-0.6-00000000000-20211022T175227Z-avarab@gmail.com","subject":"[PATCH v3 4/6] config.c: don't leak memory in handle_path_include()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-22T18:19:37Z","receivedAt":"2021-10-22T18:19:50Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Fix a memory leak in the error() path in handle_path_include(), this\nallows us to run t1305-config-include.sh under SANITIZE=leak,\npreviously 4 tests there would fail. This fixes up a leak in\n9b25a0b52e0 (config: add include directive, 2012-02-06).\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n config.c                  | 7 +++++--\n t/t1305-config-include.sh | 1 +\n 2 files changed, 6 insertions(+), 2 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex 2dcbe901b6b..c5873f3a706 100644\n--- a/config.c\n+++ b/config.c\n@@ -148,8 +148,10 @@ static int handle_path_include(const char *path, struct config_include_data *inc\n \tif (!is_absolute_path(path)) {\n \t\tchar *slash;\n \n-\t\tif (!cf || !cf->path)\n-\t\t\treturn error(_(\"relative config includes must come from files\"));\n+\t\tif (!cf || !cf->path) {\n+\t\t\tret = error(_(\"relative config includes must come from files\"));\n+\t\t\tgoto cleanup;\n+\t\t}\n \n \t\tslash = find_last_dir_sep(cf->path);\n \t\tif (slash)\n@@ -167,6 +169,7 @@ static int handle_path_include(const char *path, struct config_include_data *inc\n \t\tret = git_config_from_file(git_config_include, path, inc);\n \t\tinc->depth--;\n \t}\n+cleanup:\n \tstrbuf_release(&buf);\n \tfree(expanded);\n \treturn ret;\ndiff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh\nindex ccbb116c016..5cde79ef8c4 100755\n--- a/t/t1305-config-include.sh\n+++ b/t/t1305-config-include.sh\n@@ -1,6 +1,7 @@\n #!/bin/sh\n \n test_description='test config file include directives'\n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n # Force setup_explicit_git_dir() to run until the end. This is needed\n-- \n2.33.1.1494.g88b39a443e1\n\n"},{"id":"439403","messageId":"patch-v3-5.6-9a44204c4c9-20211022T175227Z-avarab@gmail.com","threadId":"56746","inReplyTo":"cover-v3-0.6-00000000000-20211022T175227Z-avarab@gmail.com","subject":"[PATCH v3 5/6] config.c: free(expanded) before die(), work around GCC oddity","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-22T18:19:38Z","receivedAt":"2021-10-22T18:19:55Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On my GCC version (10.2.1-6), but not the clang I have available t0017\nwill fail under SANITIZE=leak on optimization levels higher than -O0,\nwhich is annoying when combined with the change in 956d2e4639b (tests:\nadd a test mode for SANITIZE=leak, run it in CI, 2021-09-23).\n\nWe really do have a memory leak here in either case, as e.g. running\nthe pre-image under valgrind(1) will reveal. It's documented\nSANITIZE=leak (and \"address\", which exhibits the same behavior) might\ninteract with compiler optimization in this way in some cases. Since\nthis function is called recursively it's going to be especially\ninteresting as an optimization target.\n\nLet's work around this issue by freeing the \"expanded\" memory before\nwe call die(), using a combination of the \"goto cleanup\" pattern\nintroduced in a preceding commit, and the newly introduced\ndie_message() function.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n config.c | 15 ++++++++++-----\n 1 file changed, 10 insertions(+), 5 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex c5873f3a706..c36e85c2077 100644\n--- a/config.c\n+++ b/config.c\n@@ -132,6 +132,7 @@ static int handle_path_include(const char *path, struct config_include_data *inc\n \tint ret = 0;\n \tstruct strbuf buf = STRBUF_INIT;\n \tchar *expanded;\n+\tint exit_with = 0;\n \n \tif (!path)\n \t\treturn config_error_nonbool(\"include.path\");\n@@ -161,17 +162,21 @@ static int handle_path_include(const char *path, struct config_include_data *inc\n \t}\n \n \tif (!access_or_die(path, R_OK, 0)) {\n-\t\tif (++inc->depth > MAX_INCLUDE_DEPTH)\n-\t\t\tdie(_(include_depth_advice), MAX_INCLUDE_DEPTH, path,\n-\t\t\t    !cf ? \"<unknown>\" :\n-\t\t\t    cf->name ? cf->name :\n-\t\t\t    \"the command line\");\n+\t\tif (++inc->depth > MAX_INCLUDE_DEPTH) {\n+\t\t\texit_with = die_message(_(include_depth_advice),\n+\t\t\t\t\t\tMAX_INCLUDE_DEPTH, path,\n+\t\t\t\t\t\t!cf ? \"<unknown>\" : cf->name ?\n+\t\t\t\t\t\tcf->name : \"the command line\");\n+\t\t\tgoto cleanup;\n+\t\t}\n \t\tret = git_config_from_file(git_config_include, path, inc);\n \t\tinc->depth--;\n \t}\n cleanup:\n \tstrbuf_release(&buf);\n \tfree(expanded);\n+\tif (exit_with)\n+\t\texit(exit_with);\n \treturn ret;\n }\n \n-- \n2.33.1.1494.g88b39a443e1\n\n"},{"id":"439404","messageId":"patch-v3-6.6-d2f639b53cd-20211022T175227Z-avarab@gmail.com","threadId":"56746","inReplyTo":"cover-v3-0.6-00000000000-20211022T175227Z-avarab@gmail.com","subject":"[PATCH v3 6/6] refs: plug memory leak in repo_default_branch_name()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-22T18:19:39Z","receivedAt":"2021-10-22T18:19:56Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Fix a memory leak in repo_default_branch_name(), we'll leak memory\nbefore exit(128) here.\n\nNormally we would not care much about such leaks, we do leak the\nmemory, as e.g. valgrind(1) will report. But the more commonly used\nSANITIZE=leak mode will use GCC and Clang's LSAN mode will not\nnormally report such leaks.\n\nAt least one GCC version does that in this case, and having the tests\nfail under -O3 would be annoying, so let's free() the allocated memory\nhere.\n\nThis uses a new die_message() function introduced in a preceding\ncommit. That new function makes the flow around such code easier to\nmanage. In this case we can't free(ret) before the die().\n\nIn this case only the \"free(full_ref)\" appears to be needed, but since\nwe're freeing one let's free both, some other compiler or version\nmight arrange this code in such a way as to complain about \"ret\" too\nwith SANITIZE=leak, and valgrind(1) will do so in any case.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n refs.c | 8 +++++++-\n 1 file changed, 7 insertions(+), 1 deletion(-)\n\ndiff --git a/refs.c b/refs.c\nindex 7f019c2377e..2a816c9561d 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -577,6 +577,7 @@ char *repo_default_branch_name(struct repository *r, int quiet)\n \tconst char *config_display_key = \"init.defaultBranch\";\n \tchar *ret = NULL, *full_ref;\n \tconst char *env = getenv(\"GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\");\n+\tint exit_with = 0;\n \n \tif (env && *env)\n \t\tret = xstrdup(env);\n@@ -591,8 +592,13 @@ char *repo_default_branch_name(struct repository *r, int quiet)\n \n \tfull_ref = xstrfmt(\"refs/heads/%s\", ret);\n \tif (check_refname_format(full_ref, 0))\n-\t\tdie(_(\"invalid branch name: %s = %s\"), config_display_key, ret);\n+\t\texit_with = die_message(_(\"invalid branch name: %s = %s\"),\n+\t\t\t\t\tconfig_display_key, ret);\n \tfree(full_ref);\n+\tif (exit_with) {\n+\t\tfree(ret);\n+\t\texit(exit_with);\n+\t}\n \n \treturn ret;\n }\n-- \n2.33.1.1494.g88b39a443e1\n\n"},{"id":"439422","messageId":"xmqqh7d8eox7.fsf@gitster.g","threadId":"56746","inReplyTo":"211022.86ilxpj7si.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2 2/3] config.c: don't leak memory in handle_path_include()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-22T21:21:24Z","receivedAt":"2021-10-22T21:21:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n>> Not a problem introduced by this function, but if you look at this\n>> change with \"git show -W\", we'd notice that the function name on the\n>> hunk header looks strange.  I think we should add a blank line\n>> before the beginning of the function.\n>\n> I think this is a bug in -W, after all if without it we we show the\n> function context line, but with it we advance further, then that means\n> that -W didn't find the correct function boundary.\n\nThat's a chicken-and-egg argument, and I do not think it is a bug in\n\"-W\" nor the funcname regular expression pattern we use.  We expect\na blank line there and the pattern reflects that expectation, so not\nhaving an expected blank line is what causes this problem.\n\nIn any case, we should add a blank linke before the beginning of the\nfunction, and of course that is obviously outside the scope of these\npatches.\n"},{"id":"439434","messageId":"211023.86wnm4isfc.gmgdl@evledraar.gmail.com","threadId":"56746","inReplyTo":"xmqqh7d8eox7.fsf@gitster.g","subject":"Re: [PATCH v2 2/3] config.c: don't leak memory in handle_path_include()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-22T22:30:33Z","receivedAt":"2021-10-22T22:52:15Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Oct 22 2021, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n>\n>>> Not a problem introduced by this function, but if you look at this\n>>> change with \"git show -W\", we'd notice that the function name on the\n>>> hunk header looks strange.  I think we should add a blank line\n>>> before the beginning of the function.\n>>\n>> I think this is a bug in -W, after all if without it we we show the\n>> function context line, but with it we advance further, then that means\n>> that -W didn't find the correct function boundary.\n>\n> That's a chicken-and-egg argument, and I do not think it is a bug in\n> \"-W\" nor the funcname regular expression pattern we use.  We expect\n> a blank line there and the pattern reflects that expectation, so not\n> having an expected blank line is what causes this problem.\n>\n> In any case, we should add a blank linke before the beginning of the\n> function, and of course that is obviously outside the scope of these\n> patches.\n\nSort of, if you were running with the patch I posted at [1] you wouldn't\nsee the bad value at @@, but we still extend upwards with -W, which I\nconsider a bug.\n\nI.e. both the current context we display and the over-extension there is\nultimately a symptom of the same issue, which is that what we're doing\nwith -W gets conflated with behavior that makes sense without -W, notice\nhow if you do \"git log -W\" on anything that the @@ context we display is\nthe prototype of the function /above/ the one you're likely looking at\nthe code change in.\n\nSo the blank line is the cause of the over-extension, but we'd still\nshow the (IMO) incorrect context in either case.\n\nAnyway, as you say a discussion for some other thread. I've been meaning\nto get back to those patches at some point, the first problem is that\nour test coverage for what function context we should find when is\nreally lacking, so any changes in that part of the xdiff code are likely\nto break things. I had those tests, but they got lost in some\nbikeshedding...\n\n1. https://lore.kernel.org/git/20210215155020.2804-2-avarab@gmail.com/\n"},{"id":"439478","messageId":"xmqqmtmz55wi.fsf@gitster.g","threadId":"56746","inReplyTo":"patch-v3-1.6-fe8763337ed-20211022T175227Z-avarab@gmail.com","subject":"Re: [PATCH v3 1/6] usage.c: add a die_message() routine","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-24T05:49:17Z","receivedAt":"2021-10-24T06:01:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n\n> We have code in various places that would like to call die(), but\n> wants to defer the exit(128) it would invoke, e.g. to print an\n> additional message, or adjust the exit code. Add a die_message()\n> helper routine to bridge this gap in the API.\n>\n> Functionally this behaves just like the error() routine, except it'll\n> print a \"fatal: \" prefix, and it will exit with 128 instead of -1,\n\nexit with -> return?\n\n> this is so that caller can pas the return value to exit(128), instead\n> of having to hardcode \"128\".\n\nIs it just me or do your patch always have to do about the same\namount of seemingly unnecessary and/or unadvertised changes as the\nnecessary and/or advertised changes?  I agree that adding\ndie_message() that returns 128 after giving a message is an\nexcellent idea, and I can see that it is necessary and sufficient to\nachieve the above advertised goal, but I don't see any reason why\nset/get_message_routine() must exist, especially as a part of this\nstep.\n\nIOW, perhaps that half of this patch belongs to and should be\nsquashed into one of the later steps of this series?\n\n> A subsequent commit will migrate various callers that benefit from\n> this function over to it, for now we're just migrating trivial users\n> in usage.c itself.\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n> ---\n>  git-compat-util.h |  3 +++\n>  usage.c           | 46 ++++++++++++++++++++++++++++++++++------------\n>  2 files changed, 37 insertions(+), 12 deletions(-)\n>\n> diff --git a/git-compat-util.h b/git-compat-util.h\n> index 141bb86351e..c1bb32460b6 100644\n> --- a/git-compat-util.h\n> +++ b/git-compat-util.h\n> @@ -471,6 +471,7 @@ NORETURN void usage(const char *err);\n>  NORETURN void usagef(const char *err, ...) __attribute__((format (printf, 1, 2)));\n>  NORETURN void die(const char *err, ...) __attribute__((format (printf, 1, 2)));\n>  NORETURN void die_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));\n> +int die_message(const char *err, ...) __attribute__((format (printf, 1, 2)));\n>  int error(const char *err, ...) __attribute__((format (printf, 1, 2)));\n>  int error_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));\n>  void warning(const char *err, ...) __attribute__((format (printf, 1, 2)));\n> @@ -505,6 +506,8 @@ static inline int const_error(void)\n>  typedef void (*report_fn)(const char *, va_list params);\n>  \n>  void set_die_routine(NORETURN_PTR report_fn routine);\n> +void set_die_message_routine(report_fn routine);\n> +report_fn get_die_message_routine(void);\n>  void set_error_routine(report_fn routine);\n>  report_fn get_error_routine(void);\n>  void set_warn_routine(report_fn routine);\n> diff --git a/usage.c b/usage.c\n> index c7d233b0de9..3d4b90bce1f 100644\n> --- a/usage.c\n> +++ b/usage.c\n> @@ -55,6 +55,12 @@ static NORETURN void usage_builtin(const char *err, va_list params)\n>  \texit(129);\n>  }\n>  \n> +static void die_message_builtin(const char *err, va_list params)\n> +{\n> +\ttrace2_cmd_error_va(err, params);\n> +\tvreportf(\"fatal: \", err, params);\n> +}\n> +\n>  /*\n>   * We call trace2_cmd_error_va() in the below functions first and\n>   * expect it to va_copy 'params' before using it (because an 'ap' can\n> @@ -62,10 +68,9 @@ static NORETURN void usage_builtin(const char *err, va_list params)\n>   */\n>  static NORETURN void die_builtin(const char *err, va_list params)\n>  {\n> -\ttrace2_cmd_error_va(err, params);\n> -\n> -\tvreportf(\"fatal: \", err, params);\n> +\treport_fn die_message_fn = get_die_message_routine();\n>  \n> +\tdie_message_fn(err, params);\n>  \texit(128);\n>  }\n>  \n> @@ -109,6 +114,7 @@ static int die_is_recursing_builtin(void)\n>   * (ugh), so keep things static. */\n>  static NORETURN_PTR report_fn usage_routine = usage_builtin;\n>  static NORETURN_PTR report_fn die_routine = die_builtin;\n> +static report_fn die_message_routine = die_message_builtin;\n>  static report_fn error_routine = error_builtin;\n>  static report_fn warn_routine = warn_builtin;\n>  static int (*die_is_recursing)(void) = die_is_recursing_builtin;\n> @@ -118,6 +124,16 @@ void set_die_routine(NORETURN_PTR report_fn routine)\n>  \tdie_routine = routine;\n>  }\n>  \n> +void set_die_message_routine(report_fn routine)\n> +{\n> +\tdie_message_routine = routine;\n> +}\n> +\n> +report_fn get_die_message_routine(void)\n> +{\n> +\treturn die_message_routine;\n> +}\n> +\n>  void set_error_routine(report_fn routine)\n>  {\n>  \terror_routine = routine;\n> @@ -157,14 +173,23 @@ void NORETURN usage(const char *err)\n>  \tusagef(\"%s\", err);\n>  }\n>  \n> +#undef die_message\n> +int die_message(const char *err, ...)\n> +{\n> +\tva_list params;\n> +\n> +\tva_start(params, err);\n> +\tdie_message_routine(err, params);\n> +\tva_end(params);\n> +\treturn 128;\n> +}\n> +\n>  void NORETURN die(const char *err, ...)\n>  {\n>  \tva_list params;\n>  \n> -\tif (die_is_recursing()) {\n> -\t\tfputs(\"fatal: recursion detected in die handler\\n\", stderr);\n> -\t\texit(128);\n> -\t}\n> +\tif (die_is_recursing())\n> +\t\texit(die_message(\"recursion detected in die handler\"));\n>  \n>  \tva_start(params, err);\n>  \tdie_routine(err, params);\n> @@ -200,11 +225,8 @@ void NORETURN die_errno(const char *fmt, ...)\n>  \tchar buf[1024];\n>  \tva_list params;\n>  \n> -\tif (die_is_recursing()) {\n> -\t\tfputs(\"fatal: recursion detected in die_errno handler\\n\",\n> -\t\t\tstderr);\n> -\t\texit(128);\n> -\t}\n> +\tif (die_is_recursing())\n> +\t\texit(die_message(\"recursion detected in die_errno handler\"));\n>  \n>  \tva_start(params, fmt);\n>  \tdie_routine(fmt_with_err(buf, sizeof(buf), fmt), params);\n"},{"id":"439479","messageId":"xmqqilxn55rt.fsf@gitster.g","threadId":"56746","inReplyTo":"patch-v3-3.6-6b33e394b2f-20211022T175227Z-avarab@gmail.com","subject":"Re: [PATCH v3 3/6] usage.c + gc: add and use a die_message_errno()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-24T05:52:06Z","receivedAt":"2021-10-24T06:01:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n\n> Change code the \"error: \" output when we exit with 128 due to gc.log\n> errors to use a \"fatal: \" prefix instead. This adds a sibling function\n> to the die_errno() added in a preceding commit.\n>\n> Since it returns 128 instead of -1 we'll need to adjust\n> report_last_gc_error(). Let's adjust it while we're at it to not\n> conflate the \"should skip\" and \"exit with this non-zero code\"\n> conditions, as the caller is no longer hardcoding \"128\", but relying\n> on die_errno() to return a nen-zero exit() status.\n\nOK, that sort of makes sense, and I am very glad that you didn't add\ndie_message_errno() to [1/6].  Adding this function to support the\ncaller that will benefit by using it in this same commit makes quite\na lot of sense.\n\n> diff --git a/usage.c b/usage.c\n> index 3d4b90bce1f..efc2064dde3 100644\n> --- a/usage.c\n> +++ b/usage.c\n> @@ -233,6 +233,18 @@ void NORETURN die_errno(const char *fmt, ...)\n>  \tva_end(params);\n>  }\n>  \n> +#undef die_message_errno\n> +int die_message_errno(const char *fmt, ...)\n> +{\n> +\tchar buf[1024];\n> +\tva_list params;\n> +\n> +\tva_start(params, fmt);\n> +\tdie_message_routine(fmt_with_err(buf, sizeof(buf), fmt), params);\n> +\tva_end(params);\n> +\treturn -1;\n> +}\n> +\n>  #undef error_errno\n>  int error_errno(const char *fmt, ...)\n>  {\n"},{"id":"439480","messageId":"xmqqee8b55q1.fsf@gitster.g","threadId":"56746","inReplyTo":"patch-v3-4.6-3607b905627-20211022T175227Z-avarab@gmail.com","subject":"Re: [PATCH v3 4/6] config.c: don't leak memory in handle_path_include()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-24T05:53:10Z","receivedAt":"2021-10-24T06:01:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n\n> Fix a memory leak in the error() path in handle_path_include(), this\n> allows us to run t1305-config-include.sh under SANITIZE=leak,\n> previously 4 tests there would fail. This fixes up a leak in\n> 9b25a0b52e0 (config: add include directive, 2012-02-06).\n\nI think this has been queued already separately and in 'next'.\n"},{"id":"439632","messageId":"YXfCH7I1XwH+Vetu@coredump.intra.peff.net","threadId":"56746","inReplyTo":"patch-v3-5.6-9a44204c4c9-20211022T175227Z-avarab@gmail.com","subject":"Re: [PATCH v3 5/6] config.c: free(expanded) before die(), work around GCC oddity","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-10-26T08:53:51Z","receivedAt":"2021-10-26T08:53:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 22, 2021 at 08:19:38PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> On my GCC version (10.2.1-6), but not the clang I have available t0017\n> will fail under SANITIZE=leak on optimization levels higher than -O0,\n> which is annoying when combined with the change in 956d2e4639b (tests:\n> add a test mode for SANITIZE=leak, run it in CI, 2021-09-23).\n\nThis one really makes me sad. The resulting code is more complicated,\nand what guarantee do we have that we won't run into similar problems\nwith other die() calls?\n\nIf we're getting false positives, I'd rather see us work around them\nwith annotations, or a better compiler (I couldn't reproduce with gcc\n10.3.0 or 11.2.0 from Debian, so I doubt there is even much point in\nreporting it upstream).\n\n> We really do have a memory leak here in either case, as e.g. running\n> the pre-image under valgrind(1) will reveal. It's documented\n> SANITIZE=leak (and \"address\", which exhibits the same behavior) might\n> interact with compiler optimization in this way in some cases. Since\n> this function is called recursively it's going to be especially\n> interesting as an optimization target.\n\nI don't see how we have a leak. If we hit this die code-path then we\nnever exit the function. I can't reproduce the problem, but it sounds\nlike -O2 is reusing the stack space of \"expanded\" to prepare for the\ndie() call? IMHO that is not an actual leak. It is still in scope from\nthe perspective of C, and anyway we are about to exit from within the\ndie().\n\nIf we were to do anything in the code itself, I'd much prefer to hit it\nwith an UNLEAK().\n\n-Peff\n"},{"id":"439838","messageId":"20211027215053.2257548-1-jonathantanmy@google.com","threadId":"56746","inReplyTo":"cover-v3-0.6-00000000000-20211022T175227Z-avarab@gmail.com","subject":"Re: [PATCH v3 0/6] usage.c: add die_message() & plug memory leaks in refs.c & config.c","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-10-27T21:50:53Z","receivedAt":"2021-10-27T21:50:58Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> What started as a set of small memory leak fixes is now adding and\n> usinge a die_message() function. This non-fatal-die() is useful to\n> various callers that want to print \"fatal: \" before exiting, but don't\n> want to call die() for whatever reason.\n> \n> I wasn't planning to submit that now, but these were incomplete\n> patches I had lying around, and make the 5th and 6th patch much nicer,\n> in response to comments on v1 and v2 to the effect that managing\n> free()-ing around die() functions was rather nasty.\n> \n> This doesn't conflict with anything in-flight, and the changes\n> themselves are rather simple.\n\nIs this mainly to make a CI work, or just so that a certain set of tools\nwork? (You mention an old version of GCC in patch 5.) If for CI, I think\nthat there might be a sufficient version to fix this, but if not, I\nwould think that something less intrusive would be better (e.g. a\ncomment that certain versions do not work).\n\nAs for patches 5 and 6, I think that any leak detection we use has to\nconsider any pointers still on the stack as \"live\" - if not we wouldn't\nbe able to die without returning back to the topmost function since any\nintermediate function could have allocations (unless I'm mistaking\nsomething). (Unless die() is somehow overwriting the stackframe through\nsome tail call optimization or something - in which case maybe what we\nshould do is to disable the tail call optimization when we are checking\nfor leaks.)\n"}]}