dompdf / php-svg-lib

SVG file parsing / rendering library
GNU Lesser General Public License v3.0
1.4k stars 77 forks source link

Fixed a memory leak #38

Closed UCIS closed 4 years ago

UCIS commented 6 years ago

The XML parser callback combined with the class level reference to the XML parser create a reference loop which can not be resolved by the PHP garbage collector. Therefore to prevent memory leaks either the class level reference should be cleaned (or not exist at all), or the parser callbacks should be unset (NULL) during cleanup. Just removing the class level reference, which as far as I can tell is not used anywhere, seems to be the easiest way.

Test case demonstrating the XML parser memory leak:

<?php
class A {
        function __construct() {
                $parser = xml_parser_create("utf-8");
                xml_set_element_handler($parser, [$this, 'a'], NULL); //commenting this out fixes the problem
              $this->parser = $parser; //commenting this out fixes the problem
                xml_parser_free($parser);
//              $this->parser = NULL; //uncommenting this fixes the problem
        }
        function a() {
        }
}

for ($i = 0; $i < 1000; $i++) {
        for ($j = 0; $j < 1000; $j++) new A();
        echo memory_get_usage().PHP_EOL;
}

The bug affects PHP7.1 and 7.2, and is probably something that should be fixed in PHP, but that will probably take a lot more time than this simple fix.

bsweeney commented 4 years ago

Confirmed in the PHP docs:

Caution: In addition to calling xml_parser_free() when the parsing is finished, as of PHP 7.0.0 it is necessary to also explicitly unset the reference to parser to avoid memory leaks, if the parser resource is referenced from an object, and this object references that parser resource.

I agree with the assessment, whatever the original intent it doesn't look like we're using this right now so this seems like the best resolution.