r65681 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r65680‎ | r65681 | r65682 >
Date:05:33, 30 April 2010
Author:tstarling
Status:deferred
Tags:
Comment:
MFT r65467 (and r64935, r64936, r64947 to sync the base): updated fix for bug 23148 (ImageMagick parameter escaping) and related issues.
Modified paths:
  • /branches/REL1_16/phase3/includes/media (modified) (history)
  • /branches/REL1_16/phase3/includes/media/Bitmap.php (modified) (history)

Diff [purge]

Index: branches/REL1_16/phase3/includes/media/Bitmap.php
@@ -60,7 +60,7 @@
6161 $physicalHeight = $params['physicalHeight'];
6262 $clientWidth = $params['width'];
6363 $clientHeight = $params['height'];
64 - $descriptionUrl = isset( $params['descriptionUrl'] ) ? "File source: ". $params['descriptionUrl'] : '';
 64+ $comment = isset( $params['descriptionUrl'] ) ? "File source: ". $params['descriptionUrl'] : '';
6565 $srcWidth = $image->getWidth();
6666 $srcHeight = $image->getHeight();
6767 $mimeType = $image->getMimeType();
@@ -111,7 +111,7 @@
112112
113113 $quality = '';
114114 $sharpen = '';
115 - $frame = '';
 115+ $scene = false;
116116 $animation = '';
117117 if ( $mimeType == 'image/jpeg' ) {
118118 $quality = "-quality 80"; // 80%
@@ -125,7 +125,7 @@
126126 if( $srcWidth * $srcHeight > $wgMaxAnimatedGifArea ) {
127127 // Extract initial frame only; we're so big it'll
128128 // be a total drag. :P
129 - $frame = '[0]';
 129+ $scene = 0;
130130 } else {
131131 // Coalesce is needed to scale animated GIFs properly (bug 1017).
132132 $animation = ' -coalesce ';
@@ -147,18 +147,19 @@
148148
149149 $cmd =
150150 $tempEnv .
151 - wfEscapeShellArg($wgImageMagickConvertCommand) .
 151+ wfEscapeShellArg( $wgImageMagickConvertCommand ) .
152152 " {$quality} -background white -size {$physicalWidth} ".
153 - wfEscapeShellArg($srcPath . $frame) .
 153+ wfEscapeShellArg( $this->escapeMagickInput( $srcPath, $scene ) ) .
154154 $animation .
155155 // For the -resize option a "!" is needed to force exact size,
156156 // or ImageMagick may decide your ratio is wrong and slice off
157157 // a pixel.
158158 " -thumbnail " . wfEscapeShellArg( "{$physicalWidth}x{$physicalHeight}!" ) .
159 - " -set comment " . wfEscapeShellArg( str_replace( '%', '%%', $descriptionUrl ) ) .
 159+ // Add the source url as a comment to the thumb.
 160+ " -set comment " . wfEscapeShellArg( $this->escapeMagickProperty( $comment ) ) .
160161 " -depth 8 $sharpen " .
161 - wfEscapeShellArg($dstPath) . " 2>&1";
162 - wfDebug( __METHOD__.": running ImageMagick: $cmd\n");
 162+ wfEscapeShellArg( $this->escapeMagickOutput( $dstPath ) ) . " 2>&1";
 163+ wfDebug( __METHOD__.": running ImageMagick: $cmd\n" );
163164 wfProfileIn( 'convert' );
164165 $err = wfShellExec( $cmd, $retval );
165166 wfProfileOut( 'convert' );
@@ -249,6 +250,90 @@
250251 }
251252 }
252253
 254+ /**
 255+ * Escape a string for ImageMagick's property input (e.g. -set -comment)
 256+ * See InterpretImageProperties() in magick/property.c
 257+ */
 258+ function escapeMagickProperty( $s ) {
 259+ // Double the backslashes
 260+ $s = str_replace( '\\', '\\\\', $s );
 261+ // Double the percents
 262+ $s = str_replace( '%', '%%', $s );
 263+ // Escape initial - or @
 264+ if ( strlen( $s ) > 0 && ( $s[0] === '-' || $s[0] === '@' ) ) {
 265+ $s = '\\' . $s;
 266+ }
 267+ return $s;
 268+ }
 269+
 270+ /**
 271+ * Escape a string for ImageMagick's input filenames. See ExpandFilenames()
 272+ * and GetPathComponent() in magick/utility.c.
 273+ *
 274+ * This won't work with an initial ~ or @, so input files should be prefixed
 275+ * with the directory name.
 276+ *
 277+ * Glob character unescaping is broken in ImageMagick before 6.6.1-5, but
 278+ * it's broken in a way that doesn't involve trying to convert every file
 279+ * in a directory, so we're better off escaping and waiting for the bugfix
 280+ * to filter down to users.
 281+ *
 282+ * @param $path string The file path
 283+ * @param $scene string The scene specification, or false if there is none
 284+ */
 285+ function escapeMagickInput( $path, $scene = false ) {
 286+ # Die on initial metacharacters (caller should prepend path)
 287+ $firstChar = substr( $path, 0, 1 );
 288+ if ( $firstChar === '~' || $firstChar === '@' ) {
 289+ throw new MWException( __METHOD__.': cannot escape this path name' );
 290+ }
 291+
 292+ # Escape glob chars
 293+ $path = preg_replace( '/[*?\[\]{}]/', '\\\\\0', $path );
 294+
 295+ return $this->escapeMagickPath( $path, $scene );
 296+ }
 297+
 298+ /**
 299+ * Escape a string for ImageMagick's output filename. See
 300+ * InterpretImageFilename() in magick/image.c.
 301+ */
 302+ function escapeMagickOutput( $path, $scene = false ) {
 303+ $path = str_replace( '%', '%%', $path );
 304+ return $this->escapeMagickPath( $path, $scene );
 305+ }
 306+
 307+ /**
 308+ * Armour a string against ImageMagick's GetPathComponent(). This is a
 309+ * helper function for escapeMagickInput() and escapeMagickOutput().
 310+ *
 311+ * @param $path string The file path
 312+ * @param $scene string The scene specification, or false if there is none
 313+ */
 314+ protected function escapeMagickPath( $path, $scene = false ) {
 315+ # Die on format specifiers (other than drive letters). The regex is
 316+ # meant to match all the formats you get from "convert -list format"
 317+ if ( preg_match( '/^([a-zA-Z0-9-]+):/', $path, $m ) ) {
 318+ if ( wfIsWindows() && is_dir( $m[0] ) ) {
 319+ // OK, it's a drive letter
 320+ // ImageMagick has a similar exception, see IsMagickConflict()
 321+ } else {
 322+ throw new MWException( __METHOD__.': unexpected colon character in path name' );
 323+ }
 324+ }
 325+
 326+ # If there are square brackets, add a do-nothing scene specification
 327+ # to force a literal interpretation
 328+ if ( $scene === false ) {
 329+ if ( strpos( $path, '[' ) !== false ) {
 330+ $path .= '[0--1]';
 331+ }
 332+ } else {
 333+ $path .= "[$scene]";
 334+ }
 335+ return $path;
 336+ }
 337+
