[PATCH 05 of 11] cmdutil: securely rename a to A on disk

[Mads Kiilerich]

On 08/11/2009 11:10 AM, Simon Heimberg wrote:
> Am Freitag, den 07.08.2009, 23:20 +0200 schrieb Mads Kiilerich:
>    
>> Simon Heimberg wrote, On 08/07/2009 09:43 PM:
>>      
>>> # HG changeset patch
>>> # User Simon Heimberg<simohe at besonet.ch>
>>> # Date 1249656995 -7200
>>> # Node ID 50e9f6b4d8878fd867a250c5be2578330b7f4769
>>> # Parent  76406c3720caa61cf9d466b43d2aac68b15ec362
>>> cmdutil: securely rename a to A on disk
>>>
>>>        
> [snip]
>
>    
>>> +def temprename(src):
>>> +    """renames a file to a temporary name
>>> +       the new name is returned"""
>>> +    for tries in xrange(10):
>>> +        temp = '%s-%08x' % (src, random.randint(0, 0xffffffff))
>>> +        try:
>>> +            os.rename(src, temp)
>>> +            return temp
>>> +        except IOError:
>>> +            continue
>>> +    raise IOError, (errno.EEXIST, "No usable temporary filename found")
>>> +
>>>
>>>        
>> Does moving a function count as touching it?
>>      
> In this case definitively yes, I have already changed some things.
>    
>> IF you are touching it anyway then it it would be more secure with an
>> extra "if os.path.exists(temp): continue".
>>      
> You are right. I thought os.rename would raise an error when the
> destination file exists. So I removed os.path.exists, but that is wrong.
> The file could be created after os.path.exists is called and before
> os.rename. Is this problematic? Should the file be created with
> tempfile.mkstemp and overwritten with os.rename?
>    

I don't think we care about such races. We are in the users working 
directory, so if the user wants to shoot while the gun points at his 
foot he should be free to do that.

>> This function can also give strange errors on filenames close to the max
>> length. Perhaps we could drop the basename and use a short "random" name
>> in dirname...
>>      
> Does this mean the length of the filename or the entire path?

Many file systems have limits for both, but on most file systems the 
limits are so high that it doesn't matter.

AFAIK nobody complained, and if someone complain they are close to the 
limit anyway, so perhaps we don't care?

> If it is
> the first, temprename could look like this:
>
> def temprename(src):
>      """renames a file to a temporary name
>         the new name is returned"""
>      temp = tempname.mktemp(suffix='tmpmv', dir=os.path.dirname(src))
>      os.rename(src, temp)
>      return temp
>
> Is there a reason why tempname.mktemp was not used?
>    

You mean tempfile.mktemp / tempfile.mkstemp? Dunno. Perhaps the current 
code is simpler and faster and stresses the file system less.

/Mads