Skip to content

Commit f1dc82f

Browse files
clueclue
authored
Merge pull request #10 from clue-labs/fds
Improve platform support (chroot environments, Mac and others) and do not inherit open FDs to SSH child process by overwriting and closing
2 parents 7ac2c24 + 52d1386 commit f1dc82f

7 files changed

+168
-56
lines changed

‎composer.json‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,12 +11,13 @@
1111
}
1212
],
1313
"autoload": {
14-
"psr-4": { "Clue\\React\\SshProxy\\": "src/" }
14+
"psr-4": { "Clue\\React\\SshProxy\\": "src/" },
15+
"files": [ "src/Io/functions.php" ]
1516
},
1617
"require": {
1718
"php": ">=5.3",
1819
"clue/socks-react": "^1.0",
19-
"react/child-process": "^0.5",
20+
"react/child-process": "^0.6",
2021
"react/event-loop": "^1.0 || ^0.5",
2122
"react/promise": "^2.1 || ^1.2.1",
2223
"react/socket": "^1.1",

‎src/Io/functions.php‎

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,70 @@
1+
<?php
2+
3+
namespace Clue\React\SshProxy\Io;
4+
5+
use React\ChildProcess\Process;
6+
7+
/**
8+
* Returns a list of active file descriptors (may contain bogus entries)
9+
*
10+
* @param string $path
11+
* @return array
12+
* @internal
13+
*/
14+
function fds($path = '/dev/fd')
15+
{
16+
// Try to get list of all open FDs (Linux/Mac and others)
17+
$fds = @\scandir($path);
18+
19+
// Otherwise try temporarily duplicating file descriptors in the range 0-1024 (FD_SETSIZE).
20+
// This is known to work on more exotic platforms and also inside chroot
21+
// environments without /dev/fd. Causes many syscalls, but still rather fast.
22+
if ($fds === false) {
23+
$fds = array();
24+
for ($i = 0; $i <= 1024; ++$i) {
25+
$copy = @\fopen('php://fd/' . $i, 'r');
26+
if ($copy !== false) {
27+
$fds[] = $i;
28+
\fclose($copy);
29+
}
30+
}
31+
}
32+
33+
return $fds;
34+
}
35+
36+
/**
37+
* Creates a Process with the given command modified in such a way that any additional FDs are explicitly not passed along
38+
*
39+
* @param string $command
40+
* @return Process
41+
* @internal
42+
*/
43+
function processWithoutFds($command)
44+
{
45+
// launch process with default STDIO pipes
46+
$pipes = array(
47+
array('pipe', 'r'),
48+
array('pipe', 'w'),
49+
array('pipe', 'w')
50+
);
51+
52+
// try to get list of all open FDs
53+
$fds = fds();
54+
55+
// do not inherit open FDs by explicitly overwriting existing FDs with dummy files
56+
// additionally, close all dummy files in the child process again
57+
foreach ($fds as $fd) {
58+
if ($fd > 2) {
59+
$pipes[$fd] = array('file', '/dev/null', 'r');
60+
$command .= ' ' . $fd . '>&-';
61+
}
62+
}
63+
64+
// default `sh` only accepts single-digit FDs, so run in bash if needed
65+
if ($fds && max($fds) > 9) {
66+
$command = 'exec bash -c ' . escapeshellarg($command);
67+
}
68+
69+
return new Process($command, null, null, $pipes);
70+
}

‎src/SshProcessConnector.php‎

Lines changed: 1 addition & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@
55
use Clue\React\SshProxy\Io\CompositeConnection;
66
use Clue\React\SshProxy\Io\LineSeparatedReader;
77
use React\EventLoop\LoopInterface;
8-
use React\ChildProcess\Process;
98
use React\Promise\Deferred;
109
use React\Socket\ConnectorInterface;
1110

@@ -67,26 +66,7 @@ public function connect($uri)
6766
}
6867

