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.

Sign up for GitHub

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

Jump to bottom

FIX 5284: Prevent crash for lager images. #2569

Closed
wants to merge 2 commits into from
Closed

FIX 5284: Prevent crash for lager images. #2569

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4d3b59b
FIX 5284: Prevent crash for lager images.
jakr Oct 20, 2013
e9b8cd7
Include improvements suggested by simonwelsh.
jakr Oct 20, 2013
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions filesystem/GD.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,32 @@ public static function set_default_quality($quality) {
}
}

/**
* @param filename string The filename of the image
* @param height int The height of the image
* @return boolean true if the image might fit in memory, false otherwise
*/
private static function image_fits_available_memory($filename) {
$memory_limit = translate_memstring(ini_get('memory_limit'));
if($memory_limit < 0) return true; //no memory_limit == -1
$imageInfo = getimagesize($filename);
//bytes per channel (round up, default 1) * channels (default 4 rgba).
$bytes_per_pixel = (isset($imageInfo['bits']) ? ($imageInfo['bits']+7)/8 : 1)
* (isset($imageInfo['channels']) ? $imageInfo['channels'] : 4);
//number of pixels (width * height) * bytes per pixel
$memory_required = $imageInfo[0]*$imageInfo[1]*$bytes_per_pixel;
return $memory_required + memory_get_usage() < $memory_limit;
Copy link
Contributor

Choose a reason for hiding this comment

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

What if the memory limit's -1? And GD requires memory for the destination image as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for catching that. I'll add a check for memory_limit == -1.
The current algorithm is only an educated guess. If our guess is wrong, we will run out of memory - same as now. A simple equation (image size * 2) risks raising a lot of false positives. Trying to accurately estimate how much memory GD will use for the destination requires information about the implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Furthermore, on shared hosts memory_limit is not always the limiting factor. This function will always be a rough guess and will not be able to catch all problems.

}

private static function get_lockfile_name($filename) {
$pathInfo = pathinfo($filename);
return $pathInfo['dirname'] . '.lock.' . $pathInfo['filename'];
}

public function __construct($filename = null) {
// Check for _lock file, if it exists we crashed while opening this file --> do not try again
if(file_exists(self::get_lockfile_name($filename)) || !self::image_fits_available_memory($filename)) return;
touch(self::get_lockfile_name($filename));
// If we're working with image resampling, things could take a while. Bump up the time-limit
increase_time_limit_to(300);

Expand All @@ -53,6 +78,7 @@ public function __construct($filename = null) {
}
}

unlink(self::get_lockfile_name($filename)); //opening the file was successfull --> delete the _lock file.
parent::__construct();

$this->quality = $this->config()->default_quality;
Expand Down
3 changes: 3 additions & 0 deletions model/Image.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -644,6 +644,9 @@ public function getOrientation() {
protected function onBeforeDelete() {
parent::onBeforeDelete();

$pathInfo = pathinfo($this->getFullPath());
$lockfile = $pathInfo['dirname'] . '.lock.' . $pathInfo['filename'];
if(file_exists($lockfile)) unlink($lockfile);
$this->deleteFormattedImages();
}
}
Expand Down
10 changes: 10 additions & 0 deletions tests/model/ImageTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,4 +214,14 @@ public function testPathPropertiesCachedImage() {
$secondImagePath = $secondImage->getRelativePath();
$this->assertEquals($secondImagePath, $secondImage->Filename);
}

public function testLockfileRemovalOnImageDelete() {
$testFile = $this->objFromFixture('Image', 'deletion_test_image');
$pathInfo = pathinfo($testFile->getFullPath());
$lockfile = $pathInfo['dirname'] . '.lock.' . $pathInfo['filename'];
touch($lockfile); //create .lock file
$this->assertTrue(file_exists($lockfile));
$testFile->delete();
$this->assertFalse(file_exists($lockfile));
}
}
4 changes: 4 additions & 0 deletions tests/model/ImageTest.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,3 +16,7 @@ Image:
Title: This is a/an image Title
Filename: assets/ImageTest/test_image).png
Parent: =>Folder.folder1
deletion_test_image:
Title: This is a/an image Title
Filename: assets/ImageTest/test_image.png
Parent: =>Folder.folder1