Avoid possibility of missing remote address when TLS handshake fails

This fixes a race condition where the remote address could appear to be
"emtpy" in the server exception message, that could occur when the
connection to the peer was already closed during the TLS handshake. This
is simply avoided by getting the remote address prior to starting the
TLS handshake, in which case the remote address is known to exist.

Author: clue

src/SecureServer.php

@@ -169,28 +169,30 @@ public function handleConnection(ConnectionInterface $connection)
     {
         if (!$connection instanceof Connection) {
             $this->emit('error', array(new \UnexpectedValueException('Base server does not use internal Connection class exposing stream resource')));
-            $connection->end();
+            $connection->close();
             return;
         }
 
         foreach ($this->context as $name => $value) {
             \stream_context_set_option($connection->stream, 'ssl', $name, $value);
         }
 
+        // get remote address before starting TLS handshake in case connection closes during handshake
+        $remote = $connection->getRemoteAddress();
         $that = $this;
 
         $this->encryption->enable($connection)->then(
             function ($conn) use ($that) {
                 $that->emit('connection', array($conn));
             },
-            function ($error) use ($that, $connection) {
+            function ($error) use ($that, $connection, $remote) {
                 $error = new \RuntimeException(
-                    'Connection from ' . $connection->getRemoteAddress() . ' failed during TLS handshake: ' . $error->getMessage(),
+                    'Connection from ' . $remote . ' failed during TLS handshake: ' . $error->getMessage(),
                     $error->getCode()
                 );
 
                 $that->emit('error', array($error));
-                $connection->end();
+                $connection->close();
             }
         );
     }

tests/SecureServerTest.php

@@ -4,6 +4,7 @@
 
 use React\Socket\SecureServer;
 use React\Socket\TcpServer;
+use React\Promise\Promise;
 
 class SecureServerTest extends TestCase
 {
@@ -74,14 +75,14 @@ public function testCloseWillBePassedThroughToTcpServer()
         $server->close();
     }
 
-    public function testConnectionWillBeEndedWithErrorIfItIsNotAStream()
+    public function testConnectionWillBeClosedWithErrorIfItIsNotAStream()
     {
         $loop = $this->getMockBuilder('React\EventLoop\LoopInterface')->getMock();
 
         $tcp = new TcpServer(0, $loop);
 
         $connection = $this->getMockBuilder('React\Socket\ConnectionInterface')->getMock();
-        $connection->expects($this->once())->method('end');
+        $connection->expects($this->once())->method('close');
 
         $server = new SecureServer($tcp, $loop, array());
 
@@ -90,6 +91,74 @@ public function testConnectionWillBeEndedWithErrorIfItIsNotAStream()
         $tcp->emit('connection', array($connection));
     }
 
+    public function testConnectionWillTryToEnableEncryptionAndWaitForHandshake()
+    {
+        $loop = $this->getMockBuilder('React\EventLoop\LoopInterface')->getMock();
+
+        $tcp = new TcpServer(0, $loop);
+
+        $connection = $this->getMockBuilder('React\Socket\Connection')->disableOriginalConstructor()->getMock();
+        $connection->expects($this->once())->method('getRemoteAddress')->willReturn('tcp://127.0.0.1:1234');
+        $connection->expects($this->never())->method('close');
+
+        $server = new SecureServer($tcp, $loop, array());
+
+        $pending = new Promise(function () { });
+
+        $encryption = $this->getMockBuilder('React\Socket\StreamEncryption')->disableOriginalConstructor()->getMock();
+        $encryption->expects($this->once())->method('enable')->willReturn($pending);
+
+        $ref = new \ReflectionProperty($server, 'encryption');
+        $ref->setAccessible(true);
+        $ref->setValue($server, $encryption);
+
+        $ref = new \ReflectionProperty($server, 'context');
+        $ref->setAccessible(true);
+        $ref->setValue($server, array());
+
+        $server->on('error', $this->expectCallableNever());
+        $server->on('connection', $this->expectCallableNever());
+
+        $tcp->emit('connection', array($connection));
+    }
+
+    public function testConnectionWillBeClosedWithErrorIfEnablingEncryptionFails()
+    {
+        $loop = $this->getMockBuilder('React\EventLoop\LoopInterface')->getMock();
+
+        $tcp = new TcpServer(0, $loop);
+
+        $connection = $this->getMockBuilder('React\Socket\Connection')->disableOriginalConstructor()->getMock();
+        $connection->expects($this->once())->method('getRemoteAddress')->willReturn('tcp://127.0.0.1:1234');
+        $connection->expects($this->once())->method('close');
+
+        $server = new SecureServer($tcp, $loop, array());
+
+        $error = new \RuntimeException('Original');
+
+        $encryption = $this->getMockBuilder('React\Socket\StreamEncryption')->disableOriginalConstructor()->getMock();
+        $encryption->expects($this->once())->method('enable')->willReturn(\React\Promise\reject($error));
+
+        $ref = new \ReflectionProperty($server, 'encryption');
+        $ref->setAccessible(true);
+        $ref->setValue($server, $encryption);
+
+        $ref = new \ReflectionProperty($server, 'context');
+        $ref->setAccessible(true);
+        $ref->setValue($server, array());
+
+        $error = null;
+        $server->on('error', $this->expectCallableOnce());
+        $server->on('error', function ($e) use (&$error) {
+            $error = $e;
+        });
+
+        $tcp->emit('connection', array($connection));
+
+        $this->assertInstanceOf('RuntimeException', $error);
+        $this->assertEquals('Connection from tcp://127.0.0.1:1234 failed during TLS handshake: Original', $error->getMessage());
+    }
+
     public function testSocketErrorWillBeForwarded()
     {
         $loop = $this->getMockBuilder('React\EventLoop\LoopInterface')->getMock();