Skip to content

Commit

Permalink
Do not inherit open FDs to SSH child process by overwriting and closing
Browse files Browse the repository at this point in the history
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 reactphp/child-process#65
  • Loading branch information
clue committed Jan 19, 2019
1 parent e7512e6 commit 52d1386
Show file tree
Hide file tree
Showing 4 changed files with 20 additions and 15 deletions.
2 changes: 1 addition & 1 deletion composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
13 changes: 11 additions & 2 deletions src/Io/functions.php
Original file line number Diff line number Diff line change
Expand Up @@ -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 . '>&-';
}
}
Expand All @@ -57,5 +66,5 @@ function processWithoutFds($command)
$command = 'exec bash -c ' . escapeshellarg($command);
}

return new Process($command);
return new Process($command, null, null, $pipes);
}
10 changes: 4 additions & 6 deletions tests/FunctionalSshProcessConnectorTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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();
}
}
10 changes: 4 additions & 6 deletions tests/FunctionalSshSocksConnectorTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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();
}
}

0 comments on commit 52d1386

Please sign in to comment.