FileSystemStorage with allow_overwrite=True does not truncate previous file

[Gaël UTARD]

The new allow_overwrite parameter of FileSystemStorage class allows to overwrite an existing file. However, the previous file is not truncated. So, if the previous file was bigger than the new, the file gets corrupted.

It is possible to workaround with :

storage.OS_OPEN_FLAGS = storage.OS_OPEN_FLAGS & ~os.O_EXCL | os.O_TRUNC

[Jacob Walls]

Thanks for the report, reproduced by adjusting this existing test case:

tests/file_storage/tests.py

@@ -616 +616 @@
         """Saving to same file name twice overwrites the first file."""
         name = "test.file"
         self.assertFalse(self.storage.exists(name))
-        content_1 = b"content one"
-        content_2 = b"second content"
+        content_1 = b"content one: this part should be truncated."
+        content_2 = b"content two"
         f_1 = ContentFile(content_1)
         f_2 = ContentFile(content_2)
         stored_name_1 = self.storage.save(name, f_1)

---

======================================================================
FAIL: test_save_overwrite_behavior (file_storage.tests.OverwritingStorageTests.test_save_overwrite_behavior)
Saving to same file name twice overwrites the first file.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/.../django/tests/file_storage/tests.py", line 633, in test_save_overwrite_behavior
    self.assertEqual(fp.read(), content_2)
AssertionError: b'content two: this part should be truncated.' != b'content two'

----------------------------------------------------------------------
Ran 2 tests in 0.002s

FAILED (failures=1)

Test passes with the suggested patch:

diff --git a/django/core/files/storage/filesystem.py b/django/core/files/storage/filesystem.py
index b8de9b0a58..54c31e536a 100644
--- a/django/core/files/storage/filesystem.py
+++ b/django/core/files/storage/filesystem.py
@@ -113,7 +113,7 @@ class FileSystemStorage(Storage, StorageSettingsMixin):
                         | getattr(os, "O_BINARY", 0)
                     )
                     if self._allow_overwrite:
-                        open_flags = open_flags & ~os.O_EXCL
+                        open_flags = open_flags & ~os.O_EXCL | os.O_TRUNC
                     fd = os.open(full_path, open_flags, 0o666)
                     _file = None
                     try:

---

Would you like to submit a PR?

[Gaël UTARD]

Jacob, thanks for your confirmation of the bug, and confirmation of the fix I had in mind.
I pushed a PR, including your idea to assert the length of both contents in the test.
It's my first contribution to Django. I read the docs. I hope I did it right.

[Sarah Boyce <42296566+sarahboyce@…>]

In 0d1dd6bb:

Fixed #36191 -- Truncated the overwritten file content in FileSystemStorage.

[Sarah Boyce <42296566+sarahboyce@…>]

In ae391ca3:

[5.2.x] Fixed #36191 -- Truncated the overwritten file content in FileSystemStorage.

Backport of 0d1dd6bba0c18b7feb6caa5cbd8df80fbac54afd from main.

[Sarah Boyce <42296566+sarahboyce@…>]

In a9d03c4:

[5.1.x] Fixed #36191 -- Truncated the overwritten file content in FileSystemStorage.

Backport of 0d1dd6bba0c18b7feb6caa5cbd8df80fbac54afd from main.

[Baptiste Mispelon]

The bug is still present in the file_move_safe() function [1] which can be triggered under some circumstances when calling FileSystemStorage._save() [2] (observed in 5.2 but I'm fairly sure the same is true in main).

Because it's essentially the same bug I'm reopening this one instead of creating a new one, I hope that's OK.

I'm also leaving the Release Blocker status on due to the data corruption aspect of this issue.

[1] ​https://github.com/django/django/blob/f7f38f3a0b44d8c6d14344dae66b6ce52cd77b55/django/core/files/move.py#L57
[2] ​https://github.com/django/django/blob/f7f38f3a0b44d8c6d14344dae66b6ce52cd77b55/django/core/files/storage/filesystem.py#L99

[Sarah Boyce]

Please open a new ticket so we can replicate and triage the issue