bug #10493 [2.3] Fix libxml_use_internal_errors and libxml_disable_entity_loader usage (romainneutron)

This PR was merged into the 2.3 branch.

Discussion
----------

[2.3] Fix libxml_use_internal_errors and libxml_disable_entity_loader usage

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #9731, #9483
| License       | MIT

This should fix #9731 and #9483 that seems to be triggered when `libxml_disable_entity_loader` has been called with `true` (see https://bugs.php.net/bug.php?id=62577)
As `libxml_disable_entity_loader` is non thread safe (see https://bugs.php.net/bug.php?id=64938) and as we have some calls that might leave the setting to `true`, I think the bug should be fixed.

I've checked the use of both `libxml_use_internal_errors` and `libxml_disable_entity_loader` among symfony code.

You can see I prefered to skip DomDocument::loadXML warnings using the `@` instead of using `LIBXML_NOERROR | LIBXML_NO_WARNING` because we can log these errors whereas libxml errors would be furtives.

 - [x] Check calls to DOMDocument::load
 - [x] Check calls to DOMDocument::loadXML
 - [x] Check calls to DOMDocument::loadHTML
 - [x] Check calls to DOMDocument::loadHTMLFile
 - [x] Add more tests

Commits
-------

489b8ae Fix libxml_use_internal_errors and libxml_disable_entity_loader usage

src/Symfony/Component/Config/Tests/Util/XmlUtilsTest.php

@@ -130,6 +130,45 @@ class XmlUtilsTest extends \PHPUnit_Framework_TestCase
             array('foo', 'foo'),
         );
     }
+
+    public function testLoadEmptyXmlFile()
+    {
+        $file = __DIR__.'/../Fixtures/foo.xml';
+        $this->setExpectedException('InvalidArgumentException', 'File '.$file.' does not contain valid XML, it is empty.');
+        XmlUtils::loadFile($file);
+    }
+
+    // test for issue https://github.com/symfony/symfony/issues/9731
+    public function testLoadWrongEmptyXMLWithErrorHandler()
+    {
+        $originalDisableEntities = libxml_disable_entity_loader(false);
+        $errorReporting = error_reporting(-1);
+
+        set_error_handler(function ($errno, $errstr) {
+            throw new \Exception($errstr, $errno);
+        });
+
+        $file = __DIR__.'/../Fixtures/foo.xml';
+        try {
+            XmlUtils::loadFile($file);
+            $this->fail('An exception should have been raised');
+        } catch (\InvalidArgumentException $e) {
+            $this->assertEquals(sprintf('File %s does not contain valid XML, it is empty.', $file), $e->getMessage());
+        }
+
+        restore_error_handler();
+        error_reporting($errorReporting);
+
+        $disableEntities = libxml_disable_entity_loader(true);
+        libxml_disable_entity_loader($disableEntities);
+
+        libxml_disable_entity_loader($originalDisableEntities);
+
+        $this->assertFalse($disableEntities);
+
+        // should not throw an exception
+        XmlUtils::loadFile(__DIR__.'/../Fixtures/Util/valid.xml', __DIR__.'/../Fixtures/Util/schema.xsd');
+    }
 }
 
 interface Validator

src/Symfony/Component/Config/Util/XmlUtils.php

