23

Can someone explain, what the following Code introduced between Magento CE 1.9.1.0 and 1.9.2.0 is used for?

class Varien_Image_Adapter_Gd2:

public function __construct()
{
    // Initialize shutdown function
    register_shutdown_function(array($this, 'destruct'));
}

/**
 * Destroy object image on shutdown
 */
public function destruct()
{
    @imagedestroy($this->_imageHandler);
}

After those two functions has been added, our import of product gallery images with the ImportExport interface stopped working. The error is due to a memory limit (which comes out to be the max open file size limit).

My idea is, that the files opened by the import won't be closed correctly.

I also saw that there were some empty destruct() functions introduced (Mage_ImportExport_Model_Import_Adapter_Abstract) - but extending those to match the parent logic doesn't help.

14

That looks like they tried to make sure to destroy the image resource, but instead introduced a memory leak. I cannot think of a valid reason for this code, to be honest, but I can explain what has been changed:

Originally, imagedestroy() would have been called in the desctructor __destruct()

function __destruct()
{
    @imagedestroy($this->_imageHandler);
}

The destructor is called whenever the PHP garbage collector destroys unused objects (i.e. objects in memory that are not referenced anymore).

Now, imagedestroy() is instead called in a shutdown function and since this is a callback to a method of the Varien_Image_Adapter_Gd2 object, it cannot even be garbage collected until the very end. This way all image resources stay open until script execution finishes.

  • Thanks for the explanation - this is what I thought of. So overall, this introduced code makes the majority of imports useless on 1.9.2. in my eyes. Hope this will be fixed soon. Any advise where to open a Bug report? – Achim Rosenhagen Aug 24 '15 at 18:27
5

It was part of fixing security issues with unserialize. Magic methods like __destruct have inherent issues with serialization.

We have seen exploits proposed that were using serialization and __destruct to create files in the file system - and this change (you will see more similar changes in other places) was done to avoid this.

Does it cause memory leak or just use more memory until script finishes?

https://security.stackexchange.com/questions/77549/is-php-unserialize-exploitable-without-any-interesting-methods

  • Thanks for the context. Has this particular change been done to prevent a specific exploit or just to be sure? – Fabian Schmengler Aug 24 '15 at 18:38
  • And no, it probably just makes the script consume more memory, not a real memory leak – Fabian Schmengler Aug 24 '15 at 18:40
  • It cause a huge memory leak especially when importing images, as it'll keep all image files open until the end of the import processing. This way we can only import approx 50 products (wheras before we can do an import of >2k seemlessly). I ran my test on a local VM with 8G RAM and the source files are all approx 300KB. Before the change the used memory by PHP rermains at 1k during the whole import. – Achim Rosenhagen Aug 24 '15 at 18:41
  • fschmengler is right - it might not be a 'memory leak' but the consumption goes up thraight the hill ;-) – Achim Rosenhagen Aug 24 '15 at 18:43
  • 1
    @Alex thanks for the advise. I reverse patched it. Now, memory leak is gone, but no solution for the future. – Arne Oct 15 '15 at 16:21
5

Having the same problems with my Magento 1.9.2.0...

I only get this to work by changing Varien_Image_Adapter_Gd2 in /lib/Varien/Image/Adapter/Gd2.php as follows:

public function __construct()
{
    // Initialize shutdown function
    // register_shutdown_function(array($this, 'destruct'));
}

/**
 * Destroy object image on shutdown
 */
public function __destruct()
{
    @imagedestroy($this->_imageHandler);
}
  • remove line with register_shutdown_function (or comment out)
  • change function name destruct to __destruct

I have set memory_limit back to 1G (previously i raised up to 32GB) and now it works...

