Patchwork util: fix race in makedirs()

login
register
mail settings
Submitter Adam Simpkins
Date April 27, 2016, 11:31 p.m.
Message ID <e1d38ca347b8192b4cd9.1461799902@devbig126.prn1.facebook.com>
Download mbox | patch
Permalink /patch/14818/
State Accepted
Headers show

Comments

Adam Simpkins - April 27, 2016, 11:31 p.m.
# HG changeset patch
# User Adam Simpkins <simpkins@fb.com>
# Date 1461709979 25200
#      Tue Apr 26 15:32:59 2016 -0700
# Branch stable
# Node ID e1d38ca347b8192b4cd9c081b7d52b1f82f64936
# Parent  97811ff7964710d32cae951df1da8019b46151a2
util: fix race in makedirs()

Update makedirs() to ignore EEXIST in case someone else has already created the
directory in question.  Previously the ensuredirs() function existed, and was
nearly identical to makedirs() except that it fixed this race.  Unfortunately
ensuredirs() was only used in 3 places, and most code uses the racy makedirs()
function.  This fixes makedirs() to be non-racy, and replaces calls to
ensuredirs() with makedirs().

In particular, mercurial.scmutil.origpath() used the racy makedirs() code,
which could cause failures during "hg update" as it tried to create backup
directories.

This does slightly change the behavior of call sites using ensuredirs():
previously ensuredirs() would throw EEXIST if the path existed but was a
regular file instead of a directory.  It did this by explicitly checking
os.path.isdir() after getting EEXIST.  The makedirs() code did not do this and
swallowed all EEXIST errors.  I kept the makedirs() behavior, since it seemed
preferable to avoid the extra stat call in the common case where this directory
already exists.  If the path does happen to be a file, the caller will almost
certainly fail with an ENOTDIR error shortly afterwards anyway.  I checked
the 3 existing call sites of ensuredirs(), and this seems to be the case for
them.
Matt Mackall - April 28, 2016, 10:23 p.m.
On Wed, 2016-04-27 at 16:31 -0700, Adam Simpkins wrote:
> # HG changeset patch
> # User Adam Simpkins <simpkins@fb.com>
> # Date 1461709979 25200
> #      Tue Apr 26 15:32:59 2016 -0700
> # Branch stable
> # Node ID e1d38ca347b8192b4cd9c081b7d52b1f82f64936
> # Parent  97811ff7964710d32cae951df1da8019b46151a2
> util: fix race in makedirs()

Queued for stable, thanks. Congratulations on your first Mercurial patch!

-- 
Mathematics is the supreme nostalgia of our time.

Patch

diff --git a/mercurial/scmutil.py b/mercurial/scmutil.py
--- a/mercurial/scmutil.py
+++ b/mercurial/scmutil.py
@@ -539,7 +539,7 @@ 
             # to a directory. Let the posixfile() call below raise IOError.
             if basename:
                 if atomictemp:
-                    util.ensuredirs(dirname, self.createmode, notindexed)
+                    util.makedirs(dirname, self.createmode, notindexed)
                     return util.atomictempfile(f, mode, self.createmode)
                 try:
                     if 'w' in mode:
@@ -556,7 +556,7 @@ 
                     if e.errno != errno.ENOENT:
                         raise
                     nlink = 0
-                    util.ensuredirs(dirname, self.createmode, notindexed)
+                    util.makedirs(dirname, self.createmode, notindexed)
                 if nlink > 0:
                     if self._trustnlink is None:
                         self._trustnlink = nlink > 1 or util.checknlink(f)
@@ -583,7 +583,7 @@ 
         except OSError:
             pass
 
-        util.ensuredirs(os.path.dirname(linkname), self.createmode)
+        util.makedirs(os.path.dirname(linkname), self.createmode)
 
         if self._cansymlink:
             try:
diff --git a/mercurial/util.py b/mercurial/util.py
--- a/mercurial/util.py
+++ b/mercurial/util.py
@@ -1420,7 +1420,12 @@ 
             self.discard()
 
 def makedirs(name, mode=None, notindexed=False):
-    """recursive directory creation with parent mode inheritance"""
+    """recursive directory creation with parent mode inheritance
+
+    Newly created directories are marked as "not to be indexed by
+    the content indexing service", if ``notindexed`` is specified
+    for "write" mode access.
+    """
     try:
         makedir(name, notindexed)
     except OSError as err:
@@ -1432,29 +1437,13 @@ 
         if parent == name:
             raise
         makedirs(parent, mode, notindexed)
-        makedir(name, notindexed)
-    if mode is not None:
-        os.chmod(name, mode)
-
-def ensuredirs(name, mode=None, notindexed=False):
-    """race-safe recursive directory creation
-
-    Newly created directories are marked as "not to be indexed by
-    the content indexing service", if ``notindexed`` is specified
-    for "write" mode access.
-    """
-    if os.path.isdir(name):
-        return
-    parent = os.path.dirname(os.path.abspath(name))
-    if parent != name:
-        ensuredirs(parent, mode, notindexed)
-    try:
-        makedir(name, notindexed)
-    except OSError as err:
-        if err.errno == errno.EEXIST and os.path.isdir(name):
-            # someone else seems to have won a directory creation race
-            return
-        raise
+        try:
+            makedir(name, notindexed)
+        except OSError as err:
+            # Catch EEXIST to handle races
+            if err.errno == errno.EEXIST:
+                return
+            raise
     if mode is not None:
         os.chmod(name, mode)