Merge pull request #171 from clue-labs/fwrite-error

Improve error handling when sending data to DNS server fails (macOS)
reactphp · Feb 11, 2021 · 5f8494c · 5f8494c
2 parents 900bb63 + 93a2954
commit 5f8494c
Show file tree

Hide file tree

Showing 3 changed files with 110 additions and 26 deletions.
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
@@ -33,6 +33,19 @@ jobs:
       - run: vendor/bin/phpunit --coverage-text -c phpunit.xml.legacy
         if: ${{ matrix.php < 7.3 }}
 
+  PHPUnit-macOS:
+    name: PHPUnit (macOS)
+    runs-on: macos-10.15
+    continue-on-error: true
+    steps:
+      - uses: actions/checkout@v2
+      - uses: shivammathur/setup-php@v2
+        with:
+          php-version: 8.0
+          coverage: xdebug
+      - run: composer install
+      - run: vendor/bin/phpunit --coverage-text
+
   PHPUnit-hhvm:
     name: PHPUnit (HHVM)
     runs-on: ubuntu-18.04

diff --git a/src/Query/UdpTransportExecutor.php b/src/Query/UdpTransportExecutor.php
@@ -92,6 +92,13 @@ final class UdpTransportExecutor implements ExecutorInterface
     private $parser;
     private $dumper;
 
