Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

API: Prevent large images from repeatedly crashing PHP on resize #2859

Merged
merged 1 commit into from Jul 16, 2014
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions _config.php
Expand Up @@ -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');

Expand Down
120 changes: 101 additions & 19 deletions filesystem/GD.php
Expand Up @@ -8,6 +8,7 @@ class GDBackend extends Object implements Image_Backend {
protected $gd, $width, $height;
protected $quality;
protected $interlace;
protected $cache, $cacheKey, $manipulation;

/**
* @config
Expand All @@ -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;
}
}
}

Expand Down Expand Up @@ -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.
*/
Expand Down Expand Up @@ -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);
}
}

Expand Down
18 changes: 18 additions & 0 deletions filesystem/ImagickBackend.php
Expand Up @@ -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
*
Expand Down Expand Up @@ -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
}
}
}
8 changes: 6 additions & 2 deletions model/Image.php
Expand Up @@ -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()) {
Expand Down Expand Up @@ -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();
}
}

Expand Down
17 changes: 17 additions & 0 deletions model/Image_Backend.php
Expand Up @@ -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);

}
59 changes: 59 additions & 0 deletions 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.";
}

}
79 changes: 78 additions & 1 deletion tests/filesystem/GDTest.php
Expand Up @@ -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
Expand Down Expand Up @@ -135,4 +140,76 @@ function testGreyscale() {
$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');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should have a $this->failure() to make sure an exception's thrown.

} 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');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likewise.

} 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 {

}