253338 static function imageJpegWrapper( $dst_image, $thumbPath ) {
254339 imageinterlace( $dst_image );
255340 imagejpeg( $dst_image, $thumbPath, 95 );
Property changes on: branches/REL1_16/phase3/includes/media/Bitmap.php
___________________________________________________________________
Name: svn:mergeinfo
256341 - /branches/REL1_15/phase3/includes/media/Bitmap.php:51646
/branches/sqlite/includes/media/Bitmap.php:58211-58321
/branches/wmf-deployment/includes/media/Bitmap.php:53381
/trunk/phase3/includes/media/Bitmap.php:63549,63764,63897-63901,64876,64881,64932
257342 + /branches/REL1_15/phase3/includes/media/Bitmap.php:51646
/branches/sqlite/includes/media/Bitmap.php:58211-58321
/branches/wmf-deployment/includes/media/Bitmap.php:53381
/trunk/phase3/includes/media/Bitmap.php:63549,63764,63897-63901,64876,64881,64932,64935-64936,64947,65467
Property changes on: branches/REL1_16/phase3/includes/media
___________________________________________________________________
Name: svn:mergeinfo
258343 + /branches/REL1_15/phase3/includes/media:51646
/branches/sqlite/includes/media:58211-58321
/branches/wmf-deployment/includes/media:53381
/trunk/phase3/includes/media:63549,63764,63897-63901,64876,64881,64935-64936,64947,65467

Follow-up revisions

RevisionCommit summaryAuthorDate
r65682MFT r65467 (and r64935, r64936, r64947 to sync the base): updated fix for bug...tstarling05:37, 30 April 2010

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r64932(bug 23148) Escape % in ImageMagick comments. Patch by Derk-Jan Hartmancatrope12:38, 11 April 2010
r64935Follow up r64932: Add comment on why % is escapedbtongminh13:54, 11 April 2010
r64936Follow up r64935: Add whitespace and clarify comment a bit more.btongminh13:59, 11 April 2010
r64947Follow up r64936. Even better, provide the bug number and imagemagick doc page.platonides20:47, 11 April 2010
r65401MFT r64932, bug 23148: ImageMagick comment escaping. Noted security implicati...tstarling15:35, 21 April 2010
r65402MFT r64932, bug 23148: ImageMagick comment escaping bugtstarling15:37, 21 April 2010
r65467More rigorous fix for ImageMagick parameter interpretation (bug 23148 etc.) b...tstarling16:24, 23 April 2010

Status & tagging log