Re: [PATCH] hooks: fix "invoked hook" regression in a8cc5943338
On Tue, Mar 22 2022, Ævar Arnfjörð Bjarmason wrote:
Show 52 quoted lines
> 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.
Show changes to 4 files +11 −1
.github/workflows/main.yml, ci/install-dependencies.sh, ci/lib.sh, t/test-lib.sh
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
Ævar Arnfjörð Bjarmason