bug #17276 [Process] Fix potential race condition (nicolas-grekas)
This PR was merged into the 2.3 branch.
Discussion
----------
[Process] Fix potential race condition
| Q | A
| ------------- | ---
| Bug fix? | yes
| New feature? | no
| BC breaks? | no
| Deprecations? | no
| Tests pass? | yes
| Fixed tickets | -
| License | MIT
| Doc PR | -
As seen at https://travis-ci.org/symfony/symfony/jobs/100448880#L2768, ProcessTest::testRunProcessWithTimeout can create an infinite blocking situation.
I found two potential code paths that could be subject to a race condition. This fixes them.
Commits
-------
b114a85
[Process] Fix potential race condition
This commit is contained in:
commit
707d359c0c
@ -651,8 +651,12 @@ class Process
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
$this->updateStatus(false);
|
if ($this->isRunning()) {
|
||||||
if ($this->processInformation['running']) {
|
if (isset($this->fallbackStatus['pid'])) {
|
||||||
|
unset($this->fallbackStatus['pid']);
|
||||||
|
|
||||||
|
return $this->stop(0, $signal);
|
||||||
|
}
|
||||||
$this->close();
|
$this->close();
|
||||||
}
|
}
|
||||||
|
|
||||||
@ -1145,7 +1149,7 @@ class Process
|
|||||||
*/
|
*/
|
||||||
private function doSignal($signal, $throwException)
|
private function doSignal($signal, $throwException)
|
||||||
{
|
{
|
||||||
if (!$this->isRunning()) {
|
if (null === $pid = $this->getPid()) {
|
||||||
if ($throwException) {
|
if ($throwException) {
|
||||||
throw new LogicException('Can not send signal on a non running process.');
|
throw new LogicException('Can not send signal on a non running process.');
|
||||||
}
|
}
|
||||||
@ -1154,7 +1158,7 @@ class Process
|
|||||||
}
|
}
|
||||||
|
|
||||||
if ('\\' === DIRECTORY_SEPARATOR) {
|
if ('\\' === DIRECTORY_SEPARATOR) {
|
||||||
exec(sprintf('taskkill /F /T /PID %d 2>&1', $this->getPid()), $output, $exitCode);
|
exec(sprintf('taskkill /F /T /PID %d 2>&1', $pid), $output, $exitCode);
|
||||||
if ($exitCode && $this->isRunning()) {
|
if ($exitCode && $this->isRunning()) {
|
||||||
if ($throwException) {
|
if ($throwException) {
|
||||||
throw new RuntimeException(sprintf('Unable to kill the process (%s).', implode(' ', $output)));
|
throw new RuntimeException(sprintf('Unable to kill the process (%s).', implode(' ', $output)));
|
||||||
@ -1166,8 +1170,8 @@ class Process
|
|||||||
if (!$this->enhanceSigchildCompatibility || !$this->isSigchildEnabled()) {
|
if (!$this->enhanceSigchildCompatibility || !$this->isSigchildEnabled()) {
|
||||||
$ok = @proc_terminate($this->process, $signal);
|
$ok = @proc_terminate($this->process, $signal);
|
||||||
} elseif (function_exists('posix_kill')) {
|
} elseif (function_exists('posix_kill')) {
|
||||||
$ok = @posix_kill($this->getPid(), $signal);
|
$ok = @posix_kill($pid, $signal);
|
||||||
} elseif ($ok = proc_open(sprintf('kill -%d %d', $signal, $this->getPid()), array(2 => array('pipe', 'w')), $pipes)) {
|
} elseif ($ok = proc_open(sprintf('kill -%d %d', $signal, $pid), array(2 => array('pipe', 'w')), $pipes)) {
|
||||||
$ok = false === fgets($pipes[2]);
|
$ok = false === fgets($pipes[2]);
|
||||||
}
|
}
|
||||||
if (!$ok) {
|
if (!$ok) {
|
||||||
|
@ -685,22 +685,16 @@ class ProcessTest extends \PHPUnit_Framework_TestCase
|
|||||||
*/
|
*/
|
||||||
public function testRunProcessWithTimeout()
|
public function testRunProcessWithTimeout()
|
||||||
{
|
{
|
||||||
$timeout = 0.1;
|
$process = $this->getProcess(self::$phpBin.' -r "sleep(30);"');
|
||||||
$process = $this->getProcess(self::$phpBin.' -r "sleep(1);"');
|
$process->setTimeout(0.1);
|
||||||
$process->setTimeout($timeout);
|
|
||||||
$start = microtime(true);
|
$start = microtime(true);
|
||||||
try {
|
try {
|
||||||
$process->run();
|
$process->run();
|
||||||
$this->fail('A RuntimeException should have been raised');
|
$this->fail('A RuntimeException should have been raised');
|
||||||
} catch (RuntimeException $e) {
|
} catch (RuntimeException $e) {
|
||||||
}
|
}
|
||||||
$duration = microtime(true) - $start;
|
|
||||||
|
|
||||||
if ('\\' !== DIRECTORY_SEPARATOR) {
|
$this->assertLessThan(15, microtime(true) - $start);
|
||||||
// On Windows, timers are too transient
|
|
||||||
$maxDuration = $timeout + 2 * Process::TIMEOUT_PRECISION;
|
|
||||||
$this->assertLessThan($maxDuration, $duration);
|
|
||||||
}
|
|
||||||
|
|
||||||
throw $e;
|
throw $e;
|
||||||
}
|
}
|
||||||
|
Reference in New Issue
Block a user