ImageField's to_python() stores reference to closed Image object

[Felix Dreissig]

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 ​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.

[Tim Graham]

Can you offer a test for Django's test suite that demonstrates the issue? Do you have a fix in mind?

[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.

[Tim Graham]

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.

[Felix Dreissig]

Patch containing a test demonstrating the issue

[Felix Dreissig]

Patch containing a possible fix

[Felix Dreissig]

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.

[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. Does your purposed change affect performance? An alternative could be to clarify that subclasses need to reopen the image to access certain methods.

[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?

[Carlton Gibson]

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.

[Felix Dreissig]

Replying to 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 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 ​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.

[Mariusz Felisiak <felisiak.mariusz@…>]

In 2282d9f:

Fixed #30252 -- Clarified need to reopen forms.fields.ImageField.image file to access raw image data.

[Mariusz Felisiak <felisiak.mariusz@…>]

In 6722416:

[3.0.x] Fixed #30252 -- Clarified need to reopen forms.fields.ImageField.image file to access raw image data.

Backport of 2282d9f2e5d08fc782087ebe97ab195303a6e79b from master

[Mariusz Felisiak <felisiak.mariusz@…>]

In 6429d71e:

[2.2.x] Fixed #30252 -- Clarified need to reopen forms.fields.ImageField.image file to access raw image data.

Backport of 2282d9f2e5d08fc782087ebe97ab195303a6e79b from master