Imagick / imagick

🌈 The Imagick PHP extension 🌈
http://pecl.php.net/imagick
Other
540 stars 136 forks source link

Broken JPEG no longer causes Imagick to throw an exception #580

Open christeredvartsen opened 1 year ago

christeredvartsen commented 1 year ago

I have a broken JPEG that previously (Imagick-3.4.4) caused Imagick to throw an exception, which no longer seems to be the case.

Imagick will load the image, and $imagick->valid() returns true.

When checking the image with identify I get some warnings though:

chredvar@red:~$ identify broken-image.jpg 
broken-image.jpg JPEG 0x0 16-bit sRGB 4096B 0.000u 0:00.001
identify-im6.q16: Premature end of JPEG file `broken-image.jpg' @ warning/jpeg.c/JPEGWarningHandler/389.
identify-im6.q16: Bogus DQT index 15 `broken-image.jpg' @ error/jpeg.c/JPEGErrorHandler/338.

The errors you see from identify could previously be seen in the exception thrown by Imagick.

I don't know if this is fixable in the Imagick extension or if the issue is in ImageMagick itself.

As a sidenote, the only way I seem to be able to detect if the image is broken or not is to look at width and height keys in the array returned by $imagick->getImageGeometry();, which both is 0 for the above mentioned image.

I have tested with Imagick 3.5.0, 3.6.0 and 3.7.0. Some other version info if it might help:

chredvar@red:~$ identify --version
Version: ImageMagick 6.9.11-60 Q16 x86_64 2021-01-25 https://imagemagick.org
Copyright: (C) 1999-2021 ImageMagick Studio LLC
License: https://imagemagick.org/script/license.php
Features: Cipher DPC Modules OpenMP(4.5) 
Delegates (built-in): bzlib djvu fftw fontconfig freetype heic jbig jng jp2 jpeg lcms lqr ltdl lzma openexr pangocairo png tiff webp wmf x xml zlib
Danack commented 1 year ago

