file_move_safe overwrites destination file

[anonymous]

On linux with python 2.7 django 1.3 overwrites destination file when using file_move_safe.

It calls os.rename without any checks. But on linux this function overwrites destination file, if it is a file and you have permission to do so.

There should be placed a check just before this call, to ensure, that destination does not exists.

I have also looked to git master and the code have not changed.

[Aviral Dasgupta]

Confirmed and added patch. (​Github Pull Request)

[Aviral Dasgupta]

(I've added a regression test to the Github pull request.)

[Moritz Sichert]

The attached file should either contain all commits (including the tests) or be removed.

Besides, the IOError's message should read 'Destination file already exists'.

[Aviral Dasgupta]

Fixed and added tests.

[Aymeric Augustin]

A few remarks:

• There's a race condition between the test and the move. I don't know if it's possible to avoid this problem. The check must be done by the OS (like the O_EXCL flag to open), because a file could be created at the target location by any process in the system.
• There's a comment in django.core.files.move that suggests that moving open files behaves differently from moving closed files. I'd rather create a temporary dir, create two files inside, close them, and then perform the test. To clean up, the easiest is to shutil.rmtree the temporary directory.
• If the test fails, tearDown will raise an exception, because it'll try to remove a file that doesn't exist. Using the technique I suggested above avoids this problem.
• Finally, the list of contributors is in alphabetical order.

[Aymeric Augustin]

I don't know how we could fix the race condition. Creating an empty target file with O_EXCL, then moving the source file would work under Unix. However that would trigger as OSError under Windows.

The docs say:

On Windows, if dst already exists, OSError will be raised even if it is a file; there may be no way to implement an atomic rename when dst names an existing file.

[Aviral Dasgupta]

Should we leave it as it is with a warning, then?

[jnnt]

This has been fixed in 20486