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] 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(); } }