Patchwork [1,of,4,STABLE,V3] lock: avoid unintentional lock acquisition at failure of readlock

login
register
mail settings
Submitter Katsunori FUJIWARA
Date May 1, 2017, 11:02 a.m.
Message ID <e1938d6051da1eff0c0d.1493636569@juju>
Download mbox | patch
Permalink /patch/20326/
State Accepted
Headers show

Comments

Katsunori FUJIWARA - May 1, 2017, 11:02 a.m.
# HG changeset patch
# User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
# Date 1493636332 -32400
#      Mon May 01 19:58:52 2017 +0900
# Branch stable
# Node ID e1938d6051da1eff0c0d78fc7aae0d426e99aad2
# Parent  b59a292d0a536ee21e17d018829c36d8d4415569
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 ENOENT at vfs.readlock(), because these None are
recognized as equal to each other.

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 retries lock acquisition immediately,
if lock._readlock() returns None "locker".

This issue will be covered by a test added in subsequent patch,
because simple emulation of ENOENT at vfs.readlock() easily causes
another issue.  "raising ENOENT only at the first vfs.readlock()
invocation" is too complicated for unit test, IMHO.
Yuya Nishihara - May 1, 2017, 12:56 p.m.
On Mon, 01 May 2017 20:02:49 +0900, FUJIWARA Katsunori wrote:
> # HG changeset patch
> # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
> # Date 1493636332 -32400
> #      Mon May 01 19:58:52 2017 +0900
> # Branch stable
> # Node ID e1938d6051da1eff0c0d78fc7aae0d426e99aad2
> # Parent  b59a292d0a536ee21e17d018829c36d8d4415569
> lock: avoid unintentional lock acquisition at failure of readlock

Queued the first two, thanks. Since we're very close to the release, let's
revisit the message improvements later.

> diff --git a/mercurial/lock.py b/mercurial/lock.py
> --- a/mercurial/lock.py
> +++ b/mercurial/lock.py
> @@ -131,6 +131,9 @@ class lock(object):
>              except (OSError, IOError) as why:
>                  if why.errno == errno.EEXIST:
>                      locker = self._readlock()
> +                    if locker is None:
> +                        continue

Perhaps we can get rid of the subsequent "if locker is not None" condition,
as a follow up.
Yuya Nishihara - May 1, 2017, 1:02 p.m.
On Mon, 1 May 2017 21:56:29 +0900, Yuya Nishihara wrote:
> Perhaps we can get rid of the subsequent "if locker is not None" condition,
> as a follow up.

Never mind. You're right. _testlock() has an implicit 'return None'.

Patch

diff --git a/mercurial/lock.py b/mercurial/lock.py
--- a/mercurial/lock.py
+++ b/mercurial/lock.py
@@ -131,6 +131,9 @@  class lock(object):
             except (OSError, IOError) as why:
                 if why.errno == errno.EEXIST:
                     locker = self._readlock()
+                    if locker is None:
+                        continue
+
                     # special case where a parent process holds the lock -- this
                     # is different from the pid being different because we do
                     # want the unlock and postrelease functions to be called,