Fix mdns object lifetimes: don't hold pointers we don't own

ports/espressif/common-hal/mdns/RemoteService.c

@@ -9,56 +9,27 @@
 #include "shared-bindings/ipaddress/IPv4Address.h"
 
 const char *common_hal_mdns_remoteservice_get_service_type(mdns_remoteservice_obj_t *self) {
-    if (self->result == NULL) {
-        return "";
-    }
-    return self->result->service_type;
+    return self->service_name;
 }
 
 const char *common_hal_mdns_remoteservice_get_protocol(mdns_remoteservice_obj_t *self) {
-    if (self->result == NULL) {
-        return "";
-    }
-    return self->result->proto;
+    return self->protocol;
 }
 
 const char *common_hal_mdns_remoteservice_get_instance_name(mdns_remoteservice_obj_t *self) {
-    if (self->result == NULL) {
-        return "";
-    }
-    return self->result->instance_name;
+    return self->instance_name;
 }
 
 const char *common_hal_mdns_remoteservice_get_hostname(mdns_remoteservice_obj_t *self) {
-    if (self->result == NULL) {
-        return "";
-    }
-    return self->result->hostname;
+    return self->hostname;
 }
 
 mp_int_t common_hal_mdns_remoteservice_get_port(mdns_remoteservice_obj_t *self) {
-    if (self->result == NULL) {
-        return 0;
-    }
-    return self->result->port;
+    return self->port;
 }
 
 uint32_t mdns_remoteservice_get_ipv4_address(mdns_remoteservice_obj_t *self) {
-    if (self->result == NULL ||
-        self->result->ip_protocol != MDNS_IP_PROTOCOL_V4 ||
-        self->result->addr == NULL) {
-        return 0;
-    }
-    mdns_ip_addr_t *cur = self->result->addr;
-    while (cur != NULL) {
-        if (cur->addr.type == ESP_IPADDR_TYPE_V4) {
-            return cur->addr.u_addr.ip4.addr;
-        }
-
-        cur = cur->next;
-    }
-
-    return 0;
+    return self->ipv4_address;
 }
 
 mp_obj_t common_hal_mdns_remoteservice_get_ipv4_address(mdns_remoteservice_obj_t *self) {
@@ -68,8 +39,3 @@ mp_obj_t common_hal_mdns_remoteservice_get_ipv4_address(mdns_remoteservice_obj_t
     }
     return common_hal_ipaddress_new_ipv4address(addr);
 }
-
-void common_hal_mdns_remoteservice_deinit(mdns_remoteservice_obj_t *self) {
-    mdns_query_results_free(self->result);
-    self->result = NULL;
-}

ports/espressif/common-hal/mdns/RemoteService.h

@@ -8,7 +8,15 @@
 
 #include "mdns.h"
 
+// The IDF's mdns_result_t lives on the IDF heap and is only valid while mdns
+// is inited. Copy what we need into the object instead so that the object's
+// lifetime is independent of the mdns.Server's.
 typedef struct {
     mp_obj_base_t base;
-    mdns_result_t *result;
+    uint32_t ipv4_address;
+    uint16_t port;
+    char protocol[5]; // RFC 6763 Section 7.2 - 4 bytes + 1 for NUL
+    char service_name[17]; // RFC 6763 Section 7.2 - 16 bytes + 1 for NUL
+    char instance_name[64]; // RFC 6763 Section 7.2 - 63 bytes + 1 for NUL
+    char hostname[64]; // RFC 6762 Appendix A - 63 bytes for label + 1 for NUL
 } mdns_remoteservice_obj_t;

ports/espressif/common-hal/mdns/Server.c

@@ -6,6 +6,8 @@
 
 #include "shared-bindings/mdns/Server.h"
 
+#include <string.h>
+
 #include "py/gc.h"
 #include "py/runtime.h"
 #include "shared-bindings/mdns/RemoteService.h"
@@ -18,6 +20,13 @@
 // could be created.)
 static mdns_server_obj_t *_active_object = NULL;
 
