[Django] #36191: FileSystemStorage with allow_overwrite=True does not truncate previous file

[Django]

#36191: FileSystemStorage with allow_overwrite=True does not truncate previous file
------------------------+------------------------------------------------
Reporter: gutard | Type: Bug
Status: new | Component: File uploads/storage
Version: 5.1 | Severity: Normal
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
------------------------+------------------------------------------------
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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/36191>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#36191: FileSystemStorage with allow_overwrite=True does not truncate previous file

--------------------------------------+------------------------------------
Reporter: Gaël UTARD | Owner: (none)

Type: Bug | Status: new
Component: File uploads/storage | Version: 5.1

Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0

--------------------------------------+------------------------------------
Changes (by Jacob Walls):

* severity: Normal => Release blocker
* stage: Unreviewed => Accepted

Comment:

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

{{{#!diff
diff --git a/tests/file_storage/tests.py b/tests/file_storage/tests.py
index c048b8f071..b1dafd7cb9 100644
--- a/tests/file_storage/tests.py
+++ b/tests/file_storage/tests.py
@@ -617,7 +617,8 @@ class OverwritingStorageTests(FileStorageTests):
name = "test.file"
self.assertFalse(self.storage.exists(name))
content_1 = b"content one"
- content_2 = b"second content"
+ content_2 = b"overwrite"
+ assert len(content_1) > len(content_2), "Ensure truncation is
tested."
f_1 = ContentFile(content_1)
f_2 = ContentFile(content_2)
stored_name_1 = self.storage.save(name, f_1)
}}}
----
{{{#!py
======================================================================
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'overwritene' != b'overwrite'

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

FAILED (failures=1)
}}}

Test passes with the suggested patch:
{{{#!py
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?
--
Ticket URL: <https://code.djangoproject.com/ticket/36191#comment:1>

[Django]

#36191: FileSystemStorage with allow_overwrite=True does not truncate previous file

-------------------------------------+-------------------------------------
Reporter: Gaël UTARD | Owner: Gaël
| UTARD
Type: Bug | Status: assigned
Component: File | Version: 5.1
uploads/storage |

Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0

-------------------------------------+-------------------------------------
Changes (by Gaël UTARD):

* owner: (none) => Gaël UTARD
* status: new => assigned

--
Ticket URL: <https://code.djangoproject.com/ticket/36191#comment:2>

[Django]

#36191: FileSystemStorage with allow_overwrite=True does not truncate previous file
-------------------------------------+-------------------------------------
Reporter: Gaël UTARD | Owner: Gaël
| UTARD
Type: Bug | Status: assigned
Component: File | Version: 5.1
uploads/storage |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Gaël UTARD):

* has_patch: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/36191#comment:3>

[Django]

#36191: FileSystemStorage with allow_overwrite=True does not truncate previous file
-------------------------------------+-------------------------------------
Reporter: Gaël UTARD | Owner: Gaël
| UTARD
Type: Bug | Status: assigned
Component: File | Version: 5.1
uploads/storage |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/36191#comment:4>

[Django]

#36191: FileSystemStorage with allow_overwrite=True does not truncate previous file
-------------------------------------+-------------------------------------
Reporter: Gaël UTARD | Owner: Gaël
| UTARD
Type: Bug | Status: assigned
Component: File | Version: 5.1
uploads/storage |
Severity: Release blocker | Resolution:

Keywords: | Triage Stage: Ready for
| checkin

Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Sarah Boyce):

* stage: Accepted => Ready for checkin

--
Ticket URL: <https://code.djangoproject.com/ticket/36191#comment:5>

[Django]

#36191: FileSystemStorage with allow_overwrite=True does not truncate previous file
-------------------------------------+-------------------------------------
Reporter: Gaël UTARD | Owner: Gaël
| UTARD

Type: Bug | Status: closed

Component: File | Version: 5.1
uploads/storage |

Severity: Release blocker | Resolution: fixed

Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Sarah Boyce <42296566+sarahboyce@…>):

* resolution: => fixed
* status: assigned => closed

Comment:

In [changeset:"0d1dd6bba0c18b7feb6caa5cbd8df80fbac54afd" 0d1dd6bb]:
{{{#!CommitTicketReference repository=""
revision="0d1dd6bba0c18b7feb6caa5cbd8df80fbac54afd"
Fixed #36191 -- Truncated the overwritten file content in
FileSystemStorage.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/36191#comment:6>

[Django]

#36191: FileSystemStorage with allow_overwrite=True does not truncate previous file
-------------------------------------+-------------------------------------
Reporter: Gaël UTARD | Owner: Gaël
| UTARD
Type: Bug | Status: closed
Component: File | Version: 5.1
uploads/storage |
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Sarah Boyce <42296566+sarahboyce@…>):

In [changeset:"ae391ca368b80e6a3888af859437a5499c451e0c" ae391ca3]:
{{{#!CommitTicketReference repository=""
revision="ae391ca368b80e6a3888af859437a5499c451e0c"
[5.2.x] Fixed #36191 -- Truncated the overwritten file content in
FileSystemStorage.

Backport of 0d1dd6bba0c18b7feb6caa5cbd8df80fbac54afd from main.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/36191#comment:7>

[Django]

#36191: FileSystemStorage with allow_overwrite=True does not truncate previous file
-------------------------------------+-------------------------------------
Reporter: Gaël UTARD | Owner: Gaël
| UTARD
Type: Bug | Status: closed
Component: File | Version: 5.1
uploads/storage |
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Sarah Boyce <42296566+sarahboyce@…>):

In [changeset:"a9d03c409406f3f95f692bc5545ebacc7f4e2e4f" a9d03c4]:
{{{#!CommitTicketReference repository=""
revision="a9d03c409406f3f95f692bc5545ebacc7f4e2e4f"
[5.1.x] Fixed #36191 -- Truncated the overwritten file content in

FileSystemStorage.

Backport of 0d1dd6bba0c18b7feb6caa5cbd8df80fbac54afd from main.
}}}
--

Ticket URL: <https://code.djangoproject.com/ticket/36191#comment:8>

[Django]

#36191: FileSystemStorage with allow_overwrite=True does not truncate previous file
-------------------------------------+-------------------------------------
Reporter: Gaël UTARD | Owner: Gaël
| UTARD

Type: Bug | Status: new
Component: File | Version: 5.2

uploads/storage |
Severity: Release blocker | Resolution:

Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Baptiste Mispelon):

* has_patch: 1 => 0
* resolution: fixed =>
* stage: Ready for checkin => Accepted
* status: closed => new
* version: 5.1 => 5.2

Comment:

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
--
Ticket URL: <https://code.djangoproject.com/ticket/36191#comment:9>

[Django]

#36191: FileSystemStorage with allow_overwrite=True does not truncate previous file
-------------------------------------+-------------------------------------
Reporter: Gaël UTARD | Owner: Gaël
| UTARD

Type: Bug | Status: closed
Component: File | Version: 5.1

uploads/storage |

Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin

Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Sarah Boyce):

* resolution: => fixed

* stage: Accepted => Ready for checkin

* status: new => closed
* version: 5.2 => 5.1

Comment:

Please open a new ticket so we can replicate and triage the issue
--
Ticket URL: <https://code.djangoproject.com/ticket/36191#comment:10>