Add TCP support to DNS server and client #27
Conversation
- Updated README to reflect TCP support on port 5300 - Modified docker-compose to expose TCP port 5300 - Enhanced Native adapter to handle TCP connections - Updated Swoole adapter for TCP support - Improved Client class to support TCP queries - Added unit test for TCP queries in ClientTest
WalkthroughAdds TCP DNS support across adapters and client: Native adapter gains a TCP listener, per‑connection buffering, length‑prefixed framing, accept/read/write and client lifecycle handling; Swoole adapter gains optional TCP port with open_length_check framing and TCP receive/send logic. Client adds an optional TCP query path, framing, read helpers and conditional useTcp flag. Message.encode accepts an optional maxSize and can produce truncated responses; Server.onPacket and adapter docblocks now accept an optional maxResponseSize. Tests for TCP queries and UDP truncation fallback added. docker-compose exposes TCP 5300; README and test zone files updated; several decoding validations and PHPStan level tightened. Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (6)
🧰 Additional context used🧬 Code graph analysis (5)src/DNS/Message/Header.php (1)
src/DNS/Message/Question.php (1)
src/DNS/Message/Record.php (5)
src/DNS/Client.php (2)
src/DNS/Adapter/Native.php (2)
🔇 Additional comments (19)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
README.md (1)
40-54: The code example is malformed and will not execute.The quick start example has corrupted content. Line 40 contains prose text inside what should be a PHP array of
Recordobjects, and lines 41-45 have orphaned PHP fragments. Additionally, there's now duplicate/conflicting text about port behavior (line 40 mentions "UDP and TCP port 5300" while line 54 mentions only "UDP port 5300").The intended structure should be something like:
$zone = new Zone( name: 'example.test', records: [ new Record(name: 'example.test', type: Record::TYPE_A, rdata: '192.0.2.1', ttl: 60), new Record(name: 'www.example.test', type: Record::TYPE_CNAME, rdata: 'example.test', ttl: 60), new Record( name: 'example.test', type: Record::TYPE_SOA, rdata: 'ns1.example.test hostmaster.example.test 1 7200 1800 1209600 3600', ttl: 60 ), ] );Please also update line 54 to reflect that the server now listens on both UDP and TCP.
src/DNS/Client.php (1)
53-79: Add null check to satisfy static analysis.The pipeline reports that
$this->socketmay benullwhen passed tosocket_sendtoandsocket_recvfrom. While this path is only reached whenuseTcpis false (where$socketis initialized), static analysis cannot infer this.🔎 Proposed fix
public function query(Message $message): Message { if ($this->useTcp) { return $this->queryTcp($message); } + if ($this->socket === null) { + throw new Exception('UDP socket not initialized.'); + } + $packet = $message->encode(); if (socket_sendto($this->socket, $packet, strlen($packet), 0, $this->server, $this->port) === false) {
🧹 Nitpick comments (2)
tests/e2e/DNS/ClientTest.php (1)
15-28: Consider adding more assertions for parity with UDP tests.The test validates TCP connectivity and basic response structure but is less thorough than the UDP counterpart (
testARecords) which verifiesttl,class, andrdatavalues for the same domain. Consider adding similar assertions for consistency:$this->assertCount(2, $records); $this->assertSame('dev2.appwrite.io', $records[0]->name); $this->assertSame(Record::TYPE_A, $records[0]->type); +$this->assertSame(Record::CLASS_IN, $records[0]->class); +$this->assertSame(1800, $records[0]->ttl); +$this->assertSame('142.6.0.1', $records[0]->rdata); +$this->assertSame('142.6.0.2', $records[1]->rdata);src/DNS/Adapter/Native.php (1)
198-209: Consider adding a maximum payload length check.The code trusts the 2-byte length prefix from clients. A malicious client could send a large length value (up to 65535), causing the server to buffer significant data. While DNS messages are typically small, adding a sanity check would be defensive.
🔎 Suggested improvement
while (strlen($this->tcpBuffers[$clientId]) >= 2) { $length = unpack('nlen', substr($this->tcpBuffers[$clientId], 0, 2)); $payloadLength = $length['len'] ?? 0; if ($payloadLength === 0) { $this->closeTcpClient($client); return; } + // DNS messages are limited to ~512 bytes for UDP, but TCP allows larger. + // Cap at a reasonable limit to prevent memory exhaustion. + if ($payloadLength > 65535) { + $this->closeTcpClient($client); + return; + } + if (strlen($this->tcpBuffers[$clientId]) < ($payloadLength + 2)) { return; }Note: The 2-byte length prefix already caps at 65535, so this is more about documentation and explicit handling. The Swoole adapter uses
package_max_length => 65537which provides similar protection.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
README.mddocker-compose.ymlsrc/DNS/Adapter/Native.phpsrc/DNS/Adapter/Swoole.phpsrc/DNS/Client.phpsrc/DNS/Validator/DNS.phptests/e2e/DNS/ClientTest.php
🧰 Additional context used
🧬 Code graph analysis (4)
tests/e2e/DNS/ClientTest.php (2)
src/DNS/Client.php (2)
Client(8-174)query(51-79)src/DNS/Message/Question.php (1)
Question(7-54)
src/DNS/Client.php (1)
src/DNS/Resolver/Proxy.php (1)
__construct(21-26)
src/DNS/Adapter/Native.php (1)
src/DNS/Adapter.php (2)
onWorkerStart(13-13)onPacket(21-21)
src/DNS/Adapter/Swoole.php (3)
src/DNS/Server.php (2)
Server(60-308)onPacket(171-283)src/DNS/Adapter.php (2)
Adapter(5-34)onPacket(21-21)src/DNS/Adapter/Native.php (1)
onPacket(74-77)
🪛 GitHub Actions: CodeQL
src/DNS/Client.php
[error] 58-58: Parameter #1 $socket of function socket_sendto expects Socket, Socket|null given.
[error] 66-66: Parameter #1 $socket of function socket_recvfrom expects Socket, Socket|null given.
[error] 144-144: Parameter #2 $length of function fread expects int<1, max>, int given.
src/DNS/Adapter/Native.php
[error] 163-163: Instanceof between Socket and Socket will always evaluate to true. 🪪 instanceof.alwaysTrue
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Unit & E2E
🔇 Additional comments (7)
docker-compose.yml (1)
17-18: LGTM!The TCP port mapping complements the existing UDP mapping, enabling both protocols on port 5300 as expected for DNS servers per RFC 5966.
src/DNS/Validator/DNS.php (1)
61-61: LGTM!Minor formatting adjustment adding a space after the
matchkeyword, aligning with PSR-12 style conventions.src/DNS/Adapter/Swoole.php (2)
26-36: LGTM on TCP server configuration.The length-prefixed framing configuration follows RFC 1035 DNS-over-TCP correctly:
- 2-byte network-order length prefix (
package_length_type => 'n')- Body starts after the 2-byte header (
package_body_offset => 2)- Max length accounts for the 65535-byte payload plus 2-byte prefix
74-88: LGTM on TCP receive handler.The handler correctly strips the 2-byte length prefix (already validated by Swoole's
open_length_check), processes the payload, and frames the response. The pattern matches the Native adapter's TCP handling.src/DNS/Client.php (1)
81-126: LGTM on TCP query implementation.The
queryTcpmethod correctly implements RFC 1035 DNS-over-TCP:
- Length-prefixed framing with 2-byte network-order header
- Proper connection management with
try/finallyensuring socket closure- Comprehensive error handling for partial writes and incomplete reads
- Transaction ID validation in
decodeResponsesrc/DNS/Adapter/Native.php (2)
228-244: LGTM on TCP response handling.The
sendTcpResponsemethod correctly handles partial writes with a loop and properly cleans up the client on failure.
246-253: LGTM on client cleanup.The
closeTcpClientmethod properly removes all references to the client (preventing memory leaks in long-running servers) and suppresses errors onsocket_closewhich may already be closed.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/DNS/Adapter/Native.phpsrc/DNS/Client.php
🧰 Additional context used
🧬 Code graph analysis (2)
src/DNS/Client.php (1)
src/DNS/Resolver/Proxy.php (1)
__construct(21-26)
src/DNS/Adapter/Native.php (2)
src/DNS/Server.php (2)
onWorkerStart(122-129)onPacket(171-283)src/DNS/Adapter.php (2)
onWorkerStart(13-13)onPacket(21-21)
🔇 Additional comments (13)
src/DNS/Client.php (6)
10-11: LGTM! Socket property correctly made nullable.The nullable type is appropriate since TCP mode bypasses UDP socket initialization.
16-18: LGTM! TCP flag added with backward-compatible default.The
$useTcpparameter defaults tofalse, preserving existing behavior for callers that don't specify it.Also applies to: 28-32
53-59: LGTM! Query routing correctly delegates based on protocol.The TCP/UDP routing is clear, and the defensive null-check at lines 57-59 ensures safety even though the constructor guarantees UDP socket initialization when
useTcpis false.
85-130: LGTM! TCP query implementation follows RFC 1035 framing.The method correctly implements DNS-over-TCP with proper length-prefix framing, error handling, and resource cleanup in the finally block.
132-141: LGTM! Response decoding includes transaction ID validation.The transaction ID check at line 136 prevents accepting mismatched responses, which is a good security practice.
176-183: LGTM! IPv6 addresses correctly wrapped for stream socket URI.The bracket wrapping at line 179 is necessary for IPv6 addresses in stream socket URIs (e.g.,
tcp://[::1]:53).src/DNS/Adapter/Native.php (7)
11-19: LGTM! Property declarations properly typed.The separation of
$udpServerand$tcpServer, along with per-client state arrays, provides a clear structure for multiplexing.Also applies to: 30-30
36-36: LGTM! TCP enabled by default for the server.Note that the server enables TCP by default (
$enableTcp = true), whileClientdefaults to UDP ($useTcp = false). This is appropriate since servers typically listen on both protocols while clients choose one.Also applies to: 40-40
42-56: LGTM! Socket creation includes SO_REUSEADDR for TCP.The
SO_REUSEADDRoption at line 54 is good practice, allowing the server to restart quickly without "address already in use" errors.
84-98: LGTM! TCP server properly configured with non-blocking mode.The non-blocking mode at line 97 is essential for multiplexing with
socket_select.
106-166: LGTM! Socket multiplexing correctly handles UDP and TCP.The
socket_selectat line 119 efficiently multiplexes the UDP server, TCP listen socket, and multiple TCP client connections. The per-protocol handling is clear and correct.
179-225: LGTM! TCP client handler correctly implements length-prefixed framing.The buffering and frame extraction logic (lines 197-224) properly handles partial reads and multiple frames per recv, aligning with DNS-over-TCP requirements.
245-252: LGTM! TCP client cleanup properly manages state.The cleanup at line 249 removes both the socket and buffer entries, preventing memory leaks.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/DNS/Adapter/Native.phpsrc/DNS/Client.php
🧰 Additional context used
🧬 Code graph analysis (2)
src/DNS/Adapter/Native.php (1)
src/DNS/Adapter.php (2)
onWorkerStart(13-13)onPacket(21-21)
src/DNS/Client.php (4)
src/DNS/Message/Header.php (3)
__construct(11-31)encode(64-84)decode(33-62)src/DNS/Message/Question.php (3)
__construct(11-17)encode(48-53)decode(22-46)src/DNS/Message.php (5)
__construct(32-68)Message(11-208)encode(186-207)response(111-139)decode(141-184)src/DNS/Resolver/Proxy.php (1)
__construct(21-26)
🔇 Additional comments (12)
src/DNS/Client.php (6)
16-32: LGTM! Clean TCP mode initialization.The
$useTcpflag and early-return pattern correctly establish TCP-only mode, skipping UDP socket initialization when TCP is enabled. The default value preserves backward compatibility.
53-59: LGTM! Proper routing with defensive guard.The TCP/UDP routing logic is clear, and the
instanceofcheck ensures the UDP socket is initialized before use, preventing potential runtime errors.
85-130: LGTM! Robust TCP query implementation.The TCP query flow correctly implements DNS-over-TCP framing (2-byte length prefix), handles timeouts, validates response completeness, and ensures socket cleanup with try-finally. Error handling is comprehensive.
132-141: LGTM! Essential transaction ID verification.Verifying that the response transaction ID matches the query is critical for security and correctness, preventing acceptance of mismatched responses.
143-174: LGTM! Defensive read implementation addresses past concerns.The method correctly handles partial reads, timeouts, and EOF conditions. The explicit guards at lines 150-152 and the EOF check at line 163 address the static analysis warnings and infinite loop risk from previous reviews.
176-183: LGTM! Correct IPv6 formatting.Properly wraps IPv6 addresses in brackets for TCP connection URIs, which is required for correct parsing of host:port combinations.
src/DNS/Adapter/Native.php (6)
11-57: LGTM! Clean dual-protocol architecture.The property structure and constructor correctly establish optional TCP support alongside UDP. Per-client buffering (
tcpBuffers) is essential for TCP frame reassembly, and theenableTcpflag provides deployment flexibility.
84-98: LGTM! Proper TCP server initialization.The TCP bind, listen (with reasonable backlog), and non-blocking mode setup for the server socket are all correct for multiplexed accept operations.
105-145: LGTM! Well-structured event loop multiplexing.The socket_select-based multiplexing correctly handles UDP, TCP server accept, and TCP client I/O in a single event loop. The UDP path is straightforward and includes appropriate error handling.
179-225: LGTM! Solid TCP frame handling with proper buffering.The method correctly handles partial reads, EOF detection (line 188), and frame reassembly. The while loop (line 197) efficiently processes multiple frames per read event, and reusing the
onPacketcallback maintains consistent semantics across UDP and TCP.
227-251: LGTM! Write loop correctly handles transient errors.The EAGAIN/EWOULDBLOCK handling (lines 239-243) properly distinguishes transient from fatal errors. The
socket_clear_errorand brief sleep before retry are good practices. This addresses the previous review concern.
253-260: LGTM! Clean resource cleanup.Properly removes client state from both tracking arrays and closes the socket. The
@suppression is acceptable since the socket may already be closed by the peer.
stnguyen90
left a comment
stnguyen90
left a comment
There was a problem hiding this comment.
Do we have any benchmarks we can run to compare this with the UDP only server to make sure the performance of the server is still good?
Has this been tested on staging?
src/DNS/Adapter/Swoole.php
Outdated
| $this->udpPort->on('Packet', function ($server, $data, $clientInfo) { | ||
| $ip = $clientInfo['address'] ?? ''; | ||
| $port = (int) ($clientInfo['port'] ?? 0); | ||
| $answer = \call_user_func($this->onPacket, $data, $ip, $port); | ||
|
|
||
| // Swoole UDP sockets reject zero-length payloads; skip responding instead. | ||
| if ($answer === '') { | ||
| return; | ||
| } | ||
|
|
||
| $server->sendto($ip, $port, $answer); |
There was a problem hiding this comment.
There's a bit of duplication here between UDP and TCP listener, can we extract that do a private function handleQuery to keep things consistent?
There was a problem hiding this comment.
now refactored.
src/DNS/Adapter/Swoole.php
Outdated
| $this->host = $host; | ||
| $this->port = $port; | ||
| $this->server = new Server($this->host, $this->port, SWOOLE_PROCESS, SWOOLE_SOCK_UDP); | ||
| $this->server = new Server($this->host, $this->port, SWOOLE_PROCESS, SWOOLE_SOCK_TCP); |
There was a problem hiding this comment.
Small detail, but I think important to encapsulate: This seems backwards to me, UDP is the primary protocol for DNS (vast majority of queries).
Do you think UDP should be the primary Server and TCP should be tacked on as an additional listener. Probably guarded by $enableTcp flag the same way Native adapter does it
There was a problem hiding this comment.
The TCP implementation looks good, but we're missing truncation handling for UDP responses.
DNS over UDP has a 512-byte limit (or higher with EDNS0). When a response exceeds this, the server should:
- Set the
TC(truncated) flag in the header - Send a truncated response
- Client then retries over TCP to get the full response
Currently Message::encode() has no size awareness - it'll return arbitrarily large packets. The UDP send path will happily transmit these, but they may be dropped or fragmented by the network.
Message::encode($maxSize = null) so it can self-truncate and set the TC flag.
This is why TCP support matters - without it, large responses (big TXT records, DNSSEC, many A records) silently fail. With TCP but without truncation signaling, clients don't know to retry.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/DNS/Adapter/Native.php (1)
112-114: Implement a connection limit to prevent resource exhaustion.As previously noted, the unbounded
tcpClientsarray creates a DoS vector where attackers can exhaust file descriptors or exceedsocket_selectlimits (typically ~1024 FDs). The server will crash or become unresponsive when these limits are reached.🔎 Recommended fix
+ protected int $maxTcpConnections = 100; + // In start() method, before accepting: foreach ($this->tcpClients as $client) { $readSockets[] = $client; }Then in the accept path (around line 148):
if ($this->tcpServer !== null && $socket === $this->tcpServer) { + if (count($this->tcpClients) >= $this->maxTcpConnections) { + $client = @socket_accept($this->tcpServer); + if ($client instanceof Socket) { + @socket_close($client); + } + continue; + } + $client = @socket_accept($this->tcpServer);Based on learnings, this concern was previously raised but not yet addressed.
🧹 Nitpick comments (1)
src/DNS/Adapter/Native.php (1)
93-93: Consider making the listen backlog configurable.The hardcoded backlog of 128 is reasonable for most use cases, but making it a constructor parameter or class constant would improve flexibility for high-traffic scenarios.
🔎 Optional refactor
- if (socket_listen($this->tcpServer, 128) == false) { + if (socket_listen($this->tcpServer, $this->listenBacklog) == false) {Add a constructor parameter or class constant:
protected int $listenBacklog = 128;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/DNS/Adapter/Native.php
🧰 Additional context used
🧬 Code graph analysis (1)
src/DNS/Adapter/Native.php (1)
src/DNS/Adapter.php (2)
onWorkerStart(13-13)onPacket(21-21)
🔇 Additional comments (4)
src/DNS/Adapter/Native.php (4)
11-57: LGTM! Well-structured addition of TCP support.The property declarations are properly typed, the constructor signature cleanly extends the existing API with a sensible default, and SO_REUSEADDR on the TCP socket is appropriate for server restart scenarios.
151-154: Excellent fix for non-blocking mode issue.The addition of
socket_set_nonblock()with proper error handling addresses the previous critical concern. Closing the client on failure prevents partially-configured sockets from entering the event loop.
238-252: Excellent EAGAIN/EWOULDBLOCK handling.The write loop now correctly distinguishes transient non-blocking errors from fatal errors. The retry logic with
socket_clear_error()and brief sleep is appropriate for the non-blocking socket model.
258-265: LGTM! Clean and correct resource cleanup.The method properly removes the client from both tracking arrays and closes the socket. Error suppression on
socket_close()is acceptable since the socket may already be closed by the peer.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/DNS/Adapter/Native.php (1)
194-240: Buffer and frame size limits are still missing.While this method correctly implements TCP framing and error handling, the concerns raised in previous reviews about unbounded buffer growth (line 210) and missing maximum frame size validation (lines 213-219) remain unaddressed. A malicious client can exhaust memory by:
- Sending large partial frames without completing them (buffer grows indefinitely)
- Announcing a 65KB frame size (max 2-byte value) for each request
🔎 Recommended safeguards
Add class-level limits:
protected int $maxTcpBufferSize = 16384; // 16KB per client protected int $maxTcpFrameSize = 8192; // 8KB per frame (typical DNS is <4KB)Check before appending:
+ if (strlen($this->tcpBuffers[$clientId] ?? '') + strlen($chunk) > $this->maxTcpBufferSize) { + $this->closeTcpClient($client); + return; + } + $this->tcpBuffers[$clientId] = ($this->tcpBuffers[$clientId] ?? '') . $chunk;Validate frame size:
$length = unpack('nlen', substr($this->tcpBuffers[$clientId], 0, 2)); $payloadLength = $length['len'] ?? 0; - if ($payloadLength === 0) { + if ($payloadLength === 0 || $payloadLength > $this->maxTcpFrameSize) { $this->closeTcpClient($client); return; }Based on learnings, this issue was previously flagged and remains unresolved.
🧹 Nitpick comments (1)
src/DNS/Adapter/Swoole.php (1)
77-93: Handle send() failures for TCP responses.Line 91 calls
$server->send()without checking the return value. If the send fails (e.g., client disconnected), this will silently ignore the error.🔎 Proposed fix
$frame = pack('n', strlen($answer)) . $answer; - $server->send($fd, $frame); + if ($server->send($fd, $frame) === false) { + // Log or handle send failure + $server->close($fd); + }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/DNS/Adapter/Native.phpsrc/DNS/Adapter/Swoole.php
🧰 Additional context used
🧬 Code graph analysis (1)
src/DNS/Adapter/Native.php (1)
src/DNS/Adapter.php (2)
onWorkerStart(13-13)onPacket(21-21)
🔇 Additional comments (8)
src/DNS/Adapter/Swoole.php (1)
64-75: UDP handler looks good.The explicit int cast for port and empty answer check are appropriate defensive measures.
src/DNS/Adapter/Native.php (7)
40-62: Constructor changes look good.The conditional TCP socket creation and SO_REUSEADDR configuration are appropriate for enabling TCP support alongside UDP.
87-103: Server initialization looks correct.The bind/listen sequence with non-blocking mode and backlog of 128 is appropriate for a select-based TCP server.
110-129: Socket multiplexing implementation is sound.The select loop correctly includes all relevant sockets (UDP server, TCP server, TCP clients) for I/O readiness detection.
131-150: UDP handling preserved correctly.The UDP receive/response flow is unchanged and appropriate.
152-176: TCP accept handling is now correct.The addition of the max clients check (line 156) and non-blocking mode enforcement (line 161) address the critical issues raised in previous reviews. The socket option configuration is appropriate.
242-266: TCP response handling is now robust.The write loop with proper EAGAIN/EWOULDBLOCK handling (lines 254-258) correctly addresses the concerns from previous reviews. The implementation will retry transient errors and only close the connection on fatal errors.
268-275: Client cleanup is straightforward and correct.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/DNS/Adapter/Swoole.php (2)
25-25: Consider making maxUdpSize configurable.The hardcoded 512-byte limit is the traditional DNS UDP size. While safe for backward compatibility, modern clients with EDNS0 support can handle larger payloads. Consider adding a constructor parameter to allow customization.
🔎 Example refactor
- public function __construct(string $host = '0.0.0.0', int $port = 53, bool $enableTcp = true) + public function __construct(string $host = '0.0.0.0', int $port = 53, bool $enableTcp = true, int $maxUdpSize = 512) { $this->host = $host; $this->port = $port; $this->enableTcp = $enableTcp; + $this->maxUdpSize = $maxUdpSize;
122-143: Well-designed truncation helper with reasonable fallback.The method correctly creates a truncated DNS response with the TC flag set and no answer records. The catch block's fallback to the original response is pragmatic—while the oversized response may fail at the network layer, this prevents crashes during error conditions.
📝 Optional: Add comment explaining fallback behavior
Consider adding a comment at line 141 to document the fallback rationale:
} catch (\Throwable $e) { + // Fallback to original response; may fail at network layer but prevents crashes return $encodedResponse; }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/DNS/Adapter/Swoole.phptests/e2e/DNS/ClientTest.php
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/e2e/DNS/ClientTest.php
🧰 Additional context used
🧬 Code graph analysis (1)
src/DNS/Adapter/Swoole.php (1)
src/DNS/Message.php (4)
Message(11-208)decode(141-184)response(111-139)encode(186-207)
🔇 Additional comments (5)
src/DNS/Adapter/Swoole.php (5)
8-9: LGTM: Clean imports for TCP support.Both imports are necessary and properly used throughout the file.
27-44: TCP listener setup looks correct.The constructor properly adds TCP support with appropriate DNS over TCP framing:
- 2-byte length prefix configuration matches RFC 1035 Section 4.2.2
package_max_lengthof 65537 correctly accounts for 2-byte prefix + 65535 max message size- Primary UDP server remains the base, with TCP as an optional listener
69-69: Good defensive type cast.Explicitly casting the port to int ensures type safety for the onPacket callback.
77-79: Correct implementation of DNS truncation.Properly handles oversized UDP responses by creating truncated messages with the TC flag set, prompting clients to retry over TCP for the full answer per RFC 1035.
84-100: TCP handler correctly implements DNS over TCP framing.The implementation properly:
- Strips the 2-byte length prefix from received data (line 90)
- Adds the 2-byte length prefix to responses (line 97)
- Handles empty responses gracefully
The framing matches DNS over TCP specifications.
src/DNS/Adapter/Swoole.php
Outdated
| protected string $host; | ||
| protected int $port; | ||
|
|
||
| public function __construct(string $host = '0.0.0.0', int $port = 53) | ||
| protected bool $enableTcp; | ||
|
|
||
| protected int $maxUdpSize = 512; |
There was a problem hiding this comment.
nit/ignorable: why not use https://stitcher.io/blog/constructor-promotion-in-php-8 here? much more readable imo
src/DNS/Adapter/Swoole.php
Outdated
| } | ||
|
|
||
| if (strlen($answer) > $this->maxUdpSize) { | ||
| $answer = $this->truncateResponse($answer); |
There was a problem hiding this comment.
There are some downsides to making the adapter responsibility for truncation
- duplication, it's implemented in both Swoole and Native, and every future adapter
- inverted dependency, they both now depend on Message
- testing, you have to spin up Swoole environment to test truncation
Instead, I'd suggest to put the logic inside the Message::encode method. You can thread it there from the adapter using passing $maxSize param to onPacket function, then the server can invoke encode($maxSize)
That way:
- there's no duplication, you implement once and it works as long as the adapter passes the maxSize
- testing, you can easily unit test Message::encode with maxPacketSize
| { | ||
| public const int PORT = 5300; | ||
|
|
||
| public function testTcpQueries(): void |
There was a problem hiding this comment.
Can we add e2e and unit tests for truncation?
| $this->tcpPort->on('Receive', function (Server $server, int $fd, int $reactorId, string $data) { | ||
| $info = $server->getClientInfo($fd, $reactorId) ?: []; | ||
| $ip = $info['remote_ip'] ?? ''; | ||
| $port = $info['remote_port'] ?? 0; | ||
|
|
||
| $payload = substr($data, 2); // strip 2-byte length prefix | ||
| $answer = \call_user_func($this->onPacket, $payload, $ip, $port); | ||
|
|
||
| if ($answer === '') { | ||
| return; | ||
| } | ||
|
|
||
| $frame = pack('n', strlen($answer)) . $answer; | ||
| $server->send($fd, $frame); | ||
| }); |
There was a problem hiding this comment.
We can probably merge these to handlers if the design is cleaner/polymorphic as described above
There was a problem hiding this comment.
couldn't find a cleaner way to do it, as UDP and TCP swoole server have different parameter names, not much similarility
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/e2e/DNS/ClientTest.php (1)
15-28: LGTM! Good basic TCP query test.This test validates the TCP query path end-to-end. Consider using named parameters for clarity:
$client = new Client('127.0.0.1', self::PORT, timeout: 5, useTcp: true);This is optional but improves readability.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
composer.jsonsrc/DNS/Adapter.phpsrc/DNS/Adapter/Native.phpsrc/DNS/Adapter/Swoole.phpsrc/DNS/Client.phpsrc/DNS/Message.phpsrc/DNS/Server.phptests/e2e/DNS/ClientTest.phptests/resources/zone-valid-localhost.txttests/unit/DNS/MessageTest.php
✅ Files skipped from review due to trivial changes (1)
- src/DNS/Adapter.php
🧰 Additional context used
🧬 Code graph analysis (6)
tests/e2e/DNS/ClientTest.php (1)
src/DNS/Client.php (2)
Client(8-193)query(51-83)
src/DNS/Message.php (4)
src/DNS/Message/Header.php (1)
encode(64-84)src/DNS/Message/Question.php (1)
encode(48-53)src/DNS/Message/Domain.php (1)
encode(19-72)src/DNS/Message/Record.php (1)
encode(353-375)
src/DNS/Adapter/Native.php (1)
src/DNS/Adapter.php (2)
onWorkerStart(13-13)onPacket(21-21)
src/DNS/Adapter/Swoole.php (3)
src/DNS/Server.php (2)
Server(60-308)onPacket(172-284)src/DNS/Adapter.php (2)
Adapter(5-34)onPacket(21-21)src/DNS/Adapter/Native.php (1)
onPacket(83-86)
src/DNS/Client.php (11)
src/DNS/Message.php (6)
__construct(32-68)Message(11-225)encode(186-224)query(70-95)response(111-139)decode(141-184)src/DNS/Validator/DNS.php (1)
__construct(35-37)src/DNS/Message/Header.php (3)
__construct(11-31)encode(64-84)decode(33-62)src/DNS/Message/Question.php (3)
__construct(11-17)encode(48-53)decode(22-46)src/DNS/Message/Record.php (3)
__construct(113-124)encode(353-375)decode(136-324)src/DNS/Resolver/Cloudflare.php (1)
__construct(13-16)src/DNS/Resolver/Google.php (1)
__construct(13-16)src/DNS/Resolver/Memory.php (1)
__construct(12-14)src/DNS/Resolver/Proxy.php (1)
__construct(21-26)src/DNS/Zone.php (1)
__construct(19-46)src/DNS/Message/Domain.php (2)
encode(19-72)decode(83-148)
src/DNS/Server.php (2)
src/DNS/Adapter/Swoole.php (1)
onPacket(65-104)src/DNS/Message.php (2)
response(111-139)encode(186-224)
🪛 GitHub Actions: CodeQL
src/DNS/Adapter/Native.php
[error] 227-237: PHPStan: Parameter #2 ...$values of function printf expects bool|float|int|string|null, mixed given. argument.type; Binary operation '+' between mixed and 2 results in an error; Parameter #3 $length of function substr expects int|null, mixed given. argument.type
src/DNS/Client.php
[error] 119-119: PHPStan: Cannot cast mixed to int.
🪛 PHPMD (2.15.0)
src/DNS/Adapter/Native.php
222-222: Avoid unused local variables such as '$length'. (undefined)
(UnusedLocalVariable)
🔇 Additional comments (22)
composer.json (1)
11-11: PHPStan is configured to use max level.The check script is already set to run PHPStan at the max strictness level. Verify that all changes in this PR pass this configuration as part of CI validation.
tests/resources/zone-valid-localhost.txt (1)
11-19: LGTM! Test data for truncation scenarios.The addition of large TXT records provides necessary test data for validating UDP truncation and TCP fallback behavior. The records are appropriately sized to exceed the 512-byte UDP limit.
src/DNS/Server.php (2)
168-172: LGTM! Clean signature extension for max response size.The optional
maxResponseSizeparameter is properly added to theonPacketmethod signature, enabling adapters to specify size constraints for different transport protocols (512 for UDP, null for TCP).
254-256: LGTM! Proper propagation of maxResponseSize to encoding.The
maxResponseSizeis correctly passed to$response->encode(), enabling the truncation logic in the Message class when size limits are exceeded.tests/unit/DNS/MessageTest.php (2)
294-332: LGTM! Comprehensive test for truncation behavior.The test thoroughly validates the truncation logic by:
- Creating a response that exceeds 512 bytes
- Verifying the TC flag is set
- Confirming answer/authority/additional sections are cleared
- Ensuring the question is preserved
- Validating the final packet respects the size limit
334-362: LGTM! Good negative test case.This test correctly validates that encoding without a size limit preserves all data and does not set the TC flag, ensuring the truncation logic only activates when explicitly requested.
src/DNS/Message.php (1)
186-224: LGTM! Clean truncation implementation.The truncation logic correctly:
- Clears answer/authority/additional sections
- Sets the TC (truncated) flag
- Preserves questions and relevant header flags
- Re-encodes the minimal response without a size limit (since it should now fit)
One edge case to be aware of: if the question section itself exceeds
maxSize, the truncated response would still exceed the limit. This is unlikely in practice since questions are typically small, but worth noting for completeness.tests/e2e/DNS/ClientTest.php (1)
274-303: LGTM! Addresses the previous request for truncation e2e tests.This test correctly demonstrates the UDP truncation → TCP fallback pattern that clients should implement per RFC 5966. The conditional handling is appropriate since the response may or may not be truncated depending on the actual response size.
src/DNS/Client.php (5)
10-32: LGTM! Clean TCP mode initialization.The constructor properly handles both TCP and UDP modes:
- Early return when
useTcpis true avoids unnecessary UDP socket creation- Socket initialization for UDP mode is preserved with proper timeout configuration
51-83: LGTM! Clean TCP/UDP branching logic.The query method properly routes to TCP or UDP based on the
useTcpflag, with appropriate socket validation for the UDP path.
137-146: LGTM! Important security check.The transaction ID validation in
decodeResponseis a good security practice that helps prevent DNS spoofing attacks and ensures response integrity.
148-183: LGTM! Robust byte reading with proper EOF handling.The
readBytesimplementation correctly addresses previous review concerns:
- Uses
max(1, $remaining)to satisfy static analysis requirements- Checks for both timeout and EOF conditions to prevent infinite loops
185-192: LGTM! Correct IPv6 address formatting.The
formatTcpHosthelper correctly wraps IPv6 addresses in brackets as required by the URI specification for TCP connections.src/DNS/Adapter/Swoole.php (3)
24-44: LGTM! Well-configured TCP listener.The TCP listener setup correctly:
- Uses
open_length_checkfor automatic DNS message framing- Configures 2-byte big-endian length prefix (
package_length_type => 'n')- Sets appropriate body offset and max length
69-83: LGTM! UDP handler with proper RFC 1035 size limit.The UDP handler correctly enforces the 512-byte limit by passing
512as themaxResponseSizeparameter, enabling truncation for oversized responses.
85-103: LGTM! TCP handler with length-prefixed framing.The TCP handler correctly:
- Strips the 2-byte length prefix from received data
- Passes
nullformaxResponseSize(TCP has no practical size limit)- Frames responses with length prefix before sending
src/DNS/Adapter/Native.php (6)
11-36: LGTM! Well-structured TCP support properties.The new properties provide proper resource management:
- Buffer and frame size limits prevent memory exhaustion attacks
- Client tracking enables connection lifecycle management
- Per-client buffers support length-prefixed framing
44-66: LGTM! Clean TCP server initialization.The constructor properly initializes both UDP and TCP servers when TCP is enabled, with appropriate socket reuse options.
91-107: LGTM! Proper TCP server setup with non-blocking mode.The TCP server is correctly configured with:
- Socket binding and listening
- Non-blocking mode to avoid blocking the event loop
114-180: LGTM! Robust select-based multiplexing.The main loop correctly handles:
- UDP packet reception with 512-byte response limit
- TCP connection acceptance with client limits
- Non-blocking client configuration
- Proper socket option configuration (keepalive, timeouts)
253-277: LGTM! Robust TCP response handling.The
sendTcpResponsemethod correctly:
- Frames the payload with a 2-byte length prefix
- Handles partial writes in a loop
- Distinguishes between recoverable (EAGAIN/EWOULDBLOCK) and fatal errors
- Properly cleans up on fatal errors
279-286: LGTM! Clean client cleanup.The
closeTcpClientmethod properly removes all per-client state before closing the socket.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.