[Django] #18233: file_move_safe overwrites destination file
Django
--------------------------------------+--------------------
Reporter: anonymous | Owner: nobody
Type: Bug | Status: new
Component: File uploads/storage | Version: 1.3
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+--------------------
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/18233>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: anonymous | Owner: aviraldg
Type: Bug | Status: assigned
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Changes (by aviraldg):
* status: new => assigned
* needs_better_patch: => 0
* needs_tests: => 0
* owner: nobody => aviraldg
* needs_docs: => 0
* has_patch: 0 => 1
* stage: Unreviewed => Accepted
Comment:
Confirmed and added
[https://code.djangoproject.com/attachment/ticket/18233/fix_18233.diff
patch]. ([https://github.com/django/django/pull/18 Github Pull Request])
--
Ticket URL: <https://code.djangoproject.com/ticket/18233#comment:1>
Django
Reporter: anonymous | Owner: aviraldg
Type: Bug | Status: assigned
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Changes (by aviraldg):
* cc: aviraldg (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/18233#comment:2>
Django
Reporter: anonymous | Owner: aviraldg
Type: Bug | Status: assigned
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Changes (by aviraldg):
* needs_tests: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/18233#comment:3>
Django
Reporter: anonymous | Owner: aviraldg
Type: Bug | Status: assigned
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Changes (by aviraldg):
* needs_tests: 1 => 0
Comment:
(I've added a regression test to the Github pull request.)
--
Ticket URL: <https://code.djangoproject.com/ticket/18233#comment:4>
Django
Reporter: anonymous | Owner: aviraldg
Type: Bug | Status: assigned
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Changes (by moritzs):
* needs_better_patch: 0 => 1
Comment:
The attached file should contain the tests, even if they're in the pull
request, until the core devs announce how to deal with pull requests.
Besides, the IOError's message should read 'Destination file already
exists'
--
Ticket URL: <https://code.djangoproject.com/ticket/18233#comment:5>
Django
Type: Bug | Status: assigned
uploads/storage | Resolution:
Severity: Normal | Triage Stage: Ready for
Keywords: | checkin
Changes (by moritzs):
* needs_better_patch: 1 => 0
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/18233#comment:6>
Django
Type: Bug | Status: assigned
Keywords: | Triage Stage: Accepted
Changes (by aaugustin):
* needs_better_patch: 0 => 1
* stage: Ready for checkin => Accepted
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/18233#comment:7>
Django
Reporter: anonymous | Owner: aviraldg
Type: Bug | Status: assigned
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by aaugustin):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/18233#comment:8>
Django
Reporter: anonymous | Owner: aviraldg
Type: Bug | Status: assigned
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by aviraldg):
Should we leave it as it is with a warning, then?
--
Ticket URL: <https://code.djangoproject.com/ticket/18233#comment:9>
Django
Reporter: anonymous | Owner: aviraldg
Type: Bug | Status: closed
Component: File | Version: 1.3
uploads/storage |
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Changes (by jnnt):
* status: assigned => closed
* resolution: => duplicate
Comment:
This has been fixed in [ticket:20486]
--
Ticket URL: <https://code.djangoproject.com/ticket/18233#comment:10>