Preserve valid DNS leases and cover bounded diagnostics

src/helpers/NetworkLink.cpp

@@ -37,6 +37,7 @@ namespace {
 // Trailing empty slots are dropped to keep remote replies short; "-" marks an
 // empty slot before a populated one, or a list with nothing in it.
 static void formatDnsList(char* out, size_t out_size, const ip_addr_t* servers, int count) {
+  if (out == nullptr || out_size == 0) return;
   int last = count - 1;
   while (last >= 0 && ip_addr_isany_val(servers[last])) last--;
   out[0] = '\0';
@@ -158,16 +159,20 @@ class NetworkLinkBase : public NetworkLink {
   // that ever changes.)
   static esp_err_t snapshotDnsOnTcpipThread(void* ctx) {
     NetworkLinkBase* self = static_cast<NetworkLinkBase*>(ctx);
+    ip_addr_t servers[kDnsServers] = {};
     bool any = false;
     for (int i = 0; i < kDnsServers; i++) {
       const ip_addr_t* server = dns_getserver(i);
-      self->_dns_snapshot[i] = (server != nullptr) ? *server : *IP_ADDR_ANY;
-      if (!ip_addr_isany_val(self->_dns_snapshot[i])) any = true;
+      servers[i] = (server != nullptr) ? *server : *IP_ADDR_ANY;
+      if (!ip_addr_isany_val(servers[i])) any = true;
     }
     // A lease that carried no resolver at all (static configuration, or a
     // server that offered none) must not be remembered as "this medium's DNS is
     // nothing" — restoring that would wipe a working resolver for no gain.
-    if (any) self->_dns_captured = true;
+    if (any) {
+      for (int i = 0; i < kDnsServers; i++) self->_dns_snapshot[i] = servers[i];
+      self->_dns_captured = true;
+    }
     return ESP_OK;
   }
 

test/fixtures/network_dns.cpp

@@ -0,0 +1,282 @@
+// The marked DNS methods and shared base are copied verbatim from production.
+// This fixture replaces only SDK dispatch, address conversion, and link I/O.
+#include <helpers/NetworkLink.h>
+#include <array>
+#include <atomic>
+#include <cassert>
+#include <climits>
+#include <cstdio>
+#include <cstring>
+#include <iostream>
+#include <stdexcept>
+#include <string>
+#include <vector>
+
+static void require(bool condition, const char* message) {
+  if (!condition) throw std::runtime_error(message);
+}
+constexpr int DNS_MAX_SERVERS = 3;
+struct ip_addr_t {
+  std::array<char, 48> text{};
+  unsigned type = 0;
+  bool operator==(const ip_addr_t& other) const {
+    return type == other.type && text == other.text;
+  }
+};
+static ip_addr_t address(const char* text, unsigned type = 4) {
+  ip_addr_t value;
+  if (*text) {
+    require(strlen(text) < value.text.size(), "fixture address too long");
+    strcpy(value.text.data(), text);
+    value.type = type;
+  }
+  return value;
+}
+static const ip_addr_t empty_address;
+#define IP_ADDR_ANY (&empty_address)
+static bool ip_addr_isany_val(const ip_addr_t& value) { return !value.text[0]; }
+static char* ipaddr_ntoa_r(const ip_addr_t* value, char* out, int size) {
+  if (size <= 0 || strlen(value->text.data()) >= size_t(size)) return nullptr;
+  strcpy(out, value->text.data());
+  return out;
+}
+using esp_err_t = int;
+constexpr esp_err_t ESP_OK = 0;
+static bool on_tcpip_thread = false;
+static unsigned tcpip_dispatches = 0;
+static std::array<ip_addr_t, DNS_MAX_SERVERS> global_dns{};
+static std::array<bool, DNS_MAX_SERVERS> null_dns{};
+static std::vector<int> dns_reads, dns_writes;
+static esp_err_t esp_netif_tcpip_exec(esp_err_t (*callback)(void*), void* context) {
+  require(!on_tcpip_thread, "nested TCP/IP dispatch");
+  ++tcpip_dispatches;
+  on_tcpip_thread = true;
+  const esp_err_t result = callback(context);
+  on_tcpip_thread = false;
+  return result;
+}
+static const ip_addr_t* dns_getserver(int index) {
+  require(on_tcpip_thread, "DNS read escaped TCP/IP thread");
+  dns_reads.push_back(index);
+  return null_dns.at(index) ? nullptr : &global_dns.at(index);
+}
+static void dns_setserver(int index, const ip_addr_t* value) {
+  require(on_tcpip_thread, "DNS write escaped TCP/IP thread");
+  dns_writes.push_back(index);
+  global_dns.at(index) = *value;
+}
+struct LinkEndpoint {
+  IPAddress gateway;
+  IPAddress gatewayIP() const { return gateway; }
+} WiFi, CH390;
+
+@HELPERS@
+@BASE@
+
+// Unchanged interface operations are irrelevant to DNS and have no SDK effects.
+struct TestLink : NetworkLinkBase {
+  const char* mediumName() const override { return "test"; }
+  NetworkMedium medium() const override { return NetworkMedium::None; }
+  const char* statusName() const override { return "test"; }
+  int statusCode() const override { return 0; }
+  bool configValid(const char*) const override { return true; }
+  void setHostname(const char*) override {}
+  bool begin(const char*, const char*) override { return false; }
+  NetworkTransition maintain(uint32_t, uint8_t) override { return NetworkTransition::None; }
+  bool isConnected() const override { return false; }
+  IPAddress localIP() const override { return IPAddress(); }
+  int rssi() const override { return INT_MIN; }
+  bool resolveHost(const char*, IPAddress&) const override { return false; }
+  void formatDiagnostics(char*, size_t) const override {}
+};
+struct WiFiNetworkLink : TestLink { @WiFiNetworkLink@ };
+struct EthernetNetworkLink : TestLink { @EthernetNetworkLink@ };
+struct AutomaticNetworkLink : TestLink {
+  std::atomic<NetworkMedium> _selected{NetworkMedium::None};
+  WiFiNetworkLink _wifi;
+  EthernetNetworkLink _ethernet;
+  @AutomaticNetworkLink@
+};
+static AutomaticNetworkLink automatic;
+static unsigned link_lookups = 0;
+NetworkLink& activeNetworkLink() { ++link_lookups; return automatic; }
+static bool get(const char* config, char* reply) {
+  @GETTER@ else { return false; }
+  return true;
+}
+
+static void restore_scenario() {
+  WiFiNetworkLink one, all;
+  const std::array<ip_addr_t, 3> complete{{
+      address("1.1.1.1"), address("8.8.8.8"), address("2001:db8::53", 6)}};
+  const std::array<ip_addr_t, 3> single{{address("9.9.9.9"), {}, {}}};
+  global_dns = complete;
+  all.snapshotDns();
+  global_dns = single;
+  one.snapshotDns();
+  dns_writes.clear();
+  all.restoreDns();
+  require(global_dns == complete, "DNS restore lost a populated third slot or typed address");
+  require(dns_writes == std::vector<int>({0, 1, 2}), "DNS restore did not write every slot");
+  dns_writes.clear();
+  one.restoreDns();
+  require(global_dns == single, "DNS restore retained another medium's empty-slot fallbacks");
+  require(dns_writes == std::vector<int>({0, 1, 2}), "DNS restore omitted empty slots");
+  require(tcpip_dispatches == 4, "DNS snapshots/restores bypassed thread dispatch");
+}
+
+static void never_leased_scenario() {
+  WiFiNetworkLink never;
+  const std::array<ip_addr_t, 3> complete{{address("1.1.1.1"), address("8.8.8.8"), address("9.9.9.9")}};
+  global_dns = complete;
+  never.restoreDns();
+  require(global_dns == complete && dns_writes.empty(), "never-leased medium wiped live DNS");
+  char lease[64] = {};
+  never.formatLeaseDns(lease, sizeof(lease));
+  require(std::string(lease) == "none", "never-leased DNS diagnostic is misleading");
+  global_dns = {};
+  never.snapshotDns();
+  global_dns = complete;
+  never.restoreDns();
+  require(global_dns == complete && dns_writes.empty(), "resolver-free lease wiped live DNS");
+  never.formatLeaseDns(lease, sizeof(lease));
+  require(std::string(lease) == "none", "resolver-free lease invented captured DNS");
+}
+
+static void renewal_scenario() {
+  WiFiNetworkLink link;
+  const std::array<ip_addr_t, 3> third_only{{{}, {}, address("9.9.9.9")}};
+  const std::array<ip_addr_t, 3> complete{{address("1.1.1.1"), address("8.8.8.8"), address("9.9.9.9")}};
+  const std::array<ip_addr_t, 3> partial{{{}, address("10.10.10.10"), {}}};
+  global_dns = third_only;
+  link.snapshotDns();
+  char lease[64] = {};
+  link.formatLeaseDns(lease, sizeof(lease));
+  require(std::string(lease) == "-,-,9.9.9.9", "third-slot-only lease was not captured");
+  global_dns = complete;
+  link.restoreDns();
+  require(global_dns == third_only, "third-slot-only DNS restore lost its resolver");
+  global_dns = complete;
+  link.snapshotDns();
+  global_dns = partial;
+  link.snapshotDns();
+  global_dns = complete;
+  link.restoreDns();
+  require(global_dns == partial, "valid DNS renewal retained obsolete resolver slots");
+  global_dns = {};
+  link.snapshotDns();
+  global_dns = complete;
+  link.restoreDns();
+  require(global_dns == partial, "DNS-free renewal wiped the last valid lease");
+  link.formatLeaseDns(lease, sizeof(lease));
+  require(std::string(lease) == "-,10.10.10.10", "DNS-free renewal diagnostic lost the last valid lease");
+}
+
+static void prepare_automatic() {
+  WiFi.gateway = IPAddress("10.0.0.1");
+  CH390.gateway = IPAddress("192.168.1.1");
+  global_dns = {{address("1.1.1.1"), {}, {}}};
+  automatic._wifi.snapshotDns();
+  global_dns = {{address("8.8.8.8"), {}, address("9.9.9.9")}};
+  automatic._ethernet.snapshotDns();
+  global_dns = {{address("4.4.4.4"), {}, {}}};
+  automatic._selected = NetworkMedium::Ethernet;
+}
+
+static void format_scenario() {
+  char list[64] = {};
+  std::array<ip_addr_t, 3> servers{};
+  formatDnsList(list, sizeof(list), servers.data(), 3);
+  require(std::string(list) == "-", "empty resolver list differs from documented sentinel");
+  servers = {{address("1.1.1.1"), {}, {}}};
+  formatDnsList(list, sizeof(list), servers.data(), 3);
+  require(std::string(list) == "1.1.1.1", "trailing empty resolver slots were retained");
+  servers[2] = address("9.9.9.9");
+  formatDnsList(list, sizeof(list), servers.data(), 3);
+  require(std::string(list) == "1.1.1.1,-,9.9.9.9", "interior empty resolver slot was dropped");
+  global_dns = servers;
+  null_dns[0] = true;
+  formatCurrentDnsServers(list, sizeof(list));
+  require(std::string(list) == "-,-,9.9.9.9", "null lwIP resolver was not treated as empty");
+  null_dns = {};
+
+  prepare_automatic();
+  const size_t writes_before = dns_writes.size();
+  char reply[256] = {};
+  const NetworkLink& wifi = automatic._wifi;
+  wifi.formatDns(reply, sizeof(reply));
+  require(std::string(reply) == "> dns:4.4.4.4 gw:10.0.0.1 lease:1.1.1.1", "WiFi DNS interface lost its own lease/gateway");
+  const NetworkLink& ethernet = automatic._ethernet;
+  ethernet.formatDns(reply, sizeof(reply));
+  require(std::string(reply) == "> dns:4.4.4.4 gw:192.168.1.1 lease:8.8.8.8,-,9.9.9.9", "Ethernet DNS interface lost its own lease/gateway");
+  const NetworkLink& selected = automatic;
+  selected.formatDns(reply, sizeof(reply));
+  require(std::string(reply) == "> dns:4.4.4.4 selected:ethernet gw:192.168.1.1\neth-lease:8.8.8.8,-,9.9.9.9 wifi-lease:1.1.1.1", "automatic DNS omitted global or medium lease data");
+  automatic._selected = NetworkMedium::WiFi;
+  selected.formatDns(reply, sizeof(reply));
+  require(strstr(reply, "selected:wifi gw:10.0.0.1"), "automatic WiFi selection used the Ethernet gateway");
+  automatic._selected = NetworkMedium::None;
+  selected.formatDns(reply, sizeof(reply));
+  require(strstr(reply, "selected:none gw:0.0.0.0"), "unselected diagnostic invented a gateway");
+
+  // All three lists can exceed the real CLI's 160-byte buffer. Truncation is
+  // allowed; writing beyond the declared capacity or losing NUL is not.
+  global_dns = {{address("255.255.255.255"), address("255.255.255.255"), address("255.255.255.255")}};
+  automatic._wifi.snapshotDns();
+  automatic._ethernet.snapshotDns();
+  automatic._selected = NetworkMedium::Ethernet;
+  CH390.gateway = IPAddress("255.255.255.255");
+  selected.formatDns(reply, sizeof(reply));
+  require(strlen(reply) == 207, "long DNS diagnostic fixture no longer exercises truncation");
+  const std::string full(reply);
+  for (size_t size : {size_t(0), size_t(1), size_t(2), size_t(10), size_t(64), size_t(160)}) {
+    std::array<unsigned char, 192> bounded;
+    bounded.fill(0xA5);
+    char* out = reinterpret_cast<char*>(bounded.data() + 8);
+    selected.formatDns(out, size);
+    for (size_t i = 0; i < 8; ++i) require(bounded[i] == 0xA5, "DNS formatter wrote before destination");
+    for (size_t i = 8 + size; i < bounded.size(); ++i) require(bounded[i] == 0xA5, "DNS formatter overran declared capacity");
+    if (size) require(std::string(out) == full.substr(0, size - 1), "bounded DNS reply lost NUL or changed prefix");
+  }
+  require(dns_writes.size() == writes_before, "read-only DNS diagnostics changed resolver state");
+}
+
+static void zero_scenario() {
+  char canary = 'Z';
+  formatDnsList(&canary, 0, global_dns.data(), 3);
+  require(canary == 'Z', "zero-size DNS formatter wrote to its destination");
+  formatDnsList(nullptr, 0, global_dns.data(), 3);
+  formatDnsList(nullptr, 7, global_dns.data(), 3);
+}
+
+static void getter_scenario() {
+  prepare_automatic();
+  struct Reply { unsigned char before[8]; char text[160]; unsigned char after[8]; } reply;
+  memset(&reply, 0xA5, sizeof(reply));
+  require(get("link.dns", reply.text), "observer DNS getter was not recognized");
+  require(link_lookups == 1 && strstr(reply.text, "> dns:4.4.4.4 selected:ethernet"), "observer DNS getter did not use the live NetworkLink interface");
+  for (unsigned char c : reply.before) require(c == 0xA5, "observer DNS getter underflowed reply");
+  for (unsigned char c : reply.after) require(c == 0xA5, "observer DNS getter overflowed reply");
+  const std::string original(reply.text);
+  for (const char* rejected : {"link.dnsX", "LINK.DNS", "link.dns ", "wifi.status"}) {
+    require(!get(rejected, reply.text), "observer DNS getter accepted another command");
+    require(std::string(reply.text) == original && link_lookups == 1, "rejected DNS command changed reply or queried the link");
+  }
+}
+
+int main(int argc, char** argv) {
+  try {
+    require(argc == 2, "missing DNS scenario");
+    const std::string scenario(argv[1]);
+    if (scenario == "restore") restore_scenario();
+    else if (scenario == "never-leased") never_leased_scenario();
+    else if (scenario == "renewal") renewal_scenario();
+    else if (scenario == "format") format_scenario();
+    else if (scenario == "zero") zero_scenario();
+    else if (scenario == "getter") getter_scenario();
+    else throw std::runtime_error("unknown DNS scenario");
+  } catch (const std::exception& error) {
+    std::cerr << error.what() << '\n';
+    return 1;
+  }
+}

test/test_cli_settings_contract.py

@@ -63,6 +63,7 @@ COMMON_QUERY_EXCEPTIONS = {
     'wifi.pwd': 'Setter is in the observer or standalone-WiFi delegate',
 }
 OBSERVER_QUERY_EXCEPTIONS = {
+    'link.dns': 'Live resolver list, gateway, and retained per-medium lease diagnostics',
     'mqtt.config.valid': 'Configuration validation result',
     'mqtt.enabled': 'Stored in common NodePrefs; set mqtt.enabled is handled by CommonCLI',
     'mqtt.ntp.diag': 'NTP diagnostics',

test/test_network_dns.py

@@ -0,0 +1,144 @@
+#!/usr/bin/env python3
+"""Execute production DNS snapshots, formatters, and CLI wiring without a network.
+
+Only lwIP dispatch/address conversion and physical-link endpoints are mocked.
+The real NetworkLink interface, complete shared base, and each DNS formatter
+are compiled together, including the observer getter copied from production.
+"""
+from pathlib import Path
+import os
+import shutil
+import subprocess
+import sys
+import tempfile
+import unittest
+
+from test_replay_reset_integration import extract_braced
+
+ROOT = Path(__file__).resolve().parents[1]
+FIXTURE = ROOT / "test/fixtures/network_dns.cpp"
+
+IP_ADDRESS = r'''
+#pragma once
+#include <string>
+class IPAddress {
+  std::string text;
+ public:
+  IPAddress() : text("0.0.0.0") {}
+  explicit IPAddress(const char* value) : text(value) {}
+  std::string toString() const { return text; }
+};
+'''
+
+
+def production_harness():
+    network = (ROOT / "src/helpers/NetworkLink.cpp").read_text(encoding="utf-8")
+    helpers = network[network.index("namespace {") + len("namespace {"):
+                      network.index("class NetworkLinkBase")]
+    base = extract_braced(network, "class NetworkLinkBase") + ";"
+    code = FIXTURE.read_text(encoding="ascii").replace("@HELPERS@", helpers)
+    code = code.replace("@BASE@", base)
+    for name in ("WiFiNetworkLink", "EthernetNetworkLink", "AutomaticNetworkLink"):
+        implementation = extract_braced(network, "class " + name)
+        methods = extract_braced(implementation, "void formatDns(")
+        if name != "AutomaticNetworkLink":
+            methods += "\n" + extract_braced(implementation, "IPAddress gatewayIP()")
+        code = code.replace("@" + name + "@", methods)
+    cli = (ROOT / "src/helpers/CommonCLI_Observer.cpp").read_text(encoding="utf-8")
+    getter = extract_braced(cli, 'if (strcmp(config, "link.dns") == 0)')
+    return code.replace("@GETTER@", getter)
+
+
+class NetworkDnsTest(unittest.TestCase):
+    @classmethod
+    def setUpClass(cls):
+        cls.compiler = os.environ.get("CXX") or shutil.which("g++") or shutil.which("clang++")
+        if cls.compiler is None:
+            raise unittest.SkipTest("a host C++17 compiler is required")
+        cls.temporary = tempfile.TemporaryDirectory(prefix="meshcore-network-dns-")
+        cls.addClassCleanup(cls.temporary.cleanup)
+        cls.directory = Path(cls.temporary.name)
+        (cls.directory / "Arduino.h").write_text("#pragma once\n", encoding="ascii")
+        (cls.directory / "IPAddress.h").write_text(IP_ADDRESS, encoding="ascii")
+        cls.source = production_harness()
+        cls.binary = cls.compile(cls.source, "production")
+
+    @classmethod
+    def compile(cls, source, name):
+        cpp = cls.directory / (name + ".cpp")
+        binary = cls.directory / name
+        cpp.write_text(source, encoding="utf-8")
+        sanitizers = (["-fsanitize=address,undefined", "-fno-sanitize-recover=all",
+                       "-fno-pie", "-no-pie"] if sys.platform.startswith("linux") else [])
+        result = subprocess.run([
+            cls.compiler, "-std=c++17", "-Wall", "-Wextra", "-Werror", *sanitizers,
+            "-DESP_PLATFORM=1", "-I", str(cls.directory), "-I", str(ROOT / "src"),
+            str(cpp), "-o", str(binary)], capture_output=True, text=True, timeout=60)
+        if result.returncode:
+            raise AssertionError(result.stdout + result.stderr)
+        return binary
+
+    def run_scenario(self, scenario, binary=None):
+        result = subprocess.run([str(binary or self.binary), scenario],
+                                capture_output=True, text=True, timeout=10)
+        self.assertEqual(result.returncode, 0, result.stdout + result.stderr)
+
+    def test_three_slot_snapshot_restore_and_empty_slot_clearing(self):
+        self.run_scenario("restore")
+
+    def test_never_leased_medium_preserves_current_global_resolvers(self):
+        self.run_scenario("never-leased")
+
+    def test_renewals_keep_last_valid_lease_and_replace_obsolete_slots(self):
+        self.run_scenario("renewal")
+
+    def test_dns_lists_and_all_network_diagnostics_are_bounded(self):
+        self.run_scenario("format")
+
+    def test_zero_capacity_and_null_destination_do_not_write(self):
+        self.run_scenario("zero")
+
+    def test_observer_getter_uses_real_interface_and_exact_command(self):
+        self.run_scenario("getter")
+        cli = (ROOT / "src/helpers/CommonCLI_Observer.cpp").read_text(encoding="utf-8")
+        self.assertIn('#ifdef ESP_PLATFORM\n#include "NetworkLink.h"', cli)
+        self.assertIn('memcmp(config, "wifi.status", 11)', cli)
+
+    def test_webconfig_mock_exposes_the_read_only_dns_getter(self):
+        sys.path.insert(0, str(ROOT / "scripts"))
+        try:
+            import webconfig_mock_server as portal
+        finally:
+            sys.path.pop(0)
+        self.assertEqual(portal.cli_get({}, "link.dns"),
+                         (True, "> dns:192.168.1.1 gw:192.168.1.1 lease:192.168.1.1"))
+        page = (ROOT / "webui/index.html").read_text(encoding="utf-8")
+        self.assertRegex(page, r'\["link\.dns","[^"\n]+",1\]')
+
+    def test_old_snapshot_and_restore_callbacks_lose_resolvers(self):
+        snapshot = extract_braced(self.source, "static esp_err_t snapshotDnsOnTcpipThread(")
+        # Reproduce the old write-before-validation callback. The retained
+        # self-copy on a valid lease has no effect; an empty renewal overwrites
+        # the snapshot before the captured flag can reject it.
+        old_snapshot = snapshot.replace("ip_addr_t servers[kDnsServers] = {};", "")
+        old_snapshot = old_snapshot.replace("servers[i]", "self->_dns_snapshot[i]")
+        controls = (
+            (self.source.replace("kDnsServers = DNS_MAX_SERVERS;", "kDnsServers = 2;"), "restore"),
+            (self.source.replace("dns_setserver(i, &self->_dns_snapshot[i]);",
+                                 "if (!ip_addr_isany_val(self->_dns_snapshot[i])) "
+                                 "dns_setserver(i, &self->_dns_snapshot[i]);"), "restore"),
+            (self.source.replace(snapshot, old_snapshot), "renewal"),
+        )
+        for index, (broken, scenario) in enumerate(controls):
+            with self.subTest(control=index):
+                self.assertNotEqual(broken, self.source)
+                binary = self.compile(broken, "broken-" + str(index))
+                result = subprocess.run([str(binary), scenario], capture_output=True,
+                                        text=True, timeout=10)
+                self.assertNotEqual(result.returncode, 0)
+                self.assertIn("DNS-free renewal" if scenario == "renewal" else "DNS restore",
+                              result.stderr)
+
+
+if __name__ == "__main__":
+    unittest.main()