6968
$command = $this->cmd . ' -W ' . \escapeshellarg($parts['host'] . ':' . $parts['port']);
70-
71-
// try to get list of all open FDs (Linux only) or simply assume range 3-1024 (FD_SETSIZE)
72-
$fds = @scandir('/proc/self/fd');
73-
if ($fds === false) {
74-
$fds = range(3, 1024); // @codeCoverageIgnore
75-
}
76-
77-
// do not inherit open FDs by explicitly closing all of them
78-
foreach ($fds as $fd) {
79-
if ($fd > 2) {
80-
$command .= ' ' . $fd . '>&-';
81-
}
82-
}
83-
84-
// default `sh` only accepts single-digit FDs, so run in bash if needed
85-
if ($fds && max($fds) > 9) {
86-
$command = 'exec bash -c ' . escapeshellarg($command);
87-
}
88-
89-
$process = new Process($command);
69+
$process = Io\processWithoutFds($command);
9070
$process->start($this->loop);
9171

9272
$deferred = new Deferred(function () use ($process, $uri) {

‎src/SshSocksConnector.php‎

Lines changed: 1 addition & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -195,27 +195,7 @@ public function idle()
195195

196196
private function listen()
197197
{
198-
$command = $this->cmd;
199-
200-
// try to get list of all open FDs (Linux only) or simply assume range 3-1024 (FD_SETSIZE)
201-
$fds = @scandir('/proc/self/fd');
202-
if ($fds === false) {
203-
$fds = range(3, 1024); // @codeCoverageIgnore
204-
}
205-
206-
// do not inherit open FDs by explicitly closing all of them
207-
foreach ($fds as $fd) {
208-
if ($fd > 2) {
209-
$command .= ' ' . $fd . '>&-';
210-
}
211-
}
212-
213-
// default `sh` only accepts single-digit FDs, so run in bash if needed
214-
if ($fds && max($fds) > 9) {
215-
$command = 'exec bash -c ' . escapeshellarg($command);
216-
}
217-
218-
$process = new Process($command);
198+
$process = Io\processWithoutFds($this->cmd);
219199
$process->start($this->loop);
220200

221201
$deferred = new Deferred(function () use ($process) {

‎tests/FunctionalSshProcessConnectorTest.php‎

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@ public function testConnectValidTargetWillReturnPromiseWhichResolvesToConnection
6969
$connection->close();
7070
}
7171

72-
public function testConnectValidTargetWillNotInheritActiveFileDescriptors()
72+
public function testConnectPendingWillNotInheritActiveFileDescriptors()
7373
{
7474
$server = stream_socket_server('tcp://127.0.0.1:0');
7575
$address = stream_socket_get_name($server, false);
@@ -83,17 +83,15 @@ public function testConnectValidTargetWillNotInheritActiveFileDescriptors()
8383
$this->markTestSkipped('Platform does not prevent binding to same address (Windows?)');
8484
}
8585

86-
// wait for successful connection
8786
$promise = $this->connector->connect('example.com:80');
88-
$connection = \Clue\React\Block\await($promise, $this->loop, self::TIMEOUT);
8987

9088
// close server and ensure we can start a new server on the previous address
91-
// the open SSH connection process should not inherit the existing server socket
89+
// the pending SSH connection process should not inherit the existing server socket
9290
fclose($server);
9391
$server = stream_socket_server('tcp://' . $address);
9492
$this->assertTrue(is_resource($server));
95-
9693
fclose($server);
97-
$connection->close();
94+
95+
$promise->cancel();
9896
}
9997
}

‎tests/FunctionalSshSocksConnectorTest.php‎

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -89,7 +89,7 @@ public function testConnectValidTargetWillReturnPromiseWhichResolvesToConnection
8989
$connection->close();
9090
}
9191

92-
public function testConnectValidTargetWillNotInheritActiveFileDescriptors()
92+
public function testConnectPendingWillNotInheritActiveFileDescriptors()
9393
{
9494
$server = stream_socket_server('tcp://127.0.0.1:0');
9595
$address = stream_socket_get_name($server, false);
@@ -103,17 +103,15 @@ public function testConnectValidTargetWillNotInheritActiveFileDescriptors()
103103
$this->markTestSkipped('Platform does not prevent binding to same address (Windows?)');
104104
}
105105

106-
// wait for successful connection
107106
$promise = $this->connector->connect('example.com:80');
108-
$connection = \Clue\React\Block\await($promise, $this->loop, self::TIMEOUT);
109107

110108
// close server and ensure we can start a new server on the previous address
111-
// the open SSH connection process should not inherit the existing server socket
109+
// the pending SSH connection process should not inherit the existing server socket
112110
fclose($server);
113111
$server = stream_socket_server('tcp://' . $address);
114112
$this->assertTrue(is_resource($server));
115-
116113
fclose($server);
117-
$connection->close();
114+
115+
$promise->cancel();
118116
}
119117
}

