Inject dependencies into Shell\Command

This slightly changes how execution time limits fall back on each other.

Change-Id: I7754a9e6be9638eebe90cb953adb8e2a6ee97cef

diff --git a/RELEASE-NOTES-1.30 b/RELEASE-NOTES-1.30
index 2090ce9..bec7b86 100644 (file)
--- a/RELEASE-NOTES-1.30
+++ b/RELEASE-NOTES-1.30
@@ -233,7 +233,10 @@ changes to languages because of Phabricator reports.
 * DB_SLAVE is deprecated. DB_REPLICA should be used instead.
 * wfUsePHP() is deprecated.
 * wfFixSessionID() was removed.
-* wfShellExec() and related functions are deprecated, use Shell::command().
+* wfShellExec() and related functions are deprecated, use Shell::command(). This also
+  slightly changes the behavior of how execution time limits are calculated when only
+  some of defaults are overridden per-call. When in doubt, always override both wall
+  clock and CPU time.
 * (T138166) SpecialEmailUser::getTarget() now requires a second argument, the sending
   user object. Using the method without the second argument is deprecated.
 * (T67297) Browsers that don't support Unicode will have their edits rejected.

diff --git a/includes/shell/Command.php b/includes/shell/Command.php
index 864e69a..fd8f6a0 100644 (file)
--- a/includes/shell/Command.php
+++ b/includes/shell/Command.php
@@ -24,6 +24,8 @@ use Exception;
 use MediaWiki\ProcOpenError;
 use MediaWiki\ShellDisabledError;
 use Profiler;
