Skip to content

Commit 08e32e2

Browse files
clueclue
authored
Merge pull request #54 from clue-labs/socks-race
Fix race conditions and stale SSH client process in `SshSocksConnector`
2 parents 24f6702 + bab5249 commit 08e32e2

2 files changed

Lines changed: 173 additions & 13 deletions

File tree

‎src/SshSocksConnector.php‎

Lines changed: 20 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -105,13 +105,8 @@ public function connect($uri)
105105
}
106106

107107
// no listening SSH client process started? start one and remember reference
108-
// if spawning SSH client process fails, clear reference for next start
109108
if ($this->listen === null) {
110-
$listen =& $this->listen;
111-
$this->listen = $this->listen()->then(null, function ($e) use (&$listen) {
112-
$listen = null;
113-
throw $e;
114-
});
109+
$this->listen = $this->listen();
115110

116111
// @codeCoverageIgnoreStart
117112
if ($this->debug) {
@@ -146,8 +141,12 @@ public function connect($uri)
146141
throw new \RuntimeException('Connection to ' . $uri . ' cancelled while waiting for SSH client');
147142
});
148143

149-
$this->listen->then(function () use ($uri, $that, $socks, $deferred, &$connecting) {
150-
// SSH client process started => start SOCKS connection
144+
$this->listen->then(function () use ($uri, $that, $socks, $deferred, &$connecting, &$cancelled) {
145+
// SSH client process started => start SOCKS connection (unless connection attempt has already been cancelled)
146+
if ($cancelled) {
147+
return;
148+
}
149+
151150
$connecting = $socks->connect($uri)->then(function (ConnectionInterface $connection) use ($that, $deferred) {
152151
// connection to SOCKS server successful => consider idle when connection closes again
153152
$connection->on('close', function () use ($that) {
@@ -194,14 +193,15 @@ public function idle()
194193

195194
if ($this->pending < 1 && $this->idleTimer === null && $this->listen !== null) {
196195
$listen =& $this->listen;
196+
$promise = $this->listen;
197197
$debug = $this->debug;
198-
$this->idleTimer = $this->loop->addTimer($this->idlePeriod, function () use (&$listen, $debug) {
199-
$listen->then(function (Process $process) {
198+
$this->idleTimer = $this->loop->addTimer($this->idlePeriod, function () use (&$listen, $promise, $debug) {
199+
$promise->then(function (Process $process) {
200200
$process->terminate();
201201
}, function () {
202202
// ignore to avoid reporting unhandled rejection
203203
});
204-
$listen->cancel();
204+
$promise->cancel();
205205
$listen = null;
206206

207207
// @codeCoverageIgnoreStart
@@ -255,13 +255,20 @@ private function listen()
255255
$process->terminate();
256256
});
257257

258-
$process->on('exit', function ($code) use ($deferred, &$last) {
258+
// if SSH client process exits, clear reference for next start unless already replaced
259+
$listen =& $this->listen;
260+
$promise = $deferred->promise();
261+
$process->on('exit', function ($code) use ($deferred, &$last, &$listen, $promise) {
262+
if ($listen === $promise) {
263+
$listen = null;
264+
}
265+
259266
$deferred->reject(new \RuntimeException(
260267
$last,
261268
(int) $code
262269
));
263270
});
264271

265-
return $deferred->promise();
272+
return $promise;
266273
}
267274
}

‎tests/IntegrationSshSocksConnectorTest.php‎

Lines changed: 153 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -136,6 +136,159 @@ public function testConnectCancellationWillRejectWithException()
136136
Loop::run();
137137
}
138138

139+
public function testConnectCancellationWillRejectWithExceptionWhenProcessExitsBeforeIdleTimeout()
140+
{
141+
$connector = new SshSocksConnector('host');
142+
143+
$ref = new \ReflectionProperty($connector, 'cmd');
144+
if (PHP_VERSION_ID < 80100) {
145+
$ref->setAccessible(true);
146+
}
147+
$ref->setValue($connector, 'exec false; #');
148+
149+
// delay idle timer, so process exit is reported before idle timer fires
150+
$ref = new \ReflectionProperty($connector, 'idlePeriod');
151+
if (PHP_VERSION_ID < 80100) {
152+
$ref->setAccessible(true);
153+
}
154+
$ref->setValue($connector, 0.2);
155+
156+
$promise = $connector->connect('example.com:80');
157+
$promise->cancel();
158+
159+
$promise->then(null, $this->expectCallableOnceWith($this->isInstanceOf('RuntimeException')));
160+
161+
Loop::run();
162+
}
163+
164+
public function testConnectCancellationWillNotInvokeSocksConnectorWhenProcessBecomesReadyForOtherPendingConnection()
165+
{
166+
$connector = new SshSocksConnector('host');
167+
168+
$ref = new \ReflectionProperty($connector, 'cmd');
169+
if (PHP_VERSION_ID < 80100) {
170+
$ref->setAccessible(true);
171+
}
172+
$ref->setValue($connector, 'echo "debug1: channel 0: new [port listener]" >&2; exec sleep 10; #');
173+
174+
$socks = $this->getMockBuilder('React\Socket\ConnectorInterface')->getMock();
175+
$socks->expects($this->once())->method('connect')->with('example.com:81')->willReturn(\React\Promise\reject(new \RuntimeException('Connection refused')));
176+
177+
$ref = new \ReflectionProperty($connector, 'socks');
178+
if (PHP_VERSION_ID < 80100) {
179+
$ref->setAccessible(true);
180+
}
181+
$ref->setValue($connector, $socks);
182+
183+
$first = $connector->connect('example.com:80');
184+
$first->cancel();
185+
186+
$first->then(null, $this->expectCallableOnceWith($this->isInstanceOf('RuntimeException')));
187+
188+
$second = $connector->connect('example.com:81');
189+
$second->then(null, $this->expectCallableOnceWith($this->isInstanceOf('RuntimeException')));
190+
191+
Loop::run();
192+
}
193+
194+
public function testConnectWillStartNewProcessWhenPreviousProcessExitsAfterPortListenerMessage()
195+
{
196+
$connector = new SshSocksConnector('host');
197+
198+
$ref = new \ReflectionProperty($connector, 'cmd');
199+
if (PHP_VERSION_ID < 80100) {
200+
$ref->setAccessible(true);
201+
}
202+
$ref->setValue($connector, 'echo "debug1: channel 0: new [port listener]" >&2; exec sleep 0.1; #');
203+
204+
$deferred = new Deferred();
205+
$socks = $this->getMockBuilder('React\Socket\ConnectorInterface')->getMock();
206+
$socks->expects($this->once())->method('connect')->with('example.com:80')->willReturn($deferred->promise());
207+
208+
$ref = new \ReflectionProperty($connector, 'socks');
209+
if (PHP_VERSION_ID < 80100) {
210+
$ref->setAccessible(true);
211+
}
212+
$ref->setValue($connector, $socks);
213+
214+
$connector->connect('example.com:80');
215+
216+
$onRejected = $this->expectCallableOnceWith($this->isInstanceOf('RuntimeException'));
217+
Loop::addTimer(0.5, function () use ($connector, $onRejected) {
218+
$ref = new \ReflectionProperty($connector, 'cmd');
219+
if (PHP_VERSION_ID < 80100) {
220+
$ref->setAccessible(true);
221+
}
222+
$ref->setValue($connector, 'echo "Connection closed by remote host" >&2; exit 255; #');
223+
224+
$promise = $connector->connect('example.com:80');
225+
$promise->then(null, $onRejected);
226+
});
227+
228+
Loop::run();
229+
}
230+
231+
public function testConnectWillTerminateNewProcessWhenPreviousProcessExitsLateAfterIdleTimeout()
232+
{
233+
$connector = new SshSocksConnector('host');
234+
235+
$ref = new \ReflectionProperty($connector, 'cmd');
236+
if (PHP_VERSION_ID < 80100) {
237+
$ref->setAccessible(true);
238+
}
239+
$ref->setValue($connector, 'trap "sleep 0.2; exit 0" TERM; while :; do sleep 0.01; done; #');
240+
241+
$deferred = new Deferred();
242+
$socks = $this->getMockBuilder('React\Socket\ConnectorInterface')->getMock();
243+
$socks->expects($this->once())->method('connect')->with('example.com:81')->willReturn($deferred->promise());
244+
245+
$ref = new \ReflectionProperty($connector, 'socks');
246+
if (PHP_VERSION_ID < 80100) {
247+
$ref->setAccessible(true);
248+
}
249+
$ref->setValue($connector, $socks);
250+
251+
// first process only exits 0.2s after being terminated by the idle timer
252+
$promise = $connector->connect('example.com:80');
253+
$promise->cancel();
254+
255+
$promise->then(null, $this->expectCallableOnceWith($this->isInstanceOf('RuntimeException')));
256+
257+
$process = null;
258+
$onRejected = $this->expectCallableOnceWith($this->isInstanceOf('RuntimeException'));
259+
Loop::addTimer(0.05, function () use ($connector, &$process, $onRejected) {
260+
$ref = new \ReflectionProperty($connector, 'cmd');
261+
if (PHP_VERSION_ID < 80100) {
262+
$ref->setAccessible(true);
263+
}
264+
$ref->setValue($connector, 'echo "debug1: channel 0: new [port listener]" >&2; exec sleep 2; #');
265+
266+
$promise = $connector->connect('example.com:81');
267+
$promise->then(null, $onRejected);
268+
269+
$ref = new \ReflectionProperty($connector, 'listen');
270+
if (PHP_VERSION_ID < 80100) {
271+
$ref->setAccessible(true);
272+
}
273+
$listen = $ref->getValue($connector);
274+
assert($listen instanceof PromiseInterface);
275+
276+
$listen->then(function ($value) use (&$process) {
277+
$process = $value;
278+
});
279+
});
280+
281+
// second process becomes idle only after first process exited
282+
Loop::addTimer(0.5, function () use ($deferred) {
283+
$deferred->reject(new \RuntimeException('Connection refused'));
284+
});
285+
286+
Loop::run();
287+
288+
$this->assertInstanceOf('React\ChildProcess\Process', $process);
289+
$this->assertTrue($process->isTerminated());
290+
}
291+
139292
public function testConnectWillRejectWithExceptionAndTerminateProcessWhenProcessOutputsLineExceedingMaximumBufferSize()
140293
{
141294
$connector = new SshSocksConnector('host');

0 commit comments

Comments
 (0)