[PATCH V2] windows.rename: eliminate temp name race

Adrian Buehlmann adrian at cadifra.com
Sun Dec 26 18:31:14 CST 2010

# HG changeset patch
# User Adrian Buehlmann <adrian at cadifra.com>
# Date 1293408751 -3600
# Node ID 9009cfda3401d657420799c3c82b7c43a1b039c7
# Parent  1aea66b71f4f915d482ea2be77e5322acf14db19
windows.rename: eliminate temp name race

On Windows, os.rename reliably raises OSError with errno.EEXIST if the
destination already exists (even on shares served by Samba).

Windows does *not* silently overwrite the destination of a rename.

So there is no need to first call os.path.exists on the chosen temp path.

Trusting os.path.exists is actually harmful, since using it enables the
following racy sequence of actions:

 1) os.path.exists(temp) returns False
 2) some evil other process creates a file with name temp
 3) os.rename(dst, temp) now fails because temp has been taken

Not using os.path.exists and directly trying os.rename(dst, temp)
eliminates this race.

diff --git a/mercurial/windows.py b/mercurial/windows.py
--- a/mercurial/windows.py
+++ b/mercurial/windows.py
@@ -299,20 +299,20 @@ def rename(src, dst):
         # rename is safe to do.
         # The temporary name is chosen at random to avoid the situation
         # where a file is left lying around from a previous aborted run.
-        # The usual race condition this introduces can't be avoided as
-        # we need the name to rename into, and not the file itself. Due
-        # to the nature of the operation however, any races will at worst
-        # lead to the rename failing and the current operation aborting.
-        def tempname(prefix):
-            for tries in xrange(10):
-                temp = '%s-%08x' % (prefix, random.randint(0, 0xffffffff))
-                if not os.path.exists(temp):
-                    return temp
+        for tries in xrange(10):
+            temp = '%s-%08x' % (dst, random.randint(0, 0xffffffff))
+            try:
+                os.rename(dst, temp)  # raises OSError EEXIST if temp exists
+                break
+            except OSError, e:
+                if e.errno != errno.EEXIST:
+                    raise
+                temp = None
+        if temp is None:
             raise IOError, (errno.EEXIST, "No usable temporary filename found")
-        temp = tempname(dst)
-        os.rename(dst, temp)

More information about the Mercurial-devel mailing list