Re: [Django] #36298: file_move_safe with allow_overwrite=True does not truncate previous file when there is an OSError error on rename

[Django]

#36298: file_move_safe with allow_overwrite=True does not truncate previous file
when there is an OSError error on rename
-------------------------------------+-------------------------------------
Reporter: Sarah Boyce | Owner: Sarah
| Boyce
Type: Bug | Status: assigned
Component: File | Version: 5.2
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 Baptiste Mispelon):

* cc: Baptiste Mispelon (added)
* stage: Unreviewed => Accepted

Comment:

I agree with the "Release blocker" categorisation on the basis that this
is a data corrupting bug (which falls under "data loss" imo).

I've reviewed the proposed PR and the code change looks reasonable (and
simple enough that backporting it should hopefully not cause any issues).

The test added in the PR fails without the associated code change
(`os.O_TRUNC`) and passes with it.

I've also tested the change on the djangoproject.com codebase where it was
initially spotted [1] and it fixes the issue (it's a bit tricky to get
this to reproduce since several conditions need to be met, one of them
being that the uploaded file should be bigger than
`settings.FILE_UPLOAD_MAX_MEMORY_SIZE`).

I'm tempted to mark this as "ready for checkin", but I //think// it might
require a release note mention (or is that only for the backport?).

[1] https://github.com/django/djangoproject.com/issues/2015
--
Ticket URL: <https://code.djangoproject.com/ticket/36298#comment:3>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#36298: file_move_safe with allow_overwrite=True does not truncate previous file
when there is an OSError error on rename
-------------------------------------+-------------------------------------
Reporter: Sarah Boyce | Owner: Sarah
| Boyce
Type: Bug | Status: assigned
Component: File | Version: 5.2
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 Baptiste Mispelon):

* stage: Accepted => Ready for checkin

Comment:

Release notes are now in place, this looks good :+1: ,,oh no, why does
Trac not have emoji shortcuts?,,
--
Ticket URL: <https://code.djangoproject.com/ticket/36298#comment:4>

[Django]

#36298: file_move_safe with allow_overwrite=True does not truncate previous file
when there is an OSError error on rename
-------------------------------------+-------------------------------------
Reporter: Sarah Boyce | Owner: Sarah
| Boyce
Type: Bug | Status: assigned
Component: File | Version: 5.2
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 Natalia Bidart):

* cc: Josh Schneier (added)

Comment:

PR looks great, I requested a few minor changes. Adding Josh Schneier
(django-storages) as cc for awareness.
--
Ticket URL: <https://code.djangoproject.com/ticket/36298#comment:5>

[Django]

#36298: file_move_safe with allow_overwrite=True does not truncate previous file
when there is an OSError error on rename
-------------------------------------+-------------------------------------
Reporter: Sarah Boyce | Owner: Sarah
| Boyce

Type: Bug | Status: closed

Component: File | Version: 5.2
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:"8ad3e80e88201f4c557f6fa79fcfc0f8a0961830" 8ad3e80e]:
{{{#!CommitTicketReference repository=""
revision="8ad3e80e88201f4c557f6fa79fcfc0f8a0961830"
Fixed #36298 -- Truncated the overwritten file content in
file_move_safe().

Regression in 58cd4902a71a3695dd6c21dc957f59c333db364c.

Thanks Baptiste Mispelon for the report.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/36298#comment:6>

[Django]

#36298: file_move_safe with allow_overwrite=True does not truncate previous file
when there is an OSError error on rename
-------------------------------------+-------------------------------------
Reporter: Sarah Boyce | Owner: Sarah
| Boyce
Type: Bug | Status: closed
Component: File | Version: 5.2
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:"77d2037511cc0152af6bea402bd09632ed7ca551" 77d20375]:
{{{#!CommitTicketReference repository=""
revision="77d2037511cc0152af6bea402bd09632ed7ca551"
[5.2.x] Fixed #36298 -- Truncated the overwritten file content in

file_move_safe().

Regression in 58cd4902a71a3695dd6c21dc957f59c333db364c.

Thanks Baptiste Mispelon for the report.

Backport of 8ad3e80e88201f4c557f6fa79fcfc0f8a0961830 from main.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/36298#comment:7>

[Django]

#36298: file_move_safe with allow_overwrite=True does not truncate previous file
when there is an OSError error on rename
-------------------------------------+-------------------------------------
Reporter: Sarah Boyce | Owner: Sarah
| Boyce
Type: Bug | Status: closed
Component: File | Version: 5.2
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:"39b144baddca433b9aa28f99e595ffcc191c0bee" 39b144ba]:
{{{#!CommitTicketReference repository=""
revision="39b144baddca433b9aa28f99e595ffcc191c0bee"
[5.1.x] Fixed #36298 -- Truncated the overwritten file content in

file_move_safe().

Regression in 58cd4902a71a3695dd6c21dc957f59c333db364c.

Thanks Baptiste Mispelon for the report.

Backport of 8ad3e80e88201f4c557f6fa79fcfc0f8a0961830 from main.
}}}
--

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

[Django]

#36298: file_move_safe with allow_overwrite=True does not truncate previous file
when there is an OSError error on rename
-------------------------------------+-------------------------------------
Reporter: Sarah Boyce | Owner: Sarah
| Boyce
Type: Bug | Status: closed
Component: File | Version: 5.2
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:"318c16d2b8157b0ca3fa5f69d0306409b57314b9" 318c16d2]:
{{{#!CommitTicketReference repository=""
revision="318c16d2b8157b0ca3fa5f69d0306409b57314b9"
[4.2.x] Fixed #36298 -- Truncated the overwritten file content in

file_move_safe().

Regression in 58cd4902a71a3695dd6c21dc957f59c333db364c.

Thanks Baptiste Mispelon for the report.

Backport of 8ad3e80e88201f4c557f6fa79fcfc0f8a0961830 from main.
}}}
--

Ticket URL: <https://code.djangoproject.com/ticket/36298#comment:9>