Merge pull request #2859 from kinglozzer/gd-resize-crash-mast

API: Prevent large images from repeatedly crashing PHP on resize

_config.php

@@ -44,6 +44,8 @@ if (!is_dir($aggregatecachedir)) mkdir($aggregatecachedir);
 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');
 

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 @@ class GDBackend extends Object implements Image_Backend {
 		}
 	}
 
-	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.
+			$this->cacheKey = md5(implode('_', array($filename, filemtime($filename))));
-			list($width, $height, $type, $attr) = getimagesize($filename);
+			$this->manipulation = implode('|', $args);
-			switch($type) {
+
-				case 1:
+			$cacheData = unserialize($this->cache->load($this->cacheKey));
-					if(function_exists('imagecreatefromgif'))
+			$cacheData = ($cacheData !== false) ? $cacheData : array();
-						$this->setImageResource(imagecreatefromgif($filename));
+
-					break;
+			if ($this->imageAvailable($filename, $this->manipulation)) {
-				case 2:
+				$cacheData[$this->manipulation] = true;
-					if(function_exists('imagecreatefromjpeg'))
+				$this->cache->save(serialize($cacheData), $this->cacheKey);
-						$this->setImageResource(imagecreatefromjpeg($filename));
+
-					break;
+				// We use getimagesize instead of extension checking, because sometimes extensions are wrong.
-				case 3:
+				list($width, $height, $type, $attr) = getimagesize($filename);
-					if(function_exists('imagecreatefrompng')) {
+				switch($type) {
-						$img = imagecreatefrompng($filename);
+					case 1:
-						imagesavealpha($img, true); // save alphablending setting (important)
+						if(function_exists('imagecreatefromgif'))
-						$this->setImageResource($img);
+							$this->setImageResource(imagecreatefromgif($filename));
-					}
+						break;
-					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 @@ class GDBackend extends Object implements Image_Backend {
 		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 @@ class GDBackend extends Object implements Image_Backend {
 					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);
 		}
 	}
 	

filesystem/ImagickBackend.php

@@ -96,6 +96,16 @@ class ImagickBackend extends Imagick implements Image_Backend {
 		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 @@ class ImagickBackend extends Imagick implements Image_Backend {
 		$new->ThumbnailImage($width,$height,true);
 		return $new;
 	}
+
+	/**
+	 * @param Image $frontend
+	 * @return void
+	 */
+	public function onBeforeDelete($frontend) {
+		// Not in use
+	}
 }
 }

model/Image.php

@@ -466,7 +466,8 @@ class Image extends File {
 		$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 @@ class Image extends File {
 	}
 	
 	protected function onBeforeDelete() {
-		parent::onBeforeDelete(); 
+		$backend = Injector::inst()->create(self::$backend);
+		$backend->onBeforeDelete($this);
 
 		$this->deleteFormattedImages();
+
+		parent::onBeforeDelete();
 	}
 }
 

model/Image_Backend.php

@@ -110,4 +110,21 @@ interface Image_Backend {
 	 * @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);
+
 }

tasks/CleanImageManipulationCache.php

@@ -0,0 +1,59 @@
+<?php
+/**
+ * Wipe the cache of failed image manipulations. When {@link GDBackend} attempts to resample an image, it will write
+ * the attempted manipulation to the cache and remove it from the cache if the resample is successful. The objective
+ * of the cache is to prevent fatal errors (for example from exceeded memory limits) from occurring more than once.
+ *
+ * @package framework
+ * @subpackage filesystem
+ */
+class CleanImageManipulationCache extends BuildTask {
+	
+	protected $title = 'Clean Image Manipulation Cache';
+	
+	protected $description = 'Clean the failed image manipulation cache. Use this to allow SilverStripe to attempt
+		to resample images that have previously failed to resample (for example if memory limits were exceeded).';
+	
+	/**
+	 * Check that the user has appropriate permissions to execute this task
+	 */
+	public function init() {
+		if(!Director::is_cli() && !Director::isDev() && !Permission::check('ADMIN')) {
+			return Security::permissionFailure();
+		}
+		
+		parent::init();
+	}
+	
+	/**
+	 * Clear out the image manipulation cache
+	 * @param SS_HTTPRequest $request
+	 */
+	public function run($request) {
+		$failedManipulations = 0;
+		$processedImages = 0;
+		$images = DataObject::get('Image');
+		
+		if($images && Image::get_backend() == "GDBackend") {
+			$cache = SS_Cache::factory('GDBackend_Manipulations');
+
+			foreach($images as $image) {
+				$path = $image->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.";
+	}
+	
+}

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 @@ class GDTest extends SapphireTest {
 		$samplesPNG32 = $this->sampleAreas($images['png32']);
 		$this->assertGreyscale($samplesPNG32, 8);
 	}
+
+	/**
+	 * 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 {
+
 }

tests/model/GDImageTest.php

@@ -1,5 +1,7 @@
 <?php
+
 class GDImageTest extends ImageTest {
+
 	public function setUp() {
 		if(!extension_loaded("gd")) {
 			$this->markTestSkipped("The GD extension is required");
@@ -25,4 +27,41 @@ class GDImageTest extends ImageTest {
 			$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);
+		}
+	}
+
 }