This project implements said procedure in a modman friendly way. Just install it with composer and you're good to go.

  • This does not really answer the question. If you have a different question, you can ask it by clicking Ask Question. You can also add a bounty to draw more attention to this question once you have enough reputation. – Rajeev K Tomy Oct 15 '15 at 1:06
  • Yes, this does not answer the question but will help people who needs a temporary solution and no discussion – dkr Oct 15 '15 at 9:38
  • fixed an issue with memory consumption during import. Interesting, does Magento test somehow what they are releasing? – klipach Feb 24 '16 at 14:24
  • This solve not only the import problem. This solve a big memory eating by the process that create/recreate the cache and resized versions for every products images. If i upload png images in my products, without this "hack" i can't work and i obtain a lots of memory exhausted errors. – Simbus82 Aug 8 '17 at 14:48
  • Today I found this suggestion. I implemented it and the memory leak is gone. Then I created this github.com/borasocom-team/magento-gd2-memoryleak to install it in a clean way. – Dr. Gianluigi Zane Zanettini Aug 14 '18 at 12:07
4

So I did raise a bug with Magento including a "solution" that should deal with the memory usage issues in the image import process.

The solution can be found on github under https://github.com/sitewards/import_image_memory_leak_fix but the basic idea is.

Fixing the Mage_Catalog_Helper_Image::validateUploadFile to actually call the destruct method on the image processor. Sadly it seems that the default Varien_Image does not deal with a destruct so we have had to add our own class that does.

<?php
/**
 * @category    Sitewards
 * @package     Sitewards_ImportImageMemoryLeakFix
 * @copyright   Copyright (c) Sitewards GmbH (http://www.sitewards.com/)
 */
class Sitewards_ImportImageMemoryLeakFix_Model_Destructable_Image extends Varien_Image
{
    /**
     * Constructor,
     * difference from original constructor - we register a destructor here.
     *
     * @param string $sFileName
     * @param Varien_Image_Adapter $oAdapter Default value is GD2
     */
    public function __construct($sFileName = null, $oAdapter = Varien_Image_Adapter::ADAPTER_GD2)
    {
        parent::__construct($sFileName, $oAdapter);

        // Initialize shutdown function
        register_shutdown_function(array($this, 'destruct'));
    }

    /**
     * Destroy object image on shutdown
     */
    public function destruct()
    {
        $oAdapter = $this->_getAdapter();
        if (method_exists($oAdapter, 'destruct')) {
            $oAdapter->destruct();
        } else {
            Mage::log('Image can not be destructed properly, adapter doesn\'t support the method.');
        }
    }
}

And then a rewrite of the helper.

<?xml version="1.0"?>
<config>
    <modules>
        <Sitewards_ImportImageMemoryLeakFix>
            <version>0.1.0</version>
        </Sitewards_ImportImageMemoryLeakFix>
    </modules>
    <global>
        <models>
            <sitewards_importimagememoryleakfix>
                <class>Sitewards_ImportImageMemoryLeakFix_Model</class>
            </sitewards_importimagememoryleakfix>
        </models>
        <helpers>
            <catalog>
                <rewrite>
                    <image>Sitewards_ImportImageMemoryLeakFix_Helper_Catalog_Helper_Image</image>
                </rewrite>
            </catalog>
        </helpers>
    </global>
</config>

And the new function calls the new destructable image class.

<?php
/**
 * @category    Sitewards
 * @package     Sitewards_ImportImageMemoryLeakFix
 * @copyright   Copyright (c) Sitewards GmbH (http://www.sitewards.com/)
 */
class Sitewards_ImportImageMemoryLeakFix_Helper_Catalog_Helper_Image extends Mage_Catalog_Helper_Image
{
    /**
     * Check - is this file an image
     *
     * Difference from original method - we destroy the image object here,
     * i.e. we are not wasting memory, without that fix product import with images
     * easily goes over 4Gb on memory with just couple hundreds of products.
     *
     * @param string $sFilePath
     *
     * @return bool
     * @throws Mage_Core_Exception
     */
    public function validateUploadFile($sFilePath) {
        if (!getimagesize($sFilePath)) {
            Mage::throwException($this->__('Disallowed file type.'));
        }

        /** @var Sitewards_ImportImageMemoryLeakFix_Model_Destructable_Image $oImageProcessor */
        $oImageProcessor = Mage::getModel('sitewards_importimagememoryleakfix/destructable_image', $sFilePath);
        $sMimeType       = $oImageProcessor->getMimeType();
        $oImageProcessor->destruct();

        return $sMimeType !== null;
    }
}

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.