filter senders, add timeout, 16-bit txn ids

Author: msimerson

client/udp.js

@@ -1,15 +1,21 @@
 const udp = require('dgram');
+const net = require('net');
+const crypto = require('crypto');
 const Packet = require('../packet');
-const { equal } = require('assert');
 const { debuglog } = require('util');
 
 const debug = debuglog('dns2');
 
-module.exports = ({ dns = '8.8.8.8', port = 53, socketType = 'udp4' } = {}) => {
+module.exports = ({
+  dns = '8.8.8.8',
+  port = 53,
+  socketType = 'udp4',
+  timeout = 10000,
+} = {}) => {
   return (name, type = 'A', cls = Packet.CLASS.IN, options = {}) => {
     const { clientIp, recursive = true } = options;
     const query = new Packet();
-    query.header.id = (Math.random() * 1e4) | 0;
+    query.header.id = crypto.randomInt(0x10000);
     // see https://github.com/song940/node-dns/issues/29
     if (recursive) {
       query.header.rd = 1;
@@ -25,15 +31,66 @@ module.exports = ({ dns = '8.8.8.8', port = 53, socketType = 'udp4' } = {}) => {
       type  : Packet.TYPE[type],
     });
     const client = new udp.Socket(socketType);
+    // Only enforce a strict source-address check when `dns` is an IP literal;
+    // hostnames would require an extra resolve to compare against.
+    const expectedAddress = net.isIP(dns) ? dns : null;
     return new Promise((resolve, reject) => {
-      client.once('message', function onMessage(message) {
-        client.close();
-        const response = Packet.parse(message);
-        equal(response.header.id, query.header.id);
+      let settled = false;
+      let timer;
+      const cleanup = () => {
+        if (settled) return;
+        settled = true;
+        if (timer) clearTimeout(timer);
+        client.removeListener('message', onMessage);
+        client.removeListener('error', onError);
+        try { client.close(); } catch (_) { /* already closed */ }
+      };
+      function onMessage(message, rinfo) {
+        // Drop packets that didn't come from the configured resolver.
+        if (rinfo.port !== port || (expectedAddress && rinfo.address !== expectedAddress)) {
+          debug('udp: dropping packet from unexpected sender %s:%d', rinfo.address, rinfo.port);
+          return;
+        }
+        let response;
+        try {
+          response = Packet.parse(message);
+        } catch (e) {
+          debug('udp: dropping unparseable packet: %s', e.message);
+          return;
+        }
+        // Stray / late reply from a reused ephemeral port — keep listening.
+        if (response.header.id !== query.header.id) {
+          debug('udp: dropping response with mismatched id %d (expected %d)',
+            response.header.id, query.header.id);
+          return;
+        }
+        cleanup();
         resolve(response);
-      });
+      }
+      function onError(err) {
+        cleanup();
+        reject(err);
+      }
+      client.on('message', onMessage);
+      client.on('error', onError);
+
+      if (timeout > 0) {
+        timer = setTimeout(() => {
+          cleanup();
+          const err = new Error(`DNS query timed out after ${timeout}ms`);
+          err.code = 'ETIMEDOUT';
+          reject(err);
+        }, timeout);
+        timer.unref();
+      }
+
       debug('send', dns, query.toBuffer());
-      client.send(query.toBuffer(), port, dns, err => err && reject(err));
+      client.send(query.toBuffer(), port, dns, err => {
+        if (err) {
+          cleanup();
+          reject(err);
+        }
+      });
     });
   };
 };

test/index.js

@@ -401,6 +401,61 @@ test('server/udp-tcp#simple-request-async-response', async() => {
   await server.close();
 });
 
+test('client/udp ignores stray response and resolves on matching id', async() => {
+  // Simulate the scenario from upstream issue #100: a stray UDP packet (e.g.
+  // late reply on a reused ephemeral port) arrives before the real response.
+  // The client must drop it and keep listening rather than asserting/crashing.
+  const server = udp.createSocket('udp4');
+  await new Promise(resolve => server.bind(0, '127.0.0.1', resolve));
+  const { port: serverPort } = server.address();
+
+  server.on('message', (msg, rinfo) => {
+    const request = Packet.parse(msg);
+
+    // Stray packet: same socket, but a different (wrong) transaction id.
+    const stray = new Packet();
+    stray.header.id = (request.header.id + 1) & 0xffff;
+    stray.header.qr = 1;
+    server.send(stray.toBuffer(), rinfo.port, rinfo.address);
+
+    // Real reply, slightly delayed so the stray definitely lands first.
+    const response = Packet.createResponseFromRequest(request);
+    response.answers.push({
+      name    : request.questions[0].name,
+      type    : Packet.TYPE.A,
+      class   : Packet.CLASS.IN,
+      ttl     : 300,
+      address : '1.2.3.4',
+    });
+    setTimeout(() => server.send(response.toBuffer(), rinfo.port, rinfo.address), 5);
+  });
+
+  const query = UDPClient({ dns: '127.0.0.1', port: serverPort, timeout: 2000 });
+  const reply = await query('stray.test');
+  assert.equal(reply.answers.length, 1);
+  assert.equal(reply.answers[0].address, '1.2.3.4');
+  await new Promise(resolve => server.close(resolve));
+});
+
+test('client/udp times out when no matching response arrives', async() => {
+  // Server replies with only stray packets; client must time out, not hang.
+  const server = udp.createSocket('udp4');
+  await new Promise(resolve => server.bind(0, '127.0.0.1', resolve));
+  const { port: serverPort } = server.address();
+
+  server.on('message', (msg, rinfo) => {
+    const request = Packet.parse(msg);
+    const stray = new Packet();
+    stray.header.id = (request.header.id + 1) & 0xffff;
+    stray.header.qr = 1;
+    server.send(stray.toBuffer(), rinfo.port, rinfo.address);
+  });
+
+  const query = UDPClient({ dns: '127.0.0.1', port: serverPort, timeout: 100 });
+  await assert.rejects(query('timeout.test'), err => err.code === 'ETIMEDOUT');
+  await new Promise(resolve => server.close(resolve));
+});
+
 test('server/all#invalid-request', async() => {
   const server = createServer({
     doh    : true,