FIX 5284: Prevent crash for lager images. #2569
Conversation
Implemented check in GD that estimates the memory required to load the image and rejects larger images. Created lock files that remains after a large image has previously crashed GD.
|
This is the result of the discussion in the mailinglist and on the previous commit jakr@344a464. Thanks to all who participated! This fixes the problem, but I am still a bit concerned that it might need some refactoring - currently we have to know the convention in for the lock file name in three places: GD (where it is generated), Image (so that it is removed upon deletion) and the test (so that we can check if it works). This seems hacky to me. Despite these concerns, I think the benefit of fixing this overweight the problems of the solution. @wilr FYI: This is an updated version of the patch in http://open.silverstripe.org/ticket/5284. I don't know if you can or want to link this pull request to the old tracker. |
| 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; | ||
| fclose(fopen(self::get_lockfile_name($filename), 'w')); //create .lock file |
There was a problem hiding this comment.
Using touch() looks much cleaner than this.
There was a problem hiding this comment.
I agree, thanks. Did not know that touch was available in php.
|
I'm concerned that this has one lock per image. Why should being unable to create a large image prevent you from creating a thumbnail? (Say, |
|
I think that these just not right (anti-goldilock) images are a rare edge case. I came across the problem that an image was so large (and memory_limit so low) that not even a small thumbnail could be created. |
|
@dhensby and I had a few comments on this pull request that we left in the mailing list: https://groups.google.com/forum/#!topic/silverstripe-dev/B57a3KYeAVQ. They mainly discussed making sure we don't leave any code in An example is the changes to |
|
I read your comments and agree. In my opinion the cleanest way to do this would be to add onBeforeDelete to Image_Backend and to move the deletion code down to GD. Image then would pass on the onBeforeDelete to the backend. Of course the test cases would also move to GD. I'll try to write something up over the weekend and create a new pull. |
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
Implemented check in GD that estimates the memory required to load the image and rejects larger images. Created lock files that remains after a large image has previously crashed GD.