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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxx>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx>
---

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




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux