core.files.images.get_image_dimensions broken on invalid images

[artscoop]

Each time you access a model instance which contains an ImageField, core.files.images.get_image_dimensions is called. I have always found it overkill but whatever.
The bug is that the function chokes because get_image_dimensions returns None on a valid image (which is a 1x1 BMP32ARGB, not recognized by Pillow).
I end with a

>> return self._get_image_dimensions()[0]
TypeError: 'NoneType' object has no attribute '__getitem__'

When hitting an unrecognized image file, the function should still return a tuple like (0, 0) or (-1, -1) (but it returns None). I changed the function code to this:

    from PIL import Image
    try:
        image = Image.open(file_or_path)
        return image.size  # raster data is not loaded here
    except IOError:
        return [-1, -1]

what do you think ? It appears as a bug to me.

[Tim Graham]

My opinion is that if you need to use "images" that Pillow doesn't support, you should simply use FileField (or send a patch to Pillow if the bug is there). Can you provide a counter argument to that?

[artscoop]

The field has always been an ImageField, used in production since 2011.
99.99% of user uploads were valid images, but once I discovered some user uploaded an HTML file.
This time I found another instance breaking because of get_image_dimensions (on a perfectly valid .bmp file).
May I provide a history of my usage of the field:

• First, I used the field as is. People could upload images, and sometimes other files (breaking their profile page with a 500)
• Made numerous tests a few months ago with Django, Magic (MIME discovery) and Pillow to find that at least 32-bit bitmaps were not working well with Django/Pillow
• Created a derived ImageField.to_python to only accept GIF, PNG and JP?G files.
• Exported the old production site to a local dev version (with a fixed ImageField). Past bogus files are still there and break loudly as soon as Django finds the image field.

I don't even blame Pillow. I really think get_image_dimensions is bogus because it should never return None. Instead it should return a tuple with empty dimensions or error dimensions (0, 0) or (-1, -1) because the calling function simply chokes when receiving a None (instead of dealing with None it *always* assumes it's a tuple, and *this* is a bug, either in the calling function or the called one)

Forgot something obvious: the bug occurs only if the ImageField has a width_field and height_field.

[Tim Graham]

Thanks for elaborating. Would (None, None) be an option for invalid dimensions?

[artscoop]

Your new title is far more on point, thanks. There are several possible choices.

First choices options:

• return (None, None). Seems good, but if it's used to populate a concrete width_field or height_field, one must not forget that these fields must be nullable.
• return (0, 0). Good to me too, since no valid image in the world can be less than 1x1
• return (-1, -1). This is a perfectly invalid dimension for any object, and it somewhat fits here.

Second choice:
Accept None as an input, and change core.files.image.py from

class ImageFile(File):
    def _get_width(self):
        return self._get_image_dimensions()[0]
    width = property(_get_width)

    def _get_height(self):
        return self._get_image_dimensions()[1]
    height = property(_get_height)

to code which accepts None as a dimension:

    def _get_width(self):
        width = self._get_image_dimensions()
        return width[0] if width else **invalid value - None, 0 or -1**

    def _get_height(self):
        height = self._get_image_dimensions()
        return height[0] if height else **invalid value - None, 0 or -1**

[Tim Graham]

-1 wouldn't be work if someone is using PositiveIntegerField. Besides that, I don't have a strong opinion.

[artscoop]

Good catch. Then the most important thing would be to consider existing databases with ImageFielded models.
Those existing projects have:

• no width_field or height_field, but cached attributes in the ImageField (here, any value should be ok)
• width_field and height_field which are NOT NULL IntegerFields. (these would only accept 0 or -1)
• width_field and height_field which are NULL IntegerFields. (these would only accept None, 0 or -1)
• width_field and height_field which are NOT NULL PositiveIntegerFields. (these would only accept 0)
• width_field and height_field which are NULL PositiveIntegerFields. (these would only accept None or 0)

0 is the only value that cannot break an existing database and would thus not need a migration.
New projects could also use any type of IntegerField for their dimensions.
Seems right, right ?

[Tim Graham]

Some might consider it a "feature" that Django prevents images with invalid dimensions from being populated in the first place. Anyway, I'm fine with using zero for this until any further arguments are presented.

[andrei kulakov]

It seems more explicit and more proper that an image with unknown dimension should have them set as (None, None). For example, if I filter out images smaller than 64,64, it would be incorrect to filter out unknown sizes.

If an existing project made an assumption that Pillow is always able to calculate image dimensions, that's simply a wrong
assumption and it's probably reasonable if they work around it or migrate.

I suggest returning (None, None).

[Raúl Cumplido]

PR:
​https://github.com/django/django/pull/4374

[Tim Graham <timograham@…>]

In cda74c7f:

Fixed #24441 -- Changed get_image_dimensions() return value for broken images