Overhaul SimpleMDNS handling of WiFi::begin/end (#3166)

Fixes #3157

LWIP MDNS was not being removed from a netif when a device ::end was
called, resulting in a crash on reconnect when MDNS or things like
ArduinoOTA (which uses SimpleMDNS) were used.

They would not crash, however, if the initial WiFi::begin failed, because
LWIP MDNS does not automatically add existing services to a "new" netif
(i.e. there was no MDNS on the 2nd and following WiFi.begin() calls).

To fix the crash due to null-derefernce, add a way for SimpleMDNS
to remove MSND from a netif before it is netif_removed.

To fix the lack of MDNS services after a WiFi reconnect, keep track of
the services we've already added and add a call to hook into LwipIntfDev
after a new netif has been brought up (link-state at least, if not
DHCP).

Author: earlephilhower

libraries/SimpleMDNS/src/SimpleMDNS.cpp

@@ -22,31 +22,73 @@
 #include <Arduino.h>
 #include "SimpleMDNS.h"
 #include <LwipEthernet.h>
-#include <lwip/apps/mdns.h>
+
+// LWIP MDNS doesn't expose a way for us to know if MDNS has been enabled on a netif, so use the private accessor.  Sorry...
+extern "C" struct mdns_host* netif_mdns_data(struct netif *netif);
 
 bool SimpleMDNS::begin(const char *hostname, unsigned int ttl) {
     (void) ttl;
 
     if (_running) {
         return false;
     }
-    mdns_resp_init();
+    if (!_lwipMSNDInitted) {
+        mdns_resp_init();
+        _lwipMSNDInitted = true;
+    }
     struct netif *n = netif_list;
     while (n) {
         mdns_resp_add_netif(n, hostname);
         n = n->next;
     }
     __setStateChangeCallback(_statusCB);
+    __setAddNetifCallback(_addNetifCB);
+    __setRemoveNetifCallback(_removeNetifCB);
+
     _hostname = strdup(hostname);
     _running = true;
 
     return true;
 }
 
+void SimpleMDNS::_addNetifCB(struct netif *n) {
+    MDNS.addNetif(n);
+}
+
+void SimpleMDNS::_removeNetifCB(struct netif *n) {
+    MDNS.removeNetif(n);
+}
+
+
+// Only call when __useSimpleMDNS == true!
+void SimpleMDNS::removeNetif(struct netif *n) {
+    if (netif_mdns_data(n)) {
+        mdns_resp_remove_netif(n);
+    }
+}
+
+// Only call when __useSimpleMDNS == true!
+void SimpleMDNS::addNetif(struct netif *n) {
+    mdns_resp_add_netif(n, _hostname);
+    for (auto svc : _svcMap) {
+        char s[128];
+        snprintf(s, sizeof(s), "_%s", svc.second->_service);
+        s[sizeof(s) - 1] = 0;
+        mdns_resp_add_service(n, _hostname, s, (mdns_sd_proto)svc.second->_proto, svc.second->_port, svc.second->_fn, svc.second->_userdata);
+    }
+}
+
 void SimpleMDNS::enableArduino(unsigned int port, bool passwd) {
     if (!_running || _arduinoAdded) {
         return;
     }
+    SimpleMDNSService *svc = new SimpleMDNSService();
+    svc->_service = "arduino";
+    svc->_proto = DNSSD_PROTO_TCP;
+    svc->_port = port;
+    svc->_fn = _arduinoGetTxt;
+    svc->_userdata = (void *)passwd;
+    _svcMap.insert({svc->_service, svc});
     struct netif *n = netif_list;
     while (n) {
         mdns_resp_add_service(n, _hostname, "_arduino", DNSSD_PROTO_TCP, port, _arduinoGetTxt, (void *)passwd);
@@ -67,10 +109,15 @@ hMDNSService SimpleMDNS::addService(const char *service, const char *proto, unsi
     snprintf(s, sizeof(s), "_%s", service);
     s[sizeof(s) - 1] = 0;
     SimpleMDNSService *svc = new SimpleMDNSService();
-    _svcMap.insert({strdup(service), svc});
+    svc->_service = strdup(service);
+    svc->_proto = !strcasecmp("tcp", proto) ? DNSSD_PROTO_TCP : DNSSD_PROTO_UDP;
+    svc->_port = port;
+    svc->_fn = SimpleMDNSService::callback;
+    svc->_userdata = (void *)svc;
+    _svcMap.insert({svc->_service, svc});
     struct netif *n = netif_list;
     while (n) {
-        mdns_resp_add_service(n, _hostname, s, !strcasecmp("tcp", proto) ? DNSSD_PROTO_TCP : DNSSD_PROTO_UDP, port, SimpleMDNSService::callback, (void *)svc);
+        mdns_resp_add_service(n, _hostname, s, (mdns_sd_proto)svc->_proto, svc->_port, svc->_fn, svc->_userdata);
         n = n->next;
     }
     return (hMDNSService*) service;
@@ -80,8 +127,22 @@ void SimpleMDNS::update() {
     /* No-op */
 }
 
+
 void SimpleMDNS::end() {
-    /* No-op */
+    if (_running) {
+        struct netif *n = netif_list;
+        while (n) {
+            if (netif_mdns_data(n)) {
+                mdns_resp_remove_netif(n);
+            }
+            n = n->next;
+        }
+        __setStateChangeCallback(nullptr);
+        __setAddNetifCallback(nullptr);
+        __setRemoveNetifCallback(nullptr);
+
+    }
+    _running = false;
 }
 
 void SimpleMDNS::_statusCB(struct netif *netif) {

libraries/SimpleMDNS/src/SimpleMDNS.h

@@ -24,6 +24,7 @@
 #include <string>
 #include <map>
 #include <vector>
+#include <lwip/apps/mdns.h>
 
 typedef void* hMDNSTxt; // Unusable in SimpleMDNS, for signature compatibility only
 
@@ -32,6 +33,13 @@ class SimpleMDNSService {
     SimpleMDNSService();
     static void callback(struct mdns_service *service, void *txt_userdata);
     hMDNSTxt add(const char *key, const char *val);
+
+    const char *_service;
+    uint16_t _proto;
+    uint16_t _port;
+    service_get_txt_fn_t _fn;
+    void *_userdata;
+
 private:
     std::vector<const char *> txt;
 };
@@ -60,8 +68,9 @@ class SimpleMDNS {
     hMDNSTxt addServiceTxt(const hMDNSService p_hService, const char* p_pcKey, int16_t p_i16Value);
     hMDNSTxt addServiceTxt(const hMDNSService p_hService, const char* p_pcKey, int8_t p_i8Value);
 
-    // No-ops here
     void end();
+
+    // No-ops here
     void update();
 
 private:
@@ -70,9 +79,16 @@ class SimpleMDNS {
     static void _arduinoGetTxt(struct mdns_service *service, void *txt_userdata);
 
     bool _running = false;
+    bool _lwipMSNDInitted = false;
     static const char *_hostname;
     std::map<std::string, SimpleMDNSService*> _svcMap;
     bool _arduinoAdded = false;
+
+    // LwipIntfDev helpers when netifs come up and down, to ensure we set the old services on the new netif
+    static void _addNetifCB(struct netif *n);
+    static void _removeNetifCB(struct netif *n);
+    void removeNetif(struct netif *n);
+    void addNetif(struct netif *n);
 };
 
 extern SimpleMDNS MDNS;

libraries/lwIP_Ethernet/src/LwipEthernet.cpp

@@ -293,3 +293,13 @@ std::function<void(struct netif *)>  _scb;
 void __setStateChangeCallback(std::function<void(struct netif *)> s) {
     _scb = s;
 }
+
+std::function<void(struct netif *)> _addNetifCB;
+void __setAddNetifCallback(std::function<void(struct netif *)> s) {
+    _addNetifCB = s;
+}
+
+std::function<void(struct netif *)> _removeNetifCB;
+void __setRemoveNetifCallback(std::function<void(struct netif *)> s) {
+    _removeNetifCB = s;
+}

libraries/lwIP_Ethernet/src/LwipEthernet.h

@@ -47,3 +47,7 @@ void lwipPollingPeriod(int ms);
 
 // Sets the global netif state change callback
 void __setStateChangeCallback(std::function<void(struct netif *)> s);
+
+// Set callback when a netif is added after bring-up, removed before netif_remove
+void __setAddNetifCallback(std::function<void(struct netif *)> s);
+void __setRemoveNetifCallback(std::function<void(struct netif *)> s);

libraries/lwIP_Ethernet/src/LwipIntfDev.h

@@ -55,7 +55,6 @@
 #define DEFAULT_MTU 1500
 #endif
 
-
 // Dup'd to avoid CYW43 dependency
 // Generate a mac address if one is not set in otp
 static void _cyw43_hal_generate_laa_mac(__unused int idx, uint8_t buf[6]) {
@@ -340,6 +339,7 @@ bool LwipIntfDev<RawDev>::config(IPAddress local_ip, IPAddress dns) {
 }
 
 extern char wifi_station_hostname[];
+extern std::function<void(struct netif *)> _addNetifCB;
 template<class RawDev>
 bool LwipIntfDev<RawDev>::begin(const uint8_t* macAddress, const uint16_t mtu) {
     if (_started) {
@@ -369,15 +369,9 @@ bool LwipIntfDev<RawDev>::begin(const uint8_t* macAddress, const uint16_t mtu) {
                 _netif.num = n->num + 1;
             }
 
-#if 1
         // forge a new mac-address from the esp's wifi sta one
         // I understand this is cheating with an official mac-address
         _cyw43_hal_generate_laa_mac(0, _macAddress);
-#else
-        // https://serverfault.com/questions/40712/what-range-of-mac-addresses-can-i-safely-use-for-my-virtual-machines
-        memset(_macAddress, 0, 6);
-        _macAddress[0] = 0xEE;
-#endif
         _macAddress[3] += _netif.num;  // alter base mac address
         _macAddress[0] &= 0xfe;        // set as locally administered, unicast, per
         _macAddress[0] |= 0x02;  // https://en.wikipedia.org/wiki/MAC_address#Universal_vs._local
@@ -459,9 +453,15 @@ bool LwipIntfDev<RawDev>::begin(const uint8_t* macAddress, const uint16_t mtu) {
         }
     }
 
+    if (_addNetifCB) {
+        _addNetifCB(&_netif);
+    }
     return true;
 }
 
+
+extern std::function<void(struct netif *)> _removeNetifCB;
+
 template<class RawDev>
 void LwipIntfDev<RawDev>::end() {
     if (_started) {
@@ -472,6 +472,10 @@ void LwipIntfDev<RawDev>::end() {
             __removeEthernetGPIO(_intrPin);
         }
 
+        if (_removeNetifCB) {
+            _removeNetifCB(&_netif);
+        }
+
         RawDev::end();
 
         netif_remove(&_netif);
@@ -493,11 +497,6 @@ void LwipIntfDev<RawDev>::_irq(void *param) {
     LwipIntfDev *d = static_cast<LwipIntfDev*>(param);
     ethernet_arch_lwip_gpio_mask(); // Disable other IRQs until we're done processing this one
     lwip_callback(_lwipCallback, param, &d->_irqBuffer);
-    //ethernet_arch_lwip_begin();
-    //    d->handlePackets();
-    //    sys_check_timeouts();
-    //ethernet_arch_lwip_end();
-
 }
 
 template<class RawDev>
@@ -658,9 +657,6 @@ err_t LwipIntfDev<RawDev>::handlePackets() {
         }
 
         _packetsReceived++;
-        //printf("recv pkt %d: ", tot_len);
-        //for (int i=0; i < tot_len; i++)  printf("%02x ", ((uint8_t*)pbuf->payload)[i]);
-        //printf("\n");
         err_t err = _netif.input(pbuf, &_netif);
 
 #if PHY_HAS_CAPTURE