* Re: [PATCH] hooks: fix "invoked hook" regression in a8cc5943338
2022-03-21 23:15 ` [PATCH] hooks: fix "invoked hook" regression in a8cc5943338 Ævar Arnfjörð Bjarmason
@ 2022-03-22 10:12 ` Ævar Arnfjörð Bjarmason
0 siblings, 0 replies; 2+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-03-22 10:12 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jonathan Tan, Bagas Sanjaya, Emily Shaffer,
Æ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
^ permalink raw reply related [flat|nested] 2+ messages in thread