diff --git a/composer.json b/composer.json index edc14fe..61633dd 100644 --- a/composer.json +++ b/composer.json @@ -11,12 +11,13 @@ } ], "autoload": { - "psr-4": { "Clue\\React\\SshProxy\\": "src/" } + "psr-4": { "Clue\\React\\SshProxy\\": "src/" }, + "files": [ "src/Io/functions.php" ] }, "require": { "php": ">=5.3", "clue/socks-react": "^1.0", - "react/child-process": "^0.5", + "react/child-process": "^0.6", "react/event-loop": "^1.0 || ^0.5", "react/promise": "^2.1 || ^1.2.1", "react/socket": "^1.1", diff --git a/src/Io/functions.php b/src/Io/functions.php new file mode 100644 index 0000000..0f2eb8f --- /dev/null +++ b/src/Io/functions.php @@ -0,0 +1,70 @@ + 2) { + $pipes[$fd] = array('file', '/dev/null', 'r'); + $command .= ' ' . $fd . '>&-'; + } + } + + // default `sh` only accepts single-digit FDs, so run in bash if needed + if ($fds && max($fds) > 9) { + $command = 'exec bash -c ' . escapeshellarg($command); + } + + return new Process($command, null, null, $pipes); +} diff --git a/src/SshProcessConnector.php b/src/SshProcessConnector.php index 6a50c5a..e9b212e 100644 --- a/src/SshProcessConnector.php +++ b/src/SshProcessConnector.php @@ -5,7 +5,6 @@ use Clue\React\SshProxy\Io\CompositeConnection; use Clue\React\SshProxy\Io\LineSeparatedReader; use React\EventLoop\LoopInterface; -use React\ChildProcess\Process; use React\Promise\Deferred; use React\Socket\ConnectorInterface; @@ -67,26 +66,7 @@ public function connect($uri) } $command = $this->cmd . ' -W ' . \escapeshellarg($parts['host'] . ':' . $parts['port']); - - // try to get list of all open FDs (Linux only) or simply assume range 3-1024 (FD_SETSIZE) - $fds = @scandir('/proc/self/fd'); - if ($fds === false) { - $fds = range(3, 1024); // @codeCoverageIgnore - } - - // do not inherit open FDs by explicitly closing all of them - foreach ($fds as $fd) { - if ($fd > 2) { - $command .= ' ' . $fd . '>&-'; - } - } - - // default `sh` only accepts single-digit FDs, so run in bash if needed - if ($fds && max($fds) > 9) { - $command = 'exec bash -c ' . escapeshellarg($command); - } - - $process = new Process($command); + $process = Io\processWithoutFds($command); $process->start($this->loop); $deferred = new Deferred(function () use ($process, $uri) { diff --git a/src/SshSocksConnector.php b/src/SshSocksConnector.php index 62c3e77..064b673 100644 --- a/src/SshSocksConnector.php +++ b/src/SshSocksConnector.php @@ -195,27 +195,7 @@ public function idle() private function listen() { - $command = $this->cmd; - - // try to get list of all open FDs (Linux only) or simply assume range 3-1024 (FD_SETSIZE) - $fds = @scandir('/proc/self/fd'); - if ($fds === false) { - $fds = range(3, 1024); // @codeCoverageIgnore - } - - // do not inherit open FDs by explicitly closing all of them - foreach ($fds as $fd) { - if ($fd > 2) { - $command .= ' ' . $fd . '>&-'; - } - } - - // default `sh` only accepts single-digit FDs, so run in bash if needed - if ($fds && max($fds) > 9) { - $command = 'exec bash -c ' . escapeshellarg($command); - } - - $process = new Process($command); + $process = Io\processWithoutFds($this->cmd); $process->start($this->loop); $deferred = new Deferred(function () use ($process) { diff --git a/tests/FunctionalSshProcessConnectorTest.php b/tests/FunctionalSshProcessConnectorTest.php index 268f0f7..553595e 100644 --- a/tests/FunctionalSshProcessConnectorTest.php +++ b/tests/FunctionalSshProcessConnectorTest.php @@ -69,7 +69,7 @@ public function testConnectValidTargetWillReturnPromiseWhichResolvesToConnection $connection->close(); } - public function testConnectValidTargetWillNotInheritActiveFileDescriptors() + public function testConnectPendingWillNotInheritActiveFileDescriptors() { $server = stream_socket_server('tcp://127.0.0.1:0'); $address = stream_socket_get_name($server, false); @@ -83,17 +83,15 @@ public function testConnectValidTargetWillNotInheritActiveFileDescriptors() $this->markTestSkipped('Platform does not prevent binding to same address (Windows?)'); } - // wait for successful connection $promise = $this->connector->connect('example.com:80'); - $connection = \Clue\React\Block\await($promise, $this->loop, self::TIMEOUT); // close server and ensure we can start a new server on the previous address - // the open SSH connection process should not inherit the existing server socket + // the pending SSH connection process should not inherit the existing server socket fclose($server); $server = stream_socket_server('tcp://' . $address); $this->assertTrue(is_resource($server)); - fclose($server); - $connection->close(); + + $promise->cancel(); } } diff --git a/tests/FunctionalSshSocksConnectorTest.php b/tests/FunctionalSshSocksConnectorTest.php index 3c746a3..2379ade 100644 --- a/tests/FunctionalSshSocksConnectorTest.php +++ b/tests/FunctionalSshSocksConnectorTest.php @@ -89,7 +89,7 @@ public function testConnectValidTargetWillReturnPromiseWhichResolvesToConnection $connection->close(); } - public function testConnectValidTargetWillNotInheritActiveFileDescriptors() + public function testConnectPendingWillNotInheritActiveFileDescriptors() { $server = stream_socket_server('tcp://127.0.0.1:0'); $address = stream_socket_get_name($server, false); @@ -103,17 +103,15 @@ public function testConnectValidTargetWillNotInheritActiveFileDescriptors() $this->markTestSkipped('Platform does not prevent binding to same address (Windows?)'); } - // wait for successful connection $promise = $this->connector->connect('example.com:80'); - $connection = \Clue\React\Block\await($promise, $this->loop, self::TIMEOUT); // close server and ensure we can start a new server on the previous address - // the open SSH connection process should not inherit the existing server socket + // the pending SSH connection process should not inherit the existing server socket fclose($server); $server = stream_socket_server('tcp://' . $address); $this->assertTrue(is_resource($server)); - fclose($server); - $connection->close(); + + $promise->cancel(); } } diff --git a/tests/Io/FunctionsTest.php b/tests/Io/FunctionsTest.php new file mode 100644 index 0000000..494be8e --- /dev/null +++ b/tests/Io/FunctionsTest.php @@ -0,0 +1,85 @@ +assertInternalType('array', $fds); + } + + public function testFdsReturnsArrayWithStdioHandles() + { + // skip when running with closed handles: vendor/bin/phpunit 0<&- + if (!defined('STDIN') || !defined('STDOUT') || !defined('STDERR') || !@fstat(STDIN) || !@fstat(STDOUT) || !@fstat(STDERR)) { + $this->markTestSkipped('Test suite does not appear to run with standard I/O handles'); + } + + $fds = Io\fds(); + + $this->assertContains(0, $fds); + $this->assertContains(1, $fds); + $this->assertContains(2, $fds); + } + + public function testFdsReturnsSameArrayTwice() + { + $fds = Io\fds(); + $second = Io\fds(); + + $this->assertEquals($fds, $second); + } + + public function testFdsWithInvalidPathReturnsArray() + { + $fds = Io\fds('/dev/null'); + + $this->assertInternalType('array', $fds); + } + + public function testFdsWithInvalidPathReturnsSubsetOfFdsFromDevFd() + { + if (@scandir('/dev/fd') === false) { + $this->markTestSkipped('Unable to read /dev/fd'); + } + + $fds = Io\fds(); + $second = Io\fds('/dev/null'); + + foreach ($second as $one) { + $this->assertContains($one, $fds); + } + } + + public function testProcessWithoutFdsReturnsProcessWithoutClosingDefaultHandles() + { + $process = Io\processWithoutFds('sleep 10'); + + $this->assertInstanceOf('React\ChildProcess\Process', $process); + + $this->assertNotContains(' 0>&-', $process->getCommand()); + $this->assertNotContains(' 1>&-', $process->getCommand()); + $this->assertNotContains(' 2>&-', $process->getCommand()); + } + + public function testProcessWithoutFdsReturnsProcessWithOriginalCommandPartOfActualCommandWhenDescriptorsNeedToBeClosed() + { + // skip when running with closed handles: vendor/bin/phpunit 0<&- + // bypass for example with dummy handles: vendor/bin/phpunit 8<&- + $fds = Io\fds(); + if (!$fds || max($fds) < 3) { + $this->markTestSkipped('Did not detect additional file descriptors to be closed'); + } + + $process = Io\processWithoutFds('sleep 10'); + + $this->assertInstanceOf('React\ChildProcess\Process', $process); + + $this->assertNotEquals('sleep 10', $process->getCommand()); + $this->assertContains('sleep 10', $process->getCommand()); + } +}