From 791ee7171b20ffd66f2d6d0c18589adfc22b6680 Mon Sep 17 00:00:00 2001 From: Loz Calver Date: Thu, 2 Jan 2014 15:59:34 +0000 Subject: [PATCH] API: Prevent large images from repeatedly crashing PHP on resize --- _config.php | 2 + filesystem/GD.php | 120 ++++++++++++++++++++++---- filesystem/ImagickBackend.php | 18 ++++ model/Image.php | 8 +- model/Image_Backend.php | 17 ++++ tasks/CleanImageManipulationCache.php | 59 +++++++++++++ tests/filesystem/GDTest.php | 79 ++++++++++++++++- tests/model/GDImageTest.php | 39 +++++++++ 8 files changed, 320 insertions(+), 22 deletions(-) create mode 100644 tasks/CleanImageManipulationCache.php diff --git a/_config.php b/_config.php index def52dcf4a0..71244c7cacd 100644 --- a/_config.php +++ b/_config.php @@ -44,6 +44,8 @@ SS_Cache::add_backend('aggregatestore', 'File', array('cache_dir' => $aggregatecachedir)); SS_Cache::pick_backend('aggregatestore', 'aggregate', 1000); +SS_Cache::set_cache_lifetime('GDBackend_Manipulations', null, 100); + // If you don't want to see deprecation errors for the new APIs, change this to 3.2.0-dev. Deprecation::notification_version('3.2.0'); diff --git a/filesystem/GD.php b/filesystem/GD.php index de657e0e6fc..65395010a12 100644 --- a/filesystem/GD.php +++ b/filesystem/GD.php @@ -8,6 +8,7 @@ class GDBackend extends Object implements Image_Backend { protected $gd, $width, $height; protected $quality; protected $interlace; + protected $cache, $cacheKey, $manipulation; /** * @config @@ -34,29 +35,42 @@ public static function set_default_quality($quality) { } } - public function __construct($filename = null) { + public function __construct($filename = null, $args = array()) { // If we're working with image resampling, things could take a while. Bump up the time-limit increase_time_limit_to(300); + $this->cache = SS_Cache::factory('GDBackend_Manipulations'); + if($filename) { - // We use getimagesize instead of extension checking, because sometimes extensions are wrong. - list($width, $height, $type, $attr) = getimagesize($filename); - switch($type) { - case 1: - if(function_exists('imagecreatefromgif')) - $this->setImageResource(imagecreatefromgif($filename)); - break; - case 2: - if(function_exists('imagecreatefromjpeg')) - $this->setImageResource(imagecreatefromjpeg($filename)); - break; - case 3: - if(function_exists('imagecreatefrompng')) { - $img = imagecreatefrompng($filename); - imagesavealpha($img, true); // save alphablending setting (important) - $this->setImageResource($img); - } - break; + $this->cacheKey = md5(implode('_', array($filename, filemtime($filename)))); + $this->manipulation = implode('|', $args); + + $cacheData = unserialize($this->cache->load($this->cacheKey)); + $cacheData = ($cacheData !== false) ? $cacheData : array(); + + if ($this->imageAvailable($filename, $this->manipulation)) { + $cacheData[$this->manipulation] = true; + $this->cache->save(serialize($cacheData), $this->cacheKey); + + // We use getimagesize instead of extension checking, because sometimes extensions are wrong. + list($width, $height, $type, $attr) = getimagesize($filename); + switch($type) { + case 1: + if(function_exists('imagecreatefromgif')) + $this->setImageResource(imagecreatefromgif($filename)); + break; + case 2: + if(function_exists('imagecreatefromjpeg')) + $this->setImageResource(imagecreatefromjpeg($filename)); + break; + case 3: + if(function_exists('imagecreatefrompng')) { + $img = imagecreatefrompng($filename); + imagesavealpha($img, true); // save alphablending setting (important) + $this->setImageResource($img); + } + break; + } } } @@ -86,6 +100,56 @@ public function getGD() { return $this->getImageResource(); } + /** + * @param string $filename + * @return boolean + */ + public function imageAvailable($filename, $manipulation) { + return ($this->checkAvailableMemory($filename) && ! $this->failedResample($filename, $manipulation)); + } + + /** + * Check if we've got enough memory available for resampling this image. This check is rough, + * so it will not catch all images that are too large - it also won't work accurately on large, + * animated GIFs as bits per pixel can't be calculated for an animated GIF with a global color + * table. + * + * @param string $filename + * @return boolean + */ + public function checkAvailableMemory($filename) { + $limit = translate_memstring(ini_get('memory_limit')); + + if ($limit < 0) return true; // memory_limit == -1 + + $imageInfo = getimagesize($filename); + + // bits per channel (rounded up, default to 1) + $bits = isset($imageInfo['bits']) ? ($imageInfo['bits'] + 7) / 8 : 1; + + // channels (default 4 rgba) + $channels = isset($imageInfo['channels']) ? $imageInfo['channels'] : 4; + + $bytesPerPixel = $bits * $channels; + + // width * height * bytes per pixel + $memoryRequired = $imageInfo[0] * $imageInfo[1] * $bytesPerPixel; + + return $memoryRequired + memory_get_usage() < $limit; + } + + /** + * Check if this image has previously crashed GD when attempting to open it - if it's opened + * successfully, the manipulation's cache key is removed. + * + * @param string $filename + * @return boolean + */ + public function failedResample($filename, $manipulation) { + $cacheData = unserialize($this->cache->load($this->cacheKey)); + return ($cacheData && array_key_exists($manipulation, $cacheData)); + } + /** * Set the image quality, used when saving JPEGs. */ @@ -480,6 +544,24 @@ public function writeTo($filename) { imagepng($this->gd, $filename); break; } if(file_exists($filename)) @chmod($filename,0664); + + // Remove image manipulation from cache now that it's complete + $cacheData = unserialize($this->cache->load($this->cacheKey)); + if(isset($cacheData[$this->manipulation])) unset($cacheData[$this->manipulation]); + $this->cache->save(serialize($cacheData), $this->cacheKey); + } + } + + /** + * @param Image $frontend + * @return void + */ + public function onBeforeDelete($frontend) { + $file = Director::baseFolder() . "/" . $frontend->Filename; + + if (file_exists($file)) { + $key = md5(implode('_', array($file, filemtime($file)))); + $this->cache->remove($key); } } diff --git a/filesystem/ImagickBackend.php b/filesystem/ImagickBackend.php index 000d366ecc6..363c0e1c3da 100644 --- a/filesystem/ImagickBackend.php +++ b/filesystem/ImagickBackend.php @@ -96,6 +96,16 @@ public function hasImageResource() { return true; // $this is the resource, necessarily } + /** + * @todo Implement memory checking for Imagick? See {@link GD} + * + * @param string $filename + * @return boolean + */ + public function imageAvailable($filename) { + return true; + } + /** * resize * @@ -264,5 +274,13 @@ public function croppedResize($width, $height) { $new->ThumbnailImage($width,$height,true); return $new; } + + /** + * @param Image $frontend + * @return void + */ + public function onBeforeDelete($frontend) { + // Not in use + } } } diff --git a/model/Image.php b/model/Image.php index 4ba07c428f1..22a0e594070 100644 --- a/model/Image.php +++ b/model/Image.php @@ -466,7 +466,8 @@ public function generateFormattedImage($format) { $cacheFile = call_user_func_array(array($this, "cacheFilename"), $args); $backend = Injector::inst()->createWithArgs(self::$backend, array( - Director::baseFolder()."/" . $this->Filename + Director::baseFolder()."/" . $this->Filename, + $args )); if($backend->hasImageResource()) { @@ -725,9 +726,12 @@ public function onAfterUpload() { } protected function onBeforeDelete() { - parent::onBeforeDelete(); + $backend = Injector::inst()->create(self::$backend); + $backend->onBeforeDelete($this); $this->deleteFormattedImages(); + + parent::onBeforeDelete(); } } diff --git a/model/Image_Backend.php b/model/Image_Backend.php index 7e5a0ba3aa9..0318b2236f2 100644 --- a/model/Image_Backend.php +++ b/model/Image_Backend.php @@ -110,4 +110,21 @@ public function paddedResize($width, $height, $backgroundColor = "FFFFFF"); * @return Image_Backend */ public function croppedResize($width, $height); + + /** + * imageAvailable + * + * @param string $filename + * @return boolean + */ + public function imageAvailable($filename, $manipulation); + + /** + * onBeforeDelete + * + * @param Image $frontend + * @return void + */ + public function onBeforeDelete($frontend); + } diff --git a/tasks/CleanImageManipulationCache.php b/tasks/CleanImageManipulationCache.php new file mode 100644 index 00000000000..55f55f0159f --- /dev/null +++ b/tasks/CleanImageManipulationCache.php @@ -0,0 +1,59 @@ +getFullPath(); + + if (file_exists($path)) { + $key = md5(implode('_', array($path, filemtime($path)))); + + if ($manipulations = unserialize($cache->load($key))) { + $failedManipulations += count($manipulations); + $processedImages++; + $cache->remove($key); + } + } + } + } + + echo "Cleared $failedManipulations failed manipulations from + $processedImages Image objects stored in the Database."; + } + +} diff --git a/tests/filesystem/GDTest.php b/tests/filesystem/GDTest.php index e34bf6478cd..9a9aeee45bc 100644 --- a/tests/filesystem/GDTest.php +++ b/tests/filesystem/GDTest.php @@ -15,6 +15,11 @@ class GDTest extends SapphireTest { 'png32' => 'test_png32.png' ); + public function tearDown() { + $cache = SS_Cache::factory('GDBackend_Manipulations'); + $cache->clean(Zend_Cache::CLEANING_MODE_ALL); + } + /** * Loads all images into an associative array of GD objects. * Optionally applies an operation to each GD @@ -135,4 +140,76 @@ function testGreyscale() { $samplesPNG32 = $this->sampleAreas($images['png32']); $this->assertGreyscale($samplesPNG32, 8); } -} \ No newline at end of file + + /** + * Tests that GD doesn't attempt to load images when they're deemed unavailable + * @return void + */ + public function testImageSkippedWhenUnavailable() { + $fullPath = realpath(dirname(__FILE__) . '/gdtest/test_jpg.jpg'); + $gd = new GDBackend_ImageUnavailable($fullPath); + + /* Ensure no image resource is created if the image is unavailable */ + $this->assertNull($gd->getImageResource()); + } + + /** + * Tests the integrity of the manipulation cache when an error occurs + * @return void + */ + public function testCacheIntegrity() { + $fullPath = realpath(dirname(__FILE__) . '/gdtest/test_jpg.jpg'); + + try { + $gdFailure = new GDBackend_Failure($fullPath, array('SetWidth', 123)); + $this->fail('GDBackend_Failure should throw an exception when setting image resource'); + } catch (GDBackend_Failure_Exception $e) { + $cache = SS_Cache::factory('GDBackend_Manipulations'); + $key = md5(implode('_', array($fullPath, filemtime($fullPath)))); + + $data = unserialize($cache->load($key)); + + $this->assertArrayHasKey('SetWidth|123', $data); + $this->assertTrue($data['SetWidth|123']); + } + } + + /** + * Test that GD::failedResample() returns true for the current image + * manipulation only if it previously failed + * @return void + */ + public function testFailedResample() { + $fullPath = realpath(dirname(__FILE__) . '/gdtest/test_jpg.jpg'); + + try { + $gdFailure = new GDBackend_Failure($fullPath, array('SetWidth-failed', 123)); + $this->fail('GDBackend_Failure should throw an exception when setting image resource'); + } catch (GDBackend_Failure_Exception $e) { + $gd = new GDBackend($fullPath, array('SetWidth', 123)); + $this->assertTrue($gd->failedResample($fullPath, 'SetWidth-failed|123')); + $this->assertFalse($gd->failedResample($fullPath, 'SetWidth-not-failed|123')); + } + } + +} + +class GDBackend_ImageUnavailable extends GDBackend implements TestOnly { + + public function imageAvailable($filename, $manipulation) { + return false; + } + +} + +class GDBackend_Failure extends GDBackend implements TestOnly { + + public function setImageResource($resource) { + throw new GDBackend_Failure_Exception('GD failed to load image'); + } + +} + +class GDBackend_Failure_Exception extends Exception { + +} diff --git a/tests/model/GDImageTest.php b/tests/model/GDImageTest.php index 36efdd44097..c55abd68ae5 100644 --- a/tests/model/GDImageTest.php +++ b/tests/model/GDImageTest.php @@ -1,5 +1,7 @@ markTestSkipped("The GD extension is required"); @@ -25,4 +27,41 @@ public function setUp() { $file->write(); } } + + public function tearDown() { + $cache = SS_Cache::factory('GDBackend_Manipulations'); + $cache->clean(Zend_Cache::CLEANING_MODE_ALL); + parent::tearDown(); + } + + /** + * Test that the cache of manipulation failures is cleared when deleting + * the image object + * @return void + */ + public function testCacheCleaningOnDelete() { + $image = $this->objFromFixture('Image', 'imageWithTitle'); + $cache = SS_Cache::factory('GDBackend_Manipulations'); + $fullPath = $image->getFullPath(); + $key = md5(implode('_', array($fullPath, filemtime($fullPath)))); + + try { + // Simluate a failed manipulation + $gdFailure = new GDBackend_Failure($fullPath, array('SetWidth', 123)); + $this->fail('GDBackend_Failure should throw an exception when setting image resource'); + } catch (GDBackend_Failure_Exception $e) { + // Check that the cache has stored the manipulation failure + $data = unserialize($cache->load($key)); + $this->assertArrayHasKey('SetWidth|123', $data); + $this->assertTrue($data['SetWidth|123']); + + // Delete the image object + $image->delete(); + + // Check that the cache has been removed + $data = unserialize($cache->load($key)); + $this->assertFalse($data); + } + } + }