@@ -40,13 +40,18 @@ class XmlUtils
      */
     public static function loadFile($file, $schemaOrCallable = null)
     {
+        $content = @file_get_contents($file);
+        if ('' === trim($content)) {
+            throw new \InvalidArgumentException(sprintf('File %s does not contain valid XML, it is empty.', $file));
+        }
+
         $internalErrors = libxml_use_internal_errors(true);
         $disableEntities = libxml_disable_entity_loader(true);
         libxml_clear_errors();
 
         $dom = new \DOMDocument();
         $dom->validateOnParse = true;
-        if (!$dom->loadXML(file_get_contents($file), LIBXML_NONET | (defined('LIBXML_COMPACT') ? LIBXML_COMPACT : 0))) {
+        if (!$dom->loadXML($content, LIBXML_NONET | (defined('LIBXML_COMPACT') ? LIBXML_COMPACT : 0))) {
             libxml_disable_entity_loader($disableEntities);
 
             throw new \InvalidArgumentException(implode("\n", static::getXmlErrors($internalErrors)));

src/Symfony/Component/CssSelector/Tests/XPath/TranslatorTest.php

@@ -49,7 +49,7 @@ class TranslatorTest extends \PHPUnit_Framework_TestCase
         $translator->registerExtension(new HtmlExtension($translator));
         $document = new \DOMDocument();
         $document->strictErrorChecking = false;
-        libxml_use_internal_errors(true);
+        $internalErrors = libxml_use_internal_errors(true);
         $document->loadHTMLFile(__DIR__.'/Fixtures/ids.html');
         $document = simplexml_import_dom($document);
         $elements = $document->xpath($translator->cssToXPath($css));
@@ -59,6 +59,8 @@ class TranslatorTest extends \PHPUnit_Framework_TestCase
                 $this->assertTrue(in_array($element->attributes()->id, $elementsId));
             }
         }
+        libxml_clear_errors();
+        libxml_use_internal_errors($internalErrors);
     }
 
     /** @dataProvider getHtmlShakespearTestData */

src/Symfony/Component/DomCrawler/Crawler.php

@@ -141,7 +141,7 @@ class Crawler extends \SplObjectStorage
      */
     public function addHtmlContent($content, $charset = 'UTF-8')
     {
-        $current = libxml_use_internal_errors(true);
+        $internalErrors = libxml_use_internal_errors(true);
         $disableEntities = libxml_disable_entity_loader(true);
 
         $dom = new \DOMDocument('1.0', $charset);
@@ -161,9 +161,11 @@ class Crawler extends \SplObjectStorage
             }
         }
 
-        @$dom->loadHTML($content);
+        if ('' !== trim($content)) {
+            @$dom->loadHTML($content);
+        }
 
-        libxml_use_internal_errors($current);
+        libxml_use_internal_errors($internalErrors);
         libxml_disable_entity_loader($disableEntities);
 
         $this->addDocument($dom);
