{"thread":{"id":"57597","subject":"[PATCH] hooks: fix \"invoked hook\" regression in a8cc5943338","startedAt":"2022-03-21T23:23:10Z","lastAt":"2022-03-22T10:19:40Z","messageCount":2,"participants":["Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"451820","messageId":"patch-1.1-d0c9b430b2c-20220321T230933Z-avarab@gmail.com","threadId":"57597","inReplyTo":"0220321203019.2614799-1-jonathantanmy@google.com","subject":"[PATCH] hooks: fix \"invoked hook\" regression in a8cc5943338","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-03-21T23:15:13Z","receivedAt":"2022-03-21T23:23:10Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Fix a regression in a8cc5943338 (hooks: fix an obscure TOCTOU \"did we\njust run a hook?\" race, 2022-03-07): The \"invoked_hook\" variable\npassed to run_commit_hook() wasn't passed forward to run_hooks_opt(),\nas push_to_checkout() in that commit correctly did.\n\nWhether we ran the code contingent on having run the hook or not was\nthus undefined, but in practice on most (all?) modern platforms we'd\nhave run it (almost?) all the time, since stack variables will get\ninitialized to some random value, which most of the time isn't \"0\".\n\nThis bug was revealed by running e.g. \"t5537-fetch-shallow.sh\" with\nthe --valgrind option. Unfortunately running the whole test suite with\n--valgrind is really slow, so we didn't have a CI job that spotted\nthis. The --valgrind output was:\n\n    ==31275== Conditional jump or move depends on uninitialised value(s)\n    ==31275==    at 0x43C63F: prepare_to_commit (commit.c:1058)\n    ==31275==    by 0x4396A5: cmd_commit (commit.c:1722)\n    ==31275==    by 0x407C8A: run_builtin (git.c:465)\n    ==31275==    by 0x406741: handle_builtin (git.c:718)\n    ==31275==    by 0x407665: run_argv (git.c:785)\n    ==31275==    by 0x406500: cmd_main (git.c:916)\n    ==31275==    by 0x510839: main (common-main.c:56)\n    ==31275==  Uninitialised value was created by a stack allocation\n    ==31275==    at 0x43B344: prepare_to_commit (commit.c:719)\n\nReported-by: Jonathan Tan <jonathantanmy@google.com>\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n\nOn Mon, Mar 21 2022, Jonathan Tan wrote:\n\n> This commit causes Git to fail Valgrind (tested using \"cd t && sh\n> t5537*.sh -i -v --valgrind-only=10\"). You can see here that a caller of\n> run_commit_hook() expects invoked_hook to be set, but...\n\nThat's a really stupid bug, sorry :( This fixes it.\n\n commit.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/commit.c b/commit.c\nindex 98b2e556653..ec1207ebef4 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -1732,5 +1732,6 @@ int run_commit_hook(int editor_is_used, const char *index_file,\n \t\tstrvec_push(&opt.args, arg);\n \tva_end(args);\n \n+\topt.invoked_hook = invoked_hook;\n \treturn run_hooks_opt(name, &opt);\n }\n-- \n2.35.1.1441.g1e9a595f48f\n\n"},{"id":"451839","messageId":"220322.86ee2unvbe.gmgdl@evledraar.gmail.com","threadId":"57597","inReplyTo":"patch-1.1-d0c9b430b2c-20220321T230933Z-avarab@gmail.com","subject":"Re: [PATCH] hooks: fix \"invoked hook\" regression in a8cc5943338","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-03-22T10:12:38Z","receivedAt":"2022-03-22T10:19:40Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Mar 22 2022, Ævar Arnfjörð Bjarmason wrote:\n\n> Fix a regression in a8cc5943338 (hooks: fix an obscure TOCTOU \"did we\n> just run a hook?\" race, 2022-03-07): The \"invoked_hook\" variable\n> passed to run_commit_hook() wasn't passed forward to run_hooks_opt(),\n> as push_to_checkout() in that commit correctly did.\n>\n> Whether we ran the code contingent on having run the hook or not was\n> thus undefined, but in practice on most (all?) modern platforms we'd\n> have run it (almost?) all the time, since stack variables will get\n> initialized to some random value, which most of the time isn't \"0\".\n>\n> This bug was revealed by running e.g. \"t5537-fetch-shallow.sh\" with\n> the --valgrind option. Unfortunately running the whole test suite with\n> --valgrind is really slow, so we didn't have a CI job that spotted\n> this. The --valgrind output was:\n>\n>     ==31275== Conditional jump or move depends on uninitialised value(s)\n>     ==31275==    at 0x43C63F: prepare_to_commit (commit.c:1058)\n>     ==31275==    by 0x4396A5: cmd_commit (commit.c:1722)\n>     ==31275==    by 0x407C8A: run_builtin (git.c:465)\n>     ==31275==    by 0x406741: handle_builtin (git.c:718)\n>     ==31275==    by 0x407665: run_argv (git.c:785)\n>     ==31275==    by 0x406500: cmd_main (git.c:916)\n>     ==31275==    by 0x510839: main (common-main.c:56)\n>     ==31275==  Uninitialised value was created by a stack allocation\n>     ==31275==    at 0x43B344: prepare_to_commit (commit.c:719)\n>\n> Reported-by: Jonathan Tan <jonathantanmy@google.com>\n> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n> ---\n>\n> On Mon, Mar 21 2022, Jonathan Tan wrote:\n>\n>> This commit causes Git to fail Valgrind (tested using \"cd t && sh\n>> t5537*.sh -i -v --valgrind-only=10\"). You can see here that a caller of\n>> run_commit_hook() expects invoked_hook to be set, but...\n>\n> That's a really stupid bug, sorry :( This fixes it.\n>\n>  commit.c | 1 +\n>  1 file changed, 1 insertion(+)\n>\n> diff --git a/commit.c b/commit.c\n> index 98b2e556653..ec1207ebef4 100644\n> --- a/commit.c\n> +++ b/commit.c\n> @@ -1732,5 +1732,6 @@ int run_commit_hook(int editor_is_used, const char *index_file,\n>  \t\tstrvec_push(&opt.args, arg);\n>  \tva_end(args);\n>  \n> +\topt.invoked_hook = invoked_hook;\n>  \treturn run_hooks_opt(name, &opt);\n>  }\n\nI tried adding a valgrind CI job, but even running --valgrind-only=1\n(i.e. only the first test in each file would run with valgrind) took\naround 5h40m to complete with the patch below.\n\nI wonder if it's still worth it in some form, it would suck to have to\nwait that long by default for \"green\" CI, but e.g. for Junio's \"master\"\nand \"next\" (and maybe \"seen\") maybe it's fine if some of the extended\nchecks take a long time to complete. It would have caught the problem in\nthis case.\n\nWe could probably run the entire test suite through valgrind by sharding\nthe test jobs similarly to what the Windows jobs do now, and could also\nrun e.g. SANITIZE=address and any other opt-in long-running CI.\n\ndiff --git a/.github/workflows/main.yml b/.github/workflows/main.yml\nindex c35200defb9..58a0c0c8966 100644\n--- a/.github/workflows/main.yml\n+++ b/.github/workflows/main.yml\n@@ -251,6 +251,9 @@ jobs:\n           - jobname: linux-leaks\n             cc: gcc\n             pool: ubuntu-latest\n+          - jobname: linux-valgrind\n+            cc: gcc\n+            pool: ubuntu-latest\n     env:\n       CC: ${{matrix.vector.cc}}\n       CC_PACKAGE: ${{matrix.vector.cc_package}}\ndiff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh\nindex dbcebad2fb2..098ffde3a57 100755\n--- a/ci/install-dependencies.sh\n+++ b/ci/install-dependencies.sh\n@@ -76,6 +76,9 @@ linux-gcc-default)\n \tsudo apt-get -q update\n \tsudo apt-get -q -y install $UBUNTU_COMMON_PKGS\n \t;;\n+linux-valgrind)\n+\tsudo apt-get -q -y install valgrind\n+\t;;\n esac\n \n if type p4d >/dev/null && type p4 >/dev/null\ndiff --git a/ci/lib.sh b/ci/lib.sh\nindex cbc2f8f1caa..1a78dab7a47 100755\n--- a/ci/lib.sh\n+++ b/ci/lib.sh\n@@ -208,6 +208,10 @@ linux-leaks)\n \texport SANITIZE=leak\n \texport GIT_TEST_PASSING_SANITIZE_LEAK=true\n \t;;\n+linux-valgrind)\n+\tGIT_TEST_OPTS=\"$GIT_TEST_OPTS --valgrind-only=1\"\n+\texport GIT_TEST_OPTS\n+\t;;\n esac\n \n MAKEFLAGS=\"$MAKEFLAGS CC=${CC:-cc}\"\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 9af5fb7674d..8157edcfee0 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -274,7 +274,7 @@ fi\n if test -n \"$valgrind_only\"\n then\n \ttest -z \"$valgrind\" && valgrind=memcheck\n-\ttest -z \"$verbose\" && verbose_only=\"$valgrind_only\"\n+\ttest -z \"$verbose$verbose_log\" && verbose_only=\"$valgrind_only\"\n elif test -n \"$valgrind\"\n then\n \ttest -z \"$verbose_log\" && verbose=t\n\n"}]}