Opened 13 years ago
Closed 9 years ago
#18233 closed Bug (duplicate)
file_move_safe overwrites destination file
Reported by: | anonymous | Owned by: | Aviral Dasgupta |
---|---|---|---|
Component: | File uploads/storage | Version: | 1.3 |
Severity: | Normal | Keywords: | |
Cc: | magon@…, Aviral Dasgupta | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | yes |
Easy pickings: | no | UI/UX: | no |
Description
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.
Attachments (1)
Change History (11)
comment:1 by , 13 years ago
Has patch: | set |
---|---|
Owner: | changed from | to
Status: | new → assigned |
Triage Stage: | Unreviewed → Accepted |
comment:2 by , 13 years ago
Cc: | added |
---|
comment:3 by , 13 years ago
Needs tests: | set |
---|
comment:4 by , 13 years ago
Needs tests: | unset |
---|
(I've added a regression test to the Github pull request.)
comment:5 by , 13 years ago
Patch needs improvement: | set |
---|
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'.
comment:6 by , 13 years ago
Patch needs improvement: | unset |
---|---|
Triage Stage: | Accepted → Ready for checkin |
comment:7 by , 13 years ago
Patch needs improvement: | set |
---|---|
Triage Stage: | Ready for checkin → Accepted |
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 toshutil.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.
comment:8 by , 13 years ago
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.
comment:10 by , 9 years ago
Resolution: | → duplicate |
---|---|
Status: | assigned → closed |
This has been fixed in 20486
Confirmed and added patch. (Github Pull Request)