From d1386a8e9dcdeed4c50dd0628462b14c9295d304 Mon Sep 17 00:00:00 2001 From: Piotr Szulc Date: Fri, 8 Mar 2024 12:21:14 +0100 Subject: [PATCH] [libs] Fix mDNS string memory corruption, print error on record add failure (#260) * Fixed unsafe conversion to underscore string * Fixed formatting * Save one byte if underscore not needed * Don't allocate new string if already underscored * Fix missing first character while copying * Renamed function and made it inline * Don't use signed index variable when searching for service * Add proper cleanup of LwIPmDNS - Free allocated memory both on end() and in the destructor - Unregister callback from netif * Don't free const pointer * Removed unneeded casting * Don't break the loop if failed to add txt record * Fixed code formatting --- .../libraries/common/mDNS/LwIPmDNS.cpp | 47 +++++++++++++++---- .../arduino/libraries/common/mDNS/mDNS.cpp | 19 ++++++-- .../arduino/libraries/common/mDNS/mDNS.h | 1 + 3 files changed, 54 insertions(+), 13 deletions(-) diff --git a/cores/common/arduino/libraries/common/mDNS/LwIPmDNS.cpp b/cores/common/arduino/libraries/common/mDNS/LwIPmDNS.cpp index 0f602dbc5..0b8c435df 100644 --- a/cores/common/arduino/libraries/common/mDNS/LwIPmDNS.cpp +++ b/cores/common/arduino/libraries/common/mDNS/LwIPmDNS.cpp @@ -31,9 +31,34 @@ static const char *hostName; NETIF_DECLARE_EXT_CALLBACK(netif_callback) #endif +static inline void freeAllocatedStrings(const std::vector &strings) { + for (auto &str : strings) { + free(str); + } +} + mDNS::mDNS() {} -mDNS::~mDNS() {} +mDNS::~mDNS() { + cleanup(); +} + +void mDNS::cleanup() { + freeAllocatedStrings(services_name); + services_name.clear(); + freeAllocatedStrings(services); + services.clear(); + for (auto &record : records) { + freeAllocatedStrings(record); + } + records.clear(); + + free((void *)hostName); + hostName = NULL; + + free((void *)instanceName); + instanceName = NULL; +} static void mdnsTxtCallback(struct mdns_service *service, void *userdata) { size_t index = (size_t)userdata; @@ -42,8 +67,9 @@ static void mdnsTxtCallback(struct mdns_service *service, void *userdata) { for (const auto record : records[index]) { err_t err = mdns_resp_add_service_txtitem(service, record, strlen(record)); - if (err != ERR_OK) - return; + if (err != ERR_OK) { + LT_DM(MDNS, "Error %d while adding txt record: %s", err, record); + } } } @@ -136,12 +162,18 @@ bool mDNS::begin(const char *hostname) { } void mDNS::end() { +#ifdef LWIP_NETIF_EXT_STATUS_CALLBACK + netif_remove_ext_callback(&netif_callback); +#endif + struct netif *netif = netif_list; while (netif != NULL) { if (netif_is_up(netif)) mdns_resp_remove_netif(netif); netif = netif->next; } + + cleanup(); } bool mDNS::addServiceImpl(const char *name, const char *service, uint8_t proto, uint16_t port) { @@ -181,18 +213,17 @@ bool mDNS::addServiceImpl(const char *name, const char *service, uint8_t proto, } bool mDNS::addServiceTxtImpl(const char *service, uint8_t proto, const char *item) { - int8_t index = -1; - for (uint8_t i = 0; i < services.size(); i++) { + uint8_t i; + for (i = 0; i < services.size(); i++) { // find a matching service if (strcmp(services[i], service) == 0 && protos[i] == proto) { - index = i; break; } } - if (index == -1) + if (i == services.size()) return false; - records[index].push_back(strdup(item)); + records[i].push_back(strdup(item)); return true; } diff --git a/cores/common/arduino/libraries/common/mDNS/mDNS.cpp b/cores/common/arduino/libraries/common/mDNS/mDNS.cpp index 4de68fdea..75af2a4d1 100644 --- a/cores/common/arduino/libraries/common/mDNS/mDNS.cpp +++ b/cores/common/arduino/libraries/common/mDNS/mDNS.cpp @@ -2,14 +2,23 @@ #include "mDNS.h" -static char *ensureUnderscore(const char *value) { - uint8_t len = strlen(value) + 1; +static char *ensureUnderscore(char *value) { + if (value[0] == '_') { + return value; + } + size_t len = strlen(value) + 1 + 1; // 1 for underscore, 1 for null-terminator char *result = (char *)malloc(len); result[0] = '_'; - strcpy(result + 1, value + (value[0] == '_')); + strcpy(result + 1, value); return result; } +static inline void freeIfCopied(const char *original, char *duplicate) { + if ((duplicate) && (original != duplicate)) { + free(duplicate); + } +} + void mDNS::setInstanceName(const char *name) { if (instanceName) free(instanceName); @@ -21,7 +30,7 @@ bool mDNS::addService(char *service, char *proto, uint16_t port) { uint8_t _proto = strncmp(proto + (proto[0] == '_'), "tcp", 3) == 0 ? MDNS_TCP : MDNS_UDP; bool result = addServiceImpl(instanceName ? instanceName : "LT mDNS", _service, _proto, port); - free(_service); + freeIfCopied(service, _service); return result; } @@ -34,7 +43,7 @@ bool mDNS::addServiceTxt(char *service, char *proto, char *key, char *value) { sprintf(txt, "%s=%s", key, value); bool result = addServiceTxtImpl(_service, _proto, txt); - free(_service); + freeIfCopied(service, _service); free(txt); return result; } diff --git a/cores/common/arduino/libraries/common/mDNS/mDNS.h b/cores/common/arduino/libraries/common/mDNS/mDNS.h index 6a15cd39b..ec45010ca 100644 --- a/cores/common/arduino/libraries/common/mDNS/mDNS.h +++ b/cores/common/arduino/libraries/common/mDNS/mDNS.h @@ -51,6 +51,7 @@ class mDNS { private: bool addServiceImpl(const char *name, const char *service, uint8_t proto, uint16_t port); bool addServiceTxtImpl(const char *service, uint8_t proto, const char *item); + void cleanup(); char *instanceName = NULL;