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..1afccf4eb48 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,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; 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, "]}");