+// strlcpy(), but a NULL src yields an empty string instead of undefined
+// behavior. The IDF leaves mdns_result_t fields NULL when a query didn't
+// resolve them.
+static void strlcpy_or_empty(char *dest, const char *src, size_t dest_len) {
+    strlcpy(dest, src == NULL ? "" : src, dest_len);
+}
+
 void mdns_server_construct(mdns_server_obj_t *self, bool workflow) {
     if (_active_object != NULL) {
         if (self == _active_object) {
@@ -33,9 +42,12 @@ void mdns_server_construct(mdns_server_obj_t *self, bool workflow) {
     }
     _active_object = self;
 
+    self->instance_name[0] = '\0';
+
     // Match the netif hostname set when `import wifi` was called.
-    esp_netif_get_hostname(common_hal_wifi_radio_obj.netif, &self->hostname);
-    common_hal_mdns_server_set_hostname(self, self->hostname);
+    const char *netif_hostname;
+    esp_netif_get_hostname(common_hal_wifi_radio_obj.netif, &netif_hostname);
+    common_hal_mdns_server_set_hostname(self, netif_hostname);
 
     self->inited = true;
 
@@ -95,19 +107,40 @@ void common_hal_mdns_server_set_hostname(mdns_server_obj_t *self, const char *ho
     while (!mdns_hostname_exists(hostname)) {
         RUN_BACKGROUND_TASKS;
     }
-    self->hostname = hostname;
+    strlcpy_or_empty(self->hostname, hostname, sizeof(self->hostname));
 }
 
 const char *common_hal_mdns_server_get_instance_name(mdns_server_obj_t *self) {
-    if (self->instance_name == NULL) {
+    if (self->instance_name[0] == '\0') {
         return self->hostname;
     }
     return self->instance_name;
 }
 
 void common_hal_mdns_server_set_instance_name(mdns_server_obj_t *self, const char *instance_name) {
     mdns_instance_name_set(instance_name);
-    self->instance_name = instance_name;
+    strlcpy_or_empty(self->instance_name, instance_name, sizeof(self->instance_name));
+}
+
+// Copy everything we expose out of the IDF's result so that the RemoteService
+// no longer references IDF-owned memory. The caller is responsible for freeing
+// the result itself.
+static void copy_data_into_remote_service(mdns_result_t *result, mdns_remoteservice_obj_t *out) {
+    out->base.type = &mdns_remoteservice_type;
+    out->port = result->port;
+    out->ipv4_address = 0;
+    if (result->ip_protocol == MDNS_IP_PROTOCOL_V4) {
+        for (mdns_ip_addr_t *cur = result->addr; cur != NULL; cur = cur->next) {
+            if (cur->addr.type == ESP_IPADDR_TYPE_V4) {
+                out->ipv4_address = cur->addr.u_addr.ip4.addr;
+                break;
+            }
+        }
+    }
+    strlcpy_or_empty(out->protocol, result->proto, sizeof(out->protocol));
+    strlcpy_or_empty(out->service_name, result->service_type, sizeof(out->service_name));
+    strlcpy_or_empty(out->instance_name, result->instance_name, sizeof(out->instance_name));
+    strlcpy_or_empty(out->hostname, result->hostname, sizeof(out->hostname));
 }
 
 size_t mdns_server_find(mdns_server_obj_t *self, const char *service_type, const char *protocol,
@@ -123,23 +156,15 @@ size_t mdns_server_find(mdns_server_obj_t *self, const char *service_type, const
     }
     mdns_query_async_delete(search);
     mdns_result_t *next = results;
-    // Don't error if we're out of memory. Instead, truncate the tuple.
-    uint8_t added = 0;
+    // Truncate if we don't have space for everything the IDF found.
+    size_t added = 0;
     while (next != NULL && added < out_len) {
-        mdns_remoteservice_obj_t *service = &out[added];
-
-        service->result = next;
-        service->base.type = &mdns_remoteservice_type;
+        copy_data_into_remote_service(next, &out[added]);
         next = next->next;
-        // Break the linked list so we free each result separately.
-        service->result->next = NULL;
         added++;
     }
-    if (added < out_len) {
-        // Free the remaining results from the IDF because we don't have
-        // enough space in Python.
-        mdns_query_results_free(next);
-    }
+    // We've copied out everything we need, so release the IDF's copy.
+    mdns_query_results_free(results);
     return num_results;
 }
 
@@ -158,36 +183,40 @@ mp_obj_t common_hal_mdns_server_find(mdns_server_obj_t *self, const char *servic
     // The empty tuple object is shared and stored in flash so return early if
     // we got it. Without this we'll crash when trying to set len below.
     if (num_results == 0) {
+        mdns_query_results_free(results);
         return MP_OBJ_FROM_PTR(tuple);
     }
     mdns_result_t *next = results;
     // Don't error if we're out of memory. Instead, truncate the tuple.
     uint8_t added = 0;
     while (next != NULL) {
-        mdns_remoteservice_obj_t *service = gc_alloc(sizeof(mdns_remoteservice_obj_t), GC_ALLOC_FLAG_HAS_FINALISER);
+        mdns_remoteservice_obj_t *service = m_malloc_maybe(sizeof(mdns_remoteservice_obj_t));
         if (service == NULL) {
             if (added == 0) {
+                mdns_query_results_free(results);
                 m_malloc_fail(sizeof(mdns_remoteservice_obj_t));
             }
-            // Free the remaining results from the IDF because we don't have
-            // enough space in Python.
-            mdns_query_results_free(next);
             break;
         }
-        service->result = next;
-        service->base.type = &mdns_remoteservice_type;
+        copy_data_into_remote_service(next, service);
         next = next->next;
-        // Break the linked list so we free each result separately.
-        service->result->next = NULL;
         tuple->items[added] = MP_OBJ_FROM_PTR(service);
         added++;
     }
     tuple->len = added;
 
+    // We've copied out everything we need, so release the IDF's copy.
+    mdns_query_results_free(results);
+
     return MP_OBJ_FROM_PTR(tuple);
 }
 
 void common_hal_mdns_server_advertise_service(mdns_server_obj_t *self, const char *service_type, const char *protocol, mp_int_t port, const char *txt_records[], size_t num_txt_records) {
+    // Reject rather than silently drop them. See the TODO below.
+    if (num_txt_records > 0) {
+        mp_raise_NotImplementedError_varg(MP_ERROR_TEXT("%q"), MP_QSTR_txt_records);
+    }
+
     if (mdns_service_exists(service_type, protocol, NULL)) {
         mdns_service_port_set(service_type, protocol, port);
     } else {

ports/espressif/common-hal/mdns/Server.h

@@ -10,8 +10,11 @@
 
 typedef struct {
     mp_obj_base_t base;
-    const char *hostname;
-    const char *instance_name;
+    // Store copies rather than the caller's pointers. The setters are handed
+    // GC-heap strings, which are recycled when the VM resets while this object
+    // (and the web workflow's static one) lives on.
+    char hostname[64]; // RFC 6762 Appendix A - 63 bytes for label + 1 for NUL
+    char instance_name[64]; // RFC 6763 Section 7.2 - 63 bytes + 1 for NUL
     // Track if this object owns access to the underlying MDNS service.
     bool inited;
 } mdns_server_obj_t;

ports/raspberrypi/common-hal/mdns/RemoteService.c

@@ -39,6 +39,3 @@ mp_obj_t common_hal_mdns_remoteservice_get_ipv4_address(mdns_remoteservice_obj_t
     }
     return common_hal_ipaddress_new_ipv4address(addr);
 }
-
-void common_hal_mdns_remoteservice_deinit(mdns_remoteservice_obj_t *self) {
-}

ports/raspberrypi/common-hal/mdns/Server.c

@@ -42,10 +42,15 @@ void mdns_server_construct(mdns_server_obj_t *self, bool workflow) {
     }
     self->inited = true;
 
+    self->instance_name[0] = '\0';
+    self->num_txt_records = 0;
+    self->txt_storage = NULL;
+
     uint8_t mac[6];
     wifi_radio_get_mac_address(&common_hal_wifi_radio_obj, mac);
-    snprintf(self->default_hostname, sizeof(self->default_hostname), "cpy-%02x%02x%02x", mac[3], mac[4], mac[5]);
-    common_hal_mdns_server_set_hostname(self, self->default_hostname);
+    char default_hostname[sizeof("cpy-XXXXXX")];
+    snprintf(default_hostname, sizeof(default_hostname), "cpy-%02x%02x%02x", mac[3], mac[4], mac[5]);
+    common_hal_mdns_server_set_hostname(self, default_hostname);
 
     if (workflow) {
         // Add a second host entry to respond to "circuitpython.local" queries as well.
@@ -91,15 +96,18 @@ void common_hal_mdns_server_set_hostname(mdns_server_obj_t *self, const char *ho
         mdns_resp_add_netif(NETIF_STA, hostname);
     }
 
-    self->hostname = hostname;
+    strlcpy(self->hostname, hostname, sizeof(self->hostname));
 }
 
 const char *common_hal_mdns_server_get_instance_name(mdns_server_obj_t *self) {
+    if (self->instance_name[0] == '\0') {
+        return self->hostname;
+    }
     return self->instance_name;
 }
 
 void common_hal_mdns_server_set_instance_name(mdns_server_obj_t *self, const char *instance_name) {
-    self->instance_name = instance_name;
+    strlcpy(self->instance_name, instance_name, sizeof(self->instance_name));
 }
 
 typedef struct {
@@ -288,15 +296,64 @@ static void srv_txt_cb(struct mdns_service *service, void *ptr) {
     }
 }
 
+// Take our own copies of the TXT records. lwip only stores srv_txt_cb and this
+// object, and calls back at packet-build time, so the caller's strings must
+// outlive the call -- and the caller hands us pointers into GC-heap strings.
+//
+// WARNING: the copies live on the GC heap, which is only safe because TXT
+// records can reach us solely through the Python binding, so the VM is
+// necessarily running and this object is itself a GC object that dies with the
+// same heap. The supervisor's static mdns_server_obj_t never gets TXT records
+// (web_workflow.c passes NULL, 0). If supervisor code ever needs to advertise
+// TXT records, this must move off the GC heap first -- an inline pool in
+// mdns_server_obj_t, or port_malloc -- or the records will dangle after the
+// first VM reset.
 static void assign_txt_records(mdns_server_obj_t *self, const char *txt_records[], size_t num_txt_records) {
-    size_t allowed_num_txt_records = MDNS_MAX_TXT_RECORDS < num_txt_records ? MDNS_MAX_TXT_RECORDS : num_txt_records;
-    self->num_txt_records = allowed_num_txt_records;
-    for (size_t i = 0; i < allowed_num_txt_records; i++) {
-        self->txt_records[i] = txt_records[i];
+    size_t total = 0;
+    for (size_t i = 0; i < num_txt_records; i++) {
+        total += strlen(txt_records[i]) + 1;
     }
+
+    // Build the replacement before touching self, so that the allocation, and
+    // any MemoryError it raises, happens outside the lwip lock below.
+    char *storage = NULL;
+    const char *records[MDNS_MAX_TXT_RECORDS];
+    if (total > 0) {
+        storage = m_malloc_maybe(total);
+        if (storage == NULL) {
+            m_malloc_fail(total);
+        }
+        char *next = storage;
+        for (size_t i = 0; i < num_txt_records; i++) {
+            size_t size = strlen(txt_records[i]) + 1;
+            memcpy(next, txt_records[i], size);
+            records[i] = next;
+            next += size;
+        }
+    }
+
+    // srv_txt_cb reads these from the lwip IRQ, so hold lwip off while they
+    // change. Otherwise a callback already part way through the old records
+    // keeps pointers into storage we are about to release.
+    MICROPY_PY_LWIP_ENTER
+    char *old_storage = self->txt_storage;
+    for (size_t i = 0; i < num_txt_records; i++) {
+        self->txt_records[i] = records[i];
+    }
+    self->txt_storage = storage;
+    self->num_txt_records = num_txt_records;
+    MICROPY_PY_LWIP_EXIT
+
+    // Nothing can be holding pointers into it now, so release it here rather
+    // than leaving the GC to do it at an unpredictable time.
+    m_free(old_storage);
 }
 
 void common_hal_mdns_server_advertise_service(mdns_server_obj_t *self, const char *service_type, const char *protocol, mp_int_t port, const char *txt_records[], size_t num_txt_records) {
+    // Check before touching any state, so a rejected call leaves the existing
+    // advertisement alone.
+    mp_arg_validate_length_max(num_txt_records, MDNS_MAX_TXT_RECORDS, MP_QSTR_txt_records);
+
     enum mdns_sd_proto proto = DNSSD_PROTO_UDP;
     if (strcmp(protocol, "_tcp") == 0) {
         proto = DNSSD_PROTO_TCP;
@@ -316,7 +373,7 @@ void common_hal_mdns_server_advertise_service(mdns_server_obj_t *self, const cha
     }
 
     assign_txt_records(self, txt_records, num_txt_records);
-    int8_t slot = mdns_resp_add_service(NETIF_STA, self->instance_name, service_type, proto, port, srv_txt_cb, self);
+    int8_t slot = mdns_resp_add_service(NETIF_STA, common_hal_mdns_server_get_instance_name(self), service_type, proto, port, srv_txt_cb, self);
     if (slot < 0) {
         mp_raise_RuntimeError(MP_ERROR_TEXT("Out of MDNS service slots"));
         return;