maria-developers team mailing list archive

[Kristian Nielsen <knielsen@xxxxxxxxxxxxxxx>]

Hi Andrei (Cc: Sujatha),

I noticed another thing with the wait_for_commit error handling while
looking at the MDEV-18648 patch.

We have code like this:

// Wakeup code in wait_for_commit::wakeup():
  mysql_mutex_lock(&LOCK_wait_commit);
  waitee= NULL;
  this->wakeup_error= wakeup_error;

// Wait code in wait_for_prior_commit():
  if (waitee)
    return wait_for_prior_commit2(thd);
  else
  {
    if (wakeup_error)
      my_error(ER_PRIOR_COMMIT_FAILED, MYF(0));

So the waiter code runs a "fast path" without locks. It is ok if we race on
the assignment of NULL to wait_for_commit::waitee variable, because then the
waiter will take the slow path and do proper locking.

But it looks like there is a race as follows:

  1. wakeup() sets waitee= NULL
  2. wait_for_prior_commit() sees waitee==NULL _and_ wakeup_error==0, and
     incorrectly returns without error.
  3. wakeup() too late sets wait_for_commit::wakeup_error.

It is not enough of course to swap the assignments in wakeup(). A
write-write memory barrier is needed between them in wakeup(), and a
corresponding read-read barrier is needed in wait_for_prior_commit().

With proper barriers, the waiter cannot see the write of waitee=NULL without
also seeing the write to wakeup_error. So it will either return with
non-zero wakeup_error or take the slow path with proper locking. Both of
which are fine.

Andrei, what do you think? Can you see something in the above analysis that
I missed? It seems like such an obvious miss in the code that I wonder how
it remained undetected for so long (or how I could have written that in the
first place...). But I suppose the race is very unlikely to hit in practice,
especially in a code path that only triggers in the error case...

 - Kristian.