Opened 3 years ago

Last modified 3 years ago

#18233 assigned Bug

file_move_safe overwrites destination file

Reported by: anonymous Owned by: aviraldg
Component: File uploads/storage Version: 1.3
Severity: Normal Keywords:
Cc: magon@…, aviraldg 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)

fix_18233.diff (2.3 KB) - added by aviraldg 3 years ago.
Fixed and added tests.

Download all attachments as: .zip

Change History (10)

comment:1 Changed 3 years ago by aviraldg

  • Has patch set
  • Needs documentation unset
  • Needs tests unset
  • Owner changed from nobody to aviraldg
  • Patch needs improvement unset
  • Status changed from new to assigned
  • Triage Stage changed from Unreviewed to Accepted

Confirmed and added patch. (Github Pull Request)

comment:2 Changed 3 years ago by aviraldg

  • Cc aviraldg added

comment:3 Changed 3 years ago by aviraldg

  • Needs tests set

comment:4 Changed 3 years ago by aviraldg

  • Needs tests unset

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

comment:5 Changed 3 years ago by moritzs

  • 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 3 years ago by moritzs (previous) (diff)

Changed 3 years ago by aviraldg

Fixed and added tests.

comment:6 Changed 3 years ago by moritzs

  • Patch needs improvement unset
  • Triage Stage changed from Accepted to Ready for checkin

comment:7 Changed 3 years ago by aaugustin

  • Patch needs improvement set
  • Triage Stage changed from Ready for checkin to 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 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 3 years ago 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.

comment:9 Changed 3 years ago by aviraldg

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

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