+    /**
+     * maximum UDP packet size to send and receive
+     *
+     * @var int
+     */
+    private $maxPacketSize = 512;
+
     /**
      * @param string        $nameserver
      * @param LoopInterface $loop
@@ -119,7 +126,7 @@ public function query(Query $query)
         $request = Message::createRequestForQuery($query);
 
         $queryData = $this->dumper->toBinary($request);
-        if (isset($queryData[512])) {
+        if (isset($queryData[$this->maxPacketSize])) {
             return \React\Promise\reject(new \RuntimeException(
                 'DNS query for ' . $query->name . ' failed: Query too large for UDP transport',
                 \defined('SOCKET_EMSGSIZE') ? \SOCKET_EMSGSIZE : 90
@@ -137,7 +144,20 @@ public function query(Query $query)
 
         // set socket to non-blocking and immediately try to send (fill write buffer)
         \stream_set_blocking($socket, false);
-        \fwrite($socket, $queryData);
+        $written = @\fwrite($socket, $queryData);
+
+        if ($written !== \strlen($queryData)) {
+            // Write may potentially fail, but most common errors are already caught by connection check above.
+            // Among others, macOS is known to report here when trying to send to broadcast address.
+            // This can also be reproduced by writing data exceeding `stream_set_chunk_size()` to a server refusing UDP data.
+            // fwrite(): send of 8192 bytes failed with errno=111 Connection refused
+            $error = \error_get_last();
+            \preg_match('/errno=(\d+) (.+)/', $error['message'], $m);
+            return \React\Promise\reject(new \RuntimeException(
+                'DNS query for ' . $query->name . ' failed: Unable to send query to DNS server ('  . (isset($m[2]) ? $m[2] : $error['message']) . ')',
+                isset($m[1]) ? (int) $m[1] : 0
+            ));
+        }
 
         $loop = $this->loop;
         $deferred = new Deferred(function () use ($loop, $socket, $query) {
@@ -148,11 +168,15 @@ public function query(Query $query)
             throw new CancellationException('DNS query for ' . $query->name . ' has been cancelled');
         });
 
+        $max = $this->maxPacketSize;
         $parser = $this->parser;
-        $loop->addReadStream($socket, function ($socket) use ($loop, $deferred, $query, $parser, $request) {
+        $loop->addReadStream($socket, function ($socket) use ($loop, $deferred, $query, $parser, $request, $max) {
             // try to read a single data packet from the DNS server
             // ignoring any errors, this is uses UDP packets and not a stream of data
-            $data = @\fread($socket, 512);
+            $data = @\fread($socket, $max);
+            if ($data === false) {
+                return;
+            }
 
             try {
                 $response = $parser->parseMessage($data);

diff --git a/tests/Query/UdpTransportExecutorTest.php b/tests/Query/UdpTransportExecutorTest.php
@@ -120,7 +120,75 @@ public function testQueryRejectsIfServerConnectionFails()
         $promise = $executor->query($query);
 
         $this->assertInstanceOf('React\Promise\PromiseInterface', $promise);
-        $promise->then(null, $this->expectCallableOnce());
+
+        $exception = null;
+        $promise->then(null, function ($reason) use (&$exception) {
+            $exception = $reason;
+        });
+
+        // PHP (Failed to parse address "///") differs from HHVM (Name or service not known)
+        $this->setExpectedException('RuntimeException', 'Unable to connect to DNS server');
+        throw $exception;
+    }
+
+    public function testQueryRejectsIfSendToServerFailsAfterConnection()
+    {
+        $loop = $this->getMockBuilder('React\EventLoop\LoopInterface')->getMock();
+        $loop->expects($this->never())->method('addReadStream');
+
+        $executor = new UdpTransportExecutor('0.0.0.0', $loop);
+
+        // increase hard-coded maximum packet size to allow sending excessive data
+        $ref = new \ReflectionProperty($executor, 'maxPacketSize');
+        $ref->setAccessible(true);
+        $ref->setValue($executor, PHP_INT_MAX);
+
+        $query = new Query(str_repeat('a.', 100000) . '.example', Message::TYPE_A, Message::CLASS_IN);
+        $promise = $executor->query($query);
+
+        $this->assertInstanceOf('React\Promise\PromiseInterface', $promise);
+
+        $exception = null;
+        $promise->then(null, function ($reason) use (&$exception) {
+            $exception = $reason;
+        });
+
+        // ECONNREFUSED( Connection refused) on Linux, EMSGSIZE (Message too long) on macOS
+        $this->setExpectedException('RuntimeException', 'Unable to send query to DNS server');
+        throw $exception;
+    }
+
+    public function testQueryKeepsPendingIfReadFailsBecauseServerRefusesConnection()
+    {
+        $socket = null;
+        $callback = null;
+        $loop = $this->getMockBuilder('React\EventLoop\LoopInterface')->getMock();
+        $loop->expects($this->once())->method('addReadStream')->with($this->callback(function ($ref) use (&$socket) {
+            $socket = $ref;
+            return true;
+        }), $this->callback(function ($ref) use (&$callback) {
+            $callback = $ref;
+            return true;
+        }));
+
+        $executor = new UdpTransportExecutor('0.0.0.0', $loop);
+
+        $query = new Query('reactphp.org', Message::TYPE_A, Message::CLASS_IN);
+        $promise = $executor->query($query);
+
+        $this->assertNotNull($socket);
+        $callback($socket);
+
+        $this->assertInstanceOf('React\Promise\PromiseInterface', $promise);
+
+        $pending = true;
+        $promise->then(function () use (&$pending) {
+            $pending = false;
+        }, function () use (&$pending) {
+            $pending = false;
+        });
+
+        $this->assertTrue($pending);
     }
 
     /**
@@ -142,27 +210,6 @@ public function testQueryRejectsOnCancellation()
         $promise->then(null, $this->expectCallableOnce());
     }
 
-    public function testQueryKeepsPendingIfServerRejectsNetworkPacket()
-    {
-        $loop = Factory::create();
-
-        $executor = new UdpTransportExecutor('127.0.0.1:1', $loop);
-
-        $query = new Query('google.com', Message::TYPE_A, Message::CLASS_IN);
-
-        $wait = true;
-        $promise = $executor->query($query)->then(
-            null,
-            function ($e) use (&$wait) {
-                $wait = false;
-                throw $e;
-            }
-        );
-
-        \Clue\React\Block\sleep(0.2, $loop);
-        $this->assertTrue($wait);
-    }
-
     public function testQueryKeepsPendingIfServerSendsInvalidMessage()
     {
         $loop = Factory::create();