Skip to content

Commit

Permalink
Refactor prepareRestart to make it more robust
Browse files Browse the repository at this point in the history
  • Loading branch information
@johnstevenson
johnstevenson committed Mar 20, 2024
1 parent 0755533 commit 6282b17
Show file tree
Hide file tree
Showing 4 changed files with 110 additions and 43 deletions.
1 change: 1 addition & 0 deletions phpstan.neon.dist
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
includes:
- vendor/phpstan/phpstan/conf/bleedingEdge.neon
- vendor/phpstan/phpstan-strict-rules/rules.neon

parameters:
Expand Down
133 changes: 90 additions & 43 deletions src/XdebugHandler.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -143,9 +143,9 @@ public function check(): void
if (!((bool) $envArgs[0]) && $this->requiresRestart(self::$xdebugActive)) {
// Restart required
$this->notify(Status::RESTART);
$command = $this->prepareRestart();

if ($this->prepareRestart()) {
$command = $this->getCommand();
if ($command !== null) {
$this->restart($command);
}
return;
Expand Down Expand Up @@ -267,7 +267,7 @@ protected function requiresRestart(bool $default): bool
/**
* Allows an extending class to access the tmpIni
*
* @param string[] $command *
* @param non-empty-list<string> $command
*/
protected function restart(array $command): void
{
Expand All @@ -277,7 +277,7 @@ protected function restart(array $command): void
/**
* Executes the restarted command then deletes the tmp ini
*
* @param string[] $command
* @param non-empty-list<string> $command
* @phpstan-return never
*/
private function doRestart(array $command): void
Expand Down Expand Up @@ -318,52 +318,74 @@ private function doRestart(array $command): void
}

/**
* Returns true if everything was written for the restart
* Returns the command line array if everything was written for the restart
*
* If any of the following fails (however unlikely) we must return false to
* stop potential recursion:
* - tmp ini file creation
* - environment variable creation
*
* @return non-empty-list<string>|null
*/
private function prepareRestart(): bool
private function prepareRestart(): ?array
{
if (!$this->cli) {
$this->notify(Status::ERROR, 'Unsupported SAPI: '.PHP_SAPI);
return null;
}

if (($argv = $this->checkServerArgv()) === null) {
$this->notify(Status::ERROR, '$_SERVER[argv] is not as expected');
return null;
}

if (!$this->checkConfiguration($info)) {
$this->notify(Status::ERROR, $info);
return null;
}

$mainScript = (string) $this->script;
if (!$this->checkMainScript($mainScript, $argv)) {
$this->notify(Status::ERROR, 'Unable to access main script: '.$mainScript);
return null;
}

$tmpDir = sys_get_temp_dir();
$iniError = 'Unable to create temp ini file at: '.$tmpDir;

if (($tmpfile = @tempnam($tmpDir, '')) === false) {
$this->notify(Status::ERROR, $iniError);
return null;
}

$error = null;
$iniFiles = self::getAllIniFiles();
$scannedInis = count($iniFiles) > 1;
$tmpDir = sys_get_temp_dir();

if (!$this->cli) {
$error = 'Unsupported SAPI: '.PHP_SAPI;
} elseif (!$this->checkConfiguration($info)) {
$error = $info;
} elseif (!$this->checkMainScript()) {
$error = 'Unable to access main script: '.$this->script;
} elseif (!$this->writeTmpIni($iniFiles, $tmpDir, $error)) {
$error = $error !== null ? $error : 'Unable to create temp ini file at: '.$tmpDir;
} elseif (!$this->setEnvironment($scannedInis, $iniFiles)) {
$error = 'Unable to set environment variables';
if (!$this->writeTmpIni($tmpfile, $iniFiles, $error)) {
$this->notify(Status::ERROR, $error ?? $iniError);
@unlink($tmpfile);
return null;
}

if ($error !== null) {
$this->notify(Status::ERROR, $error);
if (!$this->setEnvironment($scannedInis, $iniFiles, $tmpfile)) {
$this->notify(Status::ERROR, 'Unable to set environment variables');
@unlink($tmpfile);
return null;
}

return $error === null;
$this->tmpIni = $tmpfile;

return $this->getCommand($argv, $tmpfile, $mainScript);
}

/**
* Returns true if the tmp ini file was written
*
* @param string[] $iniFiles All ini files used in the current process
* @param non-empty-list<string> $iniFiles All ini files used in the current process
*/
private function writeTmpIni(array $iniFiles, string $tmpDir, ?string &$error): bool
private function writeTmpIni(string $tmpFile, array $iniFiles, ?string &$error): bool
{
if (($tmpfile = @tempnam($tmpDir, '')) === false) {
return false;
}

$this->tmpIni = $tmpfile;

// $iniFiles has at least one item and it may be empty
if ($iniFiles[0] === '') {
array_shift($iniFiles);
Expand Down Expand Up @@ -400,35 +422,36 @@ private function writeTmpIni(array $iniFiles, string $tmpDir, ?string &$error):
// Work-around for https://bugs.php.net/bug.php?id=75932
$content .= 'opcache.enable_cli=0'.PHP_EOL;

return (bool) @file_put_contents($this->tmpIni, $content);
return (bool) @file_put_contents($tmpFile, $content);
}

/**
* Returns the command line arguments for the restart
*
* @return string[]
* @param non-empty-list<string> $argv
* @return non-empty-list<string>
*/
private function getCommand(): array
private function getCommand(array $argv, string $tmpIni, string $mainScript): array
{
$php = [PHP_BINARY];
$args = array_slice($_SERVER['argv'], 1);
$args = array_slice($argv, 1);

if (!$this->persistent) {
// Use command-line options
array_push($php, '-n', '-c', $this->tmpIni);
array_push($php, '-n', '-c', $tmpIni);
}

return array_merge($php, [$this->script], $args);
return array_merge($php, [$mainScript], $args);
}

/**
* Returns true if the restart environment variables were set
*
* No need to update $_SERVER since this is set in the restarted process.
*
* @param string[] $iniFiles All ini files used in the current process
* @param non-empty-list<string> $iniFiles All ini files used in the current process
*/
private function setEnvironment(bool $scannedInis, array $iniFiles): bool
private function setEnvironment(bool $scannedInis, array $iniFiles, string $tmpIni): bool
{
$scanDir = getenv('PHP_INI_SCAN_DIR');
$phprc = getenv('PHPRC');
Expand All @@ -440,7 +463,7 @@ private function setEnvironment(bool $scannedInis, array $iniFiles): bool

if ($this->persistent) {
// Use the environment to persist the settings
if (!putenv('PHP_INI_SCAN_DIR=') || !putenv('PHPRC='.$this->tmpIni)) {
if (!putenv('PHP_INI_SCAN_DIR=') || !putenv('PHPRC='.$tmpIni)) {
return false;
}
}
Expand Down Expand Up @@ -495,15 +518,17 @@ private function mergeLoadedConfig(array $loadedConfig, array $iniConfig): strin

/**
* Returns true if the script name can be used
*
* @param non-empty-list<string> $argv
*/
private function checkMainScript(): bool
private function checkMainScript(string &$mainScript, array $argv): bool
{
if ($this->script !== null) {
if ($mainScript !== '') {
// Allow an application to set -- for standard input
return file_exists($this->script) || '--' === $this->script;
return file_exists($mainScript) || '--' === $mainScript;
}

if (file_exists($this->script = $_SERVER['argv'][0])) {
if (file_exists($mainScript = $argv[0])) {
return true;
}

Expand All @@ -512,7 +537,7 @@ private function checkMainScript(): bool
$main = end($trace);

if ($main !== false && isset($main['file'])) {
return file_exists($this->script = $main['file']);
return file_exists($mainScript = $main['file']);
}

return false;
Expand All @@ -521,7 +546,7 @@ private function checkMainScript(): bool
/**
* Adds restart settings to the environment
*
* @param string[] $envArgs
* @param non-empty-list<string> $envArgs
*/
private function setEnvRestartSettings(array $envArgs): void
{
Expand Down Expand Up @@ -619,6 +644,28 @@ private function tryEnableSignals(): void
}
}

/**
* Returns $_SERVER['argv'] if it is as expected
*
* @return non-empty-list<string>|null
*/
private function checkServerArgv(): ?array
{
$result = [];

if (isset($_SERVER['argv']) && is_array($_SERVER['argv'])) {
foreach ($_SERVER['argv'] as $value) {
if (!is_string($value)) {
return null;
}

$result[] = $value;
}
}

return count($result) > 0 ? $result : null;
}

/**
* Sets static properties $xdebugActive, $xdebugVersion and $xdebugMode
*/
Expand Down
1 change: 1 addition & 0 deletions tests/Helpers/BaseTestCase.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ public static function setUpBeforeClass(): void
// Note $_SERVER will already match
}

// @phpstan-ignore-next-line
self::$argv = $_SERVER['argv'];
}

Expand Down
18 changes: 18 additions & 0 deletions tests/RestartTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,23 @@ public function testFailedRestart(): void
$this->checkRestart($xdebug);
}

public function testNoRestartWithUnexpectedArgv(): void
{
$loaded = true;

$_SERVER['argv'] = false;
$xdebug = CoreMock::createAndCheck($loaded);
$this->checkNoRestart($xdebug);

$_SERVER['argv'] = [];
$xdebug = CoreMock::createAndCheck($loaded);
$this->checkNoRestart($xdebug);

$_SERVER['argv'] = [1, 2];
$xdebug = CoreMock::createAndCheck($loaded);
$this->checkNoRestart($xdebug);
}

/**
* @dataProvider unreachableScriptProvider
*/
Expand Down Expand Up @@ -135,6 +152,7 @@ public function testRestartWithScriptSetter(string $script): void
public static function scriptSetterProvider(): array
{
return [
// @phpstan-ignore-next-line
[(string) realpath($_SERVER['argv'][0])],
['--'],
];
Expand Down

0 comments on commit 6282b17

Please sign in to comment.