‎tests/Io/FunctionsTest.php‎

Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,85 @@
1+
<?php
2+
3+
use Clue\React\SshProxy\Io;
4+
use PHPUnit\Framework\TestCase;
5+
6+
class FunctionsTest extends TestCase
7+
{
8+
public function testFdsReturnsArray()
9+
{
10+
$fds = Io\fds();
11+
12+
$this->assertInternalType('array', $fds);
13+
}
14+
15+
public function testFdsReturnsArrayWithStdioHandles()
16+
{
17+
// skip when running with closed handles: vendor/bin/phpunit 0<&-
18+
if (!defined('STDIN') || !defined('STDOUT') || !defined('STDERR') || !@fstat(STDIN) || !@fstat(STDOUT) || !@fstat(STDERR)) {
19+
$this->markTestSkipped('Test suite does not appear to run with standard I/O handles');
20+
}
21+
22+
$fds = Io\fds();
23+
24+
$this->assertContains(0, $fds);
25+
$this->assertContains(1, $fds);
26+
$this->assertContains(2, $fds);
27+
}
28+
29+
public function testFdsReturnsSameArrayTwice()
30+
{
31+
$fds = Io\fds();
32+
$second = Io\fds();
33+
34+
$this->assertEquals($fds, $second);
35+
}
36+
37+
public function testFdsWithInvalidPathReturnsArray()
38+
{
39+
$fds = Io\fds('/dev/null');
40+
41+
$this->assertInternalType('array', $fds);
42+
}
43+
44+
public function testFdsWithInvalidPathReturnsSubsetOfFdsFromDevFd()
45+
{
46+
if (@scandir('/dev/fd') === false) {
47+
$this->markTestSkipped('Unable to read /dev/fd');
48+
}
49+
50+
$fds = Io\fds();
51+
$second = Io\fds('/dev/null');
52+
53+
foreach ($second as $one) {
54+
$this->assertContains($one, $fds);
55+
}
56+
}
57+
58+
public function testProcessWithoutFdsReturnsProcessWithoutClosingDefaultHandles()
59+
{
60+
$process = Io\processWithoutFds('sleep 10');
61+
62+
$this->assertInstanceOf('React\ChildProcess\Process', $process);
63+
64+
$this->assertNotContains(' 0>&-', $process->getCommand());
65+
$this->assertNotContains(' 1>&-', $process->getCommand());
66+
$this->assertNotContains(' 2>&-', $process->getCommand());
67+
}
68+
69+
public function testProcessWithoutFdsReturnsProcessWithOriginalCommandPartOfActualCommandWhenDescriptorsNeedToBeClosed()
70+
{
71+
// skip when running with closed handles: vendor/bin/phpunit 0<&-
72+
// bypass for example with dummy handles: vendor/bin/phpunit 8<&-
73+
$fds = Io\fds();
74+
if (!$fds || max($fds) < 3) {
75+
$this->markTestSkipped('Did not detect additional file descriptors to be closed');
76+
}
77+
78+
$process = Io\processWithoutFds('sleep 10');
79+
80+
$this->assertInstanceOf('React\ChildProcess\Process', $process);
81+
82+
$this->assertNotEquals('sleep 10', $process->getCommand());
83+
$this->assertContains('sleep 10', $process->getCommand());
84+
}
85+
}

0 commit comments

Comments
 (0)