From d8c9530d3cc9773ce0fb76671bfe1d54b04e6294 Mon Sep 17 00:00:00 2001 From: David Cermak Date: Fri, 30 Nov 2018 17:00:05 +0100 Subject: [PATCH 1/4] mdns: skip sending search when finished, not properly locked timer task --- components/mdns/mdns.c | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/components/mdns/mdns.c b/components/mdns/mdns.c index 31fb592f29..c732ec5fa5 100644 --- a/components/mdns/mdns.c +++ b/components/mdns/mdns.c @@ -3465,6 +3465,22 @@ static void _mdns_search_send_pcb(mdns_search_once_t * search, tcpip_adapter_if_ */ static void _mdns_search_send(mdns_search_once_t * search) { + mdns_search_once_t* queue = _mdns_server->search_once; + bool found = false; + // looking for this search in active searches + while (queue) { + if (queue == search) { + found = true; + break; + } + queue = queue->next; + } + + if (!found) { + // no longer active -> skip sending this search + return; + } + uint8_t i, j; for (i=0; isearch_once; uint32_t now = xTaskGetTickCount() * portTICK_PERIOD_MS; if (!s) { + MDNS_SERVICE_UNLOCK(); return; } - MDNS_SERVICE_LOCK(); while (s) { if (s->state != SEARCH_OFF) { if (now > (s->started_at + s->timeout)) { From 44811c68961520ca278e77f67cb422e24b420340 Mon Sep 17 00:00:00 2001 From: David Cermak Date: Thu, 6 Dec 2018 16:46:31 +0100 Subject: [PATCH 2/4] mdns: resolve memory leak when txt record received multiple times --- components/mdns/mdns.c | 28 +++++++++++++++------------- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/components/mdns/mdns.c b/components/mdns/mdns.c index c732ec5fa5..46b5b0632c 100644 --- a/components/mdns/mdns.c +++ b/components/mdns/mdns.c @@ -2754,10 +2754,6 @@ void mdns_parse_packet(mdns_rx_packet_t * packet) if (search_result) { mdns_txt_item_t * txt = NULL; size_t txt_count = 0; - _mdns_result_txt_create(data_ptr, data_len, &txt, &txt_count); - if (!txt_count) { - continue; - } mdns_result_t * result = NULL; if (search_result->type == MDNS_TYPE_PTR) { @@ -2777,8 +2773,11 @@ void mdns_parse_packet(mdns_rx_packet_t * packet) } } if (!result->txt) { - result->txt = txt; - result->txt_count = txt_count; + _mdns_result_txt_create(data_ptr, data_len, &txt, &txt_count); + if (txt_count) { + result->txt = txt; + result->txt_count = txt_count; + } } } else { _mdns_search_result_add_txt(search_result, txt, txt_count, packet->tcpip_if, packet->ip_protocol); @@ -3290,7 +3289,7 @@ static void _mdns_search_result_add_txt(mdns_search_once_t * search, mdns_txt_it while (r) { if (r->tcpip_if == tcpip_if && r->ip_protocol == ip_protocol) { if (r->txt) { - return; + goto free_txt; } r->txt = txt; r->txt_count = txt_count; @@ -3301,12 +3300,7 @@ static void _mdns_search_result_add_txt(mdns_search_once_t * search, mdns_txt_it if (!search->max_results || search->num_results < search->max_results) { r = (mdns_result_t *)malloc(sizeof(mdns_result_t)); if (!r) { - for (i=0; iresult = r; search->num_results++; } + return; + +free_txt: + for (i=0; i Date: Fri, 7 Dec 2018 20:43:13 +0100 Subject: [PATCH 3/4] mdns: check all mallocs for failure and add default hook to log error with free heap solves crash about _mdns_result_txt_create when stress test --- components/mdns/mdns.c | 60 +++++++++++++++++++ components/mdns/mdns_networking.c | 3 + .../mdns/private_include/mdns_private.h | 3 + 3 files changed, 66 insertions(+) diff --git a/components/mdns/mdns.c b/components/mdns/mdns.c index 46b5b0632c..68e58ec6fa 100644 --- a/components/mdns/mdns.c +++ b/components/mdns/mdns.c @@ -15,6 +15,7 @@ #include "mdns.h" #include "mdns_private.h" #include "mdns_networking.h" +#include "esp_log.h" #include #ifdef MDNS_ENABLE_DEBUG @@ -26,6 +27,8 @@ static const char * MDNS_SUB_STR = "_sub"; mdns_server_t * _mdns_server = NULL; +static const char *TAG = "MDNS"; + static volatile TaskHandle_t _mdns_service_task_handle = NULL; static SemaphoreHandle_t _mdns_service_semaphore = NULL; @@ -63,11 +66,16 @@ static char * _mdns_mangle_name(char* in) { //need to add -2 to string ret = malloc(strlen(in) + 3); if (ret == NULL) { + HOOK_MALLOC_FAILED; return NULL; } sprintf(ret, "%s-2", in); } else { ret = malloc(strlen(in) + 2); //one extra byte in case 9-10 or 99-100 etc + if (ret == NULL) { + HOOK_MALLOC_FAILED; + return NULL; + } strcpy(ret, in); int baseLen = p - in; //length of 'bla' in 'bla-123' //overwrite suffix with new suffix @@ -102,6 +110,7 @@ esp_err_t _mdns_send_rx_action(mdns_rx_packet_t * packet) action = (mdns_action_t *)malloc(sizeof(mdns_action_t)); if (!action) { + HOOK_MALLOC_FAILED; return ESP_ERR_NO_MEM; } @@ -542,6 +551,9 @@ static uint16_t _mdns_append_txt_record(uint8_t * packet, uint16_t * index, mdns return 0; } data_len += l; + } else { + HOOK_MALLOC_FAILED; + // continue } txt = txt->next; } @@ -1115,6 +1127,7 @@ static bool _mdns_alloc_answer(mdns_out_answer_t ** destnation, uint16_t type, m mdns_out_answer_t * a = (mdns_out_answer_t *)malloc(sizeof(mdns_out_answer_t)); if (!a) { + HOOK_MALLOC_FAILED; return false; } a->type = type; @@ -1133,6 +1146,7 @@ static mdns_tx_packet_t * _mdns_alloc_packet_default(tcpip_adapter_if_t tcpip_if { mdns_tx_packet_t * packet = (mdns_tx_packet_t*)malloc(sizeof(mdns_tx_packet_t)); if (!packet) { + HOOK_MALLOC_FAILED; return NULL; } memset((uint8_t*)packet, 0, sizeof(mdns_tx_packet_t)); @@ -1270,6 +1284,7 @@ static mdns_tx_packet_t * _mdns_create_probe_packet(tcpip_adapter_if_t tcpip_if, for (i=0; ihostname)) { mdns_out_question_t * q = (mdns_out_question_t *)malloc(sizeof(mdns_out_question_t)); if (!q) { + HOOK_MALLOC_FAILED; _mdns_free_tx_packet(packet); return NULL; } @@ -1442,6 +1458,7 @@ static void _mdns_init_pcb_probe(tcpip_adapter_if_t tcpip_if, mdns_ip_protocol_t if (services_final_len) { _services = (mdns_srv_item_t **)malloc(sizeof(mdns_srv_item_t *) * services_final_len); if (!_services) { + HOOK_MALLOC_FAILED; return; } @@ -1722,6 +1739,7 @@ static mdns_txt_linked_item_t * _mdns_allocate_txt(size_t num_items, mdns_txt_it for (i=0; ikey = strdup(txt[i].key); @@ -1768,6 +1786,7 @@ static mdns_service_t * _mdns_create_service(const char * service, const char * { mdns_service_t * s = (mdns_service_t *)malloc(sizeof(mdns_service_t)); if (!s) { + HOOK_MALLOC_FAILED; return NULL; } @@ -2031,6 +2050,9 @@ static int _mdns_check_txt_collision(mdns_service_t * service, const uint8_t * d sprintf(tmp, "%s=%s", txt->key, txt->value); _mdns_append_string(ours, &index, tmp); free(tmp); + } else { + HOOK_MALLOC_FAILED; + // continue } txt = txt->next; } @@ -2387,6 +2409,10 @@ static void _mdns_result_txt_create(const uint8_t * data, size_t len, mdns_txt_i } mdns_txt_item_t * txt = (mdns_txt_item_t *)malloc(sizeof(mdns_txt_item_t) * num_items); + if (!txt) { + HOOK_MALLOC_FAILED; + return; + } memset(txt, 0, sizeof(mdns_txt_item_t) * num_items); size_t txt_num = 0; @@ -2407,6 +2433,7 @@ static void _mdns_result_txt_create(const uint8_t * data, size_t len, mdns_txt_i } char * key = (char *)malloc(name_len + 1); if (!key) { + HOOK_MALLOC_FAILED; goto handle_error;//error } @@ -2420,6 +2447,10 @@ static void _mdns_result_txt_create(const uint8_t * data, size_t len, mdns_txt_i int value_len = partLen - name_len - 1; if (value_len > 0) { char * value = (char *)malloc(value_len + 1); + if (!value) { + HOOK_MALLOC_FAILED; + goto handle_error;//error + } memcpy(value, data + i, value_len); value[value_len] = 0; i += value_len; @@ -2483,6 +2514,7 @@ void mdns_parse_packet(mdns_rx_packet_t * packet) mdns_parsed_packet_t * parsed_packet = (mdns_parsed_packet_t *)malloc(sizeof(mdns_parsed_packet_t)); if (!parsed_packet) { + HOOK_MALLOC_FAILED; return; } memset(parsed_packet, 0, sizeof(mdns_parsed_packet_t)); @@ -2545,6 +2577,7 @@ void mdns_parse_packet(mdns_rx_packet_t * packet) while (a) { mdns_parsed_question_t * question = (mdns_parsed_question_t *)malloc(sizeof(mdns_parsed_question_t)); if (!question) { + HOOK_MALLOC_FAILED; goto clear_rx_packet; } question->next = parsed_packet->questions; @@ -2572,6 +2605,7 @@ void mdns_parse_packet(mdns_rx_packet_t * packet) mdns_parsed_question_t * question = (mdns_parsed_question_t *)malloc(sizeof(mdns_parsed_question_t)); if (!question) { + HOOK_MALLOC_FAILED; goto clear_rx_packet; } question->next = parsed_packet->questions; @@ -3031,6 +3065,7 @@ static mdns_search_once_t * _mdns_search_init(const char * name, const char * se { mdns_search_once_t * search = (mdns_search_once_t *)malloc(sizeof(mdns_search_once_t)); if (!search) { + HOOK_MALLOC_FAILED; return NULL; } memset(search, 0, sizeof(mdns_search_once_t)); @@ -3122,6 +3157,7 @@ static mdns_ip_addr_t * _mdns_result_addr_create_ip(ip_addr_t * ip) { mdns_ip_addr_t * a = (mdns_ip_addr_t *)malloc(sizeof(mdns_ip_addr_t)); if (!a) { + HOOK_MALLOC_FAILED; return NULL; } memset(a, 0 , sizeof(mdns_ip_addr_t)); @@ -3181,6 +3217,7 @@ static void _mdns_search_result_add_ip(mdns_search_once_t * search, const char * if (!search->max_results || search->num_results < search->max_results) { r = (mdns_result_t *)malloc(sizeof(mdns_result_t)); if (!r) { + HOOK_MALLOC_FAILED; return; } @@ -3226,6 +3263,7 @@ static mdns_result_t * _mdns_search_result_add_ptr(mdns_search_once_t * search, if (!search->max_results || search->num_results < search->max_results) { r = (mdns_result_t *)malloc(sizeof(mdns_result_t)); if (!r) { + HOOK_MALLOC_FAILED; return NULL; } @@ -3261,6 +3299,7 @@ static void _mdns_search_result_add_srv(mdns_search_once_t * search, const char if (!search->max_results || search->num_results < search->max_results) { r = (mdns_result_t *)malloc(sizeof(mdns_result_t)); if (!r) { + HOOK_MALLOC_FAILED; return; } @@ -3300,6 +3339,7 @@ static void _mdns_search_result_add_txt(mdns_search_once_t * search, mdns_txt_it if (!search->max_results || search->num_results < search->max_results) { r = (mdns_result_t *)malloc(sizeof(mdns_result_t)); if (!r) { + HOOK_MALLOC_FAILED; goto free_txt; } @@ -3405,6 +3445,7 @@ static mdns_tx_packet_t * _mdns_create_search_packet(mdns_search_once_t * search mdns_out_question_t * q = (mdns_out_question_t *)malloc(sizeof(mdns_out_question_t)); if (!q) { + HOOK_MALLOC_FAILED; _mdns_free_tx_packet(packet); return NULL; } @@ -3427,6 +3468,7 @@ static mdns_tx_packet_t * _mdns_create_search_packet(mdns_search_once_t * search } mdns_out_answer_t * a = (mdns_out_answer_t *)malloc(sizeof(mdns_out_answer_t)); if (!a) { + HOOK_MALLOC_FAILED; _mdns_free_tx_packet(packet); return NULL; } @@ -3672,6 +3714,7 @@ static void _mdns_execute_action(mdns_action_t * action) if (!txt) { txt = (mdns_txt_linked_item_t *)malloc(sizeof(mdns_txt_linked_item_t)); if (!txt) { + HOOK_MALLOC_FAILED; _mdns_free_action(action); return; } @@ -3780,6 +3823,7 @@ static esp_err_t _mdns_send_search_action(mdns_action_type_t type, mdns_search_o action = (mdns_action_t *)malloc(sizeof(mdns_action_t)); if (!action) { + HOOK_MALLOC_FAILED; return ESP_ERR_NO_MEM; } @@ -3815,6 +3859,9 @@ static void _mdns_scheduler_run() free(action); _mdns_server->tx_queue_head = p; } + } else { + HOOK_MALLOC_FAILED; + // continue } } MDNS_SERVICE_UNLOCK(); @@ -3977,6 +4024,7 @@ esp_err_t mdns_handle_system_event(void *ctx, system_event_t *event) mdns_action_t * action = (mdns_action_t *)malloc(sizeof(mdns_action_t)); if (!action) { + HOOK_MALLOC_FAILED; return ESP_OK; } action->type = ACTION_SYSTEM_EVENT; @@ -3998,6 +4046,7 @@ esp_err_t mdns_init() _mdns_server = (mdns_server_t *)malloc(sizeof(mdns_server_t)); if (!_mdns_server) { + HOOK_MALLOC_FAILED; return ESP_ERR_NO_MEM; } memset((uint8_t*)_mdns_server, 0, sizeof(mdns_server_t)); @@ -4100,6 +4149,7 @@ esp_err_t mdns_hostname_set(const char * hostname) mdns_action_t * action = (mdns_action_t *)malloc(sizeof(mdns_action_t)); if (!action) { + HOOK_MALLOC_FAILED; free(new_hostname); return ESP_ERR_NO_MEM; } @@ -4128,6 +4178,7 @@ esp_err_t mdns_instance_name_set(const char * instance) mdns_action_t * action = (mdns_action_t *)malloc(sizeof(mdns_action_t)); if (!action) { + HOOK_MALLOC_FAILED; free(new_instance); return ESP_ERR_NO_MEM; } @@ -4162,6 +4213,7 @@ esp_err_t mdns_service_add(const char * instance, const char * service, const ch item = (mdns_srv_item_t *)malloc(sizeof(mdns_srv_item_t)); if (!item) { + HOOK_MALLOC_FAILED; _mdns_free_service(s); return ESP_ERR_NO_MEM; } @@ -4171,6 +4223,7 @@ esp_err_t mdns_service_add(const char * instance, const char * service, const ch mdns_action_t * action = (mdns_action_t *)malloc(sizeof(mdns_action_t)); if (!action) { + HOOK_MALLOC_FAILED; _mdns_free_service(s); free(item); return ESP_ERR_NO_MEM; @@ -4207,6 +4260,7 @@ esp_err_t mdns_service_port_set(const char * service, const char * proto, uint16 mdns_action_t * action = (mdns_action_t *)malloc(sizeof(mdns_action_t)); if (!action) { + HOOK_MALLOC_FAILED; return ESP_ERR_NO_MEM; } action->type = ACTION_SERVICE_PORT_SET; @@ -4239,6 +4293,7 @@ esp_err_t mdns_service_txt_set(const char * service, const char * proto, mdns_tx mdns_action_t * action = (mdns_action_t *)malloc(sizeof(mdns_action_t)); if (!action) { + HOOK_MALLOC_FAILED; _mdns_free_linked_txt(new_txt); return ESP_ERR_NO_MEM; } @@ -4266,6 +4321,7 @@ esp_err_t mdns_service_txt_item_set(const char * service, const char * proto, co } mdns_action_t * action = (mdns_action_t *)malloc(sizeof(mdns_action_t)); if (!action) { + HOOK_MALLOC_FAILED; return ESP_ERR_NO_MEM; } @@ -4302,6 +4358,7 @@ esp_err_t mdns_service_txt_item_remove(const char * service, const char * proto, } mdns_action_t * action = (mdns_action_t *)malloc(sizeof(mdns_action_t)); if (!action) { + HOOK_MALLOC_FAILED; return ESP_ERR_NO_MEM; } @@ -4339,6 +4396,7 @@ esp_err_t mdns_service_instance_name_set(const char * service, const char * prot mdns_action_t * action = (mdns_action_t *)malloc(sizeof(mdns_action_t)); if (!action) { + HOOK_MALLOC_FAILED; free(new_instance); return ESP_ERR_NO_MEM; } @@ -4365,6 +4423,7 @@ esp_err_t mdns_service_remove(const char * service, const char * proto) mdns_action_t * action = (mdns_action_t *)malloc(sizeof(mdns_action_t)); if (!action) { + HOOK_MALLOC_FAILED; return ESP_ERR_NO_MEM; } action->type = ACTION_SERVICE_DEL; @@ -4387,6 +4446,7 @@ esp_err_t mdns_service_remove_all() mdns_action_t * action = (mdns_action_t *)malloc(sizeof(mdns_action_t)); if (!action) { + HOOK_MALLOC_FAILED; return ESP_ERR_NO_MEM; } action->type = ACTION_SERVICES_CLEAR; diff --git a/components/mdns/mdns_networking.c b/components/mdns/mdns_networking.c index e4ab816dec..9fa5ce1d42 100644 --- a/components/mdns/mdns_networking.c +++ b/components/mdns/mdns_networking.c @@ -5,6 +5,7 @@ */ #include #include "mdns_networking.h" +#include "esp_log.h" extern mdns_server_t * _mdns_server; @@ -13,6 +14,7 @@ extern mdns_server_t * _mdns_server; * MDNS Server Networking * */ +static const char *TAG = "MDNS_Networking"; /** * @brief the receive callback of the raw udp api. Packets are received here @@ -29,6 +31,7 @@ static void _udp_recv(void *arg, struct udp_pcb *upcb, struct pbuf *pb, const ip mdns_rx_packet_t * packet = (mdns_rx_packet_t *)malloc(sizeof(mdns_rx_packet_t)); if (!packet) { + HOOK_MALLOC_FAILED; //missed packet - no memory pbuf_free(this_pb); continue; diff --git a/components/mdns/private_include/mdns_private.h b/components/mdns/private_include/mdns_private.h index 3abaa56612..cbdf830cd7 100644 --- a/components/mdns/private_include/mdns_private.h +++ b/components/mdns/private_include/mdns_private.h @@ -115,6 +115,9 @@ #define MDNS_SEARCH_LOCK() xSemaphoreTake(_mdns_server->search.lock, portMAX_DELAY) #define MDNS_SEARCH_UNLOCK() xSemaphoreGive(_mdns_server->search.lock) +#ifndef HOOK_MALLOC_FAILED +#define HOOK_MALLOC_FAILED ESP_LOGE(TAG, "Cannot allocate memory (line: %d, free heap: %d bytes)", __LINE__, esp_get_free_heap_size()); +#endif typedef enum { PCB_OFF, PCB_DUP, PCB_INIT, From db256ed1e150078c01314b996105925e2279d5fd Mon Sep 17 00:00:00 2001 From: David Cermak Date: Mon, 10 Dec 2018 16:42:10 +0100 Subject: [PATCH 4/4] mdns: fixed static memory leak --- components/mdns/mdns.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/components/mdns/mdns.c b/components/mdns/mdns.c index 68e58ec6fa..70f9be9cba 100644 --- a/components/mdns/mdns.c +++ b/components/mdns/mdns.c @@ -3996,6 +3996,7 @@ static esp_err_t _mdns_service_task_stop() { MDNS_SERVICE_LOCK(); _mdns_stop_timer(); + MDNS_SERVICE_UNLOCK(); if (_mdns_service_task_handle) { mdns_action_t action; mdns_action_t * a = &action; @@ -4008,7 +4009,6 @@ static esp_err_t _mdns_service_task_stop() vTaskDelay(10 / portTICK_PERIOD_MS); } } - MDNS_SERVICE_UNLOCK(); return ESP_OK; } @@ -4100,8 +4100,8 @@ void mdns_free() if (!_mdns_server) { return; } - _mdns_service_task_stop(); mdns_service_remove_all(_mdns_server); + _mdns_service_task_stop(); for (i=0; i