The issue will be in ImageMagick itself, but if you email the image to me (presuming Github won't accept it as an upload reliably), I can take a look.

I have tested with

Can you look to see what version of ImageMagick, and system versions of the jpeg libraries you were using when it behaved correctly, and say what versions you have now?

christeredvartsen commented 1 year ago

You should be able to get the image from here (public repo): https://github.com/imbo/imbo/blob/main/tests/Fixtures/broken-image.jpg

Can you look to see what version of ImageMagick, and system versions of the jpeg libraries you were using when it behaved correctly, and say what versions you have now?

Does the version reported by identify suffice, or do you need something else?

Version info from the system that throws the exception

chredvar@foo:~$ php --version
PHP 7.4.3-4ubuntu2.17 (cli) (built: Jan 10 2023 15:37:44) ( NTS )
Copyright (c) The PHP Group
Zend Engine v3.4.0, Copyright (c) Zend Technologies
    with Zend OPcache v7.4.3-4ubuntu2.17, Copyright (c), by Zend Technologies
chredvar@foo:~$ identify --version
Version: ImageMagick 6.9.10-23 Q16 x86_64 20190101 https://imagemagick.org
Copyright: © 1999-2019 ImageMagick Studio LLC
License: https://imagemagick.org/script/license.php
Features: Cipher DPC Modules OpenMP 
Delegates (built-in): bzlib djvu fftw fontconfig freetype jbig jng jpeg lcms lqr ltdl lzma openexr pangocairo png tiff webp wmf x xml zlib
chredvar@foo:~$ php -i|grep "imagick module version"
imagick module version => 3.4.4

Version info from the system that no longer throws the exception

chredvar@red:~$ php --version
PHP 8.1.2-1ubuntu2.10 (cli) (built: Jan 16 2023 15:19:49) (NTS)
Copyright (c) The PHP Group
Zend Engine v4.1.2, Copyright (c) Zend Technologies
    with Zend OPcache v8.1.2-1ubuntu2.10, Copyright (c), by Zend Technologies
    with Xdebug v3.1.2, Copyright (c) 2002-2021, by Derick Rethans
chredvar@red:~$ identify --version
Version: ImageMagick 6.9.11-60 Q16 x86_64 2021-01-25 https://imagemagick.org
Copyright: (C) 1999-2021 ImageMagick Studio LLC
License: https://imagemagick.org/script/license.php
Features: Cipher DPC Modules OpenMP(4.5) 
Delegates (built-in): bzlib djvu fftw fontconfig freetype heic jbig jng jp2 jpeg lcms lqr ltdl lzma openexr pangocairo png tiff webp wmf x xml zlib
chredvar@red:~$ php -i|grep "imagick module version"
imagick module version => 3.6.0
Danack commented 1 year ago

fyi, computers are terrible. I recently moved to a new machine (a lovely Apple M1 Pro) which is nice and fast but.................I apparently can't persuade ImageMagick to pickup and use libjpeg or any other jpg library, even after spending a few hours trying.

You wouldn't happen to have any clue how get ImageMagick to find the jpg headers + libraries installed through brew would you? I'm compiling ImageMagick from source.

Setting:

CPPFLAGS=-I/opt/homebrew/Cellar/jpeg/9e/include 
LDFLAGS="-L/opt/homebrew/Cellar/jpeg/9e/lib -ljpeg"

Doesn't seem to do much.

Danack commented 1 year ago

Apparently, you need to 'source' scripts for them to inherit environment variables, such as CFLAGS.

Fun.

Danack commented 1 year ago

Sourcing doesn't actually work.....as the build has errors in it. so instead just put the CFLAGS in ~/.bash_profile or ~/.zshrc https://stackoverflow.com/questions/135688/setting-environment-variables-on-os-x

Danack commented 1 year ago

BTW Do you own the copyright for that image, and would it be okay for me to include it in the Imagick tests? And I guess the slightly-broken-image.png?

christeredvartsen commented 1 year ago

IIRC we created those files specifically for the tests many years ago. And yes, you can use them for whatever you want.

christeredvartsen commented 1 year ago

You wouldn't happen to have any clue how get ImageMagick to find the jpg headers + libraries installed through brew would you? I'm compiling ImageMagick from source.

Sorry, can't help with this one, but it seems you got it working anyways?

I can probably create a couple of Dockerfiles to reproduce the different scenarios if that would help?

christeredvartsen commented 1 year ago

Did a couple of quick tests and here are some Dockerfiles if they might help:

Ubuntu 20.04, exception thrown

FROM ubuntu:20.04

RUN apt-get update && DEBIAN_FRONTEND=noninteractive TZ=Etc/UTC apt-get install -y tzdata curl php-cli php-pear php-dev libmagickwand-dev
RUN printf "\n" | pecl install imagick-3.4.4
RUN echo "extension=imagick.so" >> /etc/php/7.4/cli/php.ini

WORKDIR /app
RUN curl -s -o broken-image.jpg https://raw.githubusercontent.com/imbo/imbo/main/tests/Fixtures/broken-image.jpg
CMD ["php", "-r", "var_dump((new Imagick('broken-image.jpg'))->valid());"]

When running the above image I get:

PHP Fatal error:  Uncaught ImagickException: Bogus DQT index 15 `broken-image.jpg' @ error/jpeg.c/JPEGErrorHandler/335 in Command line code:1
Stack trace:
#0 Command line code(1): Imagick->__construct()
#1 {main}
  thrown in Command line code on line 1

Ubuntu 22.04, no exception thrown

FROM ubuntu:22.04

RUN apt-get update && DEBIAN_FRONTEND=noninteractive TZ=Etc/UTC apt-get install -y tzdata curl php-cli php-pear php-dev libmagickwand-dev
RUN printf "\n" | pecl install imagick
RUN echo "extension=imagick.so" >> /etc/php/8.1/cli/php.ini

WORKDIR /app
RUN curl -s -o broken-image.jpg https://raw.githubusercontent.com/imbo/imbo/main/tests/Fixtures/broken-image.jpg
CMD ["php", "-r", "var_dump((new Imagick('broken-image.jpg'))->valid());"]

When running the above image I get:

bool(true)
Danack commented 1 year ago

Sorry, can't help with this one, but it seems you got it working anyways?

Yeah, got it working. I'd been putting off compile ImageMagick....or anything really in a OSX environment, and had been doing all my work in Docker, but even though it's faster than it used to be, Docker on OSX is still slower than compiling in the host machine.

And that allows doing things like git bisect to identify the version that the error first appears in...or it would except that a chunk of the commits aren't compilable on OSX.

Anyway, version the regression crept in is identified and bug opened upstream. I'll probably try to think of a mitigation in Imagick itself, as opening invalid images is "not great".

christeredvartsen commented 1 year ago

I'll probably try to think of a mitigation in Imagick itself, as opening invalid images is "not great".

Sounds like an improvement.

For the broken-image.jpg above I can see that the image dimensions are 0 x 0 (as returned from $i->getImageGeometry(), but not sure if that is reliable either, as it will probably depend on how broken the image actually is.

Just looked a the commit that "broke" imagemagick, and that does indeed look at the dimensions. 😄