Re: [PATCH v2 2/2] hooks: fix an obscure TOCTOU "did we just run a hook?" race
- From
- Jonathan Tan <jonathantanmy@google.com>
- Date
- Mar 21, 2022, 20:30 UTC
- Message-ID
- <20220321203019.2614799-1-jonathantanmy@google.com>
- In-Reply-To
- <patch-v2-2.2-9d16984898c-20220307T123244Z-avarab@gmail.com>
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
Show 25 quoted lines
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 8b8bdad3959..009a1de0a3d 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -726,11 +726,13 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
> int clean_message_contents = (cleanup_mode != COMMIT_MSG_CLEANUP_NONE);
> int old_display_comment_prefix;
> int merge_contains_scissors = 0;
> + int invoked_hook;
>
> /* This checks and barfs if author is badly specified */
> determine_author_info(author_ident);
>
> - if (!no_verify && run_commit_hook(use_editor, index_file, "pre-commit", NULL))
> + if (!no_verify && run_commit_hook(use_editor, index_file, &invoked_hook,
> + "pre-commit", NULL))
> return 0;
>
> if (squash_message) {
> @@ -1053,10 +1055,10 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
> return 0;
> }
>
> - if (!no_verify && hook_exists("pre-commit")) {
> + if (!no_verify && invoked_hook) {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...
Show 14 quoted lines
> diff --git a/commit.c b/commit.c
> index d400f5dfa2b..396e14d7b32 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -1713,7 +1713,7 @@ size_t ignore_non_trailer(const char *buf, size_t len)
> }
>
> int run_commit_hook(int editor_is_used, const char *index_file,
> - const char *name, ...)
> + int *invoked_hook, const char *name, ...)
> {
> struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT;
> va_list args;
> diff --git a/commit.h b/commit.hThe quoted part is the entire diff of commit.c. You can see that we have a new argument "int *invoked_hook", but don't actually do anything with it. Could you (Ævar) take a look?