From c78186236252d9d19de5415dcdb5a6c7d4d26045 Mon Sep 17 00:00:00 2001 From: agessaman Date: Sat, 19 Sep 2026 15:45:39 -0700 Subject: [PATCH] fix(network): keep the DNS snapshot typed and on the lwIP thread Follow-up to the DNS restore, from a second review round. lwIP's DNS functions belong to the TCP/IP thread, and both halves were called from elsewhere: the Ethernet snapshot from the Arduino event task, the Wi-Fi snapshot and every restore from the MQTT task. Both now go through esp_netif_tcpip_exec(), which is also what serializes the snapshot itself, so it no longer needs atomics. Neither caller is the TCP/IP thread, so the call cannot deadlock on itself. The snapshot keeps the whole ip_addr_t instead of an IPv4 word. These builds compile lwIP with IPv6 enabled, so reinterpreting a v6 server as v4 would have installed four meaningless bytes as a resolver. RDNSS is disabled here, so a v6 server should not arise today; storing the tagged value means it stays correct if that changes. Restore now writes every slot, empty ones included, so returning to a one-server network cannot leave the other medium's second server behind as a fallback that only fails slowly. A lease that carried no resolver at all is not recorded, so a medium that has never had DNS leaves the current resolver alone instead of wiping it. --- src/helpers/NetworkLink.cpp | 56 ++++++++++++++++++++++++++++--------- 1 file changed, 43 insertions(+), 13 deletions(-) diff --git a/src/helpers/NetworkLink.cpp b/src/helpers/NetworkLink.cpp index 3590ffa8..ba87b35a 100644 --- a/src/helpers/NetworkLink.cpp +++ b/src/helpers/NetworkLink.cpp @@ -11,6 +11,7 @@ #include #include +#include #include #if defined(NETWORK_PREFER_ETHERNET) @@ -42,7 +43,9 @@ class NetworkLinkBase : public NetworkLink { bool _last_connected = false; unsigned long _last_status_check = 0; static constexpr int kDnsServers = 2; - std::atomic _dns_snapshot[kDnsServers] = {{0}, {0}}; + // Only ever touched on the lwIP thread (see snapshotDns()), so no atomics. + ip_addr_t _dns_snapshot[kDnsServers] = {}; + bool _dns_captured = false; AlertFaultPolicy::OutageSnapshot outage() const { return AlertFaultPolicy::unpackOutageSnapshot( @@ -86,6 +89,7 @@ class NetworkLinkBase : public NetworkLink { return outage(); } + private: // Remember the resolver this link's DHCP lease installed, so switching back // to it can put that resolver back. // @@ -98,26 +102,52 @@ class NetworkLinkBase : public NetworkLink { // different subnets that server is unreachable from Ethernet, and the node // reads as connected while every broker and NTP hostname fails to resolve, // until Ethernet's own DHCP renewal happens to fix it hours later. - void snapshotDns() { + // + // Both halves run through esp_netif_tcpip_exec() because lwIP's DNS functions + // belong to the TCP/IP thread, and these are called from the Arduino event + // task (Ethernet GOT_IP) and the MQTT task (Wi-Fi edge, and select()). That + // also serializes the snapshot itself, so it needs no atomics. Neither caller + // is the TCP/IP thread, so the call cannot deadlock on itself. + // + // The whole ip_addr_t is kept, not an IPv4 word: these builds compile lwIP + // with IPv6 on, and reinterpreting a v6 server as v4 would install four + // meaningless bytes as a resolver. (RDNSS is disabled here, so a v6 server + // should never appear — storing the tagged value means it stays correct if + // that ever changes.) + static esp_err_t snapshotDnsOnTcpipThread(void* ctx) { + NetworkLinkBase* self = static_cast(ctx); + bool any = false; for (int i = 0; i < kDnsServers; i++) { const ip_addr_t* server = dns_getserver(i); - _dns_snapshot[i].store( - (server != nullptr && !ip_addr_isany(server)) - ? ip4_addr_get_u32(ip_2_ip4(server)) : 0, - std::memory_order_relaxed); + self->_dns_snapshot[i] = (server != nullptr) ? *server : *IP_ADDR_ANY; + if (!ip_addr_isany_val(self->_dns_snapshot[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; + return ESP_OK; } - // Only servers this link actually leased are restored; a medium that has - // never held a lease leaves the current resolver alone rather than blanking it. - void restoreDns() const { + static esp_err_t restoreDnsOnTcpipThread(void* ctx) { + NetworkLinkBase* self = static_cast(ctx); + if (!self->_dns_captured) return ESP_OK; + // Every slot is written, empty ones included: returning to a network with + // one server must not leave the other medium's second server behind as a + // fallback that only fails slowly. for (int i = 0; i < kDnsServers; i++) { - const uint32_t addr = _dns_snapshot[i].load(std::memory_order_relaxed); - if (addr == 0) continue; - const ip_addr_t server = IPADDR4_INIT(addr); - dns_setserver(i, &server); + dns_setserver(i, &self->_dns_snapshot[i]); } + return ESP_OK; } + + public: + void snapshotDns() { esp_netif_tcpip_exec(snapshotDnsOnTcpipThread, this); } + + // A medium that has never held a lease with a resolver leaves the current one + // alone rather than blanking it. + void restoreDns() { esp_netif_tcpip_exec(restoreDnsOnTcpipThread, this); } + }; class WiFiNetworkLink final : public NetworkLinkBase {