Django

Code

Ticket #4948 (assigned)

Opened 10 months ago

Last modified 5 months ago

Saving FileField files is not thread-safe

Reported by: anonymous Assigned to: rcoup (accepted)
Component: Core framework Version: SVN
Keywords: fs-rf-docs Cc:
Triage Stage: Accepted Has patch: 0
Needs documentation: 0 Needs tests: 0
Patch needs improvement: 1

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

safe_filename_gen__modelsonly.1.diff (1.7 kB) - added by rcoup on 09/14/07 20:33:07.
safe_filename_gen.1.diff (6.5 kB) - added by rcoup on 09/14/07 20:36:39.
Been doing this too long…
safe_filename_gen.1.2.diff (6.5 kB) - added by danielr on 09/15/07 07:58:51.
Full patch+tests with better version of test 2
testfg0.jpg (1.7 kB) - added by danielr on 09/15/07 17:12:41.
Test jpeg file showing corruption incurred during save.

Change History

07/21/07 09:02:38 changed by Simon G. <dev@simon.net.nz>

  • needs_better_patch changed.
  • stage changed from Unreviewed to Accepted.
  • needs_tests changed.
  • needs_docs changed.

09/14/07 20:32:35 changed 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.

09/14/07 20:33:07 changed by rcoup

  • attachment safe_filename_gen__modelsonly.1.diff added.

09/14/07 20:36:39 changed by rcoup

  • attachment safe_filename_gen.1.diff added.

Been doing this too long...

09/14/07 20:59:04 changed by rcoup

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

09/15/07 07:11:43 changed by danielr

  • needs_better_patch set to 1.

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

09/15/07 07:58:51 changed by danielr

  • attachment safe_filename_gen.1.2.diff added.

Full patch+tests with better version of test 2

09/15/07 08:02:14 changed 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.

(follow-up: ↓ 7 ) 09/15/07 08:36:48 changed by rcoup

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

(in reply to: ↑ 6 ) 09/15/07 17:11:42 changed 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.

09/15/07 17:12:41 changed by danielr

  • attachment testfg0.jpg added.

Test jpeg file showing corruption incurred during save.

09/15/07 20:22:32 changed 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.

12/11/07 13:28:35 changed by Gulopine

  • keywords set to fs-rf.

12/16/07 21:15:55 changed by Gulopine

  • keywords changed from fs-rf to fs-rf-docs.

Add/Change #4948 (Saving FileField files is not thread-safe)




Change Properties
Action