[PATCH 1 of 4 STABLE] lock: avoid unintentional lock acquisition at failure of readlock

[Yuya Nishihara]

On Sat, 29 Apr 2017 17:38:07 +0900, FUJIWARA Katsunori wrote:
> # HG changeset patch
> # User FUJIWARA Katsunori <foozy at lares.dti.ne.jp>
> # Date 1493454422 -32400
> #      Sat Apr 29 17:27:02 2017 +0900
> # Branch stable
> # Node ID 7cb8ebcca7d3a542031c41e09dc3ca0e2db36dcc
> # Parent  fcddcf442e4f4cd15bb9334e009fb60299e0a4e6
> lock: avoid unintentional lock acquisition at failure of readlock
> 
> Acquiring lock by vfs.makelock() and getting lock holder (aka
> "locker") information by vfs.readlock() aren't atomic action.
> Therefore, failure of the former doesn't ensure success of the latter.
> 
> Before this patch, lock is unintentionally acquired, if
> self.parentlock is None (this is default value), and lock._readlock()
> returns None for ENOEXIST at vfs.readlock(), because these None are
> recognized as equal to each other.

Nit: s/ENOEXIST/ENOENT/

> In this case, lock symlink (or file) isn't created, even though lock
> is treated as acquired in memory.
> 
> To avoid this issue, this patch checks self.parentlock, only if
> lock._readlock() returns valid non-None "locker".
> 
> This issue will be covered by a test added in subsequent patch,
> because simple emulation of ENOEXIST at vfs.readlock() easily causes
> another issue.  "raising ENOEXIST only at the first vfs.readlock()
> invocation" is too complicated for unit test, IMHO.
> 
> diff --git a/mercurial/lock.py b/mercurial/lock.py
> --- a/mercurial/lock.py
> +++ b/mercurial/lock.py
> @@ -135,7 +135,7 @@ class lock(object):
>                      # is different from the pid being different because we do
>                      # want the unlock and postrelease functions to be called,
>                      # but the lockfile to not be removed.
> -                    if locker == self.parentlock:
> +                    if locker is not None and locker == self.parentlock:

Perhaps we should retry makelock() in this case?

  try:
      try:
          return self.vfs.readlock(self.f)
      except (OSError, IOError) as why:
          if why.errno == errno.ENOENT:
              # XXX: what should we do if retry - 1 == 0 ?
              continue

This patch itself seemed good for stable, but the next patch made me rethink
about the way to fix the problem.