@@ -200,16 +202,18 @@ class Crawler extends \SplObjectStorage
      */
     public function addXmlContent($content, $charset = 'UTF-8')
     {
-        $current = libxml_use_internal_errors(true);
+        $internalErrors = libxml_use_internal_errors(true);
         $disableEntities = libxml_disable_entity_loader(true);
 
         $dom = new \DOMDocument('1.0', $charset);
         $dom->validateOnParse = true;
 
-        // remove the default namespace to make XPath expressions simpler
-        @$dom->loadXML(str_replace('xmlns', 'ns', $content), LIBXML_NONET);
+        if ('' !== trim($content)) {
+            // remove the default namespace to make XPath expressions simpler
+            @$dom->loadXML(str_replace('xmlns', 'ns', $content), LIBXML_NONET);
+        }
 
-        libxml_use_internal_errors($current);
+        libxml_use_internal_errors($internalErrors);
         libxml_disable_entity_loader($disableEntities);
 
         $this->addDocument($dom);

src/Symfony/Component/DomCrawler/Tests/CrawlerTest.php

@@ -129,7 +129,7 @@ class CrawlerTest extends \PHPUnit_Framework_TestCase
      */
     public function testAddHtmlContentWithErrors()
     {
-        libxml_use_internal_errors(true);
+        $internalErrors = libxml_use_internal_errors(true);
 
         $crawler = new Crawler();
         $crawler->addHtmlContent(<<<EOF
@@ -149,7 +149,7 @@ EOF
         $this->assertEquals("Tag nav invalid\n", $errors[0]->message);
 
         libxml_clear_errors();
-        libxml_use_internal_errors(false);
+        libxml_use_internal_errors($internalErrors);
     }
 
     /**
@@ -179,7 +179,7 @@ EOF
      */
     public function testAddXmlContentWithErrors()
     {
-        libxml_use_internal_errors(true);
+        $internalErrors = libxml_use_internal_errors(true);
 
         $crawler = new Crawler();
         $crawler->addXmlContent(<<<EOF
@@ -197,7 +197,7 @@ EOF
         $this->assertTrue(count(libxml_get_errors()) > 1);
 
         libxml_clear_errors();
-        libxml_use_internal_errors(false);
+        libxml_use_internal_errors($internalErrors);
     }
 
     /**

src/Symfony/Component/Serializer/Encoder/XmlEncoder.php

@@ -66,6 +66,10 @@ class XmlEncoder extends SerializerAwareEncoder implements EncoderInterface, Dec
      */
     public function decode($data, $format, array $context = array())
     {
+        if ('' === trim($data)) {
+            throw new UnexpectedValueException('Invalid XML data, it can not be empty.');
+        }
+
         $internalErrors = libxml_use_internal_errors(true);
         $disableEntities = libxml_disable_entity_loader(true);
         libxml_clear_errors();
@@ -77,6 +81,8 @@ class XmlEncoder extends SerializerAwareEncoder implements EncoderInterface, Dec
         libxml_disable_entity_loader($disableEntities);
 
         if ($error = libxml_get_last_error()) {
+            libxml_clear_errors();
+
             throw new UnexpectedValueException($error->message);
         }
 

src/Symfony/Component/Serializer/Tests/Encoder/XmlEncoderTest.php

@@ -330,6 +330,12 @@ class XmlEncoderTest extends \PHPUnit_Framework_TestCase
         }
     }
 
+    public function testDecodeEmptyXml()
+    {
+        $this->setExpectedException('Symfony\Component\Serializer\Exception\UnexpectedValueException', 'Invalid XML data, it can not be empty.');
+        $this->encoder->decode(' ', 'xml');
+    }
+
     protected function getXmlSource()
     {
         return '<?xml version="1.0"?>'."\n".

src/Symfony/Component/Translation/Loader/QtFileLoader.php

@@ -11,6 +11,7 @@
 
 namespace Symfony\Component\Translation\Loader;
 
+use Symfony\Component\Config\Util\XmlUtils;
 use Symfony\Component\Translation\MessageCatalogue;
 use Symfony\Component\Translation\Exception\InvalidResourceException;
 use Symfony\Component\Translation\Exception\NotFoundResourceException;
@@ -40,12 +41,15 @@ class QtFileLoader implements LoaderInterface
             throw new NotFoundResourceException(sprintf('File "%s" not found.', $resource));
         }
 
-        $dom = new \DOMDocument();
-        $current = libxml_use_internal_errors(true);
-        if (!@$dom->load($resource, defined('LIBXML_COMPACT') ? LIBXML_COMPACT : 0)) {
-            throw new InvalidResourceException(implode("\n", $this->getXmlErrors()));
+        try {
+            $dom = XmlUtils::loadFile($resource);
+        } catch (\InvalidArgumentException $e) {
+            throw new InvalidResourceException(sprintf('Unable to load "%s".', $resource), $e->getCode(), $e);
         }
 
+        $internalErrors = libxml_use_internal_errors(true);
+        libxml_clear_errors();
+
         $xpath = new \DOMXPath($dom);
         $nodes = $xpath->evaluate('//TS/context/name[text()="'.$domain.'"]');
 
@@ -67,33 +71,8 @@ class QtFileLoader implements LoaderInterface
             $catalogue->addResource(new FileResource($resource));
         }
 
-        libxml_use_internal_errors($current);
+        libxml_use_internal_errors($internalErrors);
 
         return $catalogue;
     }
-
-    /**
-     * Returns the XML errors of the internal XML parser
-     *
-     * @return array An array of errors
-     */
-    private function getXmlErrors()
-    {
-        $errors = array();
-        foreach (libxml_get_errors() as $error) {
-            $errors[] = sprintf('[%s %s] %s (in %s - line %d, column %d)',
-                LIBXML_ERR_WARNING == $error->level ? 'WARNING' : 'ERROR',
-                $error->code,
-                trim($error->message),
-                $error->file ? $error->file : 'n/a',
-                $error->line,
-                $error->column
-            );
-        }
-
-        libxml_clear_errors();
-        libxml_use_internal_errors(false);
-
-        return $errors;
-    }
 }

