Subject: Re: [PATCH 27/27] hooks: fix a TOCTOU in "did we run a hook?" heuristic

From: Emily Shaffer <[email-masked]>
Date: Fri, 18 Jun 2021 15:09:51 -0700

On Thu, Jun 17, 2021 at 12:23:01PM +0200, Ævar Arnfjörð Bjarmason wrote:
> 
> Fix a Time-of-check to time-of-use (TOCTOU) race in code added in
> 680ee550d72 (commit: skip discarding the index if there is no
> pre-commit hook, 2017-08-14).
> 
> We can fix the race passing around information about whether or not we
> ran the hook in question, instead of running hook_exists() after the
> fact to check if the hook in question exists. This problem has been
> noted on-list when 680ee550d72 was discussed[1], but had not been
> fixed.
> 
> In addition to fixing this for the pre-commit hook as suggested there
> I'm also fixing this for the pre-merge-commit hook. See
> 6098817fd7f (git-merge: honor pre-merge-commit hook, 2019-08-07) for
> the introduction of its previous behavior.
> 
> Let's also change this for the push-to-checkout hook. Now instead of
> checking if the hook exists and either doing a push to checkout or a
> push to deploy we'll always attempt a push to checkout. If the hook
> doesn't exist we'll fall back on push to deploy. The same behavior as
> before, without the TOCTOU race. See 0855331941b (receive-pack:
> support push-to-checkout hook, 2014-12-01) for the introduction of the
> previous behavior.
> 
> This leaves uses of hook_exists() in two places that matter. The
> "reference-transaction" check in refs.c, see 67541597670 (refs:
> implement reference transaction hook, 2020-06-19), and the
> prepare-commit-msg hook, see 66618a50f9c (sequencer: run
> 'prepare-commit-msg' hook, 2018-01-24).
> 
> In both of those cases we're saving ourselves CPU time by not
> preparing data for the hook that we'll then do nothing with if we
> don't have the hook. So using this "invoked_hook" pattern doesn't make
> sense in those cases.
> 
> More importantly, in those cases the worst we'll do is miss that we
> "should" run the hook because a new hook appeared, whereas in the
> pre-commit and pre-merge-commit cases we'll skip an important
> discard_cache() on the bases of our faulty guess.
> 
> I do think none of these races really matter in practice. It would be
> some one-off issue as a hook was added or removed. I did think it was
> stupid that we didn't pass a "did this run?" flag instead of doing
> this guessing at a distance though, so now we're not guessing anymore.

Yeah, I think your solution is very neat. I like this patch.

 - Emily