Re: [PATCH v2 2/2] hooks: fix an obscure TOCTOU "did we just run a hook?" race

[Jonathan Tan <jonathantanmy@google.com>]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
> 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...

> 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.h

The 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?