[Django] #30252: ImageField's to_python() stores reference to closed Image object
Django
------------------------------------------------+------------------------
Reporter: Felix Dreissig | Owner: nobody
Type: Bug | Status: new
Component: File uploads/storage | Version: 2.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 |
------------------------------------------------+------------------------
In django.forms.fields.ImageField's `to_python()` method, an uploaded
image is validated by `open()`ing a PIL.Image object from it and calling
`verify()` on that. Afterwards, the Image object is saved to an `image`
attribute of the uploaded file (i.e. an InMemoryUploadedFile). According
to a comment in the source, this happens so that "subclasses can reuse it
for their own validation".
Pillow closes an Image after `verify()`ing and the docs
[https://pillow.readthedocs.io/en/stable/reference/Image.html#PIL.Image.Image.verify
state] that it cannot be used after calling `verify` on it:
If you need to load the image after using this method, you must reopen
the image file.
For me, this resulted in the following error when trying to explicitly
call `save()` on the Image, but similar effects will probably happen for
any operation:
{{{
File ".../lib/python3.6/site-packages/PIL/Image.py", line 1960, in save
self.load()
File ".../lib/python3.6/site-packages/PIL/ImageFile.py", line 165, in
load
seek = self.fp.seek
AttributeError: 'NoneType' object has no attribute 'seek'
}}}
This could be related to #13750, but I don't think so.
--
Ticket URL: <https://code.djangoproject.com/ticket/30252>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Felix Dreissig | Owner: nobody
Type: Bug | Status: new
uploads/storage |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham):
Can you offer a test for Django's test suite that demonstrates the issue?
Do you have a fix in mind?
--
Ticket URL: <https://code.djangoproject.com/ticket/30252#comment:1>
Django
Reporter: Felix Dreissig | Owner: nobody
Type: Bug | Status: new
uploads/storage |
Severity: Normal | Resolution:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Felix Dreissig):
Thanks for your quick response.
I could probably come up with a test case, but it will take some time. How
would you like it? As a GitHub pull request (which would introduce a
failing test until a fix is commited), or as a patch in this ticket?
A fix I could think of would be re-`open()` ing the Image after
verification and storing that object to the `image` attribute. Or removing
the attribute completely (since it appears useless in its current state),
but that would break an (undocumented) API.
--
Ticket URL: <https://code.djangoproject.com/ticket/30252#comment:2>
Django
Reporter: Felix Dreissig | Owner: nobody
Component: File | Version: 2.1
uploads/storage |
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* status: new => closed
* resolution: => needsinfo
Comment:
An attachment or comment in this ticket is fine. Closing ticket as
"needinfo" until we have the steps to reproduce and can better evaluate
the issue.
--
Ticket URL: <https://code.djangoproject.com/ticket/30252#comment:3>
Django
Reporter: Felix Dreissig | Owner: nobody
Component: File | Version: 2.1
uploads/storage |
Severity: Normal | Resolution: needsinfo
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Felix Dreissig):
* Attachment "0001-Refs-30252-Added-test-demonstrating-the-issue.patch"
added.
Patch containing a test demonstrating the issue
Django
Reporter: Felix Dreissig | Owner: nobody
Component: File | Version: 2.1
uploads/storage |
Severity: Normal | Resolution: needsinfo
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Felix Dreissig):
* Attachment "0002-Fixed-30252-Reopen-uploaded-image-after-verify.patch"
added.
Patch containing a possible fix
Django
Reporter: Felix Dreissig | Owner: nobody
Type: Bug | Status: new
uploads/storage |
Severity: Normal | Resolution:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Felix Dreissig):
* status: closed => new
* resolution: needsinfo =>
Comment:
Please find two patches (in `git format-patch` format) now attached to
this issue:
1. A test demonstrating the issue.
2. A possible fix. I still think a (possibly better) alternative would be
removing the attribute and let users call `open()` themselves if they need
to, but one could consider that a breaking change.
--
Ticket URL: <https://code.djangoproject.com/ticket/30252#comment:4>
Django
Reporter: Felix Dreissig | Owner: nobody
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* component: File uploads/storage => Forms
Comment:
The original commit is 8b7347220f3d86b46f5f87270c6cdcb9960895fd. As you
can see there, the non-image data attributes like `format`, `height`, and
`width` are available without reopening the image. Does your purposed
change affect performance? An alternative could be to clarify that
subclasses need to reopen the image to access certain methods.
--
Ticket URL: <https://code.djangoproject.com/ticket/30252#comment:5>
Django
Reporter: Felix Dreissig | Owner: nobody
Type: Bug | Status: new
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Carlton Gibson):
Hi Felix.
I'm inclined to see both this and #13750 as documentation issues... Let's
just be clearer about the behaviour.
Just for your proposal: if we reopen the file, where are we closing it
again?
--
Ticket URL: <https://code.djangoproject.com/ticket/30252#comment:6>
Django
Reporter: Felix Dreissig | Owner: nobody
Component: Documentation | Version: 2.1
Severity: Normal | 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 Carlton Gibson):
* component: Forms => Documentation
* type: Bug => Cleanup/optimization
* stage: Unreviewed => Accepted
Comment:
Hi Felix,
I'm going to accept this as a Documentation fix for now: there's no reason
at all why we couldn't clarify the behaviour here.
I'm happy to consider the code change as a possibility if we can show that
it's desirable and not problematic.
Thanks.
--
Ticket URL: <https://code.djangoproject.com/ticket/30252#comment:7>
Django
Reporter: Felix Dreissig | Owner: nobody
Component: Documentation | Version: 2.1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Felix Dreissig):
Replying to [comment:5 Tim Graham]:
> The original commit is 8b7347220f3d86b46f5f87270c6cdcb9960895fd. As you
can see there, the non-image data attributes like `format`, `height`, and
`width` are available without reopening the image.
Yes. If I recall my analysis correctly, basically all attributes are
accessible, while no methods are.
> Does your purposed change affect performance?
Since `open()` is a lazy operation in Pillow (image data is not loaded
until accessed), I assume the performance impact would be negligible. I
haven't run any measurements, though.
Replying to [comment:6 Carlton Gibson]:
> Just for your proposal: if we reopen the file, where are we closing it
again?
Fair point. Images themselves don't have to be closed in Pillow, but
underlying file pointers do. So this should only be relevant if we have a
temporary file path (no in-memory image data).
According to the
[https://pillow.readthedocs.io/en/stable/reference/open_files.html#file-
handling Pillow docs on file handling], file pointers for single-frame
images are automatically closed after `load()` has been called. However,
as stated above, loading might not always happen. On top of that, multi-
frame images are probably always problematic.
--
Ticket URL: <https://code.djangoproject.com/ticket/30252#comment:8>
Django
Reporter: Felix Dreissig | Owner: Hasan
Type: | Ramezani
Cleanup/optimization | Status: assigned
Component: Documentation | Version: 2.1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Hasan Ramezani):
* owner: nobody => Hasan Ramezani
* status: new => assigned
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/30252#comment:9>
Django
Reporter: Felix Dreissig | Owner: Hasan
Type: | Ramezani
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* needs_better_patch: 0 => 1
* version: 2.1 => master
--
Ticket URL: <https://code.djangoproject.com/ticket/30252#comment:10>
Django
Reporter: Felix Dreissig | Owner: Hasan
Type: | Ramezani
Cleanup/optimization | Status: assigned
Component: Documentation | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Hasan Ramezani):
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/30252#comment:11>
Django
Reporter: Felix Dreissig | Owner: Hasan
Type: | Ramezani
Component: Documentation | Version: master
Severity: Normal | Resolution: fixed
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 Mariusz Felisiak <felisiak.mariusz@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"2282d9f2e5d08fc782087ebe97ab195303a6e79b" 2282d9f]:
{{{
#!CommitTicketReference repository=""
revision="2282d9f2e5d08fc782087ebe97ab195303a6e79b"
Fixed #30252 -- Clarified need to reopen forms.fields.ImageField.image
file to access raw image data.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/30252#comment:12>
Django
Reporter: Felix Dreissig | Owner: Hasan
Type: | Ramezani
Cleanup/optimization | Status: closed
Component: Documentation | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"6722416e7037cfe0977945ec0d4d05a3ce774995" 6722416]:
{{{
#!CommitTicketReference repository=""
revision="6722416e7037cfe0977945ec0d4d05a3ce774995"
[3.0.x] Fixed #30252 -- Clarified need to reopen
forms.fields.ImageField.image file to access raw image data.
Backport of 2282d9f2e5d08fc782087ebe97ab195303a6e79b from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/30252#comment:13>
Django
Reporter: Felix Dreissig | Owner: Hasan
Type: | Ramezani
Cleanup/optimization | Status: closed
Component: Documentation | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"6429d71ecbfd778d56ede1a51224286c81eab7f6" 6429d71e]:
{{{
#!CommitTicketReference repository=""
revision="6429d71ecbfd778d56ede1a51224286c81eab7f6"
[2.2.x] Fixed #30252 -- Clarified need to reopen
forms.fields.ImageField.image file to access raw image data.
Backport of 2282d9f2e5d08fc782087ebe97ab195303a6e79b from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/30252#comment:14>