Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not use a shell in proc_open() if not really needed #5766

Merged
merged 2 commits into from Mar 28, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
37 changes: 22 additions & 15 deletions src/Util/PHP/AbstractPhpProcess.php
Expand Up @@ -13,7 +13,7 @@
use function array_keys;
use function array_merge;
use function assert;
use function escapeshellarg;
use function explode;
use function file_exists;
use function file_get_contents;
use function ini_get_all;
Expand Down Expand Up @@ -156,12 +156,15 @@ public function runTestJob(string $job, Test $test, string $processResultFile):

/**
* Returns the command based into the configurations.
*
* @return string[]
*/
public function getCommand(array $settings, ?string $file = null): string
public function getCommand(array $settings, ?string $file = null): array
{
$runtime = new Runtime;

$command = $runtime->getBinary();
$command = [];
$command[] = $runtime->getRawBinary();

if ($runtime->hasPCOV()) {
$settings = array_merge(
Expand All @@ -179,29 +182,29 @@ public function getCommand(array $settings, ?string $file = null): string
);
}

$command .= $this->settingsToParameters($settings);
$command = array_merge($command, $this->settingsToParameters($settings));

if (PHP_SAPI === 'phpdbg') {
$command .= ' -qrr';
$command[] = '-qrr';

if (!$file) {
$command .= 's=';
$command[] = 's=';
}
}

if ($file) {
$command .= ' ' . escapeshellarg($file);
$command[] = '-f';
$command[] = $file;
}

if ($this->arguments) {
if (!$file) {
$command .= ' --';
$command[] = '--';
}
$command .= ' ' . $this->arguments;
}

if ($this->stderrRedirection) {
$command .= ' 2>&1';
foreach (explode(' ', $this->arguments) as $arg) {
$command[] = trim($arg);
}
}

return $command;
Expand All @@ -212,12 +215,16 @@ public function getCommand(array $settings, ?string $file = null): string
*/
abstract public function runJob(string $job, array $settings = []): array;

protected function settingsToParameters(array $settings): string
/**
* @return list<string>
*/
protected function settingsToParameters(array $settings): array
{
$buffer = '';
$buffer = [];

foreach ($settings as $setting) {
$buffer .= ' -d ' . escapeshellarg($setting);
$buffer[] = '-d';
$buffer[] = $setting;
}

return $buffer;
Expand Down
37 changes: 7 additions & 30 deletions src/Util/PHP/DefaultPhpProcess.php
staabm marked this conversation as resolved.
Show resolved Hide resolved
Expand Up @@ -17,7 +17,6 @@
use function is_resource;
use function proc_close;
use function proc_open;
use function rewind;
use function stream_get_contents;
use function sys_get_temp_dir;
use function tempnam;
Expand Down Expand Up @@ -55,14 +54,6 @@ public function runJob(string $job, array $settings = []): array
return $this->runProcess($job, $settings);
}

/**
* Returns an array of file handles to be used in place of pipes.
*/
protected function getHandles(): array
{
return [];
}

/**
* Handles creating the child process and returning the STDOUT and STDERR.
*
Expand All @@ -73,8 +64,6 @@ protected function getHandles(): array
*/
protected function runProcess(string $job, array $settings): array
{
$handles = $this->getHandles();

$env = null;

if ($this->env) {
Expand All @@ -90,11 +79,15 @@ protected function runProcess(string $job, array $settings): array
}

$pipeSpec = [
0 => $handles[0] ?? ['pipe', 'r'],
1 => $handles[1] ?? ['pipe', 'w'],
2 => $handles[2] ?? ['pipe', 'w'],
0 => ['pipe', 'r'],
1 => ['pipe', 'w'],
2 => ['pipe', 'w'],
];

if ($this->stderrRedirection) {
$pipeSpec[2] = ['redirect', 1];
}

$process = proc_open(
$this->getCommand($settings, $this->tempFile),
$pipeSpec,
Expand Down Expand Up @@ -129,22 +122,6 @@ protected function runProcess(string $job, array $settings): array
fclose($pipes[2]);
}

if (isset($handles[1])) {
rewind($handles[1]);

$stdout = stream_get_contents($handles[1]);

fclose($handles[1]);
}

if (isset($handles[2])) {
rewind($handles[2]);

$stderr = stream_get_contents($handles[2]);

fclose($handles[2]);
}

proc_close($process);

$this->cleanup();
Expand Down
22 changes: 1 addition & 21 deletions src/Util/PHP/WindowsPhpProcess.php
Expand Up @@ -9,35 +9,15 @@
*/
namespace PHPUnit\Util\PHP;

use function tmpfile;
use PHPUnit\Framework\Exception;

/**
* @internal This class is not covered by the backward compatibility promise for PHPUnit
*
* @see https://bugs.php.net/bug.php?id=51800
*/
final class WindowsPhpProcess extends DefaultPhpProcess
{
/**
* @throws Exception
* @throws PhpProcessException
*/
protected function getHandles(): array
{
if (false === $stdout_handle = tmpfile()) {
throw new PhpProcessException(
'A temporary file could not be created; verify that your TEMP environment variable is writable',
);
}

return [
1 => $stdout_handle,
];
}

protected function useTemporaryFile(): bool
{
return true;
return false;
}
}