From 33321b43cafbb731bff062aeffd6f46e38749faf Mon Sep 17 00:00:00 2001 From: Florian Pritz Date: Wed, 27 May 2015 00:18:45 +0200 Subject: Improve running of external commands Signed-off-by: Florian Pritz --- application/controllers/file.php | 27 +++-- .../libraries/Image/Drivers/imagemagick.php | 16 +-- application/libraries/ProcRunner.php | 129 +++++++++++++++++++++ application/libraries/Pygments.php | 11 +- 4 files changed, 156 insertions(+), 27 deletions(-) create mode 100644 application/libraries/ProcRunner.php (limited to 'application') diff --git a/application/controllers/file.php b/application/controllers/file.php index 9e920b9fa..83204c318 100644 --- a/application/controllers/file.php +++ b/application/controllers/file.php @@ -130,7 +130,7 @@ class File extends MY_Controller { handle_etag($etag); header("Content-disposition: inline; filename=\"".$id."_qr.png\"\n"); header("Content-Type: image/png\n"); - passthru('qrencode -s 10 -o - '.escapeshellarg(site_url($id).'/')); + echo (new \libraries\ProcRunner(array('qrencode', '-s', '10', '-o', '-', site_url($id).'/')))->execSafe()['stdout']; exit(); case "info": @@ -235,28 +235,31 @@ class File extends MY_Controller { private function _colorify($file, $lexer, $anchor_id = false) { - $return_value = 0; $output = ""; $lines_to_remove = 0; $output .= '
'."\n"; $output .= '
'."\n";
 
-		ob_start();
 		if ($lexer == "ascii") {
-			passthru('ansi2html -p < '.escapeshellarg($file), $return_value);
+			// TODO: use exec safe and catch exception
+			$ret = (new \libraries\ProcRunner(array('ansi2html', '-p')))
+				->input(file_get_contents($file))
+				->forbid_stderr()
+				->exec();
 			// Last line is empty
 			$lines_to_remove = 1;
 		} else {
-			passthru('pygmentize -F codetagify -O encoding=guess,outencoding=utf8,stripnl=False -l '.escapeshellarg($lexer).' -f html '.escapeshellarg($file), $return_value);
+			// TODO: use exec safe and catch exception
+			$ret = (new \libraries\ProcRunner(array('pygmentize', '-F', 'codetagify', '-O', 'encoding=guess,outencoding=utf8,stripnl=False', '-l', $lexer, '-f', 'html', $file)))
+				->forbid_stderr()
+				->exec();
 			// Last 2 items are "
" and "" $lines_to_remove = 2; } - $buf = ob_get_contents(); - ob_end_clean(); - $buf = explode("\n", $buf); + $buf = explode("\n", $ret["stdout"]); $line_count = count($buf); for ($i = 1; $i <= $lines_to_remove; $i++) { @@ -287,7 +290,7 @@ class File extends MY_Controller { $output .= "
"; return array( - "return_value" => $return_value, + "return_value" => $ret["return_code"], "output" => $output ); } @@ -305,12 +308,14 @@ class File extends MY_Controller { echo '
'."\n"; echo '
'."\n"; echo '
'."\n"; - passthru(escapeshellarg(FCPATH.'scripts/Markdown.pl').' '.escapeshellarg($file), $return_value); + // TODO: use exec safe and catch exception + $r = (new \libraries\ProcRunner(array(FCPATH.'scripts/Markdown.pl', $file)))->forbid_stderr()->exec(); + echo $r['stdout']; echo '
'; return array( "output" => ob_get_clean(), - "return_value" => $return_value, + "return_value" => $r["return_code"], ); } else { return get_instance()->_colorify($file, $lexer, $is_multipaste ? $filedata["id"] : false); diff --git a/application/libraries/Image/Drivers/imagemagick.php b/application/libraries/Image/Drivers/imagemagick.php index ed0c70dc6..e37b9c8b3 100644 --- a/application/libraries/Image/Drivers/imagemagick.php +++ b/application/libraries/Image/Drivers/imagemagick.php @@ -45,13 +45,6 @@ class imagemagick implements \libraries\Image\ImageDriver { $this->arguments = array(); } - private function passthru($arguments) - { - $command_string = implode(" ", array_map("escapeshellarg", $arguments)); - passthru($command_string, $ret); - return $ret; - } - public function get($target_type = null) { if ($target_type === null) { @@ -62,7 +55,6 @@ class imagemagick implements \libraries\Image\ImageDriver { $command = array_merge($command, $this->arguments); $command[] = $this->source_file."[0]"; - ob_start(); switch ($target_type) { case IMAGETYPE_GIF: $command[] = "gif:-"; @@ -76,14 +68,14 @@ class imagemagick implements \libraries\Image\ImageDriver { default: assert(0); } - $ret = $this->passthru($command); - $result = ob_get_clean(); - if ($ret != 0 || $result === false) { + try { + $ret = (new \libraries\ProcRunner($command))->execSafe(); + } catch (\exceptions\ApiException $e) { throw new \exceptions\ApiException("libraries/Image/thumbnail-creation-failed", "Failed to create thumbnail"); } - return $result; + return $ret["stdout"]; } public function resize($width, $height) diff --git a/application/libraries/ProcRunner.php b/application/libraries/ProcRunner.php new file mode 100644 index 000000000..6aaaa1f20 --- /dev/null +++ b/application/libraries/ProcRunner.php @@ -0,0 +1,129 @@ + + * + * Licensed under AGPLv3 + * (see COPYING for full license text) + * + */ + +namespace libraries; + +class ProcRunner { + private $cmd; + private $input = NULL; + private $forbid_nonzero = false; + private $forbid_stderr = false; + + /** + * This function automatically escapes all parameters before executing the command. + * + * @param cmd array with the command and it's arguments + */ + function __construct($cmd) + { + assert(is_array($cmd)); + $this->cmd = implode(" ", array_map('escapeshellarg', $cmd)); + } + + /** + * Set stdin. You will have to set this to NULL if you call exec() a second + * time and don't want stdin to be sent again + * + * @param input string to send via stdin + */ + function input($input) + { + $this->input = $input; + return $this; + } + + function forbid_nonzero() + { + $this->forbid_nonzero = true; + return $this; + } + + + function forbid_stderr() + { + $this->forbid_stderr = true; + return $this; + } + + /** + * Run the command. + * + * @return array with keys return_code, stdout, stderr + */ + function exec() + { + $descriptorspec = array( + 1 => array('pipe', 'w'), + 2 => array('pipe', 'w'), + ); + + if ($this->input !== NULL) { + $descriptorspec[0] = array('pipe', 'r'); + } + + $proc = proc_open($this->cmd, $descriptorspec, $pipes); + + if ($proc === false) { + throw new \exceptions\ApiException('procrunner/proc_open-failed', + 'Failed to run process', + array($this->cmd, $this->input) + ); + } + + if ($this->input !== NULL) { + fwrite($pipes[0], $this->input); + fclose($pipes[0]); + } + + $stdout = stream_get_contents($pipes[1]); + fclose($pipes[1]); + assert($stdout !== false); + + $stderr = stream_get_contents($pipes[2]); + fclose($pipes[2]); + assert($stderr !== false); + + $return_code = proc_close($proc); + + $ret = array( + "return_code" => $return_code, + "stdout" => $stdout, + "stderr" => $stderr, + ); + + if ($this->forbid_nonzero && $return_code !== 0) { + throw new \exceptions\ApiException('procrunner/non-zero-exit', + 'Process exited with a non-zero status', + array($this->cmd, $this->input, $ret) + ); + } + + if ($this->forbid_stderr && $stderr !== "") { + throw new \exceptions\ApiException('procrunner/stderr', + 'Output on stderr not allowed but received', + array($this->cmd, $this->input, $ret) + ); + } + + return $ret; + } + + /** + * Run the command and enable some sanity checks such as empty stderr and + * zero exit status. Might enable more in the future. + * + * @See exec + */ + function execSafe() + { + $this->forbid_stderr(); + $this->forbid_nonzero(); + return $this->exec(); + } +} diff --git a/application/libraries/Pygments.php b/application/libraries/Pygments.php index 588f78842..5d37b69c3 100644 --- a/application/libraries/Pygments.php +++ b/application/libraries/Pygments.php @@ -22,11 +22,14 @@ class Pygments { private static function get_pygments_info() { return cache_function_full('pygments_info-v2', 1800, function() { - ob_start(); - passthru(escapeshellarg(FCPATH."scripts/get_lexer_list.py")); - $output = ob_get_clean(); + $r = (new \libraries\ProcRunner(array(FCPATH."scripts/get_lexer_list.py")))->execSafe(); - return json_decode($output, true); + $ret = json_decode($r["stdout"], true); + if ($ret === NULL) { + throw new \exceptions\ApiException('pygments/json-failed', "Failed to decode JSON", $r); + } + + return $ret; }); } -- cgit v1.2.3-24-g4f1b