[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

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<char *> &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)
+		if (err != ERR_OK) {
-			return;
+			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;
+	uint8_t i;
-	for (uint8_t i = 0; i < services.size(); 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;
 }
 

cores/common/arduino/libraries/common/mDNS/mDNS.cpp

@@ -2,14 +2,23 @@
 
 #include "mDNS.h"
 
-static char *ensureUnderscore(const char *value) {
+static char *ensureUnderscore(char *value) {
-	uint8_t len	 = strlen(value) + 1;
+	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;
 }

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;