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

From: Ævar Arnfjörð Bjarmason <[email-masked]>
Date: Fri, 28 May 2021 16:11:21 +0200

On Fri, May 28 2021, Bagas Sanjaya wrote:

> On 28/05/21 19.11, Ævar Arnfjörð Bjarmason wrote:
>> 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.
>
> What is TOCTOU? I never hear this term before.

A common race condition[1], time of use, time of check. I.e. in this
case we use the hook first, and then later we check if there's a
hook. Thus we've got a logic error where we're assuming we'll get the
same answer[2] in both instances.

In this case what we plainly want is to just save away whether we did X,
not do X, and then later check if we can do X to see if we did X
earlier.

I think this whole thing is still suspect even with my fix, there's an
inherent assumption here of a single-thread view of the world. I.e. that
you have a user running a git command at a time, and that while our hook
run didn't change the index, a background process might have.

But this probably handles the most common case, i.e. a hook explicitly
changing the index and wanting its invoker to notice, and in any case we
have the single-thread assumption already, so we might as well make it
less buggy/racy.

If you dig through the history we used to do this without the check for
"earlier X?" before for at least one of the hooks, but it got changed to
the current state as an optimization.

1. https://en.wikipedia.org/wiki/TOCTOU
2. I mean, I don't think any programmer assumed it in this case, it was
   just intentional lazyness or a legitimate trade-off, but now that
   it's easy to eliminate the race...