diff --git a/docs/safety.md b/docs/safety.md index 49abac4..9ea9a30 100644 --- a/docs/safety.md +++ b/docs/safety.md @@ -51,6 +51,11 @@ The default `Limits` policy is: | Truncated input | reject | | Projected output checks | enabled | +Imagick checks the actual sequence length with a metadata probe before decoding +pixels, and again before coalescing frames. Where available, its native list-length +limit also bounds decoding and is restored after each request. Host decoder policies +still govern the cost of probing compressed formats and external delegates. + Apply stricter limits to a request when needed: ```php diff --git a/src/Driver/Imagick/ImagickDriver.php b/src/Driver/Imagick/ImagickDriver.php index a389f2a..48aa976 100644 --- a/src/Driver/Imagick/ImagickDriver.php +++ b/src/Driver/Imagick/ImagickDriver.php @@ -21,6 +21,7 @@ use Alto\Image\Effort; use Alto\Image\Exception\CorruptImageException; use Alto\Image\Exception\DriverException; +use Alto\Image\Exception\LimitExceededException; use Alto\Image\Format; use Alto\Image\Internal\QualitySearch; use Alto\Image\Limits; @@ -151,6 +152,25 @@ public function canEncode(Encoding $encoding): Support public function process(Plan $plan): array { + return ResourcePolicy::withFrameLimit($plan->limits, fn(): array => $this->processWithinFrameLimit($plan)); + } + + /** + * @return list + */ + private function processWithinFrameLimit(Plan $plan): array + { + $probe = new \Imagick(); + + try { + $probe->pingImageBlob($plan->source->contents()); + $this->checkFrames($probe, $plan); + } catch (\ImagickException $error) { + $this->throwReadError($plan, $error); + } finally { + $probe->clear(); + } + $started = microtime(true); $master = null; $results = []; @@ -263,6 +283,15 @@ private function passThrough(Plan $plan, int $index, float $duration): Result ); } + private function checkFrames(\Imagick $image, Plan $plan): void + { + $count = $image->getNumberImages(); + + if ($count > $plan->limits->maxFrames) { + throw LimitExceededException::frames($count, $plan->limits->maxFrames, $plan->source->origin()); + } + } + private function decode(Plan $plan): \Imagick { ResourcePolicy::apply($plan->limits); @@ -286,6 +315,8 @@ private function decode(Plan $plan): \Imagick // @codeCoverageIgnoreEnd } + $this->checkFrames($image, $plan); + // Coalescing turns frame-local rectangles into full canvases before the // same transform is applied to every frame. if ($image->getNumberImages() > 1) { @@ -398,25 +429,34 @@ private function read(Plan $plan, ?string $hint): \Imagick } catch (\ImagickException $error) { $image->clear(); - // canDecode() answered optimistically to keep queryFormats() off the - // hot path, so this is where a build that really cannot read the - // format has to say so, and say where the true list is. - if (!$this->writesFormat($plan->input->format) && !$this->knowsFormat($plan->input->format)) { - throw new DriverException(\sprintf( - "This ImageMagick has no coder for %s.\n" - . " Run vendor/bin/image doctor for the available formats.\n" - . ' It said: %s', - $plan->input->format->value, - $error->getMessage(), - ), 0, $error); - } - - throw CorruptImageException::unreadableHeader($plan->source->origin() . ': ' . $error->getMessage()); + $this->throwReadError($plan, $error); } return $image; } + private function throwReadError(Plan $plan, \ImagickException $error): never + { + if (str_contains(strtolower($error->getMessage()), 'list length exceeds limit')) { + throw new LimitExceededException('ImageMagick rejected the sequence at its native frame limit: ' . $error->getMessage(), 0, $error); + } + + // canDecode() answered optimistically to keep queryFormats() off the + // hot path, so this is where a build that really cannot read the + // format has to say so, and say where the true list is. + if (!$this->writesFormat($plan->input->format) && !$this->knowsFormat($plan->input->format)) { + throw new DriverException(\sprintf( + "This ImageMagick has no coder for %s.\n" + . " Run vendor/bin/image doctor for the available formats.\n" + . ' It said: %s', + $plan->input->format->value, + $error->getMessage(), + ), 0, $error); + } + + throw CorruptImageException::unreadableHeader($plan->source->origin() . ': ' . $error->getMessage()); + } + /** * @param list $placements */ diff --git a/src/Driver/Imagick/ResourcePolicy.php b/src/Driver/Imagick/ResourcePolicy.php index 7be5bb8..6075127 100644 --- a/src/Driver/Imagick/ResourcePolicy.php +++ b/src/Driver/Imagick/ResourcePolicy.php @@ -74,6 +74,39 @@ public static function apply(Limits $limits): array return $applied; } + /** + * @template T + * + * @param \Closure(): T $work + * + * @return T + */ + public static function withFrameLimit(Limits $limits, \Closure $work): mixed + { + $constant = self::constant('listlength'); + + if (null === $constant) { + // Unavailable on older ImageMagick builds. + // @codeCoverageIgnoreStart + return $work(); + // @codeCoverageIgnoreEnd + } + + // Imagick returns a float; its unlimited value rounds above PHP_INT_MAX. + $current = \Imagick::getResourceLimit($constant); + $previous = $current >= \PHP_INT_MAX ? \PHP_INT_MAX : (int) $current; + // The decoder needs a sentinel image after the last accepted frame. + $ceiling = $limits->maxFrames < \PHP_INT_MAX ? $limits->maxFrames + 1 : \PHP_INT_MAX; + + try { + \Imagick::setResourceLimit($constant, min($previous, $ceiling)); + + return $work(); + } finally { + \Imagick::setResourceLimit($constant, $previous); + } + } + /** * The limits that did not end up where they were asked to, for doctor. * @@ -133,6 +166,7 @@ private static function bytesPerPixel(): int private static function constant(string $name): ?int { $map = [ + 'listlength' => 'RESOURCETYPE_LISTLENGTH', 'width' => 'RESOURCETYPE_WIDTH', 'height' => 'RESOURCETYPE_HEIGHT', 'area' => 'RESOURCETYPE_AREA', diff --git a/tests/Driver/AnimationLimitsTest.php b/tests/Driver/AnimationLimitsTest.php new file mode 100644 index 0000000..92bb909 --- /dev/null +++ b/tests/Driver/AnimationLimitsTest.php @@ -0,0 +1,136 @@ +sequence('gif'); + $before = \Imagick::getResourceLimit(\Imagick::RESOURCETYPE_MEMORY); + $result = Image::open(Source::bytes($bytes))->using(new ImagickDriver())->within(Limits::none())->keepMetadata()->render(); + self::assertSame($bytes, $result->bytes); + self::assertSame($before, \Imagick::getResourceLimit(\Imagick::RESOURCETYPE_MEMORY)); + } + + public function testInvalidPixelPayloadKeepsTheCorruptImageError(): void + { + $png = $this->sequence('png'); + $offset = strpos($png, 'IDAT'); + self::assertNotFalse($offset); + $length = unpack('Nlength', substr($png, $offset - 4, 4)); + self::assertIsArray($length); + self::assertIsInt($length['length']); + $bad = str_repeat('x', $length['length']); + $png = substr_replace($png, $bad . pack('N', crc32('IDAT' . $bad)), $offset + 4, $length['length'] + 4); + $this->expectException(CorruptImageException::class); + Image::open(Source::bytes($png))->using(new ImagickDriver())->grayscale()->render(); + } + + #[DataProvider('formats')] + public function testActualFrameCountCannotExceedTheLimit(string $format): void + { + $bytes = $this->sequence($format); + $request = Image::open(Source::bytes($bytes))->using(new ImagickDriver())->within(new Limits(maxFrames: 2)); + $this->expectException(LimitExceededException::class); + $request->grayscale()->png()->render(); + } + + public function testAllowedAnimationKeepsItsFramesAndTiming(): void + { + $bytes = $this->sequence('gif'); + $output = Image::open(Source::bytes($bytes))->using(new ImagickDriver())->within(new Limits(maxFrames: 3))->grayscale()->render(); + $decoded = new \Imagick(); + $decoded->readImageBlob($output->bytes); + self::assertSame(3, $decoded->getNumberImages()); + foreach ($decoded as $frame) { + self::assertSame(10, $frame->getImageDelay()); + } + $decoded->clear(); + } + + #[DataProvider('frameLimits')] + public function testFailedAndSuccessfulRequestsRestoreTheNativeFrameLimit(int $maximum): void + { + $bytes = $this->sequence('gif'); + if (!defined(\Imagick::class . '::RESOURCETYPE_LISTLENGTH')) { + self::markTestSkipped('Native list-length control is not available.'); + } + $previous = \Imagick::getResourceLimit(\Imagick::RESOURCETYPE_LISTLENGTH); + $image = Image::open(Source::bytes($bytes))->using(new ImagickDriver()); + try { + $image->within(new Limits(maxFrames: $maximum))->render(); + self::assertSame(3, $maximum); + } catch (LimitExceededException) { + self::assertSame(2, $maximum); + } + self::assertSame($previous, \Imagick::getResourceLimit(\Imagick::RESOURCETYPE_LISTLENGTH)); + $direct = new \Imagick(); + $direct->readImageBlob($bytes); + self::assertSame(3, $direct->getNumberImages()); + $direct->clear(); + } + + /** + * @return iterable + */ + public static function frameLimits(): iterable + { + yield [2]; + yield [3]; + } + + /** + * @return iterable + */ + public static function formats(): iterable + { + yield ['gif']; + yield ['webp']; + yield ['tiff']; + } + + private function sequence(string $format): string + { + if (!extension_loaded('imagick') || [] === \Imagick::queryFormats(strtoupper($format))) { + self::markTestSkipped('Imagick format not available.'); + } + $sequence = new \Imagick(); + foreach (['red', 'green', 'blue'] as $colour) { + $frame = new \Imagick(); + $frame->newImage(2, 2, new \ImagickPixel($colour)); + $frame->setImageFormat($format); + $frame->setImageDelay(10); + $sequence->addImage($frame); + $frame->clear(); + } + $sequence->setImageFormat($format); + $bytes = $sequence->getImagesBlob(); + $sequence->clear(); + return $bytes; + } +}