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
