(bug 27508) SVGMetadataExtractor takes too much resources on huge svg's. Change it...

Add a new config variable $wgSVGMetadataCutoff (currently set to 256kb, chosen rather arbitrarily)
and only read that much of the svg file when finding metadata. In general:
*Most (non-crazy huge map) svgs aren't that big, so there'd be no change in general
*Almost all files have any relevent metadata (well except for when we look for animation tags) is at the begining of the file
before actual image data.
*At the end of the day, even if this does miss metadata in some files (which I really doubt it would), I'd consider that a better
situation then the current situation where it can take 10 minutes or have OOM to parse the likes of [[:File:Puerto_Rico_ecosystems_map-fr.svg]]

Also has parts of/parts are based on Hartman's patch from bugzilla in it.

Also changes how it recurses into child elements looking for animation, to do so only when neccesary.

Trims the results of reading values, because i was getting extra leading spaces when testing this.

Last of all, add a comment to the MediaHandler class about how the first parameter of MediaHandler::getMetadata is kind of useless.
(it confused me when I was doing this)

diff --git a/RELEASE-NOTES b/RELEASE-NOTES
index a7166bf..9a3283b 100644 (file)
--- a/RELEASE-NOTES
+++ b/RELEASE-NOTES
@@ -49,6 +49,8 @@ PHP if you have not done so prior to upgrading MediaWiki.
 * The transliteration for passwords in case they were migrated from an old Latin-1
   install (previous to MediaWiki 1.5) is now only done for wikis with
   $wgLegacyEncoding set.
+* (bug 27508) Add $wgSVGMetadataCutoff to limit the maximum amount of an svg we
+  look at when finding metadata to prevent excessive resource usage.
 
 === New features in 1.18 ===
 * Added a special page, disabled by default, that allows users with the

diff --git a/includes/DefaultSettings.php b/includes/DefaultSettings.php
index 01ae9f3..aabab5b 100644 (file)
--- a/includes/DefaultSettings.php
+++ b/includes/DefaultSettings.php
@@ -675,6 +675,9 @@ $wgSVGConverter = 'ImageMagick';
 $wgSVGConverterPath = '';
 /** Don't scale a SVG larger than this */
 $wgSVGMaxSize = 2048;
+/** Don't read SVG metadata beyond this point.
+ * Default is 1024*256 bytes */
+$wgSVGMetadataCutoff = 262144; 
 
 /**
  * MediaWiki will reject HTMLesque tags in uploaded files due to idiotic browsers which can't

diff --git a/includes/media/Generic.php b/includes/media/Generic.php
index 88b5543..2e092f2 100644 (file)
--- a/includes/media/Generic.php
+++ b/includes/media/Generic.php
@@ -83,7 +83,8 @@ abstract class MediaHandler {
        /**
         * Get handler-specific metadata which will be saved in the img_metadata field.
         *
-        * @param $image File: the image object, or false if there isn't one
+        * @param $image File: the image object, or false if there isn't one.
+        *   Warning, File::getPropsFromPath might pass an (object)array() instead (!)
         * @param $path String: the filename
         * @return String
         */

diff --git a/includes/media/SVGMetadataExtractor.php b/includes/media/SVGMetadataExtractor.php
index 967d671..b5c8df1 100644 (file)
--- a/includes/media/SVGMetadataExtractor.php
+++ b/includes/media/SVGMetadataExtractor.php
@@ -47,13 +47,40 @@ class SVGReader {
         * @param $source String: URI from which to read
         */
        function __construct( $source ) {
+               global $wgSVGMetadataCutoff;
                $this->reader = new XMLReader();
-               $this->reader->open( $source, null, LIBXML_NOERROR | LIBXML_NOWARNING );
+
+               // Don't use $file->getSize() since file object passed to SVGHandler::getMetadata is bogus.
+               $size = filesize( $source );
+               if ( $size === false ) {
+                       throw new MWException( "Error getting filesize of SVG." );
+               } 
+
+               if ( $size > $wgSVGMetadataCutoff ) {
+                       $this->debug( "SVG is $size bytes, which is bigger than $wgSVGMetadataCutoff. Truncating." );
+                       $contents = file_get_contents( $source, false, null, -1, $wgSVGMetadataCutoff );
+                       if ($contents === false) {
+                               throw new MWException( 'Error reading SVG file.' );
+                       }
+                       $this->reader->XML( $contents, null, LIBXML_NOERROR | LIBXML_NOWARNING );
+               } else {
+                       $this->reader->open( $source, null, LIBXML_NOERROR | LIBXML_NOWARNING );
+               }
 
                $this->metadata['width'] = self::DEFAULT_WIDTH;
                $this->metadata['height'] = self::DEFAULT_HEIGHT;
 
-               $this->read();
+               // Because we cut off the end of the svg making an invalid one. Complicated
+               // try catch thing to make sure warnings get restored. Seems like there should
+               // be a better way.
+               wfSuppressWarnings();
+               try {
+                       $this->read();
+               } catch( Exception $e ) {
+                       wfRestoreWarnings();
+                       throw $e;
+               }
+               wfRestoreWarnings();
        }
 
        /*
@@ -83,7 +110,6 @@ class SVGReader {
 
                $exitDepth =  $this->reader->depth;
                $keepReading = $this->reader->read();
-               $skip = false;
                while ( $keepReading ) {
                        $tag = $this->reader->name;
                        $type = $this->reader->nodeType;
@@ -100,17 +126,15 @@ class SVGReader {
                                $this->readXml( $tag, 'metadata' );
                        } elseif ( $tag !== '#text' ) {
                                $this->debug( "Unhandled top-level XML tag $tag" );
-                               $this->animateFilter( $tag );
-                               //$skip = true;
-                       }
 
-                       if ($skip) {
-                               $keepReading = $this->reader->next();
-                               $skip = false;
-                               $this->debug( "Skip" );
-                       } else {
-                               $keepReading = $this->reader->read();
+                               if ( !isset( $this->metadata['animated'] ) ) {
+                                       // Recurse into children of current tag, looking for animation.
+                                       $this->animateFilter( $tag );
+                               }
                        }
+
+                       // Goto next element, which is sibling of current (Skip children).
+                       $keepReading = $this->reader->next();
                }
 
                $this->reader->close();
@@ -134,7 +158,7 @@ class SVGReader {
                        if( $this->reader->name == $name && $this->reader->nodeType == XmlReader::END_ELEMENT ) {
                                break;
                        } elseif( $this->reader->nodeType == XmlReader::TEXT ){
-                               $this->metadata[$metafield] = $this->reader->value;
+                               $this->metadata[$metafield] = trim( $this->reader->value );
                        }
                        $keepReading = $this->reader->read();
                }