From 2da0bdf2f0e0ee026284f44d95f58666bfab798b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20L=C3=BCck?= Date: Sat, 19 Jan 2019 15:37:53 +0100 Subject: [PATCH 1/4] Internal refactoring to reuse file descriptor close logic --- composer.json | 3 +- src/Io/functions.php | 47 ++++++++++++++++++++++++ src/SshProcessConnector.php | 22 +----------- src/SshSocksConnector.php | 22 +----------- tests/Io/FunctionsTest.php | 71 +++++++++++++++++++++++++++++++++++++ 5 files changed, 122 insertions(+), 43 deletions(-) create mode 100644 src/Io/functions.php create mode 100644 tests/Io/FunctionsTest.php diff --git a/composer.json b/composer.json index edc14fe..e9f9638 100644 --- a/composer.json +++ b/composer.json @@ -11,7 +11,8 @@ } ], "autoload": { - "psr-4": { "Clue\\React\\SshProxy\\": "src/" } + "psr-4": { "Clue\\React\\SshProxy\\": "src/" }, + "files": [ "src/Io/functions.php" ] }, "require": { "php": ">=5.3", diff --git a/src/Io/functions.php b/src/Io/functions.php new file mode 100644 index 0000000..57f6614 --- /dev/null +++ b/src/Io/functions.php @@ -0,0 +1,47 @@ + 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); + } + + return new Process($command); +} 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/Io/FunctionsTest.php b/tests/Io/FunctionsTest.php new file mode 100644 index 0000000..5137f54 --- /dev/null +++ b/tests/Io/FunctionsTest.php @@ -0,0 +1,71 @@ +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 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()); + } +} From 27ccbdd4139e00e4234f526990cb08d645abce33 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20L=C3=BCck?= Date: Thu, 6 Dec 2018 20:02:34 +0100 Subject: [PATCH 2/4] Improve platform support by checking /dev/fd instead of /proc/self/fd The virtual /proc/self/fd is Linux-only, while the virtual /dev/fd is available on more platforms. On Linux, the latter is simply a symlink to the former, so this shouldn't affect existing installations. --- src/Io/functions.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Io/functions.php b/src/Io/functions.php index 57f6614..45fd8a4 100644 --- a/src/Io/functions.php +++ b/src/Io/functions.php @@ -11,9 +11,9 @@ * @return array * @internal */ -function fds($path = '/proc/self/fd') +function fds($path = '/dev/fd') { - // try to get list of all open FDs (Linux only) or simply assume range 0-1024 (FD_SETSIZE) + // try to get list of all open FDs (Linux/Mac and others) or simply assume range 0-1024 (FD_SETSIZE) $fds = @\scandir($path); return $fds !== false ? $fds : \range(0, 1024); From e7512e6d58ac1203093dda4312cb566bca263dfd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20L=C3=BCck?= Date: Thu, 6 Dec 2018 12:22:17 +0100 Subject: [PATCH 3/4] Improve platform support by checking all available FDs as fallback If we can not read from /dev/fd (which is available on Linux, Mac and many others), we otherwise try temporarily duplicating file descriptors in the range 0-1024 (FD_SETSIZE) to see which one is currently in use. This is known to work on more exotic platforms and also inside chroot environments without /dev/fd. Causes many syscalls, but still rather fast. --- src/Io/functions.php | 18 ++++++++++++++++-- tests/Io/FunctionsTest.php | 20 +++++++++++++++++--- 2 files changed, 33 insertions(+), 5 deletions(-) diff --git a/src/Io/functions.php b/src/Io/functions.php index 45fd8a4..ab01d6b 100644 --- a/src/Io/functions.php +++ b/src/Io/functions.php @@ -13,10 +13,24 @@ */ function fds($path = '/dev/fd') { - // try to get list of all open FDs (Linux/Mac and others) or simply assume range 0-1024 (FD_SETSIZE) + // Try to get list of all open FDs (Linux/Mac and others) $fds = @\scandir($path); - return $fds !== false ? $fds : \range(0, 1024); + // Otherwise try temporarily duplicating file descriptors in the range 0-1024 (FD_SETSIZE). + // This is known to work on more exotic platforms and also inside chroot + // environments without /dev/fd. Causes many syscalls, but still rather fast. + if ($fds === false) { + $fds = array(); + for ($i = 0; $i <= 1024; ++$i) { + $copy = @\fopen('php://fd/' . $i, 'r'); + if ($copy !== false) { + $fds[] = $i; + \fclose($copy); + } + } + } + + return $fds; } /** diff --git a/tests/Io/FunctionsTest.php b/tests/Io/FunctionsTest.php index 5137f54..494be8e 100644 --- a/tests/Io/FunctionsTest.php +++ b/tests/Io/FunctionsTest.php @@ -41,15 +41,29 @@ public function testFdsWithInvalidPathReturnsArray() $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()); + $this->assertNotContains(' 0>&-', $process->getCommand()); + $this->assertNotContains(' 1>&-', $process->getCommand()); + $this->assertNotContains(' 2>&-', $process->getCommand()); } public function testProcessWithoutFdsReturnsProcessWithOriginalCommandPartOfActualCommandWhenDescriptorsNeedToBeClosed() From 52d1386d4f83017f66415358d8b44d0ccf5a535c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20L=C3=BCck?= Date: Mon, 14 Jan 2019 23:58:36 +0100 Subject: [PATCH 4/4] Do not inherit open FDs to SSH child process by overwriting and closing This fixes a possible race condition where open FDs where in fact inherited to the wrapping shell before it had a chance to close them again when it is being replaced with the actual SSH binary. This builds on top of https://github.com/reactphp/child-process/pull/65 --- composer.json | 2 +- src/Io/functions.php | 13 +++++++++++-- tests/FunctionalSshProcessConnectorTest.php | 10 ++++------ tests/FunctionalSshSocksConnectorTest.php | 10 ++++------ 4 files changed, 20 insertions(+), 15 deletions(-) diff --git a/composer.json b/composer.json index e9f9638..61633dd 100644 --- a/composer.json +++ b/composer.json @@ -17,7 +17,7 @@ "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 index ab01d6b..0f2eb8f 100644 --- a/src/Io/functions.php +++ b/src/Io/functions.php @@ -42,12 +42,21 @@ function fds($path = '/dev/fd') */ function processWithoutFds($command) { + // launch process with default STDIO pipes + $pipes = array( + array('pipe', 'r'), + array('pipe', 'w'), + array('pipe', 'w') + ); + // try to get list of all open FDs $fds = fds(); - // do not inherit open FDs by explicitly closing all of them + // do not inherit open FDs by explicitly overwriting existing FDs with dummy files + // additionally, close all dummy files in the child process again foreach ($fds as $fd) { if ($fd > 2) { + $pipes[$fd] = array('file', '/dev/null', 'r'); $command .= ' ' . $fd . '>&-'; } } @@ -57,5 +66,5 @@ function processWithoutFds($command) $command = 'exec bash -c ' . escapeshellarg($command); } - return new Process($command); + return new Process($command, null, null, $pipes); } 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(); } }