From 35a30072f43aaf815929b431e732f269720c67a7 Mon Sep 17 00:00:00 2001 From: David Cermak Date: Mon, 25 Feb 2019 14:29:39 +0100 Subject: [PATCH 1/5] mdns: fix possible crash when packet scheduled to transmit contained service which might have been already removed packets scheduled to transmit are pushed to action queue and removed from tx_queue_head structure, which is searched for all remaining services and while service is removed, then service questions/asnwers are also removed from this structure. This update fixes possible crash when packet is pushed to action queue, and when service is removed, its answers are removed from tx_queue_head, but not from action queue. this could lead to a crash when the packet is poped from action queue containing questions/answers to already removed (freed) service Closes IDF-504 --- components/mdns/mdns.c | 24 ++++++++++++++++--- .../mdns/private_include/mdns_private.h | 1 + 2 files changed, 22 insertions(+), 3 deletions(-) diff --git a/components/mdns/mdns.c b/components/mdns/mdns.c index 40b2cc038..18fe1a993 100644 --- a/components/mdns/mdns.c +++ b/components/mdns/mdns.c @@ -3819,7 +3819,17 @@ static void _mdns_execute_action(mdns_action_t * action) _mdns_search_finish(action->data.search_add.search); break; case ACTION_TX_HANDLE: - _mdns_tx_handle_packet(action->data.tx_handle.packet); + { + mdns_tx_packet_t * p = _mdns_server->tx_queue_head; + // packet to be handled should be at tx head, but must be consistent with the one pushed to action queue + if (p && p==action->data.tx_handle.packet && p->queued) { + p->queued = false; // clearing, as the packet might be reused (pushed and transmitted again) + _mdns_server->tx_queue_head = p->next; + _mdns_tx_handle_packet(p); + } else { + ESP_LOGD(TAG, "Skipping transmit of an unexpected packet!"); + } + } break; case ACTION_RX_HANDLE: mdns_parse_packet(action->data.rx_handle.packet); @@ -3856,6 +3866,10 @@ static esp_err_t _mdns_send_search_action(mdns_action_type_t type, mdns_search_o /** * @brief Called from timer task to run mDNS responder + * + * periodically checks first unqueued packet (from tx head). + * if it is scheduled to be transmitted, then pushes the packet to action queue to be handled. + * */ static void _mdns_scheduler_run() { @@ -3863,6 +3877,10 @@ static void _mdns_scheduler_run() mdns_tx_packet_t * p = _mdns_server->tx_queue_head; mdns_action_t * action = NULL; + // find first unqueued packet + while (p && p->queued) { + p = p->next; + } if (!p) { MDNS_SERVICE_UNLOCK(); return; @@ -3870,12 +3888,12 @@ static void _mdns_scheduler_run() if ((int32_t)(p->send_at - (xTaskGetTickCount() * portTICK_PERIOD_MS)) < 0) { action = (mdns_action_t *)malloc(sizeof(mdns_action_t)); if (action) { - _mdns_server->tx_queue_head = p->next; action->type = ACTION_TX_HANDLE; action->data.tx_handle.packet = p; + p->queued = true; if (xQueueSend(_mdns_server->action_queue, &action, (portTickType)0) != pdPASS) { free(action); - _mdns_server->tx_queue_head = p; + p->queued = false; } } else { HOOK_MALLOC_FAILED; diff --git a/components/mdns/private_include/mdns_private.h b/components/mdns/private_include/mdns_private.h index 9efd23ae7..a7383c03d 100644 --- a/components/mdns/private_include/mdns_private.h +++ b/components/mdns/private_include/mdns_private.h @@ -289,6 +289,7 @@ typedef struct mdns_tx_packet_s { mdns_out_answer_t * answers; mdns_out_answer_t * servers; mdns_out_answer_t * additional; + bool queued; } mdns_tx_packet_t; typedef struct { From e8bcda3512f0c4e96ec45af2a2e77976816242bf Mon Sep 17 00:00:00 2001 From: David Cermak Date: Fri, 1 Mar 2019 16:59:38 +0100 Subject: [PATCH 2/5] mdsn: fix race condition in updating packet data from user task when failed to allocate or queue a new service Issue: mdns_service_add API allocates and queues an action to be processed in mdns task context; when allocation or queueing fails, allocated structure needs to be freed. Function _mdns_free_service did not only fee all the structures, but also updates packet data. Resolution: Moved removal of packet data outside of _mdns_free_service function. --- components/mdns/mdns.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/components/mdns/mdns.c b/components/mdns/mdns.c index 18fe1a993..de66d9432 100644 --- a/components/mdns/mdns.c +++ b/components/mdns/mdns.c @@ -1863,6 +1863,9 @@ static void _mdns_dealloc_scheduled_service_answers(mdns_out_answer_t ** destina */ static void _mdns_remove_scheduled_service_packets(mdns_service_t * service) { + if (!service) { + return; + } mdns_tx_packet_t * p = NULL; mdns_tx_packet_t * q = _mdns_server->tx_queue_head; while (q) { @@ -1951,7 +1954,6 @@ static void _mdns_free_service(mdns_service_t * service) if (!service) { return; } - _mdns_remove_scheduled_service_packets(service); free((char *)service->instance); free((char *)service->service); free((char *)service->proto); @@ -3781,6 +3783,7 @@ static void _mdns_execute_action(mdns_action_t * action) if (_mdns_server->services == action->data.srv_del.service) { _mdns_server->services = a->next; _mdns_send_bye(&a, 1, false); + _mdns_remove_scheduled_service_packets(a->service); _mdns_free_service(a->service); free(a); } else { @@ -3791,6 +3794,7 @@ static void _mdns_execute_action(mdns_action_t * action) mdns_srv_item_t * b = a->next; a->next = a->next->next; _mdns_send_bye(&b, 1, false); + _mdns_remove_scheduled_service_packets(b->service); _mdns_free_service(b->service); free(b); } @@ -3804,6 +3808,7 @@ static void _mdns_execute_action(mdns_action_t * action) while (a) { mdns_srv_item_t * s = a; a = a->next; + _mdns_remove_scheduled_service_packets(s->service); _mdns_free_service(s->service); free(s); } From 54e5e440b1b80f44731609c721e453b41f852030 Mon Sep 17 00:00:00 2001 From: David Cermak Date: Mon, 4 Mar 2019 11:13:52 +0100 Subject: [PATCH 3/5] mdns: fix possible deadlock on mdns deinit calling mdns_free() mnds_free() initiates stop and delete timer tasks, which after locking the mutex could lead to a dead lock in case timer task executed before deleting the task, as it would wait indefinitelly for unlocking the mutex. This condition is fixed by calling _mdns_stop_timer without locking the mutex, because there's no need to protect any data when stopping and deleting the timer task Closes https://github.com/espressif/esp-idf/issues/1696 --- components/mdns/mdns.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/components/mdns/mdns.c b/components/mdns/mdns.c index de66d9432..378816ba7 100644 --- a/components/mdns/mdns.c +++ b/components/mdns/mdns.c @@ -4038,9 +4038,7 @@ static esp_err_t _mdns_service_task_start() */ 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; From c88cc4950ecf2316652eb18fcfd1d2d8fe06347a Mon Sep 17 00:00:00 2001 From: David Cermak Date: Mon, 4 Mar 2019 12:32:10 +0100 Subject: [PATCH 4/5] mdns: enable pcbs before starting service thread to avoid updating pcb's internal variables from concurent tasks possible race condition: user task runs mdns_init, which enables pcbs while mdns-task already created could execute enable/disable of the same pcbs if an appropriate system event received --- components/mdns/mdns.c | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/components/mdns/mdns.c b/components/mdns/mdns.c index 378816ba7..d302ded24 100644 --- a/components/mdns/mdns.c +++ b/components/mdns/mdns.c @@ -4107,12 +4107,6 @@ esp_err_t mdns_init() goto free_lock; } - if (_mdns_service_task_start()) { - //service start failed! - err = ESP_FAIL; - goto free_all; - } - uint8_t i; ip6_addr_t tmp_addr6; tcpip_adapter_ip_info_t if_ip_info; @@ -4126,9 +4120,19 @@ esp_err_t mdns_init() } } + if (_mdns_service_task_start()) { + //service start failed! + err = ESP_FAIL; + goto free_all_and_disable_pcbs; + } + return ESP_OK; -free_all: +free_all_and_disable_pcbs: + for (i=0; iaction_queue); free_lock: vSemaphoreDelete(_mdns_server->lock); From 936b99d756495a52e353ce32381f919614a28bf3 Mon Sep 17 00:00:00 2001 From: David Cermak Date: Mon, 11 Mar 2019 11:49:29 +0100 Subject: [PATCH 5/5] mdns: fix possible crash when probing on particular interface with duplicated service instances due to naming conflicts on network Issue: MDNS server initially sends probing packets to resolve naming confilicts with already registered service instances. In case of a conflict, instance name is altered and probing restarts. Original instance however wasnnot removed from the structure and upon service removal only one entry was removed and a dangling service might have been kept in the structure to bring about a crash. Resolution: Keep only one instance of a service in the probing structure. Closes IDF-498 --- components/mdns/mdns.c | 55 +++++++++++++++++++++++++++++++++++------- 1 file changed, 46 insertions(+), 9 deletions(-) diff --git a/components/mdns/mdns.c b/components/mdns/mdns.c index d302ded24..0f52ecf8e 100644 --- a/components/mdns/mdns.c +++ b/components/mdns/mdns.c @@ -1452,20 +1452,13 @@ static void _mdns_pcb_send_bye(tcpip_adapter_if_t tcpip_if, mdns_ip_protocol_t i } /** - * @brief Send probe for particular services on particular PCB + * @brief Send probe for additional services on particular PCB */ -static void _mdns_init_pcb_probe(tcpip_adapter_if_t tcpip_if, mdns_ip_protocol_t ip_protocol, mdns_srv_item_t ** services, size_t len, bool probe_ip) +static void _mdns_init_pcb_probe_new_service(tcpip_adapter_if_t tcpip_if, mdns_ip_protocol_t ip_protocol, mdns_srv_item_t ** services, size_t len, bool probe_ip) { mdns_pcb_t * pcb = &_mdns_server->interfaces[tcpip_if].pcbs[ip_protocol]; size_t services_final_len = len; - _mdns_clear_pcb_tx_queue_head(tcpip_if, ip_protocol); - - if (_str_null_or_empty(_mdns_server->hostname)) { - pcb->state = PCB_RUNNING; - return; - } - if (PCB_STATE_IS_PROBING(pcb)) { services_final_len += pcb->probe_services_len; } @@ -1510,6 +1503,50 @@ static void _mdns_init_pcb_probe(tcpip_adapter_if_t tcpip_if, mdns_ip_protocol_t pcb->state = PCB_PROBE_1; } +/** + * @brief Send probe for particular services on particular PCB + * + * Tests possible duplication on probing service structure and probes only for new entries. + * - If pcb probing then add only non-probing services and restarts probing + * - If pcb not probing, run probing for all specified services + */ +static void _mdns_init_pcb_probe(tcpip_adapter_if_t tcpip_if, mdns_ip_protocol_t ip_protocol, mdns_srv_item_t ** services, size_t len, bool probe_ip) +{ + mdns_pcb_t * pcb = &_mdns_server->interfaces[tcpip_if].pcbs[ip_protocol]; + + _mdns_clear_pcb_tx_queue_head(tcpip_if, ip_protocol); + + if (_str_null_or_empty(_mdns_server->hostname)) { + pcb->state = PCB_RUNNING; + return; + } + + if (PCB_STATE_IS_PROBING(pcb)) { + // Looking for already probing services to resolve duplications + mdns_srv_item_t * new_probe_services[len]; + int new_probe_service_len = 0; + bool found; + for (int j=0; j < len; ++j) { + found = false; + for (int i=0; i < pcb->probe_services_len; ++i) { + if (pcb->probe_services[i] == services[j]) { + found = true; + break; + } + } + if (!found) { + new_probe_services[new_probe_service_len++] = services[j]; + } + } + // init probing for newly added services + _mdns_init_pcb_probe_new_service(tcpip_if, ip_protocol, + new_probe_service_len?new_probe_services:NULL, new_probe_service_len, probe_ip); + } else { + // not probing, so init for all services + _mdns_init_pcb_probe_new_service(tcpip_if, ip_protocol, services, len, probe_ip); + } +} + /** * @brief Restart the responder on particular PCB */