From d8f748f155eea1f9766fa6c063f7c6a76b661594 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20L=C3=BCck?= Date: Wed, 10 Feb 2021 18:04:20 +0100 Subject: [PATCH 1/3] Improve error handling when sending data to DNS server fails (macOS) --- .github/workflows/ci.yml | 13 ++++++++++ src/Query/UdpTransportExecutor.php | 15 +++++++++++- tests/Query/UdpTransportExecutorTest.php | 30 +++++++++++++++++++++++- 3 files changed, 56 insertions(+), 2 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index cf214c83..4fc766a0 100644 --- 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 index 62ac2183..ffea0acc 100644 --- a/src/Query/UdpTransportExecutor.php +++ b/src/Query/UdpTransportExecutor.php @@ -137,7 +137,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. + // @codeCoverageIgnoreStart + $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 + )); + // @codeCoverageIgnoreEnd + } $loop = $this->loop; $deferred = new Deferred(function () use ($loop, $socket, $query) { diff --git a/tests/Query/UdpTransportExecutorTest.php b/tests/Query/UdpTransportExecutorTest.php index 78a85bb8..097b916f 100644 --- a/tests/Query/UdpTransportExecutorTest.php +++ b/tests/Query/UdpTransportExecutorTest.php @@ -120,7 +120,35 @@ 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; + }); + + $this->setExpectedException('RuntimeException', 'Unable to connect to DNS server (Failed to parse address "///")'); + throw $exception; + } + + public function testQueryRejectsIfSendToServerFailsAfterConnection() + { + $loop = $this->getMockBuilder('React\EventLoop\LoopInterface')->getMock(); + $loop->expects($this->never())->method('addReadStream'); + + $executor = new UdpTransportExecutor('255.255.255.255', $loop); + + $query = new Query('google.com', 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; + }); + + $this->setExpectedException('RuntimeException', 'to DNS server (Permission denied)', SOCKET_EACCES); + throw $exception; } /** From 07ca85475064d7734e8727fffbe743b0e1716616 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20L=C3=BCck?= Date: Wed, 10 Feb 2021 18:20:23 +0100 Subject: [PATCH 2/3] Minor refactoring to simplify testing UDP write errors --- src/Query/UdpTransportExecutor.php | 18 +++++++++++++----- tests/Query/UdpTransportExecutorTest.php | 15 +++++++++++---- 2 files changed, 24 insertions(+), 9 deletions(-) diff --git a/src/Query/UdpTransportExecutor.php b/src/Query/UdpTransportExecutor.php index ffea0acc..2ee10752 100644 --- 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 @@ -142,14 +149,14 @@ public function query(Query $query) 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. - // @codeCoverageIgnoreStart + // 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 )); - // @codeCoverageIgnoreEnd } $loop = $this->loop; @@ -161,11 +168,12 @@ 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); try { $response = $parser->parseMessage($data); diff --git a/tests/Query/UdpTransportExecutorTest.php b/tests/Query/UdpTransportExecutorTest.php index 097b916f..157b9eca 100644 --- a/tests/Query/UdpTransportExecutorTest.php +++ b/tests/Query/UdpTransportExecutorTest.php @@ -126,7 +126,8 @@ public function testQueryRejectsIfServerConnectionFails() $exception = $reason; }); - $this->setExpectedException('RuntimeException', 'Unable to connect to DNS server (Failed to parse address "///")'); + // PHP (Failed to parse address "///") differs from HHVM (Name or service not known) + $this->setExpectedException('RuntimeException', 'Unable to connect to DNS server'); throw $exception; } @@ -135,9 +136,14 @@ public function testQueryRejectsIfSendToServerFailsAfterConnection() $loop = $this->getMockBuilder('React\EventLoop\LoopInterface')->getMock(); $loop->expects($this->never())->method('addReadStream'); - $executor = new UdpTransportExecutor('255.255.255.255', $loop); + $executor = new UdpTransportExecutor('0.0.0.0', $loop); - $query = new Query('google.com', Message::TYPE_A, Message::CLASS_IN); + // 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); @@ -147,7 +153,8 @@ public function testQueryRejectsIfSendToServerFailsAfterConnection() $exception = $reason; }); - $this->setExpectedException('RuntimeException', 'to DNS server (Permission denied)', SOCKET_EACCES); + // ECONNREFUSED( Connection refused) on Linux, EMSGSIZE (Message too long) on macOS + $this->setExpectedException('RuntimeException', 'Unable to send query to DNS server'); throw $exception; } From 93a2954acca5f41a8fb56dae6c1f92cf0e7cf00d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20L=C3=BCck?= Date: Wed, 10 Feb 2021 19:06:59 +0100 Subject: [PATCH 3/3] Explicitly ignore UDP read errors --- src/Query/UdpTransportExecutor.php | 3 ++ tests/Query/UdpTransportExecutorTest.php | 54 +++++++++++++++--------- 2 files changed, 36 insertions(+), 21 deletions(-) diff --git a/src/Query/UdpTransportExecutor.php b/src/Query/UdpTransportExecutor.php index 2ee10752..ff3169db 100644 --- a/src/Query/UdpTransportExecutor.php +++ b/src/Query/UdpTransportExecutor.php @@ -174,6 +174,9 @@ public function query(Query $query) // 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, $max); + if ($data === false) { + return; + } try { $response = $parser->parseMessage($data); diff --git a/tests/Query/UdpTransportExecutorTest.php b/tests/Query/UdpTransportExecutorTest.php index 157b9eca..5a202ace 100644 --- a/tests/Query/UdpTransportExecutorTest.php +++ b/tests/Query/UdpTransportExecutorTest.php @@ -158,6 +158,39 @@ public function testQueryRejectsIfSendToServerFailsAfterConnection() 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); + } + /** * @group internet */ @@ -177,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();