Add way of including all stderr output when executing command

This adds an option to wfShellExec (and convenience function
wfShellExecWithStderr), to make sure all stderr is duplicated
to stdout. The previous method of doing this was to include
2>&1 on the command line. However this did not redirect errors
from limit.sh (For example cgroups not set up, or if a command
reached the file size limit set by ulimit).

Not sure if this is the best approach, but it seems to work well,
and compared to most other approaches I considered, actually gets
the ulimit errors redirected too.

Currently some files fail to render with no error whatsoever,
hopefully this patch will make what went wrong more obvious.

Also fix a comment in wfShellExec that was incorrect (trailing \n),
and make the initial value of the return value variable be 200, so
if there's ever a bug in php where its not being set properly, it
would be immediately obvious what is happening.

Bug: 53824
Change-Id: I833aeb3ab9da726ecb97331369ea187daad7e795

diff --git a/includes/GlobalFunctions.php b/includes/GlobalFunctions.php
index bed2c44..90d78ca 100644 (file)
--- a/includes/GlobalFunctions.php
+++ b/includes/GlobalFunctions.php
@@ -2759,9 +2759,11 @@ function wfShellExecDisabled() {
  *                 added to the executed command environment.
  * @param array $limits optional array with limits(filesize, memory, time, walltime)
  *                 this overwrites the global wgShellMax* limits.
- * @return string collected stdout as a string (trailing newlines stripped)
+ * @param array $options Array of options. Only one is "duplicateStderr" => true, which
+ *                 Which duplicates stderr to stdout, including errors from limit.sh
+ * @return string collected stdout as a string
  */
-function wfShellExec( $cmd, &$retval = null, $environ = array(), $limits = array() ) {
+function wfShellExec( $cmd, &$retval = null, $environ = array(), $limits = array(), $options = array() ) {
        global $IP, $wgMaxShellMemory, $wgMaxShellFileSize, $wgMaxShellTime,
                $wgMaxShellWallClockTime, $wgShellCgroup;
 
@@ -2773,6 +2775,8 @@ function wfShellExec( $cmd, &$retval = null, $environ = array(), $limits = array
                        'Unable to run external programs, passthru() is disabled.';
        }
 
+       $includeStderr = isset( $options['duplicateStderr'] ) && $options['duplicateStderr'];
+
        wfInitShellLocale();
 
        $envcmd = '';
@@ -2795,6 +2799,10 @@ function wfShellExec( $cmd, &$retval = null, $environ = array(), $limits = array
        $cmd = $envcmd . $cmd;
 
        if ( php_uname( 's' ) == 'Linux' ) {
+               $stderrDuplication = '';
+               if ( $includeStderr ) {
+                       $stderrDuplication = 'exec 2>&1; ';
+               }
                $time = intval ( isset( $limits['time'] ) ? $limits['time'] : $wgMaxShellTime );
                if ( isset( $limits['walltime'] ) ) {
                        $wallTime = intval( $limits['walltime'] );
@@ -2810,17 +2818,25 @@ function wfShellExec( $cmd, &$retval = null, $environ = array(), $limits = array
                        $cmd = '/bin/bash ' . escapeshellarg( "$IP/includes/limit.sh" ) . ' ' .
                                escapeshellarg( $cmd ) . ' ' .
                                escapeshellarg(
+                                       $stderrDuplication .
                                        "MW_CPU_LIMIT=$time; " .
                                        'MW_CGROUP=' . escapeshellarg( $wgShellCgroup ) . '; ' .
                                        "MW_MEM_LIMIT=$mem; " .
                                        "MW_FILE_SIZE_LIMIT=$filesize; " .
                                        "MW_WALL_CLOCK_LIMIT=$wallTime"
                                );
+               } else {
+                       $cmd .= ' 2>&1';
                }
+       } elseif ( $includeStderr ) {
+               $cmd .= ' 2>&1';
        }
        wfDebug( "wfShellExec: $cmd\n" );
 
-       $retval = 1; // error by default?
+       // Default to an unusual value that shouldn't happen naturally,
+       // so in the unlikely event of a weird php bug, it would be
+       // more obvious what happened.
+       $retval = 200;
        ob_start();
        passthru( $cmd, $retval );
        $output = ob_get_contents();
@@ -2832,6 +2848,24 @@ function wfShellExec( $cmd, &$retval = null, $environ = array(), $limits = array
        return $output;
 }
 
+/**
+ * Execute a shell command, returning both stdout and stderr. Convenience
+ * function, as all the arguments to wfShellExec can become unwieldy.
+ *
+ * @note This also includes errors from limit.sh, e.g. if $wgMaxShellFileSize is exceeded.
+ * @param string $cmd Command line, properly escaped for shell.
+ * @param &$retval null|Mixed optional, will receive the program's exit code.
+ *                 (non-zero is usually failure)
+ * @param array $environ optional environment variables which should be
+ *                 added to the executed command environment.
+ * @param array $limits optional array with limits(filesize, memory, time, walltime)
+ *                 this overwrites the global wgShellMax* limits.
+ * @return string collected stdout and stderr as a string
+ */
+function wfShellExecWithStderr( $cmd, &$retval = null, $environ = array(), $limits = array() ) {
+       return wfShellExec( $cmd, $retval, $environ, $limits, array( 'duplicateStderr' => true ) );
+}
+
 /**
  * Workaround for http://bugs.php.net/bug.php?id=45132
  * escapeshellarg() destroys non-ASCII characters if LANG is not a UTF-8 locale

diff --git a/includes/media/Bitmap.php b/includes/media/Bitmap.php
index 9f7a09c..4031b01 100644 (file)
--- a/includes/media/Bitmap.php
+++ b/includes/media/Bitmap.php
@@ -357,12 +357,12 @@ class BitmapHandler extends ImageHandler {
                        " -depth 8 $sharpen " .
                        " -rotate -$rotation " .
                        " {$animation_post} " .
-                       wfEscapeShellArg( $this->escapeMagickOutput( $params['dstPath'] ) ) . " 2>&1";
+                       wfEscapeShellArg( $this->escapeMagickOutput( $params['dstPath'] ) );
 
                wfDebug( __METHOD__ . ": running ImageMagick: $cmd\n" );
                wfProfileIn( 'convert' );
                $retval = 0;
-               $err = wfShellExec( $cmd, $retval, $env );
+               $err = wfShellExecWithStderr( $cmd, $retval, $env );
                wfProfileOut( 'convert' );
 
                if ( $retval !== 0 ) {
@@ -472,7 +472,7 @@ class BitmapHandler extends ImageHandler {
                wfDebug( __METHOD__ . ": Running custom convert command $cmd\n" );
                wfProfileIn( 'convert' );
                $retval = 0;
-               $err = wfShellExec( $cmd, $retval );
+               $err = wfShellExecWithStderr( $cmd, $retval );
                wfProfileOut( 'convert' );
 
                if ( $retval !== 0 ) {
@@ -774,11 +774,11 @@ class BitmapHandler extends ImageHandler {
                                $cmd = wfEscapeShellArg( $wgImageMagickConvertCommand ) . " " .
                                        wfEscapeShellArg( $this->escapeMagickInput( $params['srcPath'], $scene ) ) .
                                        " -rotate -$rotation " .
-                                       wfEscapeShellArg( $this->escapeMagickOutput( $params['dstPath'] ) ) . " 2>&1";
+                                       wfEscapeShellArg( $this->escapeMagickOutput( $params['dstPath'] ) );
                                wfDebug( __METHOD__ . ": running ImageMagick: $cmd\n" );
                                wfProfileIn( 'convert' );
                                $retval = 0;
-                               $err = wfShellExec( $cmd, $retval, $env );
+                               $err = wfShellExecWithStderr( $cmd, $retval, $env );
                                wfProfileOut( 'convert' );
                                if ( $retval !== 0 ) {
                                        $this->logErrorForExternalProcess( $retval, $err, $cmd );

diff --git a/includes/media/Jpeg.php b/includes/media/Jpeg.php
index 1feb378..fa76366 100644 (file)
--- a/includes/media/Jpeg.php
+++ b/includes/media/Jpeg.php
@@ -75,11 +75,11 @@ class JpegHandler extends ExifBitmapHandler {
                        $cmd = wfEscapeShellArg( $wgJpegTran ) .
                                " -rotate " . wfEscapeShellArg( $rotation ) .
                                " -outfile " . wfEscapeShellArg( $params['dstPath'] ) .
-                               " " . wfEscapeShellArg( $params['srcPath'] ) . " 2>&1";
+                               " " . wfEscapeShellArg( $params['srcPath'] );
                        wfDebug( __METHOD__ . ": running jpgtran: $cmd\n" );
                        wfProfileIn( 'jpegtran' );
                        $retval = 0;
-                       $err = wfShellExec( $cmd, $retval, $env );
+                       $err = wfShellExecWithStderr( $cmd, $retval, $env );
                        wfProfileOut( 'jpegtran' );
                        if ( $retval !== 0 ) {
                                $this->logErrorForExternalProcess( $retval, $err, $cmd );

diff --git a/includes/media/SVG.php b/includes/media/SVG.php
index 4c055a5..72a9696 100644 (file)
--- a/includes/media/SVG.php
+++ b/includes/media/SVG.php
@@ -177,7 +177,7 @@ class SvgHandler extends ImageHandler {
                                                wfEscapeShellArg( $srcPath ),
                                                wfEscapeShellArg( $dstPath ) ),
                                        $wgSVGConverters[$wgSVGConverter]
-                               ) . " 2>&1";
+                               );
 
                                $env = array();
                                if ( $lang !== false ) {
@@ -186,7 +186,7 @@ class SvgHandler extends ImageHandler {
 
                                wfProfileIn( 'rsvg' );
                                wfDebug( __METHOD__ . ": $cmd\n" );
-                               $err = wfShellExec( $cmd, $retval, $env );
+                               $err = wfShellExecWithStderr( $cmd, $retval, $env );
                                wfProfileOut( 'rsvg' );
                        }
                }

diff --git a/includes/upload/UploadBase.php b/includes/upload/UploadBase.php
index 37dc7cb..3a05f93 100644 (file)
--- a/includes/upload/UploadBase.php
+++ b/includes/upload/UploadBase.php
@@ -1323,7 +1323,7 @@ abstract class UploadBase {
                # NOTE: there's a 50 line workaround to make stderr redirection work on windows, too.
                #      that does not seem to be worth the pain.
                #      Ask me (Duesentrieb) about it if it's ever needed.
-               $output = wfShellExec( "$command 2>&1", $exitCode );
+               $output = wfShellExecWithStderr( $command, $exitCode );
 
                # map exit code to AV_xxx constants.
                $mappedCode = $exitCode;