Code

Opened 7 years ago

Closed 6 years ago

Last modified 3 years ago

#4948 closed (fixed)

Saving FileField files is not thread-safe

Reported by: anonymous Owned by: rcoup
Component: Core (Other) Version: master
Severity: Keywords: fs-rf-docs
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: UI/UX:

Description

Picking the filename in django.db.modes.Model._save_FIELD_file is not thread-safe. There is an obvious racing condition if one thread will have found a non-existing filename and then a second thread will search for a non-existing name based on the same filename before the first thread starts to write the file.

Because this function is probably most often called from the admin UI and the newforms frameworks doesn't support file upload yet, it isn't critical. Still, this kind of problems can cause difficult to debug problems.

Attachments (5)

safe_filename_gen__modelsonly.1.diff (1.7 KB) - added by rcoup 7 years ago.
safe_filename_gen.1.diff (6.5 KB) - added by rcoup 7 years ago.
Been doing this too long…
safe_filename_gen.1.2.diff (6.5 KB) - added by danielr 7 years ago.
Full patch+tests with better version of test 2
testfg0.jpg (1.7 KB) - added by danielr 7 years ago.
Test jpeg file showing corruption incurred during save.
t.py (531 bytes) - added by loewis 6 years ago.
Demonstration of race condition

Download all attachments as: .zip

Change History (21)

comment:1 Changed 7 years ago by Simon G. <dev@…>

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

comment:2 Changed 7 years ago by rcoup

  • Owner changed from nobody to rcoup
  • Status changed from new to assigned

Attached patch (safe_filename_gen.1.diff) uses the standard open(path, O_CREAT | O_EXCL) pattern.

Since the tests in bug639 had perfectly good models and files, I renamed it to file_field and added tests into there. A patch to base.py is available in safe_filename_gen__modelsonly.1.diff.

Changed 7 years ago by rcoup

Changed 7 years ago by rcoup

Been doing this too long...

comment:3 Changed 7 years ago by rcoup

Malcolm pointed out this needs confirmation on Windows wrt. O_CREAT & O_EXCL

comment:4 Changed 7 years ago by danielr

  • Patch needs improvement set

Tested this on Windows using supplied regression tests - tests 2 and 3 both fail.

Changed 7 years ago by danielr

Full patch+tests with better version of test 2

comment:5 Changed 7 years ago by danielr

The failure in test 2 turned out to be a problem in the test itself - it wasn't using os.path.normpath to normalise a hard-coded file path. I've uploaded a new version of the full patch with this corrected.

However, test 3 - comparing the uploaded data with the original file - still fails. It seems to be corrupting the image during upload. I checked this with the non-patched SVN, and the test passes there, so there is something going on with this patch that's causing corruption on Windows.

comment:6 follow-up: Changed 7 years ago by rcoup

If you skip tearDown() and manually inspect the files, do they the original jpeg (ie. is it just weird tests)?

comment:7 in reply to: ↑ 6 Changed 7 years ago by danielr

I did try that - the saved file is definitely corrupt. With the default jpeg, you get the top two-thirds of the picture OK but then it starts looking very odd. I've tried it multiple times and the same corruption happens each time. I'll upload the corrupted jpg here.

I also tried it with other image files and the corruption is even worse.

Changed 7 years ago by danielr

Test jpeg file showing corruption incurred during save.

comment:8 Changed 7 years ago by rcoup

Maybe it's doing something in text mode, even thought its set to use binary. I'll do some more experiments when I get to work tomorrow.

comment:9 Changed 6 years ago by Gulopine

  • Keywords fs-rf added

comment:10 Changed 6 years ago by Gulopine

  • Keywords fs-rf-docs added; fs-rf removed

comment:11 Changed 6 years ago by Gulopine

  • milestone set to 1.0 beta

comment:12 Changed 6 years ago by axiak

Since r7814 (the merge of #2070), there has been some file locking logic in save_FIELD_file.

To me at least, it seems like the fix in #2070 is just as adequate as the patches contained in this ticket (except cross-platform).

Of course, I don't believe the patch in this ticket or #2070 fixes this problem completely.

So the question: Anybody who was able to see this problem before, can you now? (post r7814)

Cheers,
Mike

comment:13 Changed 6 years ago by mtredinnick

Okay, so I'm not convinced this is really fixed by the #2070 changes, but it is very hard to fix due to the increased separation of responsibilities that change introduced. Leaving for "later" for the time being, but we should look at this again.

Changed 6 years ago by loewis

Demonstration of race condition

comment:14 Changed 6 years ago by loewis

I believe the race condition still exists after the file storage refactoring;the attached t.py demonstrates it. If two threads simultaneously try to create a file, they will both create the same file, and the last writer wins. If the sleep(5) is activated between in the test, then the second writer will see that the file already exists, and create the file under a different name.

The current locking in the code doesn't really help - it only means that the content of the two different files will not interleave, but instead, that one fully overwrites the other.

What should happen instead is that the open flag for exclusive open is used, which causes failure to open if the file is already present. In that case, Storage.save should go back and ask for a different available name, as the first name has proven not to be available.

comment:15 Changed 6 years ago by jacob

  • Resolution set to fixed
  • Status changed from assigned to closed

(In [8306]) Fixed #4948, a race condition in file saving. Thanks to Martin von Löwis, who diagnosed the problem and pointed the way to a fix.

comment:16 Changed 3 years ago by jacob

  • milestone 1.0 beta deleted

Milestone 1.0 beta deleted

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.