[PATCH] hooks: fix "invoked hook" regression in a8cc5943338

[PATCH] hooks: fix "invoked hook" regression in a8cc5943338

[Ævar Arnfjörð Bjarmason]

Fix a regression in a8cc5943338 (hooks: fix an obscure TOCTOU "did we
just run a hook?" race, 2022-03-07): The "invoked_hook" variable
passed to run_commit_hook() wasn't passed forward to run_hooks_opt(),
as push_to_checkout() in that commit correctly did.

Whether we ran the code contingent on having run the hook or not was
thus undefined, but in practice on most (all?) modern platforms we'd
have run it (almost?) all the time, since stack variables will get
initialized to some random value, which most of the time isn't "0".

This bug was revealed by running e.g. "t5537-fetch-shallow.sh" with
the --valgrind option. Unfortunately running the whole test suite with
--valgrind is really slow, so we didn't have a CI job that spotted
this. The --valgrind output was:

    ==31275== Conditional jump or move depends on uninitialised value(s)
    ==31275==    at 0x43C63F: prepare_to_commit (commit.c:1058)
    ==31275==    by 0x4396A5: cmd_commit (commit.c:1722)
    ==31275==    by 0x407C8A: run_builtin (git.c:465)
    ==31275==    by 0x406741: handle_builtin (git.c:718)
    ==31275==    by 0x407665: run_argv (git.c:785)
    ==31275==    by 0x406500: cmd_main (git.c:916)
    ==31275==    by 0x510839: main (common-main.c:56)
    ==31275==  Uninitialised value was created by a stack allocation
    ==31275==    at 0x43B344: prepare_to_commit (commit.c:719)

Reported-by: Jonathan Tan <jonathantanmy@google.com>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

On Mon, Mar 21 2022, Jonathan Tan wrote:

> This commit causes Git to fail Valgrind (tested using "cd t && sh
> t5537*.sh -i -v --valgrind-only=10"). You can see here that a caller of
> run_commit_hook() expects invoked_hook to be set, but...

That's a really stupid bug, sorry :( This fixes it.

 commit.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/commit.c b/commit.c
index 98b2e556653..ec1207ebef4 100644
--- a/commit.c
+++ b/commit.c
@@ -1732,5 +1732,6 @@ int run_commit_hook(int editor_is_used, const char *index_file,
 		strvec_push(&opt.args, arg);
 	va_end(args);
 
+	opt.invoked_hook = invoked_hook;
 	return run_hooks_opt(name, &opt);
 }
-- 
2.35.1.1441.g1e9a595f48f

Re: [PATCH] hooks: fix "invoked hook" regression in a8cc5943338

[Ævar Arnfjörð Bjarmason]

On Tue, Mar 22 2022, Ævar Arnfjörð Bjarmason wrote:

> Fix a regression in a8cc5943338 (hooks: fix an obscure TOCTOU "did we
> just run a hook?" race, 2022-03-07): The "invoked_hook" variable
> passed to run_commit_hook() wasn't passed forward to run_hooks_opt(),
> as push_to_checkout() in that commit correctly did.
>
> Whether we ran the code contingent on having run the hook or not was
> thus undefined, but in practice on most (all?) modern platforms we'd
> have run it (almost?) all the time, since stack variables will get
> initialized to some random value, which most of the time isn't "0".
>
> This bug was revealed by running e.g. "t5537-fetch-shallow.sh" with
> the --valgrind option. Unfortunately running the whole test suite with
> --valgrind is really slow, so we didn't have a CI job that spotted
> this. The --valgrind output was:
>
>     ==31275== Conditional jump or move depends on uninitialised value(s)
>     ==31275==    at 0x43C63F: prepare_to_commit (commit.c:1058)
>     ==31275==    by 0x4396A5: cmd_commit (commit.c:1722)
>     ==31275==    by 0x407C8A: run_builtin (git.c:465)
>     ==31275==    by 0x406741: handle_builtin (git.c:718)
>     ==31275==    by 0x407665: run_argv (git.c:785)
>     ==31275==    by 0x406500: cmd_main (git.c:916)
>     ==31275==    by 0x510839: main (common-main.c:56)
>     ==31275==  Uninitialised value was created by a stack allocation
>     ==31275==    at 0x43B344: prepare_to_commit (commit.c:719)
>
> Reported-by: Jonathan Tan <jonathantanmy@google.com>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>
> On Mon, Mar 21 2022, Jonathan Tan wrote:
>
>> This commit causes Git to fail Valgrind (tested using "cd t && sh
>> t5537*.sh -i -v --valgrind-only=10"). You can see here that a caller of
>> run_commit_hook() expects invoked_hook to be set, but...
>
> That's a really stupid bug, sorry :( This fixes it.
>
>  commit.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/commit.c b/commit.c
> index 98b2e556653..ec1207ebef4 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -1732,5 +1732,6 @@ int run_commit_hook(int editor_is_used, const char *index_file,
>  		strvec_push(&opt.args, arg);
>  	va_end(args);
>  
> +	opt.invoked_hook = invoked_hook;
>  	return run_hooks_opt(name, &opt);
>  }

I tried adding a valgrind CI job, but even running --valgrind-only=1
(i.e. only the first test in each file would run with valgrind) took
around 5h40m to complete with the patch below.

I wonder if it's still worth it in some form, it would suck to have to
wait that long by default for "green" CI, but e.g. for Junio's "master"
and "next" (and maybe "seen") maybe it's fine if some of the extended
checks take a long time to complete. It would have caught the problem in
this case.

We could probably run the entire test suite through valgrind by sharding
the test jobs similarly to what the Windows jobs do now, and could also
run e.g. SANITIZE=address and any other opt-in long-running CI.

diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
index c35200defb9..58a0c0c8966 100644
--- a/.github/workflows/main.yml
+++ b/.github/workflows/main.yml
@@ -251,6 +251,9 @@ jobs:
           - jobname: linux-leaks
             cc: gcc
             pool: ubuntu-latest
+          - jobname: linux-valgrind
+            cc: gcc
+            pool: ubuntu-latest
     env:
       CC: ${{matrix.vector.cc}}
       CC_PACKAGE: ${{matrix.vector.cc_package}}
diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
index dbcebad2fb2..098ffde3a57 100755
--- a/ci/install-dependencies.sh
+++ b/ci/install-dependencies.sh
@@ -76,6 +76,9 @@ linux-gcc-default)
 	sudo apt-get -q update
 	sudo apt-get -q -y install $UBUNTU_COMMON_PKGS
 	;;
+linux-valgrind)
+	sudo apt-get -q -y install valgrind
+	;;
 esac
 
 if type p4d >/dev/null && type p4 >/dev/null
diff --git a/ci/lib.sh b/ci/lib.sh
index cbc2f8f1caa..1a78dab7a47 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -208,6 +208,10 @@ linux-leaks)
 	export SANITIZE=leak
 	export GIT_TEST_PASSING_SANITIZE_LEAK=true
 	;;
+linux-valgrind)
+	GIT_TEST_OPTS="$GIT_TEST_OPTS --valgrind-only=1"
+	export GIT_TEST_OPTS
+	;;
 esac
 
 MAKEFLAGS="$MAKEFLAGS CC=${CC:-cc}"
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 9af5fb7674d..8157edcfee0 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -274,7 +274,7 @@ fi
 if test -n "$valgrind_only"
 then
 	test -z "$valgrind" && valgrind=memcheck
-	test -z "$verbose" && verbose_only="$valgrind_only"
+	test -z "$verbose$verbose_log" && verbose_only="$valgrind_only"
 elif test -n "$valgrind"
 then
 	test -z "$verbose_log" && verbose=t