src/Symfony/Component/Translation/Loader/XliffFileLoader.php

@@ -11,6 +11,7 @@
 
 namespace Symfony\Component\Translation\Loader;
 
+use Symfony\Component\Config\Util\XmlUtils;
 use Symfony\Component\Translation\MessageCatalogue;
 use Symfony\Component\Translation\Exception\InvalidResourceException;
 use Symfony\Component\Translation\Exception\NotFoundResourceException;
@@ -86,27 +87,13 @@ class XliffFileLoader implements LoaderInterface
      */
     private function parseFile($file)
     {
+        try {
+            $dom = XmlUtils::loadFile($file);
+        } catch (\InvalidArgumentException $e) {
+            throw new InvalidResourceException(sprintf('Unable to load "%s": %s', $file, $e->getMessage()), $e->getCode(), $e);
+        }
+
         $internalErrors = libxml_use_internal_errors(true);
-        $disableEntities = libxml_disable_entity_loader(true);
-        libxml_clear_errors();
-
-        $dom = new \DOMDocument();
-        $dom->validateOnParse = true;
-        if (!@$dom->loadXML(file_get_contents($file), LIBXML_NONET | (defined('LIBXML_COMPACT') ? LIBXML_COMPACT : 0))) {
-            libxml_disable_entity_loader($disableEntities);
-
-            throw new InvalidResourceException(implode("\n", $this->getXmlErrors($internalErrors)));
-        }
-
-        libxml_disable_entity_loader($disableEntities);
-
-        foreach ($dom->childNodes as $child) {
-            if ($child->nodeType === XML_DOCUMENT_TYPE_NODE) {
-                libxml_use_internal_errors($internalErrors);
-
-                throw new InvalidResourceException('Document types are not allowed.');
-            }
-        }
 
         $location = str_replace('\\', '/', __DIR__).'/schema/dic/xliff-core/xml.xsd';
         $parts = explode('/', $location);

src/Symfony/Component/Translation/Tests/Loader/QtFileLoaderTest.php

@@ -63,4 +63,12 @@ class QtFileLoaderTest extends \PHPUnit_Framework_TestCase
         $resource = __DIR__.'/../fixtures/invalid-xml-resources.xlf';
         $loader->load($resource, 'en', 'domain1');
     }
+
+    public function testLoadEmptyResource()
+    {
+        $loader = new QtFileLoader();
+        $resource = __DIR__.'/../fixtures/empty.xlf';
+        $this->setExpectedException('Symfony\Component\Translation\Exception\InvalidResourceException', sprintf('Unable to load "%s".', $resource));
+        $loader->load($resource, 'en', 'domain1');
+    }
 }

src/Symfony/Component/Translation/Tests/Loader/XliffFileLoaderTest.php

@@ -110,4 +110,12 @@ class XliffFileLoaderTest extends \PHPUnit_Framework_TestCase
         $loader = new XliffFileLoader();
         $loader->load(__DIR__.'/../fixtures/withdoctype.xlf', 'en', 'domain1');
     }
+
+    public function testParseEmptyFile()
+    {
+        $loader = new XliffFileLoader();
+        $resource = __DIR__.'/../fixtures/empty.xlf';
+        $this->setExpectedException('Symfony\Component\Translation\Exception\InvalidResourceException', sprintf('Unable to load "%s":', $resource));
+        $loader->load($resource, 'en', 'domain1');
+    }
 }