[PATCH] windows.rename: eliminate temp name race
Adrian Buehlmann
adrian at cadifra.com
Sun Dec 26 18:27:44 CST 2010
Please ignore. Superseded by V2 of this patch.
On 2010-12-27 00:44, Adrian Buehlmann wrote:
> # HG changeset patch
> # User Adrian Buehlmann <adrian at cadifra.com>
> # Date 1293405868 -3600
> # Node ID e9652394c81456ea84f86dec93ecebf0c0c1253a
> # 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,18 @@ 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
> + continue
...
> raise IOError, (errno.EEXIST, "No usable temporary filename found")
Is never reached.
>
> - temp = tempname(dst)
> - os.rename(dst, temp)
> try:
> os.unlink(temp)
> except:
More information about the Mercurial-devel
mailing list