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

Merged
merged 1 commit into from Jul 16, 2014

Projects

None yet

4 participants

@kinglozzer
Member

See #2753 for more discussion.

This is a (late) follow up to the discussion here about GD crashing when attempting to open large or high DPI images.

The Google Group discussion involved ideas around “image unavailable” icons in the CMS, this setup (by storing which manipulation failed) will hopefully pave the way for that functionality to be added later, while addressing the immediate issue of completely crashing entire sections of websites.

Implements the same method for checking available memory as #2569, though it’s different to the method Ingo suggested. I’m not really sure which is more robust, the method in the php.net comments doesn’t take into account that bits and channels aren’t always present in the image info.

Marked as an API change as I’ve added a few methods to the Image_Backend interface.

Doesn’t cause any issues with the CMS, the images just don’t appear for now:

screen shot 2014-01-02 at 16 15 27
screen shot 2014-01-02 at 16 17 23

All comments welcome.

@Zauberfisch
Contributor

+1 to get this done as soon as possible

great work @kinglozzer

@kinglozzer
Member

I think it’s probably important that we document this change if it’s accepted, it may be quite confusing to developers otherwise if images disappear (until we set up CMS messages or something).

Would reference/image.md be the best place for this, or would it better to create a topics/image.md? Documentation on this seems more like a ‘topic’ than ‘reference’ to me, but then we’d have two separate documents for the same subject.

@kinglozzer
Member

bump

I don’t wanna spend any more time on this until I get a nod of approval from a core committer. Any other comments/criticisms are also welcome of course! 😉

@simonwelsh simonwelsh added this to the 3.2 alpha 1 milestone Mar 15, 2014
@simonwelsh simonwelsh referenced this pull request in silverstripe/silverstripe-cms Mar 16, 2014
Closed

[2009-02-14] ImageField allowed memory size error #715

@Zauberfisch
Contributor

bump

can we please get this merged asap?
Especially with those clients we deploy to shared hosting, and the ever increasing image resolution, this is becoming more and more an issue.
I have clients uploading images with a width of 6k, and they can't even delete them anymore because after the error occurred, you never get to a point in the cms where you could delete the image again.

@chillu chillu added the critical label Apr 29, 2014
@kinglozzer
Member

bumpity bump :)

Could this (or any other proposed fix for this issue) make it into 3.2? Not sure how far off we are.

@simonwelsh simonwelsh commented on an outdated diff Jul 11, 2014
filesystem/GD.php
@@ -65,6 +81,18 @@ public function __construct($filename = null) {
$this->quality = $this->config()->default_quality;
$this->interlace = $this->config()->image_interlace;
}
+
+ /**
+ * If __destruct() is called, any attempted image manipulation must have succeeded,
+ * so it can be removed from the cache
@simonwelsh
simonwelsh Jul 11, 2014 Contributor

This isn't always true. HHVM at least still calls destructors after a fatal error.

@simonwelsh simonwelsh commented on an outdated diff Jul 11, 2014
filesystem/GD.php
@@ -482,6 +560,18 @@ public function writeTo($filename) {
if(file_exists($filename)) @chmod($filename,0664);
}
}
+
+ public function onBeforeDelete($frontend) {
+ $file = Director::baseFolder() . "/" . $frontend->Filename;
+
+ if (file_exists($file)) {
+ $key = md5(implode('_', array($file, filemtime($file))));
+
+ if (unserialize($this->cache->load($key))) {
@simonwelsh
simonwelsh Jul 11, 2014 Contributor

You can just call remove without the check.

@simonwelsh simonwelsh commented on the diff Jul 11, 2014
tests/filesystem/GDTest.php
+ $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));
@simonwelsh
simonwelsh Jul 11, 2014 Contributor

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

@simonwelsh simonwelsh commented on the diff Jul 11, 2014
tests/filesystem/GDTest.php
+
+ $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));
@simonwelsh
simonwelsh Jul 11, 2014 Contributor

Likewise.

@simonwelsh simonwelsh commented on the diff Jul 11, 2014
tests/model/GDImageTest.php
+ }
+
+ /**
+ * 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));
@simonwelsh
simonwelsh Jul 11, 2014 Contributor

Likewise.

@kinglozzer
Member

Thanks Simon, I’d not considered that there might be inconsistencies with __destruct() like that. I’m trying to think of an alternative, but it’s quite tricky without making assumptions about how GDBackend might be used: ideally this would also cater for people who use GDBackend outside of the context of Image.

I guess you could argue that we can’t really be responsible for this - we just need to try to prevent, for example, parts of the CMS being inaccessible.

So, should we:

  • A. Not worry about how people may use GDBackend and instead only focus on Image and move some of the logic (i.e. the SS_Cache/stored manipulations logic) into there
  • B. Try to find an alternative to __destruct() and try to keep this as GD-specific as possible
  • C. Forget the __destruct() stuff, just add the approximate memory calculation to GDBackend and see if that resolves the majority of people’s issues
@Zauberfisch
Contributor

sounds like a tricky issue.

I for one actually do use GBBackend without image as well on rare occasions, and I like to cleanly separate things if possible, but I see that there might not be a way, or it might be a lot harder, so I am fine with B.

Not sure if C is a good solution, but I am not that much into the the whole issue that I could speak to that (I don't know how accurate the approximation is, or how likely/unlikely it is that an error still occurs).
But if we can get C into core significantly faster than A or B; I would love to see C merged and A or B developed further.

@simonwelsh
Contributor

A couple of options:

  • Remove items from the cache when writeToFile is called
  • Remove items from the cache whenever returning from one of the modifier methods

Given that it's not actually that important to remove items from the cache, I'd prefer the first option.

@kinglozzer
Member

I’ve moved the cache cleaning to writeToFile() and updated a couple of other minor points.

@simonwelsh I’ve added $this->fail() to the unit tests. I wasn’t sure of the correct usage of it: it seems that a try/catch block catches the failure, so I’ve added an assertion for the exception message to make sure it’s highlighted. Is that correct?

@simonwelsh
Contributor

Would be better to throw a more specific exception (so, a subclass) and then catch that so that the PHPUnit exception doesn't get caught.

@kinglozzer
Member

Tests updated :)

@simonwelsh simonwelsh merged commit 1a63fa5 into silverstripe:master Jul 16, 2014
@simonwelsh
Contributor

Green button pushed! Now my slides are out of date...

@Zauberfisch
Contributor

awesome!
great work guys!

@kinglozzer
Member

687474703a2f2f77696c2e746f2f5f2f7965732e676966

Thanks @simonwelsh

@kinglozzer kinglozzer deleted the kinglozzer:gd-resize-crash-mast branch Nov 19, 2014
@chillu chillu added a commit to open-sausages/silverstripe-framework that referenced this pull request Jan 6, 2017
@chillu chillu Reinstated GDBackend->checkAvailableMemory()
This was removed as part of the SS4 assets refactor:
silverstripe@be23989
Presumably @tractorcow didn’t feel it’s possible to retain this,
because we don’t have local file handles required for getimagesize().
Since there’s getimagesizefromstring() as well (added in PHP 5.4),
we just needed a different logic branch.

Also makes the logic more resilient against missing GD resources on a backend.
Lack of available memory for a resize is only one (new) reason,
other edge cases were already causing these missing resources (e.g. an invalid file string).

Original discussion: https://groups.google.com/forum/m/#!topic/silverstripe-dev/B57a3KYeAVQ
Pull request for 3.x: silverstripe#2859
More context: silverstripe#2569
fcb511b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment