Opened 4 years ago

Closed 11 months 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


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)

fix_18233.diff (2.3 KB) - added by Aviral Dasgupta 4 years ago.
Fixed and added tests.

Download all attachments as: .zip

Change History (11)

comment:1 Changed 4 years ago by Aviral Dasgupta

Has patch: set
Needs documentation: unset
Needs tests: unset
Owner: changed from nobody to Aviral Dasgupta
Patch needs improvement: unset
Status: newassigned
Triage Stage: UnreviewedAccepted

Confirmed and added patch. (Github Pull Request)

comment:2 Changed 4 years ago by Aviral Dasgupta

Cc: Aviral Dasgupta added

comment:3 Changed 4 years ago by Aviral Dasgupta

Needs tests: set

comment:4 Changed 4 years ago by Aviral Dasgupta

Needs tests: unset

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

comment:5 Changed 4 years ago by Moritz Sichert

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'.

Last edited 4 years ago by Moritz Sichert (previous) (diff)

Changed 4 years ago by Aviral Dasgupta

Attachment: fix_18233.diff added

Fixed and added tests.

comment:6 Changed 4 years ago by Moritz Sichert

Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

comment:7 Changed 4 years ago by Aymeric Augustin

Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

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.

comment:8 Changed 4 years ago by 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.

comment:9 Changed 4 years ago by Aviral Dasgupta

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

comment:10 Changed 11 months ago by jnnt

Resolution: duplicate
Status: assignedclosed

This has been fixed in 20486

Note: See TracTickets for help on using tickets.
Back to Top