From 88868528466e8c774c2ddc10a85c86aba341795b Mon Sep 17 00:00:00 2001 From: Dan Halbert Date: Thu, 6 Aug 2026 10:27:00 -0400 Subject: [PATCH 1/2] Fix mdns object lifetimes: don't hold pointers we don't own mdns.RemoteService and mdns.Server both stored borrowed pointers whose owners could die first. Copy the data out instead. RemoteService held an IDF mdns_result_t allocated on the IDF heap. GC finalisers run in heap-address order, and the Server is allocated before the RemoteServices that find() returns, so at VM teardown the Server's __del__ ran first: mdns_free() deletes _mdns_service_semaphore, and the subsequent mdns_query_results_free() then called xSemaphoreTake(NULL) and hard faulted. Copy the fields we expose into the object at find() time and free the IDF results immediately, mirroring what raspberrypi already did. RemoteService then needs no finaliser at all, so the ordering problem stops existing rather than being worked around. mdns_server_obj_t stored the hostname and instance_name pointers handed to it by mp_obj_str_get_str(). Those point into the GC heap, which is recycled on VM reset while the web workflow's static mdns_server_obj_t lives on, so /cp/version.json served whatever landed in that memory next -- often fragments of REPL input. Store fixed-size copies instead. On raspberrypi, advertise_service() stashed borrowed txt_records pointers that lwip dereferences later from srv_txt_cb() at packet-build time, so a collection between advertising and being queried published freed memory. Pack owned copies into a single buffer. TXT records can only arrive through the Python binding, so the GC heap is necessarily available; assign_txt_records() carries a warning explaining what would break that assumption. Also make both ports reject TXT records they can't honour instead of silently truncating: more than 32 raises ValueError on raspberrypi, and espressif raises NotImplementedError rather than accepting and discarding them. Fix the advertise_service docstring signature, which omitted txt_records even though the parameter list documented it. --- .../espressif/common-hal/mdns/RemoteService.c | 46 ++--------- .../espressif/common-hal/mdns/RemoteService.h | 10 ++- ports/espressif/common-hal/mdns/Server.c | 81 +++++++++++++------ ports/espressif/common-hal/mdns/Server.h | 7 +- .../common-hal/mdns/RemoteService.c | 3 - ports/raspberrypi/common-hal/mdns/Server.c | 65 ++++++++++++--- ports/raspberrypi/common-hal/mdns/Server.h | 11 ++- shared-bindings/mdns/RemoteService.c | 14 ---- shared-bindings/mdns/RemoteService.h | 1 - shared-bindings/mdns/Server.c | 13 ++- supervisor/shared/web_workflow/web_workflow.c | 1 - 11 files changed, 149 insertions(+), 103 deletions(-) diff --git a/ports/espressif/common-hal/mdns/RemoteService.c b/ports/espressif/common-hal/mdns/RemoteService.c index 515a3f7bf02..3d80199397e 100644 --- a/ports/espressif/common-hal/mdns/RemoteService.c +++ b/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; -} diff --git a/ports/espressif/common-hal/mdns/RemoteService.h b/ports/espressif/common-hal/mdns/RemoteService.h index 6fb3000c7d1..89ff69be29d 100644 --- a/ports/espressif/common-hal/mdns/RemoteService.h +++ b/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; diff --git a/ports/espressif/common-hal/mdns/Server.c b/ports/espressif/common-hal/mdns/Server.c index e8c34ee0885..7eaa9a442a2 100644 --- a/ports/espressif/common-hal/mdns/Server.c +++ b/ports/espressif/common-hal/mdns/Server.c @@ -6,6 +6,8 @@ #include "shared-bindings/mdns/Server.h" +#include + #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,11 +107,11 @@ 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; @@ -107,7 +119,28 @@ const char *common_hal_mdns_server_get_instance_name(mdns_server_obj_t *self) { 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 { diff --git a/ports/espressif/common-hal/mdns/Server.h b/ports/espressif/common-hal/mdns/Server.h index f364a539c91..e7bfabb9528 100644 --- a/ports/espressif/common-hal/mdns/Server.h +++ b/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; diff --git a/ports/raspberrypi/common-hal/mdns/RemoteService.c b/ports/raspberrypi/common-hal/mdns/RemoteService.c index 650e86da9d9..3d80199397e 100644 --- a/ports/raspberrypi/common-hal/mdns/RemoteService.c +++ b/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) { -} diff --git a/ports/raspberrypi/common-hal/mdns/Server.c b/ports/raspberrypi/common-hal/mdns/Server.c index ac0c73389b1..41ab0d3dc3b 100644 --- a/ports/raspberrypi/common-hal/mdns/Server.c +++ b/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,54 @@ 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]; + // Stop the callback from reading the old records while we swap them out. + self->num_txt_records = 0; + self->txt_storage = NULL; + + size_t total = 0; + for (size_t i = 0; i < num_txt_records; i++) { + total += strlen(txt_records[i]) + 1; + } + if (total == 0) { + return; } + + // Dropping the old storage is enough; the GC reclaims it. Freeing it here + // could pull it out from under an in-flight srv_txt_cb. + char *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); + self->txt_records[i] = next; + next += size; + } + + self->txt_storage = storage; + self->num_txt_records = num_txt_records; } 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 +363,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; diff --git a/ports/raspberrypi/common-hal/mdns/Server.h b/ports/raspberrypi/common-hal/mdns/Server.h index 620e201c897..041add08818 100644 --- a/ports/raspberrypi/common-hal/mdns/Server.h +++ b/ports/raspberrypi/common-hal/mdns/Server.h @@ -14,11 +14,16 @@ typedef struct { mp_obj_base_t base; - const char *hostname; - const char *instance_name; - char default_hostname[sizeof("cpy-XXXXXX")]; + // 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 const char *service_type[MDNS_MAX_SERVICES]; size_t num_txt_records; + // Owned copies of the TXT records, packed NUL-separated into txt_storage, + // which lives on the GC heap. See the warning on assign_txt_records(). + char *txt_storage; const char *txt_records[MDNS_MAX_TXT_RECORDS]; // Track if this object owns access to the underlying MDNS service. bool inited; diff --git a/shared-bindings/mdns/RemoteService.c b/shared-bindings/mdns/RemoteService.c index b3ee3d7f7b7..b00313729fc 100644 --- a/shared-bindings/mdns/RemoteService.c +++ b/shared-bindings/mdns/RemoteService.c @@ -93,18 +93,6 @@ MP_DEFINE_CONST_FUN_OBJ_1(mdns_remoteservice_get_ipv4_address_obj, _mdns_remotes MP_PROPERTY_GETTER(mdns_remoteservice_ipv4_address_obj, (mp_obj_t)&mdns_remoteservice_get_ipv4_address_obj); -//| def __del__(self) -> None: -//| """Deletes the RemoteService object.""" -//| ... -//| -//| -static mp_obj_t mdns_remoteservice_obj_deinit(mp_obj_t self_in) { - mdns_remoteservice_obj_t *self = MP_OBJ_TO_PTR(self_in); - common_hal_mdns_remoteservice_deinit(self); - return mp_const_none; -} -static MP_DEFINE_CONST_FUN_OBJ_1(mdns_remoteservice_deinit_obj, mdns_remoteservice_obj_deinit); - static const mp_rom_map_elem_t mdns_remoteservice_locals_dict_table[] = { { MP_ROM_QSTR(MP_QSTR_hostname), MP_ROM_PTR(&mdns_remoteservice_hostname_obj) }, { MP_ROM_QSTR(MP_QSTR_instance_name), MP_ROM_PTR(&mdns_remoteservice_instance_name_obj) }, @@ -112,8 +100,6 @@ static const mp_rom_map_elem_t mdns_remoteservice_locals_dict_table[] = { { MP_ROM_QSTR(MP_QSTR_protocol), MP_ROM_PTR(&mdns_remoteservice_protocol_obj) }, { MP_ROM_QSTR(MP_QSTR_port), MP_ROM_PTR(&mdns_remoteservice_port_obj) }, { MP_ROM_QSTR(MP_QSTR_ipv4_address), MP_ROM_PTR(&mdns_remoteservice_ipv4_address_obj) }, - - { MP_ROM_QSTR(MP_QSTR___del__), MP_ROM_PTR(&mdns_remoteservice_deinit_obj) }, }; static MP_DEFINE_CONST_DICT(mdns_remoteservice_locals_dict, mdns_remoteservice_locals_dict_table); diff --git a/shared-bindings/mdns/RemoteService.h b/shared-bindings/mdns/RemoteService.h index a5ffe897e4e..c7efa64083d 100644 --- a/shared-bindings/mdns/RemoteService.h +++ b/shared-bindings/mdns/RemoteService.h @@ -19,7 +19,6 @@ const char *common_hal_mdns_remoteservice_get_instance_name(mdns_remoteservice_o const char *common_hal_mdns_remoteservice_get_hostname(mdns_remoteservice_obj_t *self); mp_int_t common_hal_mdns_remoteservice_get_port(mdns_remoteservice_obj_t *self); mp_obj_t common_hal_mdns_remoteservice_get_ipv4_address(mdns_remoteservice_obj_t *self); -void common_hal_mdns_remoteservice_deinit(mdns_remoteservice_obj_t *self); // For internal use. uint32_t mdns_remoteservice_get_ipv4_address(mdns_remoteservice_obj_t *self); diff --git a/shared-bindings/mdns/Server.c b/shared-bindings/mdns/Server.c index 86fb2eb6d7a..1c16348d425 100644 --- a/shared-bindings/mdns/Server.c +++ b/shared-bindings/mdns/Server.c @@ -150,7 +150,14 @@ static mp_obj_t _mdns_server_find(mp_uint_t n_args, const mp_obj_t *pos_args, mp } static MP_DEFINE_CONST_FUN_OBJ_KW(mdns_server_find_obj, 1, _mdns_server_find); -//| def advertise_service(self, *, service_type: str, protocol: str, port: int) -> None: +//| def advertise_service( +//| self, +//| *, +//| service_type: str, +//| protocol: str, +//| port: int, +//| txt_records: Optional[Sequence[str]] = None, +//| ) -> None: //| """Respond to queries for the given service with the given port. //| //| ``service_type`` and ``protocol`` can only occur on one port. Any call after the first @@ -158,8 +165,8 @@ static MP_DEFINE_CONST_FUN_OBJ_KW(mdns_server_find_obj, 1, _mdns_server_find); //| //| If web workflow is active, the port it uses can't also be used to advertise a service. //| -//| **Limitations**: Publishing up to 32 TXT records is only supported on the RP2040 Pico W board at -//| this time. +//| **Limitations**: Publishing TXT records (up to 32) is supported only on RP2xxx. +//| There is currently no TXT record support on Espressif boards. //| //| :param str service_type: The service type such as "_http" //| :param str protocol: The service protocol such as "_tcp" diff --git a/supervisor/shared/web_workflow/web_workflow.c b/supervisor/shared/web_workflow/web_workflow.c index 278823676ef..f88e3322e23 100644 --- a/supervisor/shared/web_workflow/web_workflow.c +++ b/supervisor/shared/web_workflow/web_workflow.c @@ -884,7 +884,6 @@ static void _reply_with_devices_json(socketpool_socket_obj_t *socket, _request * "\"instance_name\": \"%s\", " "\"port\": %d, " "\"ip\": \"%d.%d.%d.%d\"}", hostname, instance_name, port, octets[0], octets[1], octets[2], octets[3]); - common_hal_mdns_remoteservice_deinit(&found_devices[i]); } #endif _send_chunk(socket, "]}"); From 64a5e8d4a053c81cd95e5265e4d074d9cd2770a0 Mon Sep 17 00:00:00 2001 From: Dan Halbert Date: Thu, 6 Aug 2026 14:02:51 -0400 Subject: [PATCH 2/2] Hold the lwip lock while swapping mdns TXT records srv_txt_cb() runs from the lwip IRQ -- raspberrypi builds cyw43_arch_threadsafe_background -- so it can preempt the VM thread part way through reading self->txt_records[]. Zeroing num_txt_records first doesn't help a callback that already loaded a nonzero count. Build the replacement buffer first, so the allocation and any MemoryError stay outside the lock, then swap the pointers under MICROPY_PY_LWIP_ENTER/EXIT and free the old storage once nothing can reference it. Dropping the reference and leaving it to the GC, as before, did not avoid that hazard -- it only deferred the free to an unpredictable moment. --- ports/raspberrypi/common-hal/mdns/Server.c | 46 +++++++++++++--------- 1 file changed, 28 insertions(+), 18 deletions(-) diff --git a/ports/raspberrypi/common-hal/mdns/Server.c b/ports/raspberrypi/common-hal/mdns/Server.c index 41ab0d3dc3b..1afccf4eb48 100644 --- a/ports/raspberrypi/common-hal/mdns/Server.c +++ b/ports/raspberrypi/common-hal/mdns/Server.c @@ -309,34 +309,44 @@ static void srv_txt_cb(struct mdns_service *service, void *ptr) { // 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) { - // Stop the callback from reading the old records while we swap them out. - self->num_txt_records = 0; - self->txt_storage = NULL; - size_t total = 0; for (size_t i = 0; i < num_txt_records; i++) { total += strlen(txt_records[i]) + 1; } - if (total == 0) { - return; - } - // Dropping the old storage is enough; the GC reclaims it. Freeing it here - // could pull it out from under an in-flight srv_txt_cb. - char *storage = m_malloc_maybe(total); - if (storage == NULL) { - m_malloc_fail(total); + // 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; + } } - char *next = storage; + + // 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++) { - size_t size = strlen(txt_records[i]) + 1; - memcpy(next, txt_records[i], size); - self->txt_records[i] = next; - next += size; + 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) {