+use Psr\Log\LoggerAwareTrait;
+use Psr\Log\NullLogger;
 
 /**
  * Class used for executing shell commands
@@ -31,11 +33,22 @@ use Profiler;
  * @since 1.30
  */
 class Command {
+       use LoggerAwareTrait;
+
        /** @var string */
        private $command = '';
 
        /** @var array */
-       private $limits = [];
+       private $limits = [
+               // seconds
+               'time' => 180,
+               // seconds
+               'walltime' => 180,
+               // KB
+               'memory' => 307200,
+               // KB
+               'filesize' => 102400,
+       ];
 
        /** @var string[] */
        private $env = [];
@@ -49,13 +62,20 @@ class Command {
        /** @var bool */
        private $everExecuted = false;
 
+       /** @var string|false */
+       private $cGroup = false;
+
        /**
         * Constructor. Don't call directly, instead use Shell::command()
+        *
+        * @throws ShellDisabledError
         */
        public function __construct() {
                if ( Shell::isDisabled() ) {
                        throw new ShellDisabledError();
                }
+
+               $this->setLogger( new NullLogger() );
        }
 
        /**
@@ -112,11 +132,10 @@ class Command {
         * Sets execution limits
         *
         * @param array $limits Optional array with limits(filesize, memory, time, walltime).
-        *   This overrides the global wgMaxShell* limits.
         * @return $this
         */
        public function limits( array $limits ) {
-               $this->limits = $limits;
+               $this->limits = $limits + $this->limits;
 
                return $this;
        }
@@ -158,6 +177,18 @@ class Command {
                return $this;
        }
 
+       /**
+        * Sets cgroup for this command
+        *
+        * @param string|false $cgroup
+        * @return $this
+        */
+       public function cgroup( $cgroup ) {
+               $this->cGroup = $cgroup;
+
+               return $this;
+       }
+
        /**
         * Executes command. Afterwards, getExitCode() and getOutput() can be used to access execution
         * results.
@@ -168,8 +199,7 @@ class Command {
         * @throws ShellDisabledError
         */
        public function execute() {
-               global $IP, $wgMaxShellMemory, $wgMaxShellFileSize, $wgMaxShellTime,
-                          $wgMaxShellWallClockTime, $wgShellCgroup;
+               global $IP;
 
                $this->everExecuted = true;
 
@@ -197,18 +227,12 @@ class Command {
 
                $useLogPipe = false;
                if ( is_executable( '/bin/bash' ) ) {
-                       $time = intval( isset( $this->limits['time'] ) ? $this->limits['time'] : $wgMaxShellTime );
-                       if ( isset( $this->limits['walltime'] ) ) {
-                               $wallTime = intval( $this->limits['walltime'] );
-                       } elseif ( isset( $this->limits['time'] ) ) {
-                               $wallTime = $time;
-                       } else {
-                               $wallTime = intval( $wgMaxShellWallClockTime );
-                       }
-                       $mem = intval( isset( $this->limits['memory'] ) ? $this->limits['memory'] : $wgMaxShellMemory );
-                       $filesize = intval( isset( $this->limits['filesize'] )
-                               ? $this->limits['filesize']
-                               : $wgMaxShellFileSize );
+                       $time = intval( $this->limits['time'] );
+                       $wallTime = intval( $this->limits['walltime'] );
+                       // for b/c, wall time falls back to time
+                       $wallTime = min( $time, $wallTime );
+                       $mem = intval( $this->limits['memory'] );
+                       $filesize = intval( $this->limits['filesize'] );
 
                        if ( $time > 0 || $mem > 0 || $filesize > 0 || $wallTime > 0 ) {
                                $cmd = '/bin/bash ' . escapeshellarg( "$IP/includes/limit.sh" ) . ' ' .
@@ -216,7 +240,7 @@ class Command {
                                           escapeshellarg(
                                                   "MW_INCLUDE_STDERR=" . ( $this->useStderr ? '1' : '' ) . ';' .
                                                   "MW_CPU_LIMIT=$time; " .
-                                                  'MW_CGROUP=' . escapeshellarg( $wgShellCgroup ) . '; ' .
+                                                  'MW_CGROUP=' . escapeshellarg( $this->cGroup ) . '; ' .
                                                   "MW_MEM_LIMIT=$mem; " .
                                                   "MW_FILE_SIZE_LIMIT=$filesize; " .
                                                   "MW_WALL_CLOCK_LIMIT=$wallTime; " .
@@ -252,7 +276,7 @@ class Command {
                $scoped = Profiler::instance()->scopedProfileIn( __FUNCTION__ . '-' . $profileMethod );
                $proc = proc_open( $cmd, $desc, $pipes );
                if ( !$proc ) {
-                       wfDebugLog( 'exec', "proc_open() failed: $cmd" );
+                       $this->logger->error( "proc_open() failed: {command}", [ 'command' => $cmd ] );
                        throw new ProcOpenError();
                }
                $outBuffer = $logBuffer = '';
@@ -329,7 +353,7 @@ class Command {
                                                $lines = explode( "\n", $logBuffer );
                                                $logBuffer = array_pop( $lines );
                                                foreach ( $lines as $line ) {
-                                                       wfDebugLog( 'exec', $line );
+                                                       $this->logger->info( $line );
                                                }
                                        }
                                }
@@ -369,7 +393,7 @@ class Command {
                }
 
                if ( $logMsg !== false ) {
-                       wfDebugLog( 'exec', "$logMsg: $cmd" );
+                       $this->logger->warning( "$logMsg: {command}", [ 'command' => $cmd ] );
                }
 
                return new Result( $retval, $outBuffer );

diff --git a/includes/shell/Shell.php b/includes/shell/Shell.php
index c293ff2..f2c96ae 100644 (file)
--- a/includes/shell/Shell.php
+++ b/includes/shell/Shell.php
@@ -22,6 +22,9 @@
 
 namespace MediaWiki\Shell;
 
+use MediaWiki\Logger\LoggerFactory;
+use MediaWiki\MediaWikiServices;
+
 /**
  * Executes shell commands
  *
@@ -39,10 +42,10 @@ namespace MediaWiki\Shell;
 class Shell {
 
        /**
-        * Returns a new instance of this class
+        * Returns a new instance of Command class
         *
-        * @param string|string[] $command If string, a properly shell-escaped command line,
-        *   or an array of unescaped arguments, in which case each value will be escaped
+        * @param string|string[] $command String or array of strings representing the command to
+        * be executed, each value will be escaped.
         *   Example:   [ 'convert', '-font', 'font name' ] would produce "'convert' '-font' 'font name'"
         * @return Command
         */
@@ -54,6 +57,18 @@ class Shell {
                        $args = reset( $args );
                }
                $command = new Command();
+               $config = MediaWikiServices::getInstance()->getMainConfig();
+
+               $limits = [
+                       'time' => $config->get( 'MaxShellTime' ),
+                       'walltime' => $config->get( 'MaxShellWallClockTime' ),
+                       'memory' => $config->get( 'MaxShellMemory' ),
+                       'filesize' => $config->get( 'MaxShellFileSize' ),
+               ];
+               $command->limits( $limits );
+               $command->cgroup( $config->get( 'ShellCgroup' ) );
+               $command->setLogger( LoggerFactory::getInstance( 'exec' ) );
+
                return $command->params( $args );
        }