[PATCH] windows.rename: eliminate temp name race

[Adrian Buehlmann]

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: