{"thread":{"id":"57439","subject":"[PATCH 0/2] hooks: fix a race in hook execution","startedAt":"2022-02-18T20:44:05Z","lastAt":"2022-03-21T20:30:26Z","messageCount":12,"participants":["Ævar Arnfjörð Bjarmason","Junio C Hamano","Taylor Blau","Jonathan Tan"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"448803","messageId":"cover-0.2-00000000000-20220218T203834Z-avarab@gmail.com","threadId":"57439","inReplyTo":null,"subject":"[PATCH 0/2] hooks: fix a race in hook execution","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-02-18T20:43:50Z","receivedAt":"2022-02-18T20:44:05Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Follow-up on the recently landed c70bc338e9a (Merge branch\n'ab/config-based-hooks-2', 2022-02-09). This fixes an obscure race\ncondition in how we execute a few hooks, because we'd run the hook,\nand then check if the hook existed.\n\nWe could thus skip some post-hook logic if a hook was run, but we\nraced with the hook being removed from the repository.\n\nAs 2/2 notes being worried about that isn't very realistic, but it\nmakes sense to have the hook API expose a \"did I run it?\" as part of\nits API, so let's add such a flag, and use it in those cases.\n\nThe 2/2 has been on-list before as part of a much bigger hook topic\nsubmission[1]. I fixed up a few things in it, and added 1/2. The\nrange-diff below is to that previous submission.\n\n1. https://lore.kernel.org/git/patch-v5-36.36-fe056098534-20210902T125111Z-avarab@gmail.com/\n\nÆvar Arnfjörð Bjarmason (2):\n  merge: don't run post-hook logic on --no-verify\n  hooks: fix a TOCTOU in \"did we run a hook?\" heuristic\n\n builtin/commit.c       | 18 +++++++++++-------\n builtin/merge.c        | 28 +++++++++++++++++-----------\n builtin/receive-pack.c |  8 +++++---\n commit.c               |  2 +-\n commit.h               |  3 ++-\n hook.c                 |  7 +++++++\n hook.h                 |  9 +++++++++\n sequencer.c            |  4 ++--\n 8 files changed, 54 insertions(+), 25 deletions(-)\n\nRange-diff:\n-:  ----------- > 1:  9b5144daee6 merge: don't run post-hook logic on --no-verify\n1:  fe056098534 ! 2:  d01d088073b hooks: fix a TOCTOU in \"did we run a hook?\" heuristic\n    @@ builtin/commit.c: static int prepare_to_commit(const char *index_file, const cha\n      \tint clean_message_contents = (cleanup_mode != COMMIT_MSG_CLEANUP_NONE);\n      \tint old_display_comment_prefix;\n      \tint merge_contains_scissors = 0;\n    -+\tint invoked_hook = 0;\n    ++\tint invoked_hook;\n      \n      \t/* This checks and barfs if author is badly specified */\n      \tdetermine_author_info(author_ident);\n    @@ builtin/commit.c: int cmd_commit(int argc, const char **argv, const char *prefix\n     \n      ## builtin/merge.c ##\n     @@ builtin/merge.c: static void prepare_to_commit(struct commit_list *remoteheads)\n    - {\n    - \tstruct strbuf msg = STRBUF_INIT;\n      \tconst char *index_file = get_index_file();\n    -+\tint invoked_hook = 0;\n      \n    --\tif (!no_verify && run_commit_hook(0 < option_edit, index_file, \"pre-merge-commit\", NULL))\n    -+\tif (!no_verify && run_commit_hook(0 < option_edit, index_file,\n    -+\t\t\t\t\t  &invoked_hook, \"pre-merge-commit\",\n    -+\t\t\t\t\t  NULL))\n    - \t\tabort_commit(remoteheads, NULL);\n    - \t/*\n    --\t * Re-read the index as pre-merge-commit hook could have updated it,\n    --\t * and write it out as a tree.  We must do this before we invoke\n    -+\t * Re-read the index as the pre-merge-commit hook was invoked\n    -+\t * and could have updated it. We must do this before we invoke\n    - \t * the editor and after we invoke run_status above.\n    - \t */\n    --\tif (hook_exists(\"pre-merge-commit\"))\n    -+\tif (invoked_hook)\n    - \t\tdiscard_cache();\n    + \tif (!no_verify) {\n    +-\t\tif (run_commit_hook(0 < option_edit, index_file,\n    ++\t\tint invoked_hook;\n    ++\n    ++\t\tif (run_commit_hook(0 < option_edit, index_file, &invoked_hook,\n    + \t\t\t\t    \"pre-merge-commit\", NULL))\n    + \t\t\tabort_commit(remoteheads, NULL);\n    + \t\t/*\n    +@@ builtin/merge.c: static void prepare_to_commit(struct commit_list *remoteheads)\n    + \t\t * and write it out as a tree.  We must do this before we invoke\n    + \t\t * the editor and after we invoke run_status above.\n    + \t\t */\n    +-\t\tif (hook_exists(\"pre-merge-commit\"))\n    ++\t\tif (invoked_hook)\n    + \t\t\tdiscard_cache();\n    + \t}\n      \tread_cache_from(index_file);\n    - \tstrbuf_addbuf(&msg, &merge_msg);\n     @@ builtin/merge.c: static void prepare_to_commit(struct commit_list *remoteheads)\n      \t\tappend_signoff(&msg, ignore_non_trailer(msg.buf, msg.len), 0);\n      \twrite_merge_heads(remoteheads);\n    @@ builtin/receive-pack.c: static const char *push_to_deploy(unsigned char *sha1,\n      \tstrvec_pushv(&opt.env, env->v);\n     @@ builtin/receive-pack.c: static const char *update_worktree(unsigned char *sha1, const struct worktree *w\n      {\n    - \tconst char *retval, *work_tree, *git_dir = NULL;\n    + \tconst char *retval, *git_dir;\n      \tstruct strvec env = STRVEC_INIT;\n    -+\tint invoked_hook = 0;\n    ++\tint invoked_hook;\n      \n    - \tif (worktree && worktree->path)\n    - \t\twork_tree = worktree->path;\n    + \tif (!worktree || !worktree->path)\n    + \t\tBUG(\"worktree->path must be non-NULL\");\n     @@ builtin/receive-pack.c: static const char *update_worktree(unsigned char *sha1, const struct worktree *w\n      \n      \tstrvec_pushf(&env, \"GIT_DIR=%s\", absolute_path(git_dir));\n      \n     -\tif (!hook_exists(push_to_checkout_hook))\n    -+\tretval = push_to_checkout(sha1, &invoked_hook, &env, work_tree);\n    ++\tretval = push_to_checkout(sha1, &invoked_hook, &env, worktree->path);\n     +\tif (!invoked_hook)\n    - \t\tretval = push_to_deploy(sha1, &env, work_tree);\n    + \t\tretval = push_to_deploy(sha1, &env, worktree->path);\n     -\telse\n    --\t\tretval = push_to_checkout(sha1, &env, work_tree);\n    +-\t\tretval = push_to_checkout(sha1, &env, worktree->path);\n      \n      \tstrvec_clear(&env);\n      \treturn retval;\n    @@ commit.c: size_t ignore_non_trailer(const char *buf, size_t len)\n      }\n      \n      int run_commit_hook(int editor_is_used, const char *index_file,\n    -+\t\t    int *invoked_hook,\n    - \t\t    const char *name, ...)\n    +-\t\t    const char *name, ...)\n    ++\t\t    int *invoked_hook, const char *name, ...)\n      {\n      \tstruct run_hooks_opt opt = RUN_HOOKS_OPT_INIT;\n    + \tva_list args;\n     \n      ## commit.h ##\n     @@ commit.h: int compare_commits_by_commit_date(const void *a_, const void *b_, void *unused)\n    @@ commit.h: int compare_commits_by_commit_date(const void *a_, const void *b_, voi\n     \n      ## hook.c ##\n     @@ hook.c: static int notify_hook_finished(int result,\n    + \t\t\t\tvoid *pp_task_cb)\n    + {\n    + \tstruct hook_cb_data *hook_cb = pp_cb;\n    ++\tstruct run_hooks_opt *opt = hook_cb->options;\n      \n      \thook_cb->rc |= result;\n      \n    -+\tif (hook_cb->invoked_hook)\n    -+\t\t*hook_cb->invoked_hook = 1;\n    ++\tif (opt->invoked_hook)\n    ++\t\t*opt->invoked_hook = 1;\n     +\n      \treturn 0;\n      }\n      \n    -@@ hook.c: int run_hooks(const char *hook_name, const char *hook_path,\n    - \t\t.rc = 0,\n    - \t\t.hook_name = hook_name,\n    - \t\t.options = options,\n    -+\t\t.invoked_hook = options->invoked_hook,\n    - \t};\n    - \tint jobs = 1;\n    +@@ hook.c: int run_hooks_opt(const char *hook_name, struct run_hooks_opt *options)\n    + \tif (!options)\n    + \t\tBUG(\"a struct run_hooks_opt must be provided to run_hooks\");\n    + \n    ++\tif (options->invoked_hook)\n    ++\t\t*options->invoked_hook = 0;\n    ++\n    + \tif (!hook_path && !options->error_if_missing)\n    + \t\tgoto cleanup;\n      \n     \n      ## hook.h ##\n     @@ hook.h: struct run_hooks_opt\n    - \t * for an example.\n    + \t * translates to \"struct child_process\"'s \"dir\" member.\n      \t */\n    - \tconsume_sideband_fn consume_sideband;\n    + \tconst char *dir;\n     +\n    -+\t/*\n    ++\t/**\n     +\t * A pointer which if provided will be set to 1 or 0 depending\n     +\t * on if a hook was invoked (i.e. existed), regardless of\n     +\t * whether or not that was successful. Used for avoiding\n    @@ hook.h: struct run_hooks_opt\n      };\n      \n      #define RUN_HOOKS_OPT_INIT { \\\n    -@@ hook.h: struct hook_cb_data {\n    - \tconst char *hook_name;\n    - \tstruct hook *run_me;\n    - \tstruct run_hooks_opt *options;\n    -+\tint *invoked_hook;\n    - };\n    - \n    - /**\n     \n      ## sequencer.c ##\n     @@ sequencer.c: static int run_prepare_commit_msg_hook(struct repository *r,\n-- \n2.35.1.1031.g277d4562d2e\n\n"},{"id":"448804","messageId":"patch-1.2-9b5144daee6-20220218T203834Z-avarab@gmail.com","threadId":"57439","inReplyTo":"cover-0.2-00000000000-20220218T203834Z-avarab@gmail.com","subject":"[PATCH 1/2] merge: don't run post-hook logic on --no-verify","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-02-18T20:43:51Z","receivedAt":"2022-02-18T20:44:06Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Fix a minor bug introduced in bc40ce4de61 (merge: --no-verify to\nbypass pre-merge-commit hook, 2019-08-07), when that change made the\n--no-verify option bypass the \"pre-merge-commit\" hook it didn't update\nthe corresponding find_hook() (later hook_exists()) condition.\n\nAs can be seen in the preceding commit in 6098817fd7f (git-merge:\nhonor pre-merge-commit hook, 2019-08-07) the two should go hand in\nhand. There's no point in invoking discard_cache() here if the hook\ncouldn't have possibly updated the index.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n builtin/merge.c | 21 ++++++++++++---------\n 1 file changed, 12 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 133831d42fd..fab553e3bc4 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -845,15 +845,18 @@ static void prepare_to_commit(struct commit_list *remoteheads)\n \tstruct strbuf msg = STRBUF_INIT;\n \tconst char *index_file = get_index_file();\n \n-\tif (!no_verify && run_commit_hook(0 < option_edit, index_file, \"pre-merge-commit\", NULL))\n-\t\tabort_commit(remoteheads, NULL);\n-\t/*\n-\t * Re-read the index as pre-merge-commit hook could have updated it,\n-\t * and write it out as a tree.  We must do this before we invoke\n-\t * the editor and after we invoke run_status above.\n-\t */\n-\tif (hook_exists(\"pre-merge-commit\"))\n-\t\tdiscard_cache();\n+\tif (!no_verify) {\n+\t\tif (run_commit_hook(0 < option_edit, index_file,\n+\t\t\t\t    \"pre-merge-commit\", NULL))\n+\t\t\tabort_commit(remoteheads, NULL);\n+\t\t/*\n+\t\t * Re-read the index as pre-merge-commit hook could have updated it,\n+\t\t * and write it out as a tree.  We must do this before we invoke\n+\t\t * the editor and after we invoke run_status above.\n+\t\t */\n+\t\tif (hook_exists(\"pre-merge-commit\"))\n+\t\t\tdiscard_cache();\n+\t}\n \tread_cache_from(index_file);\n \tstrbuf_addbuf(&msg, &merge_msg);\n \tif (squash)\n-- \n2.35.1.1031.g277d4562d2e\n\n"},{"id":"448805","messageId":"patch-2.2-d01d088073b-20220218T203834Z-avarab@gmail.com","threadId":"57439","inReplyTo":"cover-0.2-00000000000-20220218T203834Z-avarab@gmail.com","subject":"[PATCH 2/2] hooks: fix a TOCTOU in \"did we run a hook?\" heuristic","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-02-18T20:43:52Z","receivedAt":"2022-02-18T20:44:08Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Fix a Time-of-check to time-of-use (TOCTOU) race in code added in\n680ee550d72 (commit: skip discarding the index if there is no\npre-commit hook, 2017-08-14).\n\nWe can fix the race passing around information about whether or not we\nran the hook in question, instead of running hook_exists() after the\nfact to check if the hook in question exists. This problem has been\nnoted on-list when 680ee550d72 was discussed[1], but had not been\nfixed.\n\nIn addition to fixing this for the pre-commit hook as suggested there\nI'm also fixing this for the pre-merge-commit hook. See\n6098817fd7f (git-merge: honor pre-merge-commit hook, 2019-08-07) for\nthe introduction of its previous behavior.\n\nLet's also change this for the push-to-checkout hook. Now instead of\nchecking if the hook exists and either doing a push to checkout or a\npush to deploy we'll always attempt a push to checkout. If the hook\ndoesn't exist we'll fall back on push to deploy. The same behavior as\nbefore, without the TOCTOU race. See 0855331941b (receive-pack:\nsupport push-to-checkout hook, 2014-12-01) for the introduction of the\nprevious behavior.\n\nThis leaves uses of hook_exists() in two places that matter. The\n\"reference-transaction\" check in refs.c, see 67541597670 (refs:\nimplement reference transaction hook, 2020-06-19), and the\nprepare-commit-msg hook, see 66618a50f9c (sequencer: run\n'prepare-commit-msg' hook, 2018-01-24).\n\nIn both of those cases we're saving ourselves CPU time by not\npreparing data for the hook that we'll then do nothing with if we\ndon't have the hook. So using this \"invoked_hook\" pattern doesn't make\nsense in those cases.\n\nMore importantly, in those cases the worst we'll do is miss that we\n\"should\" run the hook because a new hook appeared, whereas in the\npre-commit and pre-merge-commit cases we'll skip an important\ndiscard_cache() on the bases of our faulty guess.\n\nI do think none of these races really matter in practice. It would be\nsome one-off issue as a hook was added or removed. I did think it was\nstupid that we didn't pass a \"did this run?\" flag instead of doing\nthis guessing at a distance though, so now we're not guessing anymore.\n\n1. https://lore.kernel.org/git/20170810191613.kpmhzg4seyxy3cpq@sigill.intra.peff.net/\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n builtin/commit.c       | 18 +++++++++++-------\n builtin/merge.c        | 11 +++++++----\n builtin/receive-pack.c |  8 +++++---\n commit.c               |  2 +-\n commit.h               |  3 ++-\n hook.c                 |  7 +++++++\n hook.h                 |  9 +++++++++\n sequencer.c            |  4 ++--\n 8 files changed, 44 insertions(+), 18 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex b9ed0374e30..bc5d34bc31f 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -725,11 +725,13 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \tint clean_message_contents = (cleanup_mode != COMMIT_MSG_CLEANUP_NONE);\n \tint old_display_comment_prefix;\n \tint merge_contains_scissors = 0;\n+\tint invoked_hook;\n \n \t/* This checks and barfs if author is badly specified */\n \tdetermine_author_info(author_ident);\n \n-\tif (!no_verify && run_commit_hook(use_editor, index_file, \"pre-commit\", NULL))\n+\tif (!no_verify && run_commit_hook(use_editor, index_file, &invoked_hook,\n+\t\t\t\t\t  \"pre-commit\", NULL))\n \t\treturn 0;\n \n \tif (squash_message) {\n@@ -1052,10 +1054,10 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\treturn 0;\n \t}\n \n-\tif (!no_verify && hook_exists(\"pre-commit\")) {\n+\tif (!no_verify && invoked_hook) {\n \t\t/*\n-\t\t * Re-read the index as pre-commit hook could have updated it,\n-\t\t * and write it out as a tree.  We must do this before we invoke\n+\t\t * Re-read the index as the pre-commit-commit hook was invoked\n+\t\t * and could have updated it. We must do this before we invoke\n \t\t * the editor and after we invoke run_status above.\n \t\t */\n \t\tdiscard_cache();\n@@ -1067,7 +1069,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\treturn 0;\n \t}\n \n-\tif (run_commit_hook(use_editor, index_file, \"prepare-commit-msg\",\n+\tif (run_commit_hook(use_editor, index_file, NULL, \"prepare-commit-msg\",\n \t\t\t    git_path_commit_editmsg(), hook_arg1, hook_arg2, NULL))\n \t\treturn 0;\n \n@@ -1084,7 +1086,8 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t}\n \n \tif (!no_verify &&\n-\t    run_commit_hook(use_editor, index_file, \"commit-msg\", git_path_commit_editmsg(), NULL)) {\n+\t    run_commit_hook(use_editor, index_file, NULL, \"commit-msg\",\n+\t\t\t    git_path_commit_editmsg(), NULL)) {\n \t\treturn 0;\n \t}\n \n@@ -1845,7 +1848,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \n \trepo_rerere(the_repository, 0);\n \trun_auto_maintenance(quiet);\n-\trun_commit_hook(use_editor, get_index_file(), \"post-commit\", NULL);\n+\trun_commit_hook(use_editor, get_index_file(), NULL, \"post-commit\",\n+\t\t\tNULL);\n \tif (amend && !no_post_rewrite) {\n \t\tcommit_post_rewrite(the_repository, current_head, &oid);\n \t}\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex fab553e3bc4..4b2eab88b0d 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -846,7 +846,9 @@ static void prepare_to_commit(struct commit_list *remoteheads)\n \tconst char *index_file = get_index_file();\n \n \tif (!no_verify) {\n-\t\tif (run_commit_hook(0 < option_edit, index_file,\n+\t\tint invoked_hook;\n+\n+\t\tif (run_commit_hook(0 < option_edit, index_file, &invoked_hook,\n \t\t\t\t    \"pre-merge-commit\", NULL))\n \t\t\tabort_commit(remoteheads, NULL);\n \t\t/*\n@@ -854,7 +856,7 @@ static void prepare_to_commit(struct commit_list *remoteheads)\n \t\t * and write it out as a tree.  We must do this before we invoke\n \t\t * the editor and after we invoke run_status above.\n \t\t */\n-\t\tif (hook_exists(\"pre-merge-commit\"))\n+\t\tif (invoked_hook)\n \t\t\tdiscard_cache();\n \t}\n \tread_cache_from(index_file);\n@@ -878,7 +880,8 @@ static void prepare_to_commit(struct commit_list *remoteheads)\n \t\tappend_signoff(&msg, ignore_non_trailer(msg.buf, msg.len), 0);\n \twrite_merge_heads(remoteheads);\n \twrite_file_buf(git_path_merge_msg(the_repository), msg.buf, msg.len);\n-\tif (run_commit_hook(0 < option_edit, get_index_file(), \"prepare-commit-msg\",\n+\tif (run_commit_hook(0 < option_edit, get_index_file(), NULL,\n+\t\t\t    \"prepare-commit-msg\",\n \t\t\t    git_path_merge_msg(the_repository), \"merge\", NULL))\n \t\tabort_commit(remoteheads, NULL);\n \tif (0 < option_edit) {\n@@ -887,7 +890,7 @@ static void prepare_to_commit(struct commit_list *remoteheads)\n \t}\n \n \tif (!no_verify && run_commit_hook(0 < option_edit, get_index_file(),\n-\t\t\t\t\t  \"commit-msg\",\n+\t\t\t\t\t  NULL, \"commit-msg\",\n \t\t\t\t\t  git_path_merge_msg(the_repository), NULL))\n \t\tabort_commit(remoteheads, NULL);\n \ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex c427ca09aaf..75595c7af38 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -1408,10 +1408,12 @@ static const char *push_to_deploy(unsigned char *sha1,\n static const char *push_to_checkout_hook = \"push-to-checkout\";\n \n static const char *push_to_checkout(unsigned char *hash,\n+\t\t\t\t    int *invoked_hook,\n \t\t\t\t    struct strvec *env,\n \t\t\t\t    const char *work_tree)\n {\n \tstruct run_hooks_opt opt = RUN_HOOKS_OPT_INIT;\n+\topt.invoked_hook = invoked_hook;\n \n \tstrvec_pushf(env, \"GIT_WORK_TREE=%s\", absolute_path(work_tree));\n \tstrvec_pushv(&opt.env, env->v);\n@@ -1426,6 +1428,7 @@ static const char *update_worktree(unsigned char *sha1, const struct worktree *w\n {\n \tconst char *retval, *git_dir;\n \tstruct strvec env = STRVEC_INIT;\n+\tint invoked_hook;\n \n \tif (!worktree || !worktree->path)\n \t\tBUG(\"worktree->path must be non-NULL\");\n@@ -1436,10 +1439,9 @@ static const char *update_worktree(unsigned char *sha1, const struct worktree *w\n \n \tstrvec_pushf(&env, \"GIT_DIR=%s\", absolute_path(git_dir));\n \n-\tif (!hook_exists(push_to_checkout_hook))\n+\tretval = push_to_checkout(sha1, &invoked_hook, &env, worktree->path);\n+\tif (!invoked_hook)\n \t\tretval = push_to_deploy(sha1, &env, worktree->path);\n-\telse\n-\t\tretval = push_to_checkout(sha1, &env, worktree->path);\n \n \tstrvec_clear(&env);\n \treturn retval;\ndiff --git a/commit.c b/commit.c\nindex d400f5dfa2b..396e14d7b32 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -1713,7 +1713,7 @@ size_t ignore_non_trailer(const char *buf, size_t len)\n }\n \n int run_commit_hook(int editor_is_used, const char *index_file,\n-\t\t    const char *name, ...)\n+\t\t    int *invoked_hook, const char *name, ...)\n {\n \tstruct run_hooks_opt opt = RUN_HOOKS_OPT_INIT;\n \tva_list args;\ndiff --git a/commit.h b/commit.h\nindex 38cc5426615..3b174135bcf 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -369,7 +369,8 @@ int compare_commits_by_commit_date(const void *a_, const void *b_, void *unused)\n int compare_commits_by_gen_then_commit_date(const void *a_, const void *b_, void *unused);\n \n LAST_ARG_MUST_BE_NULL\n-int run_commit_hook(int editor_is_used, const char *index_file, const char *name, ...);\n+int run_commit_hook(int editor_is_used, const char *index_file,\n+\t\t    int *invoked_hook, const char *name, ...);\n \n /* Sign a commit or tag buffer, storing the result in a header. */\n int sign_with_header(struct strbuf *buf, const char *keyid);\ndiff --git a/hook.c b/hook.c\nindex 69a215b2c3c..1d51be3b77a 100644\n--- a/hook.c\n+++ b/hook.c\n@@ -96,9 +96,13 @@ static int notify_hook_finished(int result,\n \t\t\t\tvoid *pp_task_cb)\n {\n \tstruct hook_cb_data *hook_cb = pp_cb;\n+\tstruct run_hooks_opt *opt = hook_cb->options;\n \n \thook_cb->rc |= result;\n \n+\tif (opt->invoked_hook)\n+\t\t*opt->invoked_hook = 1;\n+\n \treturn 0;\n }\n \n@@ -123,6 +127,9 @@ int run_hooks_opt(const char *hook_name, struct run_hooks_opt *options)\n \tif (!options)\n \t\tBUG(\"a struct run_hooks_opt must be provided to run_hooks\");\n \n+\tif (options->invoked_hook)\n+\t\t*options->invoked_hook = 0;\n+\n \tif (!hook_path && !options->error_if_missing)\n \t\tgoto cleanup;\n \ndiff --git a/hook.h b/hook.h\nindex 18d90aedf14..735a7d97002 100644\n--- a/hook.h\n+++ b/hook.h\n@@ -18,6 +18,15 @@ struct run_hooks_opt\n \t * translates to \"struct child_process\"'s \"dir\" member.\n \t */\n \tconst char *dir;\n+\n+\t/**\n+\t * A pointer which if provided will be set to 1 or 0 depending\n+\t * on if a hook was invoked (i.e. existed), regardless of\n+\t * whether or not that was successful. Used for avoiding\n+\t * TOCTOU races in code that would otherwise call hook_exist()\n+\t * after a \"maybe hook run\" to see if a hook was invoked.\n+\t */\n+\tint *invoked_hook;\n };\n \n #define RUN_HOOKS_OPT_INIT { \\\ndiff --git a/sequencer.c b/sequencer.c\nindex fb978a53a0f..967bf054709 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -1220,7 +1220,7 @@ static int run_prepare_commit_msg_hook(struct repository *r,\n \t} else {\n \t\targ1 = \"message\";\n \t}\n-\tif (run_commit_hook(0, r->index_file, \"prepare-commit-msg\", name,\n+\tif (run_commit_hook(0, r->index_file, NULL, \"prepare-commit-msg\", name,\n \t\t\t    arg1, arg2, NULL))\n \t\tret = error(_(\"'prepare-commit-msg' hook failed\"));\n \n@@ -1552,7 +1552,7 @@ static int try_to_commit(struct repository *r,\n \t\tgoto out;\n \t}\n \n-\trun_commit_hook(0, r->index_file, \"post-commit\", NULL);\n+\trun_commit_hook(0, r->index_file, NULL, \"post-commit\", NULL);\n \tif (flags & AMEND_MSG)\n \t\tcommit_post_rewrite(r, current_head, oid);\n \n-- \n2.35.1.1031.g277d4562d2e\n\n"},{"id":"448816","messageId":"xmqqa6enybgz.fsf@gitster.g","threadId":"57439","inReplyTo":"patch-1.2-9b5144daee6-20220218T203834Z-avarab@gmail.com","subject":"Re: [PATCH 1/2] merge: don't run post-hook logic on --no-verify","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-18T23:57:32Z","receivedAt":"2022-02-18T23:57:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n\n> diff --git a/builtin/merge.c b/builtin/merge.c\n> index 133831d42fd..fab553e3bc4 100644\n> --- a/builtin/merge.c\n> +++ b/builtin/merge.c\n> @@ -845,15 +845,18 @@ static void prepare_to_commit(struct commit_list *remoteheads)\n>  \tstruct strbuf msg = STRBUF_INIT;\n>  \tconst char *index_file = get_index_file();\n>  \n> -\tif (!no_verify && run_commit_hook(0 < option_edit, index_file, \"pre-merge-commit\", NULL))\n> -\t\tabort_commit(remoteheads, NULL);\n> -\t/*\n> -\t * Re-read the index as pre-merge-commit hook could have updated it,\n> -\t * and write it out as a tree.  We must do this before we invoke\n> -\t * the editor and after we invoke run_status above.\n> -\t */\n> -\tif (hook_exists(\"pre-merge-commit\"))\n> -\t\tdiscard_cache();\n> +\tif (!no_verify) {\n> +\t\tif (run_commit_hook(0 < option_edit, index_file,\n> +\t\t\t\t    \"pre-merge-commit\", NULL))\n> +\t\t\tabort_commit(remoteheads, NULL);\n> +\t\t/*\n> +\t\t * Re-read the index as pre-merge-commit hook could have updated it,\n> +\t\t * and write it out as a tree.  We must do this before we invoke\n> +\t\t * the editor and after we invoke run_status above.\n> +\t\t */\n> +\t\tif (hook_exists(\"pre-merge-commit\"))\n> +\t\t\tdiscard_cache();\n> +\t}\n\nThe updated code not just is more correct, but it is much easier to\nfollow.\n\nI wonder if run_commit_hook() can return \"I failed to run the hook\",\n\"I ran the hook and the hook failed\", \"I successfully run the hook\",\nand \"I didn't find the hook\", instead of the current \"yes/no\".  That\nwould allow us to express this part in an even cleaner way, I would\nthink.\n\nLooking good.\n\nThanks.\n\n>  \tread_cache_from(index_file);\n>  \tstrbuf_addbuf(&msg, &merge_msg);\n>  \tif (squash)\n"},{"id":"448817","messageId":"xmqq4k4vyata.fsf@gitster.g","threadId":"57439","inReplyTo":"patch-2.2-d01d088073b-20220218T203834Z-avarab@gmail.com","subject":"Re: [PATCH 2/2] hooks: fix a TOCTOU in \"did we run a hook?\" heuristic","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-19T00:11:45Z","receivedAt":"2022-02-19T00:11:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"> -int run_commit_hook(int editor_is_used, const char *index_file, const char *name, ...);\n> +int run_commit_hook(int editor_is_used, const char *index_file,\n> +\t\t    int *invoked_hook, const char *name, ...);\n>  \n\nEven though my gut feeling tells me that turning the \"yes/no\"\ninteger into an enum that includes \"there was no such hook\", \"I\ntried to run it, but it failed to run\" [*], \"I ran it and it was\nhappy\".  would be a more viable approach for the longer term, I\nguess this extra and ad-hoc parameter would be sufficient as a\nshorter term improvement.\n\n    Side note: optionally \"failed to run\" may be split into \"failed\n    to even start (e.g. ENOEXEC)\" and \"started successfully but\n    exited with non-zero status\".  There may or may not be callers\n    that wants to see them as distinct cases right now, but an\n    interface based on returned enum value would be easier to extend\n    than having to add a pointer to return variable every time we\n    need to know more details.\n\nThanks.\n"},{"id":"448833","messageId":"YhBtI6PY5s/nC8og@nand.local","threadId":"57439","inReplyTo":"patch-2.2-d01d088073b-20220218T203834Z-avarab@gmail.com","subject":"Re: [PATCH 2/2] hooks: fix a TOCTOU in \"did we run a hook?\" heuristic","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-02-19T04:08:03Z","receivedAt":"2022-02-19T04:08:17Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Feb 18, 2022 at 09:43:52PM +0100, Ævar Arnfjörð Bjarmason wrote:\n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index b9ed0374e30..bc5d34bc31f 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -725,11 +725,13 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n>  \tint clean_message_contents = (cleanup_mode != COMMIT_MSG_CLEANUP_NONE);\n>  \tint old_display_comment_prefix;\n>  \tint merge_contains_scissors = 0;\n> +\tint invoked_hook;\n>\n>  \t/* This checks and barfs if author is badly specified */\n>  \tdetermine_author_info(author_ident);\n>\n> -\tif (!no_verify && run_commit_hook(use_editor, index_file, \"pre-commit\", NULL))\n> +\tif (!no_verify && run_commit_hook(use_editor, index_file, &invoked_hook,\n> +\t\t\t\t\t  \"pre-commit\", NULL))\n>  \t\treturn 0;\n>\n>  \tif (squash_message) {\n> @@ -1052,10 +1054,10 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n>  \t\treturn 0;\n>  \t}\n>\n> -\tif (!no_verify && hook_exists(\"pre-commit\")) {\n> +\tif (!no_verify && invoked_hook) {\n>  \t\t/*\n> -\t\t * Re-read the index as pre-commit hook could have updated it,\n> -\t\t * and write it out as a tree.  We must do this before we invoke\n> +\t\t * Re-read the index as the pre-commit-commit hook was invoked\n> +\t\t * and could have updated it. We must do this before we invoke\n>  \t\t * the editor and after we invoke run_status above.\n>  \t\t */\n>  \t\tdiscard_cache();\n\nSanity checking my own understating of this race: if we ran the\npre-commit hook and it modified the index, but hook_exists() returns\nfalse later on (e.g., because the hook itself went away, the directory\nbecame unreadable, etc.), then we won't call discard_cache() when we\nshould have?\n\nIf so, OK. This definitely seems like a pretty niche race, but\nindependent of that I think the change here is an improvement in\nreadability, and makes it clearer that calling discard_cache() depends\non whether or not we *ran* the pre-commit hook, not whether we (still)\n*have* a pre-commit hook.\n\nThanks,\nTaylor\n"},{"id":"448835","messageId":"220219.864k4vfo75.gmgdl@evledraar.gmail.com","threadId":"57439","inReplyTo":"xmqq4k4vyata.fsf@gitster.g","subject":"Re: [PATCH 2/2] hooks: fix a TOCTOU in \"did we run a hook?\" heuristic","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-02-19T04:48:42Z","receivedAt":"2022-02-19T04:57:41Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Feb 18 2022, Junio C Hamano wrote:\n\n>> -int run_commit_hook(int editor_is_used, const char *index_file, const char *name, ...);\n>> +int run_commit_hook(int editor_is_used, const char *index_file,\n>> +\t\t    int *invoked_hook, const char *name, ...);\n>>  \n>\n> Even though my gut feeling tells me that turning the \"yes/no\"\n> integer into an enum that includes \"there was no such hook\", \"I\n> tried to run it, but it failed to run\" [*], \"I ran it and it was\n> happy\".  would be a more viable approach for the longer term, I\n> guess this extra and ad-hoc parameter would be sufficient as a\n> shorter term improvement.\n>\n>     Side note: optionally \"failed to run\" may be split into \"failed\n>     to even start (e.g. ENOEXEC)\" and \"started successfully but\n>     exited with non-zero status\".  There may or may not be callers\n>     that wants to see them as distinct cases right now, but an\n>     interface based on returned enum value would be easier to extend\n>     than having to add a pointer to return variable every time we\n>     need to know more details.\n\nYes, I debated with myself whether I should add some more generic\ninterface to it, and decided just to do the bare minumum of adding\nsomething the \"struct run_hooks_opt\".\n\nFWIW the \"yes/no\" is not that, run_commit_hook() just returns the value\nof run_hooks_opt(), which is currently either an <0 error, or the status\ncode from the hook. I.e. what gets passed to the \"task_finished_fn\"\ncallback for run_processes_parallel_tr2(). I.e. the finish_command()\nreturn value.\n\nWe do cover the \"ENOEXEC\" case in ignoring it, since if we fail on\nstartup we won't say we ran the hook.\n\nI think in practice what'll matter is this \"invoked_hook\". I.e. if we\nfailed to parse our config, the hook wasn't executable or whatever\nthat's just a <0 error, and we didn't run the hook.\n\nOr, if we ran it at all (even if it failed) we'll know that we need to\ne.g. discard_index(), since we can't guarantee that the hook didn't get\nthat far that we'll need to update our own assumptions.\n\nA caller who ares about anything else will also need to deal with a lot\nmore complexity once we have config-based-hooks / parallel hooks by\ndefault. I.e. was that ENOEXEC one of N hooks, all of them, did all/one\nexit non-zero etc?\n\nWhereas \"int *invoked_hook\" we can just set as long as we invoked any of\nthem at all.\n"},{"id":"448849","messageId":"220219.861qzzdtg2.gmgdl@evledraar.gmail.com","threadId":"57439","inReplyTo":"YhBtI6PY5s/nC8og@nand.local","subject":"Re: [PATCH 2/2] hooks: fix a TOCTOU in \"did we run a hook?\" heuristic","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-02-19T10:46:33Z","receivedAt":"2022-02-19T10:47:17Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Feb 18 2022, Taylor Blau wrote:\n\n> On Fri, Feb 18, 2022 at 09:43:52PM +0100, Ævar Arnfjörð Bjarmason wrote:\n>> diff --git a/builtin/commit.c b/builtin/commit.c\n>> index b9ed0374e30..bc5d34bc31f 100644\n>> --- a/builtin/commit.c\n>> +++ b/builtin/commit.c\n>> @@ -725,11 +725,13 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n>>  \tint clean_message_contents = (cleanup_mode != COMMIT_MSG_CLEANUP_NONE);\n>>  \tint old_display_comment_prefix;\n>>  \tint merge_contains_scissors = 0;\n>> +\tint invoked_hook;\n>>\n>>  \t/* This checks and barfs if author is badly specified */\n>>  \tdetermine_author_info(author_ident);\n>>\n>> -\tif (!no_verify && run_commit_hook(use_editor, index_file, \"pre-commit\", NULL))\n>> +\tif (!no_verify && run_commit_hook(use_editor, index_file, &invoked_hook,\n>> +\t\t\t\t\t  \"pre-commit\", NULL))\n>>  \t\treturn 0;\n>>\n>>  \tif (squash_message) {\n>> @@ -1052,10 +1054,10 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n>>  \t\treturn 0;\n>>  \t}\n>>\n>> -\tif (!no_verify && hook_exists(\"pre-commit\")) {\n>> +\tif (!no_verify && invoked_hook) {\n>>  \t\t/*\n>> -\t\t * Re-read the index as pre-commit hook could have updated it,\n>> -\t\t * and write it out as a tree.  We must do this before we invoke\n>> +\t\t * Re-read the index as the pre-commit-commit hook was invoked\n>> +\t\t * and could have updated it. We must do this before we invoke\n>>  \t\t * the editor and after we invoke run_status above.\n>>  \t\t */\n>>  \t\tdiscard_cache();\n>\n> Sanity checking my own understating of this race: if we ran the\n> pre-commit hook and it modified the index, but hook_exists() returns\n> false later on (e.g., because the hook itself went away, the directory\n> became unreadable, etc.), then we won't call discard_cache() when we\n> should have?\n\nYes, it's that obscure.\n\n> If so, OK. This definitely seems like a pretty niche race, but\n> independent of that I think the change here is an improvement in\n> readability, and makes it clearer that calling discard_cache() depends\n> on whether or not we *ran* the pre-commit hook, not whether we (still)\n> *have* a pre-commit hook.\n\nYeah, that's the main reason to do it. I found this really hard to\nfollow before, why didn't we just remember if we have/ran the thing? Now\nwe do.\n"},{"id":"450538","messageId":"cover-v2-0.2-00000000000-20220307T123244Z-avarab@gmail.com","threadId":"57439","inReplyTo":"cover-0.2-00000000000-20220218T203834Z-avarab@gmail.com","subject":"[PATCH v2 0/2] hooks: fix a race in hook execution","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-03-07T12:33:44Z","receivedAt":"2022-03-07T12:34:26Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"A documentation & commit-message only change to this v1 which fixes an\nobscure race condition in hook execution. For v1 see:\nhttps://lore.kernel.org/git/cover-0.2-00000000000-20220218T203834Z-avarab@gmail.com/\n\nJunio: This topic wasn't picked up yet, but hopefully will be with the\nbelow, which should address coments you & Taylor had on the v1.\n\nÆvar Arnfjörð Bjarmason (2):\n  merge: don't run post-hook logic on --no-verify\n  hooks: fix an obscure TOCTOU \"did we just run a hook?\" race\n\n builtin/commit.c       | 18 +++++++++++-------\n builtin/merge.c        | 28 +++++++++++++++++-----------\n builtin/receive-pack.c |  8 +++++---\n commit.c               |  2 +-\n commit.h               |  3 ++-\n hook.c                 |  7 +++++++\n hook.h                 | 12 ++++++++++++\n sequencer.c            |  4 ++--\n 8 files changed, 57 insertions(+), 25 deletions(-)\n\nRange-diff against v1:\n1:  9b5144daee6 ! 1:  8f7b01ed758 merge: don't run post-hook logic on --no-verify\n    @@ Commit message\n         hand. There's no point in invoking discard_cache() here if the hook\n         couldn't have possibly updated the index.\n     \n    +    It's buggy that we use \"hook_exist()\" here, and as discussed in the\n    +    subsequent commit it's subject to obscure race conditions that we're\n    +    about to fix, but for now this change is a strict improvement that\n    +    retains any caveats to do with the use of \"hooks_exist()\" as-is.\n    +\n         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n     \n      ## builtin/merge.c ##\n2:  d01d088073b ! 2:  9d16984898c hooks: fix a TOCTOU in \"did we run a hook?\" heuristic\n    @@ Metadata\n     Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n     \n      ## Commit message ##\n    -    hooks: fix a TOCTOU in \"did we run a hook?\" heuristic\n    +    hooks: fix an obscure TOCTOU \"did we just run a hook?\" race\n     \n         Fix a Time-of-check to time-of-use (TOCTOU) race in code added in\n         680ee550d72 (commit: skip discarding the index if there is no\n         pre-commit hook, 2017-08-14).\n     \n    -    We can fix the race passing around information about whether or not we\n    -    ran the hook in question, instead of running hook_exists() after the\n    -    fact to check if the hook in question exists. This problem has been\n    +    This obscure race condition can occur if we e.g. ran the \"pre-commit\"\n    +    hook and it modified the index, but hook_exists() returns false later\n    +    on (e.g., because the hook itself went away, the directory became\n    +    unreadable, etc.). Then we won't call discard_cache() when we should\n    +    have.\n    +\n    +    The race condition itself probably doesn't matter, and users would\n    +    have been unlikely to run into it in practice. This problem has been\n         noted on-list when 680ee550d72 was discussed[1], but had not been\n         fixed.\n     \n    -    In addition to fixing this for the pre-commit hook as suggested there\n    -    I'm also fixing this for the pre-merge-commit hook. See\n    -    6098817fd7f (git-merge: honor pre-merge-commit hook, 2019-08-07) for\n    -    the introduction of its previous behavior.\n    +    This change is mainly intended to improve the readability of the code\n    +    involved, and to make reasoning about it more straightforward. It\n    +    wasn't as obvious what we were trying to do here, but by having an\n    +    \"invoked_hook\" it's clearer that e.g. our discard_cache() is happening\n    +    because of the earlier hook execution.\n     \n         Let's also change this for the push-to-checkout hook. Now instead of\n         checking if the hook exists and either doing a push to checkout or a\n    @@ Commit message\n         This leaves uses of hook_exists() in two places that matter. The\n         \"reference-transaction\" check in refs.c, see 67541597670 (refs:\n         implement reference transaction hook, 2020-06-19), and the\n    -    prepare-commit-msg hook, see 66618a50f9c (sequencer: run\n    +    \"prepare-commit-msg\" hook, see 66618a50f9c (sequencer: run\n         'prepare-commit-msg' hook, 2018-01-24).\n     \n         In both of those cases we're saving ourselves CPU time by not\n    @@ Commit message\n         don't have the hook. So using this \"invoked_hook\" pattern doesn't make\n         sense in those cases.\n     \n    -    More importantly, in those cases the worst we'll do is miss that we\n    -    \"should\" run the hook because a new hook appeared, whereas in the\n    -    pre-commit and pre-merge-commit cases we'll skip an important\n    -    discard_cache() on the bases of our faulty guess.\n    -\n    -    I do think none of these races really matter in practice. It would be\n    -    some one-off issue as a hook was added or removed. I did think it was\n    -    stupid that we didn't pass a \"did this run?\" flag instead of doing\n    -    this guessing at a distance though, so now we're not guessing anymore.\n    +    The \"reference-transaction\" and \"prepare-commit-msg\" hook also aren't\n    +    racy. In those cases we'll skip the hook runs if we race with a new\n    +    hook being added, whereas in the TOCTOU races being fixed here we were\n    +    incorrectly skipping the required post-hook logic.\n     \n         1. https://lore.kernel.org/git/20170810191613.kpmhzg4seyxy3cpq@sigill.intra.peff.net/\n     \n    @@ hook.h: struct run_hooks_opt\n     +\n     +\t/**\n     +\t * A pointer which if provided will be set to 1 or 0 depending\n    -+\t * on if a hook was invoked (i.e. existed), regardless of\n    -+\t * whether or not that was successful. Used for avoiding\n    -+\t * TOCTOU races in code that would otherwise call hook_exist()\n    -+\t * after a \"maybe hook run\" to see if a hook was invoked.\n    ++\t * on if a hook was started, regardless of whether or not that\n    ++\t * was successful. I.e. if the underlying start_command() was\n    ++\t * successful this will be set to 1.\n    ++\t *\n    ++\t * Used for avoiding TOCTOU races in code that would otherwise\n    ++\t * call hook_exist() after a \"maybe hook run\" to see if a hook\n    ++\t * was invoked.\n     +\t */\n     +\tint *invoked_hook;\n      };\n-- \n2.35.1.1242.gfeba0eae32b\n\n"},{"id":"450539","messageId":"patch-v2-1.2-8f7b01ed758-20220307T123244Z-avarab@gmail.com","threadId":"57439","inReplyTo":"cover-v2-0.2-00000000000-20220307T123244Z-avarab@gmail.com","subject":"[PATCH v2 1/2] merge: don't run post-hook logic on --no-verify","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-03-07T12:33:45Z","receivedAt":"2022-03-07T12:34:29Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Fix a minor bug introduced in bc40ce4de61 (merge: --no-verify to\nbypass pre-merge-commit hook, 2019-08-07), when that change made the\n--no-verify option bypass the \"pre-merge-commit\" hook it didn't update\nthe corresponding find_hook() (later hook_exists()) condition.\n\nAs can be seen in the preceding commit in 6098817fd7f (git-merge:\nhonor pre-merge-commit hook, 2019-08-07) the two should go hand in\nhand. There's no point in invoking discard_cache() here if the hook\ncouldn't have possibly updated the index.\n\nIt's buggy that we use \"hook_exist()\" here, and as discussed in the\nsubsequent commit it's subject to obscure race conditions that we're\nabout to fix, but for now this change is a strict improvement that\nretains any caveats to do with the use of \"hooks_exist()\" as-is.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n builtin/merge.c | 21 ++++++++++++---------\n 1 file changed, 12 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex a94a03384ae..b26b4c45157 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -845,15 +845,18 @@ static void prepare_to_commit(struct commit_list *remoteheads)\n \tstruct strbuf msg = STRBUF_INIT;\n \tconst char *index_file = get_index_file();\n \n-\tif (!no_verify && run_commit_hook(0 < option_edit, index_file, \"pre-merge-commit\", NULL))\n-\t\tabort_commit(remoteheads, NULL);\n-\t/*\n-\t * Re-read the index as pre-merge-commit hook could have updated it,\n-\t * and write it out as a tree.  We must do this before we invoke\n-\t * the editor and after we invoke run_status above.\n-\t */\n-\tif (hook_exists(\"pre-merge-commit\"))\n-\t\tdiscard_cache();\n+\tif (!no_verify) {\n+\t\tif (run_commit_hook(0 < option_edit, index_file,\n+\t\t\t\t    \"pre-merge-commit\", NULL))\n+\t\t\tabort_commit(remoteheads, NULL);\n+\t\t/*\n+\t\t * Re-read the index as pre-merge-commit hook could have updated it,\n+\t\t * and write it out as a tree.  We must do this before we invoke\n+\t\t * the editor and after we invoke run_status above.\n+\t\t */\n+\t\tif (hook_exists(\"pre-merge-commit\"))\n+\t\t\tdiscard_cache();\n+\t}\n \tread_cache_from(index_file);\n \tstrbuf_addbuf(&msg, &merge_msg);\n \tif (squash)\n-- \n2.35.1.1242.gfeba0eae32b\n\n"},{"id":"450540","messageId":"patch-v2-2.2-9d16984898c-20220307T123244Z-avarab@gmail.com","threadId":"57439","inReplyTo":"cover-v2-0.2-00000000000-20220307T123244Z-avarab@gmail.com","subject":"[PATCH v2 2/2] hooks: fix an obscure TOCTOU \"did we just run a hook?\" race","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-03-07T12:33:46Z","receivedAt":"2022-03-07T12:34:39Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Fix a Time-of-check to time-of-use (TOCTOU) race in code added in\n680ee550d72 (commit: skip discarding the index if there is no\npre-commit hook, 2017-08-14).\n\nThis obscure race condition can occur if we e.g. ran the \"pre-commit\"\nhook and it modified the index, but hook_exists() returns false later\non (e.g., because the hook itself went away, the directory became\nunreadable, etc.). Then we won't call discard_cache() when we should\nhave.\n\nThe race condition itself probably doesn't matter, and users would\nhave been unlikely to run into it in practice. This problem has been\nnoted on-list when 680ee550d72 was discussed[1], but had not been\nfixed.\n\nThis change is mainly intended to improve the readability of the code\ninvolved, and to make reasoning about it more straightforward. It\nwasn't as obvious what we were trying to do here, but by having an\n\"invoked_hook\" it's clearer that e.g. our discard_cache() is happening\nbecause of the earlier hook execution.\n\nLet's also change this for the push-to-checkout hook. Now instead of\nchecking if the hook exists and either doing a push to checkout or a\npush to deploy we'll always attempt a push to checkout. If the hook\ndoesn't exist we'll fall back on push to deploy. The same behavior as\nbefore, without the TOCTOU race. See 0855331941b (receive-pack:\nsupport push-to-checkout hook, 2014-12-01) for the introduction of the\nprevious behavior.\n\nThis leaves uses of hook_exists() in two places that matter. The\n\"reference-transaction\" check in refs.c, see 67541597670 (refs:\nimplement reference transaction hook, 2020-06-19), and the\n\"prepare-commit-msg\" hook, see 66618a50f9c (sequencer: run\n'prepare-commit-msg' hook, 2018-01-24).\n\nIn both of those cases we're saving ourselves CPU time by not\npreparing data for the hook that we'll then do nothing with if we\ndon't have the hook. So using this \"invoked_hook\" pattern doesn't make\nsense in those cases.\n\nThe \"reference-transaction\" and \"prepare-commit-msg\" hook also aren't\nracy. In those cases we'll skip the hook runs if we race with a new\nhook being added, whereas in the TOCTOU races being fixed here we were\nincorrectly skipping the required post-hook logic.\n\n1. https://lore.kernel.org/git/20170810191613.kpmhzg4seyxy3cpq@sigill.intra.peff.net/\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n builtin/commit.c       | 18 +++++++++++-------\n builtin/merge.c        | 11 +++++++----\n builtin/receive-pack.c |  8 +++++---\n commit.c               |  2 +-\n commit.h               |  3 ++-\n hook.c                 |  7 +++++++\n hook.h                 | 12 ++++++++++++\n sequencer.c            |  4 ++--\n 8 files changed, 47 insertions(+), 18 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 8b8bdad3959..009a1de0a3d 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -726,11 +726,13 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \tint clean_message_contents = (cleanup_mode != COMMIT_MSG_CLEANUP_NONE);\n \tint old_display_comment_prefix;\n \tint merge_contains_scissors = 0;\n+\tint invoked_hook;\n \n \t/* This checks and barfs if author is badly specified */\n \tdetermine_author_info(author_ident);\n \n-\tif (!no_verify && run_commit_hook(use_editor, index_file, \"pre-commit\", NULL))\n+\tif (!no_verify && run_commit_hook(use_editor, index_file, &invoked_hook,\n+\t\t\t\t\t  \"pre-commit\", NULL))\n \t\treturn 0;\n \n \tif (squash_message) {\n@@ -1053,10 +1055,10 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\treturn 0;\n \t}\n \n-\tif (!no_verify && hook_exists(\"pre-commit\")) {\n+\tif (!no_verify && invoked_hook) {\n \t\t/*\n-\t\t * Re-read the index as pre-commit hook could have updated it,\n-\t\t * and write it out as a tree.  We must do this before we invoke\n+\t\t * Re-read the index as the pre-commit-commit hook was invoked\n+\t\t * and could have updated it. We must do this before we invoke\n \t\t * the editor and after we invoke run_status above.\n \t\t */\n \t\tdiscard_cache();\n@@ -1068,7 +1070,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\treturn 0;\n \t}\n \n-\tif (run_commit_hook(use_editor, index_file, \"prepare-commit-msg\",\n+\tif (run_commit_hook(use_editor, index_file, NULL, \"prepare-commit-msg\",\n \t\t\t    git_path_commit_editmsg(), hook_arg1, hook_arg2, NULL))\n \t\treturn 0;\n \n@@ -1085,7 +1087,8 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t}\n \n \tif (!no_verify &&\n-\t    run_commit_hook(use_editor, index_file, \"commit-msg\", git_path_commit_editmsg(), NULL)) {\n+\t    run_commit_hook(use_editor, index_file, NULL, \"commit-msg\",\n+\t\t\t    git_path_commit_editmsg(), NULL)) {\n \t\treturn 0;\n \t}\n \n@@ -1841,7 +1844,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \n \trepo_rerere(the_repository, 0);\n \trun_auto_maintenance(quiet);\n-\trun_commit_hook(use_editor, get_index_file(), \"post-commit\", NULL);\n+\trun_commit_hook(use_editor, get_index_file(), NULL, \"post-commit\",\n+\t\t\tNULL);\n \tif (amend && !no_post_rewrite) {\n \t\tcommit_post_rewrite(the_repository, current_head, &oid);\n \t}\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex b26b4c45157..f178f5a3ee1 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -846,7 +846,9 @@ static void prepare_to_commit(struct commit_list *remoteheads)\n \tconst char *index_file = get_index_file();\n \n \tif (!no_verify) {\n-\t\tif (run_commit_hook(0 < option_edit, index_file,\n+\t\tint invoked_hook;\n+\n+\t\tif (run_commit_hook(0 < option_edit, index_file, &invoked_hook,\n \t\t\t\t    \"pre-merge-commit\", NULL))\n \t\t\tabort_commit(remoteheads, NULL);\n \t\t/*\n@@ -854,7 +856,7 @@ static void prepare_to_commit(struct commit_list *remoteheads)\n \t\t * and write it out as a tree.  We must do this before we invoke\n \t\t * the editor and after we invoke run_status above.\n \t\t */\n-\t\tif (hook_exists(\"pre-merge-commit\"))\n+\t\tif (invoked_hook)\n \t\t\tdiscard_cache();\n \t}\n \tread_cache_from(index_file);\n@@ -878,7 +880,8 @@ static void prepare_to_commit(struct commit_list *remoteheads)\n \t\tappend_signoff(&msg, ignore_non_trailer(msg.buf, msg.len), 0);\n \twrite_merge_heads(remoteheads);\n \twrite_file_buf(git_path_merge_msg(the_repository), msg.buf, msg.len);\n-\tif (run_commit_hook(0 < option_edit, get_index_file(), \"prepare-commit-msg\",\n+\tif (run_commit_hook(0 < option_edit, get_index_file(), NULL,\n+\t\t\t    \"prepare-commit-msg\",\n \t\t\t    git_path_merge_msg(the_repository), \"merge\", NULL))\n \t\tabort_commit(remoteheads, NULL);\n \tif (0 < option_edit) {\n@@ -887,7 +890,7 @@ static void prepare_to_commit(struct commit_list *remoteheads)\n \t}\n \n \tif (!no_verify && run_commit_hook(0 < option_edit, get_index_file(),\n-\t\t\t\t\t  \"commit-msg\",\n+\t\t\t\t\t  NULL, \"commit-msg\",\n \t\t\t\t\t  git_path_merge_msg(the_repository), NULL))\n \t\tabort_commit(remoteheads, NULL);\n \ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex d10aeb7e78f..d4db5776694 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -1408,10 +1408,12 @@ static const char *push_to_deploy(unsigned char *sha1,\n static const char *push_to_checkout_hook = \"push-to-checkout\";\n \n static const char *push_to_checkout(unsigned char *hash,\n+\t\t\t\t    int *invoked_hook,\n \t\t\t\t    struct strvec *env,\n \t\t\t\t    const char *work_tree)\n {\n \tstruct run_hooks_opt opt = RUN_HOOKS_OPT_INIT;\n+\topt.invoked_hook = invoked_hook;\n \n \tstrvec_pushf(env, \"GIT_WORK_TREE=%s\", absolute_path(work_tree));\n \tstrvec_pushv(&opt.env, env->v);\n@@ -1426,6 +1428,7 @@ static const char *update_worktree(unsigned char *sha1, const struct worktree *w\n {\n \tconst char *retval, *git_dir;\n \tstruct strvec env = STRVEC_INIT;\n+\tint invoked_hook;\n \n \tif (!worktree || !worktree->path)\n \t\tBUG(\"worktree->path must be non-NULL\");\n@@ -1436,10 +1439,9 @@ static const char *update_worktree(unsigned char *sha1, const struct worktree *w\n \n \tstrvec_pushf(&env, \"GIT_DIR=%s\", absolute_path(git_dir));\n \n-\tif (!hook_exists(push_to_checkout_hook))\n+\tretval = push_to_checkout(sha1, &invoked_hook, &env, worktree->path);\n+\tif (!invoked_hook)\n \t\tretval = push_to_deploy(sha1, &env, worktree->path);\n-\telse\n-\t\tretval = push_to_checkout(sha1, &env, worktree->path);\n \n \tstrvec_clear(&env);\n \treturn retval;\ndiff --git a/commit.c b/commit.c\nindex d400f5dfa2b..396e14d7b32 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -1713,7 +1713,7 @@ size_t ignore_non_trailer(const char *buf, size_t len)\n }\n \n int run_commit_hook(int editor_is_used, const char *index_file,\n-\t\t    const char *name, ...)\n+\t\t    int *invoked_hook, const char *name, ...)\n {\n \tstruct run_hooks_opt opt = RUN_HOOKS_OPT_INIT;\n \tva_list args;\ndiff --git a/commit.h b/commit.h\nindex 38cc5426615..3b174135bcf 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -369,7 +369,8 @@ int compare_commits_by_commit_date(const void *a_, const void *b_, void *unused)\n int compare_commits_by_gen_then_commit_date(const void *a_, const void *b_, void *unused);\n \n LAST_ARG_MUST_BE_NULL\n-int run_commit_hook(int editor_is_used, const char *index_file, const char *name, ...);\n+int run_commit_hook(int editor_is_used, const char *index_file,\n+\t\t    int *invoked_hook, const char *name, ...);\n \n /* Sign a commit or tag buffer, storing the result in a header. */\n int sign_with_header(struct strbuf *buf, const char *keyid);\ndiff --git a/hook.c b/hook.c\nindex 69a215b2c3c..1d51be3b77a 100644\n--- a/hook.c\n+++ b/hook.c\n@@ -96,9 +96,13 @@ static int notify_hook_finished(int result,\n \t\t\t\tvoid *pp_task_cb)\n {\n \tstruct hook_cb_data *hook_cb = pp_cb;\n+\tstruct run_hooks_opt *opt = hook_cb->options;\n \n \thook_cb->rc |= result;\n \n+\tif (opt->invoked_hook)\n+\t\t*opt->invoked_hook = 1;\n+\n \treturn 0;\n }\n \n@@ -123,6 +127,9 @@ int run_hooks_opt(const char *hook_name, struct run_hooks_opt *options)\n \tif (!options)\n \t\tBUG(\"a struct run_hooks_opt must be provided to run_hooks\");\n \n+\tif (options->invoked_hook)\n+\t\t*options->invoked_hook = 0;\n+\n \tif (!hook_path && !options->error_if_missing)\n \t\tgoto cleanup;\n \ndiff --git a/hook.h b/hook.h\nindex 18d90aedf14..4258b13da0d 100644\n--- a/hook.h\n+++ b/hook.h\n@@ -18,6 +18,18 @@ struct run_hooks_opt\n \t * translates to \"struct child_process\"'s \"dir\" member.\n \t */\n \tconst char *dir;\n+\n+\t/**\n+\t * A pointer which if provided will be set to 1 or 0 depending\n+\t * on if a hook was started, regardless of whether or not that\n+\t * was successful. I.e. if the underlying start_command() was\n+\t * successful this will be set to 1.\n+\t *\n+\t * Used for avoiding TOCTOU races in code that would otherwise\n+\t * call hook_exist() after a \"maybe hook run\" to see if a hook\n+\t * was invoked.\n+\t */\n+\tint *invoked_hook;\n };\n \n #define RUN_HOOKS_OPT_INIT { \\\ndiff --git a/sequencer.c b/sequencer.c\nindex 35006c0cea6..84eed9e96bc 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -1220,7 +1220,7 @@ static int run_prepare_commit_msg_hook(struct repository *r,\n \t} else {\n \t\targ1 = \"message\";\n \t}\n-\tif (run_commit_hook(0, r->index_file, \"prepare-commit-msg\", name,\n+\tif (run_commit_hook(0, r->index_file, NULL, \"prepare-commit-msg\", name,\n \t\t\t    arg1, arg2, NULL))\n \t\tret = error(_(\"'prepare-commit-msg' hook failed\"));\n \n@@ -1552,7 +1552,7 @@ static int try_to_commit(struct repository *r,\n \t\tgoto out;\n \t}\n \n-\trun_commit_hook(0, r->index_file, \"post-commit\", NULL);\n+\trun_commit_hook(0, r->index_file, NULL, \"post-commit\", NULL);\n \tif (flags & AMEND_MSG)\n \t\tcommit_post_rewrite(r, current_head, oid);\n \n-- \n2.35.1.1242.gfeba0eae32b\n\n"},{"id":"451758","messageId":"20220321203019.2614799-1-jonathantanmy@google.com","threadId":"57439","inReplyTo":"patch-v2-2.2-9d16984898c-20220307T123244Z-avarab@gmail.com","subject":"Re: [PATCH v2 2/2] hooks: fix an obscure TOCTOU \"did we just run a hook?\" race","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-03-21T20:30:18Z","receivedAt":"2022-03-21T20:30:26Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index 8b8bdad3959..009a1de0a3d 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -726,11 +726,13 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n>  \tint clean_message_contents = (cleanup_mode != COMMIT_MSG_CLEANUP_NONE);\n>  \tint old_display_comment_prefix;\n>  \tint merge_contains_scissors = 0;\n> +\tint invoked_hook;\n>  \n>  \t/* This checks and barfs if author is badly specified */\n>  \tdetermine_author_info(author_ident);\n>  \n> -\tif (!no_verify && run_commit_hook(use_editor, index_file, \"pre-commit\", NULL))\n> +\tif (!no_verify && run_commit_hook(use_editor, index_file, &invoked_hook,\n> +\t\t\t\t\t  \"pre-commit\", NULL))\n>  \t\treturn 0;\n>  \n>  \tif (squash_message) {\n> @@ -1053,10 +1055,10 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n>  \t\treturn 0;\n>  \t}\n>  \n> -\tif (!no_verify && hook_exists(\"pre-commit\")) {\n> +\tif (!no_verify && invoked_hook) {\n\nThis commit causes Git to fail Valgrind (tested using \"cd t && sh\nt5537*.sh -i -v --valgrind-only=10\"). You can see here that a caller of\nrun_commit_hook() expects invoked_hook to be set, but...\n\n> diff --git a/commit.c b/commit.c\n> index d400f5dfa2b..396e14d7b32 100644\n> --- a/commit.c\n> +++ b/commit.c\n> @@ -1713,7 +1713,7 @@ size_t ignore_non_trailer(const char *buf, size_t len)\n>  }\n>  \n>  int run_commit_hook(int editor_is_used, const char *index_file,\n> -\t\t    const char *name, ...)\n> +\t\t    int *invoked_hook, const char *name, ...)\n>  {\n>  \tstruct run_hooks_opt opt = RUN_HOOKS_OPT_INIT;\n>  \tva_list args;\n> diff --git a/commit.h b/commit.h\n\nThe quoted part is the entire diff of commit.c. You can see that we have\na new argument \"int *invoked_hook\", but don't actually do anything with\nit. Could you (Ævar) take a look?\n"}]}