From 8a690503ed38e6791714f5453b2ca1d7f8941612 Mon Sep 17 00:00:00 2001 From: David Cermak Date: Mon, 22 Jul 2024 19:16:42 +0200 Subject: [PATCH 1/9] fix(mdns): Fix services API races to directly add/remove services Original issue (data race when updating hostname): mdns_service_add() makes a copy of the local hostname and calls using the local copy mdns_service_add_for_host(). When mdns's hostname is updated the local copy gets out of sync. **API race issue**: Most of the current API correctly lock the mdns service, but sometimes unlocks it before sending an action to the action queue, so it's possible that the situation changes before the actual action takes place. **Fix**: After locking the mdns service, we proceed directly with updating internal structures and do not post actions into the action queue. **Fix wrtt hostname**: Use mdns_service_add_for_host(hostname=NULL) for all self hosted services. MAJOR CHANGE: Fixed mdns API issues when add/remove/update records from multiple threads This and the following commits fix the API race issues for these mdns APIs: * mdns_service_add_for_host * mdns_service_port_set_for_host * mdns_service_txt_set_for_host * mdns_service_txt_item_set_for_host_with_explicit_value_len * mdns_service_txt_item_remove_for_host * mdns_service_subtype_add_for_host * mdns_service_instance_name_set_for_host * mdns_service_remove_for_host * mdns_service_remove_all --- components/mdns/mdns.c | 249 ++++++------------ .../mdns/private_include/mdns_private.h | 13 +- .../tests/test_afl_fuzz_host/esp32_mock.c | 12 +- .../tests/test_afl_fuzz_host/esp32_mock.h | 7 +- .../mdns/tests/unit_test/main/test_mdns.c | 1 + 5 files changed, 97 insertions(+), 185 deletions(-) diff --git a/components/mdns/mdns.c b/components/mdns/mdns.c index d3961ab49..20e607507 100644 --- a/components/mdns/mdns.c +++ b/components/mdns/mdns.c @@ -17,6 +17,7 @@ #include "mdns_networking.h" #include "esp_log.h" #include "esp_random.h" +#include "esp_check.h" static void _mdns_browse_item_free(mdns_browse_t *browse); static esp_err_t _mdns_send_browse_action(mdns_action_type_t type, mdns_browse_t *browse); @@ -5107,10 +5108,6 @@ static void _mdns_free_action(mdns_action_t *action) case ACTION_INSTANCE_SET: free(action->data.instance); break; - case ACTION_SERVICE_ADD: - _mdns_free_service(action->data.srv_add.service->service); - free(action->data.srv_add.service); - break; case ACTION_SERVICE_INSTANCE_SET: free(action->data.srv_instance.instance); break; @@ -5194,11 +5191,6 @@ static void _mdns_execute_action(mdns_action_t *action) _mdns_server->instance = action->data.instance; _mdns_restart_all_pcbs_no_instance(); - break; - case ACTION_SERVICE_ADD: - action->data.srv_add.service->next = _mdns_server->services; - _mdns_server->services = action->data.srv_add.service; - _mdns_probe_all_pcbs(&action->data.srv_add.service, 1, false, false); break; case ACTION_SERVICE_INSTANCE_SET: if (action->data.srv_instance.service->service->instance) { @@ -5299,60 +5291,6 @@ static void _mdns_execute_action(mdns_action_t *action) subtype_item->next = service->subtype; service->subtype = subtype_item; break; - case ACTION_SERVICE_DEL: - a = _mdns_server->services; - mdns_srv_item_t *b = a; - if (action->data.srv_del.instance) { - while (a) { - if (_mdns_service_match_instance(a->service, action->data.srv_del.instance, - action->data.srv_del.service, action->data.srv_del.proto, - action->data.srv_del.hostname)) { - if (_mdns_server->services != a) { - b->next = a->next; - } else { - _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); - break; - } - b = a; - a = a->next; - } - } else { - while (a) { - if (_mdns_service_match(a->service, action->data.srv_del.service, action->data.srv_del.proto, - action->data.srv_del.hostname)) { - if (_mdns_server->services != a) { - b->next = a->next; - _mdns_send_bye(&a, 1, false); - _mdns_remove_scheduled_service_packets(a->service); - _mdns_free_service(a->service); - free(a); - a = b->next; - continue; - } else { - _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); - a = _mdns_server->services; - b = a; - continue; - } - } - b = a; - a = a->next; - } - } - free((char *)action->data.srv_del.instance); - free((char *)action->data.srv_del.service); - free((char *)action->data.srv_del.proto); - free((char *)action->data.srv_del.hostname); - break; case ACTION_SERVICES_CLEAR: _mdns_send_final_bye(false); a = _mdns_server->services; @@ -6076,7 +6014,7 @@ esp_err_t mdns_instance_name_set(const char *instance) * MDNS SERVICES * */ -esp_err_t mdns_service_add_for_host(const char *instance, const char *service, const char *proto, const char *hostname, +esp_err_t mdns_service_add_for_host(const char *instance, const char *service, const char *proto, const char *host, uint16_t port, mdns_txt_item_t txt[], size_t num_items) { if (!_mdns_server || _str_null_or_empty(service) || _str_null_or_empty(proto) || !port || !_mdns_server->hostname) { @@ -6084,69 +6022,37 @@ esp_err_t mdns_service_add_for_host(const char *instance, const char *service, c } MDNS_SERVICE_LOCK(); - if (!_mdns_can_add_more_services()) { - MDNS_SERVICE_UNLOCK(); - return ESP_ERR_NO_MEM; - } + esp_err_t ret = ESP_OK; + const char *hostname = host ? host : _mdns_server->hostname; + mdns_service_t *s = NULL; - if (!hostname) { - hostname = _mdns_server->hostname; - } + ESP_GOTO_ON_FALSE(_mdns_can_add_more_services(), ESP_ERR_NO_MEM, err, TAG, "Cannot add more services"); mdns_srv_item_t *item = _mdns_get_service_item_instance(instance, service, proto, hostname); - MDNS_SERVICE_UNLOCK(); - if (item) { - return ESP_ERR_INVALID_ARG; - } + ESP_GOTO_ON_FALSE(!item, ESP_ERR_INVALID_ARG, err, TAG, "Already exists"); - mdns_service_t *s = _mdns_create_service(service, proto, hostname, port, instance, num_items, txt); - if (!s) { - return ESP_ERR_NO_MEM; - } + s = _mdns_create_service(service, proto, hostname, port, instance, num_items, txt); + ESP_GOTO_ON_FALSE(s, ESP_ERR_NO_MEM, err, TAG, "Cannot create service: Out of memory"); 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; - } + ESP_GOTO_ON_FALSE(item, ESP_ERR_NO_MEM, err, TAG, "Cannot create service: Out of memory"); item->service = s; item->next = NULL; - 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; - } - action->type = ACTION_SERVICE_ADD; - action->data.srv_add.service = item; - if (xQueueSend(_mdns_server->action_queue, &action, (TickType_t)0) != pdPASS) { - _mdns_free_service(s); - free(item); - free(action); - return ESP_ERR_NO_MEM; - } - - size_t start = xTaskGetTickCount(); - size_t timeout_ticks = pdMS_TO_TICKS(MDNS_SERVICE_ADD_TIMEOUT_MS); - MDNS_SERVICE_LOCK(); - mdns_srv_item_t *target = _mdns_get_service_item_instance(instance, service, proto, hostname); + item->next = _mdns_server->services; + _mdns_server->services = item; + _mdns_probe_all_pcbs(&item, 1, false, false); MDNS_SERVICE_UNLOCK(); - while (target == NULL) { - uint32_t expired = xTaskGetTickCount() - start; - if (expired >= timeout_ticks) { - return ESP_FAIL; // Timeout - } - vTaskDelay(MIN(10 / portTICK_PERIOD_MS, timeout_ticks - expired)); - MDNS_SERVICE_LOCK(); - target = _mdns_get_service_item_instance(instance, service, proto, hostname); - MDNS_SERVICE_UNLOCK(); - } - return ESP_OK; + +err: + MDNS_SERVICE_UNLOCK(); + _mdns_free_service(s); + if (ret == ESP_ERR_NO_MEM) { + HOOK_MALLOC_FAILED; + } + return ret; } esp_err_t mdns_service_add(const char *instance, const char *service, const char *proto, uint16_t port, @@ -6155,7 +6061,7 @@ esp_err_t mdns_service_add(const char *instance, const char *service, const char if (!_mdns_server) { return ESP_ERR_INVALID_STATE; } - return mdns_service_add_for_host(instance, service, proto, _mdns_server->hostname, port, txt, num_items); + return mdns_service_add_for_host(instance, service, proto, NULL, port, txt, num_items); } bool mdns_service_exists(const char *service_type, const char *proto, const char *hostname) @@ -6184,6 +6090,11 @@ static mdns_txt_item_t *_copy_mdns_txt_items(mdns_txt_linked_item_t *items, uint for (mdns_txt_linked_item_t *tmp = items; tmp != NULL; tmp = tmp->next) { ret_index++; } + if (ret_index == 0) { + *txt_count = 0; + *txt_value_len = NULL; + return NULL; + } *txt_count = ret_index; if (ret_index == 0) { // handle empty TXT *txt_value_len = NULL; @@ -6618,59 +6529,65 @@ esp_err_t mdns_service_instance_name_set(const char *service, const char *proto, return mdns_service_instance_name_set_for_host(NULL, service, proto, _mdns_server->hostname, instance); } -esp_err_t mdns_service_remove_for_host(const char *instance, const char *service, const char *proto, const char *hostname) +esp_err_t mdns_service_remove_for_host(const char *instance, const char *service, const char *proto, const char *host) { MDNS_SERVICE_LOCK(); - if (!_mdns_server || !_mdns_server->services || _str_null_or_empty(service) || _str_null_or_empty(proto)) { - MDNS_SERVICE_UNLOCK(); - return ESP_ERR_INVALID_ARG; - } + esp_err_t ret = ESP_OK; + const char *hostname = host ? host : _mdns_server->hostname; + ESP_GOTO_ON_FALSE(_mdns_server && _mdns_server->services && !_str_null_or_empty(service) && !_str_null_or_empty(proto), + ESP_ERR_INVALID_ARG, err, TAG, "Invalid state or arguments"); mdns_srv_item_t *s = _mdns_get_service_item_instance(instance, service, proto, hostname); + ESP_GOTO_ON_FALSE(s, ESP_ERR_INVALID_ARG, err, TAG, "Service doesn't exist"); + + mdns_srv_item_t *a = _mdns_server->services; + mdns_srv_item_t *b = a; + if (instance) { + while (a) { + if (_mdns_service_match_instance(a->service, instance, service, proto, hostname)) { + if (_mdns_server->services != a) { + b->next = a->next; + } else { + _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); + break; + } + b = a; + a = a->next; + } + } else { + while (a) { + if (_mdns_service_match(a->service, service, proto, hostname)) { + if (_mdns_server->services != a) { + b->next = a->next; + _mdns_send_bye(&a, 1, false); + _mdns_remove_scheduled_service_packets(a->service); + _mdns_free_service(a->service); + free(a); + a = b->next; + continue; + } else { + _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); + a = _mdns_server->services; + b = a; + continue; + } + } + b = a; + a = a->next; + } + } + +err: MDNS_SERVICE_UNLOCK(); - if (!s) { - return ESP_ERR_NOT_FOUND; - } - - 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; - action->data.srv_del.instance = NULL; - action->data.srv_del.hostname = NULL; - if (!_str_null_or_empty(instance)) { - action->data.srv_del.instance = strndup(instance, MDNS_NAME_BUF_LEN - 1); - if (!action->data.srv_del.instance) { - goto fail; - } - } - - if (!_str_null_or_empty(hostname)) { - action->data.srv_del.hostname = strndup(hostname, MDNS_NAME_BUF_LEN - 1); - if (!action->data.srv_del.hostname) { - goto fail; - } - } - - action->data.srv_del.service = strndup(service, MDNS_NAME_BUF_LEN - 1); - action->data.srv_del.proto = strndup(proto, MDNS_NAME_BUF_LEN - 1); - if (!action->data.srv_del.service || !action->data.srv_del.proto) { - goto fail; - } - - if (xQueueSend(_mdns_server->action_queue, &action, (TickType_t)0) != pdPASS) { - goto fail; - } - return ESP_OK; - -fail: - free((char *)action->data.srv_del.instance); - free((char *)action->data.srv_del.service); - free((char *)action->data.srv_del.proto); - free((char *)action->data.srv_del.hostname); - free(action); - return ESP_ERR_NO_MEM; + return ret; } esp_err_t mdns_service_remove(const char *service_type, const char *proto) @@ -6678,7 +6595,7 @@ esp_err_t mdns_service_remove(const char *service_type, const char *proto) if (!_mdns_server) { return ESP_ERR_INVALID_STATE; } - return mdns_service_remove_for_host(NULL, service_type, proto, _mdns_server->hostname); + return mdns_service_remove_for_host(NULL, service_type, proto, NULL); } esp_err_t mdns_service_remove_all(void) diff --git a/components/mdns/private_include/mdns_private.h b/components/mdns/private_include/mdns_private.h index f3dbb09d2..311abd9c7 100644 --- a/components/mdns/private_include/mdns_private.h +++ b/components/mdns/private_include/mdns_private.h @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2015-2023 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2015-2024 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -187,8 +187,6 @@ typedef enum { ACTION_SYSTEM_EVENT, ACTION_HOSTNAME_SET, ACTION_INSTANCE_SET, - ACTION_SERVICE_ADD, - ACTION_SERVICE_DEL, ACTION_SERVICE_INSTANCE_SET, ACTION_SERVICE_PORT_SET, ACTION_SERVICE_TXT_REPLACE, @@ -446,15 +444,6 @@ typedef struct { mdns_if_t interface; mdns_event_actions_t event_action; } sys_event; - struct { - mdns_srv_item_t *service; - } srv_add; - struct { - char *instance; - char *service; - char *proto; - char *hostname; - } srv_del; struct { mdns_srv_item_t *service; char *instance; diff --git a/components/mdns/tests/test_afl_fuzz_host/esp32_mock.c b/components/mdns/tests/test_afl_fuzz_host/esp32_mock.c index 1f74c7833..14b913411 100644 --- a/components/mdns/tests/test_afl_fuzz_host/esp32_mock.c +++ b/components/mdns/tests/test_afl_fuzz_host/esp32_mock.c @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2022 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2022-2024 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Unlicense OR CC0-1.0 */ @@ -9,6 +9,7 @@ #include #include #include "esp32_mock.h" +#include "esp_log.h" void *g_queue; int g_queue_send_shall_fail = 0; @@ -111,3 +112,12 @@ BaseType_t xTaskNotifyWait(uint32_t bits_entry_clear, uint32_t bits_exit_clear, { return pdTRUE; } + +void esp_log_write(esp_log_level_t level, const char *tag, const char *format, ...) +{ +} + +uint32_t esp_log_timestamp(void) +{ + return 0; +} diff --git a/components/mdns/tests/test_afl_fuzz_host/esp32_mock.h b/components/mdns/tests/test_afl_fuzz_host/esp32_mock.h index ecd82e0d0..e8e84ceb4 100644 --- a/components/mdns/tests/test_afl_fuzz_host/esp32_mock.h +++ b/components/mdns/tests/test_afl_fuzz_host/esp32_mock.h @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2015-2022 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2015-2024 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -45,11 +45,6 @@ #define portMAX_DELAY 0xFFFFFFFF #define portTICK_PERIOD_MS 1 -#define ESP_LOGW(a,b) -#define ESP_LOGD(a,b) -#define ESP_LOGE(a,b,c) -#define ESP_LOGV(a,b,c,d) - #define LWIP_HDR_PBUF_H #define __ESP_RANDOM_H__ #define INC_TASK_H diff --git a/components/mdns/tests/unit_test/main/test_mdns.c b/components/mdns/tests/unit_test/main/test_mdns.c index 83e6f1b29..ff024e2fc 100644 --- a/components/mdns/tests/unit_test/main/test_mdns.c +++ b/components/mdns/tests/unit_test/main/test_mdns.c @@ -91,6 +91,7 @@ TEST(mdns, api_fails_with_expected_err) TEST_ASSERT_EQUAL(ESP_OK, mdns_service_txt_item_set(MDNS_SERVICE_NAME, MDNS_SERVICE_PROTO, "key1", "value1") ); TEST_ASSERT_EQUAL(ESP_OK, mdns_service_txt_item_remove(MDNS_SERVICE_NAME, MDNS_SERVICE_PROTO, "key1") ); TEST_ASSERT_EQUAL(ESP_OK, mdns_service_port_set(MDNS_SERVICE_NAME, MDNS_SERVICE_PROTO, 8080) ); + yield_to_all_priorities(); // to remove the service with the updated txt records TEST_ASSERT_EQUAL(ESP_OK, mdns_service_remove(MDNS_SERVICE_NAME, MDNS_SERVICE_PROTO) ); yield_to_all_priorities(); // Make sure that mdns task has executed to remove the service From 99d5fb27e998f4b3ae0cb3c1f613e2283939af36 Mon Sep 17 00:00:00 2001 From: David Cermak Date: Tue, 23 Jul 2024 08:50:18 +0200 Subject: [PATCH 2/9] fix(mdns): Fix API races while setting port for services Fixes **API race issue** (described in 8a690503) for API mdns_service_port_set_for_host() --- components/mdns/mdns.c | 41 ++++++------------- .../mdns/private_include/mdns_private.h | 1 - 2 files changed, 13 insertions(+), 29 deletions(-) diff --git a/components/mdns/mdns.c b/components/mdns/mdns.c index 20e607507..302c8312f 100644 --- a/components/mdns/mdns.c +++ b/components/mdns/mdns.c @@ -5200,11 +5200,6 @@ static void _mdns_execute_action(mdns_action_t *action) action->data.srv_instance.service->service->instance = action->data.srv_instance.instance; _mdns_probe_all_pcbs(&action->data.srv_instance.service, 1, false, false); - break; - case ACTION_SERVICE_PORT_SET: - action->data.srv_port.service->service->port = action->data.srv_port.port; - _mdns_announce_all_pcbs(&action->data.srv_port.service, 1, true); - break; case ACTION_SERVICE_TXT_REPLACE: service = action->data.srv_txt_replace.service->service; @@ -6233,32 +6228,22 @@ handle_error: return NULL; } -esp_err_t mdns_service_port_set_for_host(const char *instance, const char *service, const char *proto, const char *hostname, uint16_t port) +esp_err_t mdns_service_port_set_for_host(const char *instance, const char *service, const char *proto, const char *host, uint16_t port) { MDNS_SERVICE_LOCK(); - if (!_mdns_server || !_mdns_server->services || _str_null_or_empty(service) || _str_null_or_empty(proto) || !port) { - MDNS_SERVICE_UNLOCK(); - return ESP_ERR_INVALID_ARG; - } + esp_err_t ret = ESP_OK; + const char *hostname = host ? host : _mdns_server->hostname; + ESP_GOTO_ON_FALSE(_mdns_server && _mdns_server->services && !_str_null_or_empty(service) && !_str_null_or_empty(proto) && port, + ESP_ERR_INVALID_ARG, err, TAG, "Invalid state or arguments"); mdns_srv_item_t *s = _mdns_get_service_item_instance(instance, service, proto, hostname); - MDNS_SERVICE_UNLOCK(); - if (!s) { - return ESP_ERR_NOT_FOUND; - } + ESP_GOTO_ON_FALSE(s, ESP_ERR_NOT_FOUND, err, TAG, "Service doesn't exist"); - 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; - action->data.srv_port.service = s; - action->data.srv_port.port = port; - if (xQueueSend(_mdns_server->action_queue, &action, (TickType_t)0) != pdPASS) { - free(action); - return ESP_ERR_NO_MEM; - } - return ESP_OK; + s->service->port = port; + _mdns_announce_all_pcbs(&s, 1, true); + +err: + MDNS_SERVICE_UNLOCK(); + return ret; } esp_err_t mdns_service_port_set(const char *service, const char *proto, uint16_t port) @@ -6266,7 +6251,7 @@ esp_err_t mdns_service_port_set(const char *service, const char *proto, uint16_t if (!_mdns_server) { return ESP_ERR_INVALID_STATE; } - return mdns_service_port_set_for_host(NULL, service, proto, _mdns_server->hostname, port); + return mdns_service_port_set_for_host(NULL, service, proto, NULL, port); } esp_err_t mdns_service_txt_set_for_host(const char *instance, const char *service, const char *proto, const char *hostname, diff --git a/components/mdns/private_include/mdns_private.h b/components/mdns/private_include/mdns_private.h index 311abd9c7..e9bb8207a 100644 --- a/components/mdns/private_include/mdns_private.h +++ b/components/mdns/private_include/mdns_private.h @@ -188,7 +188,6 @@ typedef enum { ACTION_HOSTNAME_SET, ACTION_INSTANCE_SET, ACTION_SERVICE_INSTANCE_SET, - ACTION_SERVICE_PORT_SET, ACTION_SERVICE_TXT_REPLACE, ACTION_SERVICE_TXT_SET, ACTION_SERVICE_TXT_DEL, From a927bf3a8d56fd5f1edc6188811f83fea1bc68e7 Mon Sep 17 00:00:00 2001 From: David Cermak Date: Tue, 23 Jul 2024 09:05:30 +0200 Subject: [PATCH 3/9] fix(mdns): Fix API races while setting txt for services Fixes **API race issue** (described in 8a690503) for API mdns_service_txt_set_for_host() --- components/mdns/mdns.c | 58 ++++++------------- .../mdns/private_include/mdns_private.h | 9 --- 2 files changed, 18 insertions(+), 49 deletions(-) diff --git a/components/mdns/mdns.c b/components/mdns/mdns.c index 302c8312f..aaf27d7f0 100644 --- a/components/mdns/mdns.c +++ b/components/mdns/mdns.c @@ -5111,9 +5111,6 @@ static void _mdns_free_action(mdns_action_t *action) case ACTION_SERVICE_INSTANCE_SET: free(action->data.srv_instance.instance); break; - case ACTION_SERVICE_TXT_REPLACE: - _mdns_free_linked_txt(action->data.srv_txt_replace.txt); - break; case ACTION_SERVICE_TXT_SET: free(action->data.srv_txt_set.key); free(action->data.srv_txt_set.value); @@ -5200,15 +5197,6 @@ static void _mdns_execute_action(mdns_action_t *action) action->data.srv_instance.service->service->instance = action->data.srv_instance.instance; _mdns_probe_all_pcbs(&action->data.srv_instance.service, 1, false, false); - break; - case ACTION_SERVICE_TXT_REPLACE: - service = action->data.srv_txt_replace.service->service; - txt = service->txt; - service->txt = NULL; - _mdns_free_linked_txt(txt); - service->txt = action->data.srv_txt_replace.txt; - _mdns_announce_all_pcbs(&action->data.srv_txt_replace.service, 1, false); - break; case ACTION_SERVICE_TXT_SET: service = action->data.srv_txt_set.service->service; @@ -6254,44 +6242,34 @@ esp_err_t mdns_service_port_set(const char *service, const char *proto, uint16_t return mdns_service_port_set_for_host(NULL, service, proto, NULL, port); } -esp_err_t mdns_service_txt_set_for_host(const char *instance, const char *service, const char *proto, const char *hostname, - mdns_txt_item_t txt[], uint8_t num_items) +esp_err_t mdns_service_txt_set_for_host(const char *instance, const char *service, const char *proto, const char *host, + mdns_txt_item_t txt_items[], uint8_t num_items) { MDNS_SERVICE_LOCK(); - if (!_mdns_server || !_mdns_server->services || _str_null_or_empty(service) || _str_null_or_empty(proto) || (num_items && txt == NULL)) { - MDNS_SERVICE_UNLOCK(); - return ESP_ERR_INVALID_ARG; - } + esp_err_t ret = ESP_OK; + const char *hostname = host ? host : _mdns_server->hostname; + ESP_GOTO_ON_FALSE(_mdns_server && _mdns_server->services && !_str_null_or_empty(service) && !_str_null_or_empty(proto) && !(num_items && txt_items == NULL), + ESP_ERR_INVALID_ARG, err, TAG, "Invalid state or arguments"); mdns_srv_item_t *s = _mdns_get_service_item_instance(instance, service, proto, hostname); - MDNS_SERVICE_UNLOCK(); - if (!s) { - return ESP_ERR_NOT_FOUND; - } + ESP_GOTO_ON_FALSE(s, ESP_ERR_NOT_FOUND, err, TAG, "Service doesn't exist"); mdns_txt_linked_item_t *new_txt = NULL; if (num_items) { - new_txt = _mdns_allocate_txt(num_items, txt); + new_txt = _mdns_allocate_txt(num_items, txt_items); if (!new_txt) { return ESP_ERR_NO_MEM; } } + mdns_service_t *srv = s->service; + mdns_txt_linked_item_t *txt = srv->txt; + srv->txt = NULL; + _mdns_free_linked_txt(txt); + srv->txt = new_txt; + _mdns_announce_all_pcbs(&s, 1, false); - 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; - } - action->type = ACTION_SERVICE_TXT_REPLACE; - action->data.srv_txt_replace.service = s; - action->data.srv_txt_replace.txt = new_txt; - - if (xQueueSend(_mdns_server->action_queue, &action, (TickType_t)0) != pdPASS) { - _mdns_free_linked_txt(new_txt); - free(action); - return ESP_ERR_NO_MEM; - } - return ESP_OK; +err: + MDNS_SERVICE_UNLOCK(); + return ret; } esp_err_t mdns_service_txt_set(const char *service, const char *proto, mdns_txt_item_t txt[], uint8_t num_items) @@ -6299,7 +6277,7 @@ esp_err_t mdns_service_txt_set(const char *service, const char *proto, mdns_txt_ if (!_mdns_server) { return ESP_ERR_INVALID_STATE; } - return mdns_service_txt_set_for_host(NULL, service, proto, _mdns_server->hostname, txt, num_items); + return mdns_service_txt_set_for_host(NULL, service, proto, NULL, txt, num_items); } esp_err_t mdns_service_txt_item_set_for_host_with_explicit_value_len(const char *instance, const char *service, const char *proto, diff --git a/components/mdns/private_include/mdns_private.h b/components/mdns/private_include/mdns_private.h index e9bb8207a..08a44bc4a 100644 --- a/components/mdns/private_include/mdns_private.h +++ b/components/mdns/private_include/mdns_private.h @@ -188,7 +188,6 @@ typedef enum { ACTION_HOSTNAME_SET, ACTION_INSTANCE_SET, ACTION_SERVICE_INSTANCE_SET, - ACTION_SERVICE_TXT_REPLACE, ACTION_SERVICE_TXT_SET, ACTION_SERVICE_TXT_DEL, ACTION_SERVICE_SUBTYPE_ADD, @@ -447,14 +446,6 @@ typedef struct { mdns_srv_item_t *service; char *instance; } srv_instance; - struct { - mdns_srv_item_t *service; - uint16_t port; - } srv_port; - struct { - mdns_srv_item_t *service; - mdns_txt_linked_item_t *txt; - } srv_txt_replace; struct { mdns_srv_item_t *service; char *key; From c62b920bb9885169f050c6dd79a2f0a73bb6a024 Mon Sep 17 00:00:00 2001 From: David Cermak Date: Tue, 23 Jul 2024 09:36:50 +0200 Subject: [PATCH 4/9] fix(mdns): Fix API races adding txt item for services Fixes **API race issue** (described in 8a690503) for API mdns_service_txt_item_set_for_host_with_explicit_value_len() --- components/mdns/mdns.c | 124 +++++++----------- .../mdns/private_include/mdns_private.h | 7 - 2 files changed, 44 insertions(+), 87 deletions(-) diff --git a/components/mdns/mdns.c b/components/mdns/mdns.c index aaf27d7f0..60efdfa5b 100644 --- a/components/mdns/mdns.c +++ b/components/mdns/mdns.c @@ -5111,10 +5111,6 @@ static void _mdns_free_action(mdns_action_t *action) case ACTION_SERVICE_INSTANCE_SET: free(action->data.srv_instance.instance); break; - case ACTION_SERVICE_TXT_SET: - free(action->data.srv_txt_set.key); - free(action->data.srv_txt_set.value); - break; case ACTION_SERVICE_TXT_DEL: free(action->data.srv_txt_del.key); break; @@ -5164,7 +5160,6 @@ static void _mdns_execute_action(mdns_action_t *action) mdns_srv_item_t *a = NULL; mdns_service_t *service; char *key; - char *value; char *subtype; mdns_subtype_t *subtype_item; mdns_txt_linked_item_t *txt, * t; @@ -5197,38 +5192,6 @@ static void _mdns_execute_action(mdns_action_t *action) action->data.srv_instance.service->service->instance = action->data.srv_instance.instance; _mdns_probe_all_pcbs(&action->data.srv_instance.service, 1, false, false); - break; - case ACTION_SERVICE_TXT_SET: - service = action->data.srv_txt_set.service->service; - key = action->data.srv_txt_set.key; - value = action->data.srv_txt_set.value; - txt = service->txt; - while (txt) { - if (strcmp(txt->key, key) == 0) { - free((char *)txt->value); - free(key); - txt->value = value; - txt->value_len = action->data.srv_txt_set.value_len; - break; - } - txt = txt->next; - } - 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; - } - txt->key = key; - txt->value = value; - txt->value_len = action->data.srv_txt_set.value_len; - txt->next = service->txt; - service->txt = txt; - } - - _mdns_announce_all_pcbs(&action->data.srv_txt_set.service, 1, false); - break; case ACTION_SERVICE_TXT_DEL: service = action->data.srv_txt_del.service->service; @@ -5258,7 +5221,7 @@ static void _mdns_execute_action(mdns_action_t *action) } free(key); - _mdns_announce_all_pcbs(&action->data.srv_txt_set.service, 1, false); + _mdns_announce_all_pcbs(&action->data.srv_txt_del.service, 1, false); break; case ACTION_SERVICE_SUBTYPE_ADD: @@ -6281,51 +6244,53 @@ esp_err_t mdns_service_txt_set(const char *service, const char *proto, mdns_txt_ } esp_err_t mdns_service_txt_item_set_for_host_with_explicit_value_len(const char *instance, const char *service, const char *proto, - const char *hostname, const char *key, - const char *value, uint8_t value_len) + const char *host, const char *key, const char *value_arg, uint8_t value_len) { MDNS_SERVICE_LOCK(); - if (!_mdns_server || !_mdns_server->services || _str_null_or_empty(service) || _str_null_or_empty(proto) || - _str_null_or_empty(key) || (!value && value_len)) { - MDNS_SERVICE_UNLOCK(); - return ESP_ERR_INVALID_ARG; - } + esp_err_t ret = ESP_OK; + const char *hostname = host ? host : _mdns_server->hostname; + ESP_GOTO_ON_FALSE(_mdns_server && _mdns_server->services && !_str_null_or_empty(service) && !_str_null_or_empty(proto) && !_str_null_or_empty(key) && + !((!value_arg && value_len)), ESP_ERR_INVALID_ARG, err, TAG, "Invalid state or arguments"); + mdns_srv_item_t *s = _mdns_get_service_item_instance(instance, service, proto, hostname); - MDNS_SERVICE_UNLOCK(); - if (!s) { - return ESP_ERR_NOT_FOUND; + ESP_GOTO_ON_FALSE(s, ESP_ERR_NOT_FOUND, err, TAG, "Service doesn't exist"); + + mdns_service_t *srv = s->service; + char *value = NULL; + if (value_len > 0) { + value = (char *) malloc(value_len); + ESP_GOTO_ON_FALSE(s, ESP_ERR_NO_MEM, err, TAG, "Out of memory"); + memcpy(value, value_arg, value_len); + } else { + value_len = 0; } - mdns_action_t *action = (mdns_action_t *)malloc(sizeof(mdns_action_t)); - if (!action) { - HOOK_MALLOC_FAILED; - return ESP_ERR_NO_MEM; + mdns_txt_linked_item_t *txt = srv->txt; + while (txt) { + if (strcmp(txt->key, key) == 0) { + free((char *)txt->value); + txt->value = value; + txt->value_len = value_len; + break; + } + txt = txt->next; + } + if (!txt) { + txt = (mdns_txt_linked_item_t *)malloc(sizeof(mdns_txt_linked_item_t)); + ESP_GOTO_ON_FALSE(s, ESP_ERR_NO_MEM, err, TAG, "Out of memory"); + txt->key = strdup(key); + ESP_GOTO_ON_FALSE(txt->key, ESP_ERR_NO_MEM, err, TAG, "Out of memory"); + txt->value = value; + txt->value_len = value_len; + txt->next = srv->txt; + srv->txt = txt; } - action->type = ACTION_SERVICE_TXT_SET; - action->data.srv_txt_set.service = s; - action->data.srv_txt_set.key = strdup(key); - if (!action->data.srv_txt_set.key) { - free(action); - return ESP_ERR_NO_MEM; - } - if (value_len > 0) { - action->data.srv_txt_set.value = (char *)malloc(value_len); - if (!action->data.srv_txt_set.value) { - free(action->data.srv_txt_set.key); - free(action); - return ESP_ERR_NO_MEM; - } - memcpy(action->data.srv_txt_set.value, value, value_len); - action->data.srv_txt_set.value_len = value_len; - } else { - action->data.srv_txt_set.value = NULL; - action->data.srv_txt_set.value_len = 0; - } - if (xQueueSend(_mdns_server->action_queue, &action, (TickType_t)0) != pdPASS) { - free(action->data.srv_txt_set.key); - free(action->data.srv_txt_set.value); - free(action); - return ESP_ERR_NO_MEM; + _mdns_announce_all_pcbs(&s, 1, false); + +err: + MDNS_SERVICE_UNLOCK(); + if (ret == ESP_ERR_NO_MEM) { + HOOK_MALLOC_FAILED; } return ESP_OK; } @@ -6343,7 +6308,7 @@ esp_err_t mdns_service_txt_item_set(const char *service, const char *proto, cons if (!_mdns_server) { return ESP_ERR_INVALID_STATE; } - return mdns_service_txt_item_set_for_host_with_explicit_value_len(NULL, service, proto, _mdns_server->hostname, key, + return mdns_service_txt_item_set_for_host_with_explicit_value_len(NULL, service, proto, NULL, key, value, strlen(value)); } @@ -6353,8 +6318,7 @@ esp_err_t mdns_service_txt_item_set_with_explicit_value_len(const char *service, if (!_mdns_server) { return ESP_ERR_INVALID_STATE; } - return mdns_service_txt_item_set_for_host_with_explicit_value_len(NULL, service, proto, _mdns_server->hostname, key, - value, value_len); + return mdns_service_txt_item_set_for_host_with_explicit_value_len(NULL, service, proto, NULL, key, value, value_len); } esp_err_t mdns_service_txt_item_remove_for_host(const char *instance, const char *service, const char *proto, const char *hostname, diff --git a/components/mdns/private_include/mdns_private.h b/components/mdns/private_include/mdns_private.h index 08a44bc4a..9ee9a53f1 100644 --- a/components/mdns/private_include/mdns_private.h +++ b/components/mdns/private_include/mdns_private.h @@ -188,7 +188,6 @@ typedef enum { ACTION_HOSTNAME_SET, ACTION_INSTANCE_SET, ACTION_SERVICE_INSTANCE_SET, - ACTION_SERVICE_TXT_SET, ACTION_SERVICE_TXT_DEL, ACTION_SERVICE_SUBTYPE_ADD, ACTION_SERVICES_CLEAR, @@ -446,12 +445,6 @@ typedef struct { mdns_srv_item_t *service; char *instance; } srv_instance; - struct { - mdns_srv_item_t *service; - char *key; - char *value; - uint8_t value_len; - } srv_txt_set; struct { mdns_srv_item_t *service; char *key; From 3f97a8228b743ba7b1a0fbe91e285591b69cb795 Mon Sep 17 00:00:00 2001 From: David Cermak Date: Tue, 23 Jul 2024 10:02:26 +0200 Subject: [PATCH 5/9] fix(mdns): Fix API races removing txt item for services Fixes **API race issue** (described in 8a690503) for API mdns_service_txt_item_remove_for_host() --- components/mdns/mdns.c | 117 +++++++----------- .../mdns/private_include/mdns_private.h | 5 - 2 files changed, 48 insertions(+), 74 deletions(-) diff --git a/components/mdns/mdns.c b/components/mdns/mdns.c index 60efdfa5b..9bf5e1262 100644 --- a/components/mdns/mdns.c +++ b/components/mdns/mdns.c @@ -5111,9 +5111,6 @@ static void _mdns_free_action(mdns_action_t *action) case ACTION_SERVICE_INSTANCE_SET: free(action->data.srv_instance.instance); break; - case ACTION_SERVICE_TXT_DEL: - free(action->data.srv_txt_del.key); - break; case ACTION_SERVICE_SUBTYPE_ADD: free(action->data.srv_subtype_add.subtype); break; @@ -5159,10 +5156,8 @@ static void _mdns_execute_action(mdns_action_t *action) { mdns_srv_item_t *a = NULL; mdns_service_t *service; - char *key; char *subtype; mdns_subtype_t *subtype_item; - mdns_txt_linked_item_t *txt, * t; switch (action->type) { case ACTION_SYSTEM_EVENT: @@ -5192,37 +5187,6 @@ static void _mdns_execute_action(mdns_action_t *action) action->data.srv_instance.service->service->instance = action->data.srv_instance.instance; _mdns_probe_all_pcbs(&action->data.srv_instance.service, 1, false, false); - break; - case ACTION_SERVICE_TXT_DEL: - service = action->data.srv_txt_del.service->service; - key = action->data.srv_txt_del.key; - txt = service->txt; - if (!txt) { - break; - } - if (strcmp(txt->key, key) == 0) { - service->txt = txt->next; - free((char *)txt->key); - free((char *)txt->value); - free(txt); - } else { - while (txt->next) { - if (strcmp(txt->next->key, key) == 0) { - t = txt->next; - txt->next = t->next; - free((char *)t->key); - free((char *)t->value); - free(t); - break; - } else { - txt = txt->next; - } - } - } - free(key); - - _mdns_announce_all_pcbs(&action->data.srv_txt_del.service, 1, false); - break; case ACTION_SERVICE_SUBTYPE_ADD: service = action->data.srv_subtype_add.service->service; @@ -6248,6 +6212,7 @@ esp_err_t mdns_service_txt_item_set_for_host_with_explicit_value_len(const char { MDNS_SERVICE_LOCK(); esp_err_t ret = ESP_OK; + char *value = NULL; const char *hostname = host ? host : _mdns_server->hostname; ESP_GOTO_ON_FALSE(_mdns_server && _mdns_server->services && !_str_null_or_empty(service) && !_str_null_or_empty(proto) && !_str_null_or_empty(key) && !((!value_arg && value_len)), ESP_ERR_INVALID_ARG, err, TAG, "Invalid state or arguments"); @@ -6256,10 +6221,9 @@ esp_err_t mdns_service_txt_item_set_for_host_with_explicit_value_len(const char ESP_GOTO_ON_FALSE(s, ESP_ERR_NOT_FOUND, err, TAG, "Service doesn't exist"); mdns_service_t *srv = s->service; - char *value = NULL; if (value_len > 0) { value = (char *) malloc(value_len); - ESP_GOTO_ON_FALSE(s, ESP_ERR_NO_MEM, err, TAG, "Out of memory"); + ESP_GOTO_ON_FALSE(s, ESP_ERR_NO_MEM, out_of_mem, TAG, "Out of memory"); memcpy(value, value_arg, value_len); } else { value_len = 0; @@ -6276,9 +6240,9 @@ esp_err_t mdns_service_txt_item_set_for_host_with_explicit_value_len(const char } if (!txt) { txt = (mdns_txt_linked_item_t *)malloc(sizeof(mdns_txt_linked_item_t)); - ESP_GOTO_ON_FALSE(s, ESP_ERR_NO_MEM, err, TAG, "Out of memory"); + ESP_GOTO_ON_FALSE(s, ESP_ERR_NO_MEM, out_of_mem, TAG, "Out of memory"); txt->key = strdup(key); - ESP_GOTO_ON_FALSE(txt->key, ESP_ERR_NO_MEM, err, TAG, "Out of memory"); + ESP_GOTO_ON_FALSE(txt->key, ESP_ERR_NO_MEM, out_of_mem, TAG, "Out of memory"); txt->value = value; txt->value_len = value_len; txt->next = srv->txt; @@ -6289,10 +6253,12 @@ esp_err_t mdns_service_txt_item_set_for_host_with_explicit_value_len(const char err: MDNS_SERVICE_UNLOCK(); - if (ret == ESP_ERR_NO_MEM) { - HOOK_MALLOC_FAILED; - } - return ESP_OK; + return ret; +out_of_mem: + MDNS_SERVICE_UNLOCK(); + HOOK_MALLOC_FAILED; + free(value); + return ret; } esp_err_t mdns_service_txt_item_set_for_host(const char *instance, const char *service, const char *proto, const char *hostname, @@ -6321,38 +6287,51 @@ esp_err_t mdns_service_txt_item_set_with_explicit_value_len(const char *service, return mdns_service_txt_item_set_for_host_with_explicit_value_len(NULL, service, proto, NULL, key, value, value_len); } -esp_err_t mdns_service_txt_item_remove_for_host(const char *instance, const char *service, const char *proto, const char *hostname, +esp_err_t mdns_service_txt_item_remove_for_host(const char *instance, const char *service, const char *proto, const char *host, const char *key) { MDNS_SERVICE_LOCK(); - if (!_mdns_server || !_mdns_server->services || _str_null_or_empty(service) || _str_null_or_empty(proto) || _str_null_or_empty(key)) { - MDNS_SERVICE_UNLOCK(); - return ESP_ERR_INVALID_ARG; - } + esp_err_t ret = ESP_OK; + const char *hostname = host ? host : _mdns_server->hostname; + ESP_GOTO_ON_FALSE(_mdns_server && _mdns_server->services && !_str_null_or_empty(service) && !_str_null_or_empty(proto) && !_str_null_or_empty(key), + ESP_ERR_INVALID_ARG, err, TAG, "Invalid state or arguments"); + mdns_srv_item_t *s = _mdns_get_service_item_instance(instance, service, proto, hostname); - MDNS_SERVICE_UNLOCK(); - if (!s) { - return ESP_ERR_NOT_FOUND; + ESP_GOTO_ON_FALSE(s, ESP_ERR_NOT_FOUND, err, TAG, "Service doesn't exist"); + + mdns_service_t *srv = s->service; + mdns_txt_linked_item_t *txt = srv->txt; + if (!txt) { + goto err; } - mdns_action_t *action = (mdns_action_t *)malloc(sizeof(mdns_action_t)); - if (!action) { - HOOK_MALLOC_FAILED; - return ESP_ERR_NO_MEM; + if (strcmp(txt->key, key) == 0) { + srv->txt = txt->next; + free((char *)txt->key); + free((char *)txt->value); + free(txt); + } else { + while (txt->next) { + if (strcmp(txt->next->key, key) == 0) { + mdns_txt_linked_item_t *t = txt->next; + txt->next = t->next; + free((char *)t->key); + free((char *)t->value); + free(t); + break; + } else { + txt = txt->next; + } + } } - action->type = ACTION_SERVICE_TXT_DEL; - action->data.srv_txt_del.service = s; - action->data.srv_txt_del.key = strdup(key); - if (!action->data.srv_txt_del.key) { - free(action); - return ESP_ERR_NO_MEM; + _mdns_announce_all_pcbs(&s, 1, false); + +err: + MDNS_SERVICE_UNLOCK(); + if (ret == ESP_ERR_NO_MEM) { + HOOK_MALLOC_FAILED; } - if (xQueueSend(_mdns_server->action_queue, &action, (TickType_t)0) != pdPASS) { - free(action->data.srv_txt_del.key); - free(action); - return ESP_ERR_NO_MEM; - } - return ESP_OK; + return ret; } esp_err_t mdns_service_txt_item_remove(const char *service, const char *proto, const char *key) @@ -6360,7 +6339,7 @@ esp_err_t mdns_service_txt_item_remove(const char *service, const char *proto, c if (!_mdns_server) { return ESP_ERR_INVALID_STATE; } - return mdns_service_txt_item_remove_for_host(NULL, service, proto, _mdns_server->hostname, key); + return mdns_service_txt_item_remove_for_host(NULL, service, proto, NULL, key); } esp_err_t mdns_service_subtype_add_for_host(const char *instance_name, const char *service, const char *proto, diff --git a/components/mdns/private_include/mdns_private.h b/components/mdns/private_include/mdns_private.h index 9ee9a53f1..b220041e8 100644 --- a/components/mdns/private_include/mdns_private.h +++ b/components/mdns/private_include/mdns_private.h @@ -188,7 +188,6 @@ typedef enum { ACTION_HOSTNAME_SET, ACTION_INSTANCE_SET, ACTION_SERVICE_INSTANCE_SET, - ACTION_SERVICE_TXT_DEL, ACTION_SERVICE_SUBTYPE_ADD, ACTION_SERVICES_CLEAR, ACTION_SEARCH_ADD, @@ -445,10 +444,6 @@ typedef struct { mdns_srv_item_t *service; char *instance; } srv_instance; - struct { - mdns_srv_item_t *service; - char *key; - } srv_txt_del; struct { mdns_srv_item_t *service; char *subtype; From f9f234c440703943690ffbe09aa2a225a528f7d6 Mon Sep 17 00:00:00 2001 From: David Cermak Date: Tue, 23 Jul 2024 10:27:12 +0200 Subject: [PATCH 6/9] fix(mdns): Fix API races while adding subtypes for services Fixes **API race issue** (described in 8a690503) for API mdns_service_subtype_add_for_host() --- components/mdns/mdns.c | 88 ++++++------------- .../mdns/private_include/mdns_private.h | 5 -- 2 files changed, 29 insertions(+), 64 deletions(-) diff --git a/components/mdns/mdns.c b/components/mdns/mdns.c index 9bf5e1262..d94ed8d80 100644 --- a/components/mdns/mdns.c +++ b/components/mdns/mdns.c @@ -5111,9 +5111,6 @@ static void _mdns_free_action(mdns_action_t *action) case ACTION_SERVICE_INSTANCE_SET: free(action->data.srv_instance.instance); break; - case ACTION_SERVICE_SUBTYPE_ADD: - free(action->data.srv_subtype_add.subtype); - break; case ACTION_SEARCH_ADD: //fallthrough case ACTION_SEARCH_SEND: @@ -5155,9 +5152,6 @@ static void _mdns_free_action(mdns_action_t *action) static void _mdns_execute_action(mdns_action_t *action) { mdns_srv_item_t *a = NULL; - mdns_service_t *service; - char *subtype; - mdns_subtype_t *subtype_item; switch (action->type) { case ACTION_SYSTEM_EVENT: @@ -5187,19 +5181,6 @@ static void _mdns_execute_action(mdns_action_t *action) action->data.srv_instance.service->service->instance = action->data.srv_instance.instance; _mdns_probe_all_pcbs(&action->data.srv_instance.service, 1, false, false); - break; - case ACTION_SERVICE_SUBTYPE_ADD: - service = action->data.srv_subtype_add.service->service; - subtype = action->data.srv_subtype_add.subtype; - subtype_item = (mdns_subtype_t *)malloc(sizeof(mdns_subtype_t)); - if (!subtype_item) { - HOOK_MALLOC_FAILED; - _mdns_free_action(action); - return; - } - subtype_item->subtype = subtype; - subtype_item->next = service->subtype; - service->subtype = subtype_item; break; case ACTION_SERVICES_CLEAR: _mdns_send_final_bye(false); @@ -6213,6 +6194,7 @@ esp_err_t mdns_service_txt_item_set_for_host_with_explicit_value_len(const char MDNS_SERVICE_LOCK(); esp_err_t ret = ESP_OK; char *value = NULL; + mdns_txt_linked_item_t *new_txt = NULL; const char *hostname = host ? host : _mdns_server->hostname; ESP_GOTO_ON_FALSE(_mdns_server && _mdns_server->services && !_str_null_or_empty(service) && !_str_null_or_empty(proto) && !_str_null_or_empty(key) && !((!value_arg && value_len)), ESP_ERR_INVALID_ARG, err, TAG, "Invalid state or arguments"); @@ -6223,7 +6205,7 @@ esp_err_t mdns_service_txt_item_set_for_host_with_explicit_value_len(const char mdns_service_t *srv = s->service; if (value_len > 0) { value = (char *) malloc(value_len); - ESP_GOTO_ON_FALSE(s, ESP_ERR_NO_MEM, out_of_mem, TAG, "Out of memory"); + ESP_GOTO_ON_FALSE(value, ESP_ERR_NO_MEM, out_of_mem, TAG, "Out of memory"); memcpy(value, value_arg, value_len); } else { value_len = 0; @@ -6239,14 +6221,14 @@ esp_err_t mdns_service_txt_item_set_for_host_with_explicit_value_len(const char txt = txt->next; } if (!txt) { - txt = (mdns_txt_linked_item_t *)malloc(sizeof(mdns_txt_linked_item_t)); - ESP_GOTO_ON_FALSE(s, ESP_ERR_NO_MEM, out_of_mem, TAG, "Out of memory"); - txt->key = strdup(key); - ESP_GOTO_ON_FALSE(txt->key, ESP_ERR_NO_MEM, out_of_mem, TAG, "Out of memory"); - txt->value = value; - txt->value_len = value_len; - txt->next = srv->txt; - srv->txt = txt; + new_txt = (mdns_txt_linked_item_t *)malloc(sizeof(mdns_txt_linked_item_t)); + ESP_GOTO_ON_FALSE(new_txt, ESP_ERR_NO_MEM, out_of_mem, TAG, "Out of memory"); + new_txt->key = strdup(key); + ESP_GOTO_ON_FALSE(new_txt->key, ESP_ERR_NO_MEM, out_of_mem, TAG, "Out of memory"); + new_txt->value = value; + new_txt->value_len = value_len; + new_txt->next = srv->txt; + srv->txt = new_txt; } _mdns_announce_all_pcbs(&s, 1, false); @@ -6258,6 +6240,7 @@ out_of_mem: MDNS_SERVICE_UNLOCK(); HOOK_MALLOC_FAILED; free(value); + free(new_txt); return ret; } @@ -6346,46 +6329,33 @@ esp_err_t mdns_service_subtype_add_for_host(const char *instance_name, const cha const char *hostname, const char *subtype) { MDNS_SERVICE_LOCK(); - if (!_mdns_server || !_mdns_server->services || _str_null_or_empty(service) || _str_null_or_empty(proto) || - _str_null_or_empty(subtype)) { - MDNS_SERVICE_UNLOCK(); - return ESP_ERR_INVALID_ARG; - } + esp_err_t ret = ESP_OK; + ESP_GOTO_ON_FALSE(_mdns_server && _mdns_server->services && !_str_null_or_empty(service) && !_str_null_or_empty(proto) && + !_str_null_or_empty(subtype), ESP_ERR_INVALID_ARG, err, TAG, "Invalid state or arguments"); + mdns_srv_item_t *s = _mdns_get_service_item_instance(instance_name, service, proto, hostname); - MDNS_SERVICE_UNLOCK(); - if (!s) { - return ESP_ERR_NOT_FOUND; - } + ESP_GOTO_ON_FALSE(s, ESP_ERR_NOT_FOUND, err, TAG, "Service doesn't exist"); mdns_subtype_t *srv_subtype = s->service->subtype; while (srv_subtype) { - if (strcmp(srv_subtype->subtype, subtype) == 0) { - // The same subtype has already been added - return ESP_ERR_INVALID_ARG; - } + ESP_GOTO_ON_FALSE(strcmp(srv_subtype->subtype, subtype) != 0, ESP_ERR_INVALID_ARG, err, TAG, "The same subtype has already been added"); srv_subtype = srv_subtype->next; } - mdns_action_t *action = (mdns_action_t *)malloc(sizeof(mdns_action_t)); - if (!action) { + mdns_service_t *srv = s->service; + mdns_subtype_t *subtype_item = (mdns_subtype_t *)malloc(sizeof(mdns_subtype_t)); + ESP_GOTO_ON_FALSE(subtype_item, ESP_ERR_NO_MEM, err, TAG, "Out of memory"); + subtype_item->subtype = strdup(subtype); + ESP_GOTO_ON_FALSE(subtype_item->subtype, ESP_ERR_NO_MEM, err, TAG, "Out of memory"); + subtype_item->next = srv->subtype; + srv->subtype = subtype_item; + +err: + MDNS_SERVICE_UNLOCK(); + if (ret == ESP_ERR_NO_MEM) { HOOK_MALLOC_FAILED; - return ESP_ERR_NO_MEM; } - - action->type = ACTION_SERVICE_SUBTYPE_ADD; - action->data.srv_subtype_add.service = s; - action->data.srv_subtype_add.subtype = strdup(subtype); - - if (!action->data.srv_subtype_add.subtype) { - free(action); - return ESP_ERR_NO_MEM; - } - if (xQueueSend(_mdns_server->action_queue, &action, (TickType_t)0) != pdPASS) { - free(action->data.srv_subtype_add.subtype); - free(action); - return ESP_ERR_NO_MEM; - } - return ESP_OK; + return ret; } esp_err_t mdns_service_instance_name_set_for_host(const char *instance_old, const char *service, const char *proto, const char *hostname, diff --git a/components/mdns/private_include/mdns_private.h b/components/mdns/private_include/mdns_private.h index b220041e8..131a55590 100644 --- a/components/mdns/private_include/mdns_private.h +++ b/components/mdns/private_include/mdns_private.h @@ -188,7 +188,6 @@ typedef enum { ACTION_HOSTNAME_SET, ACTION_INSTANCE_SET, ACTION_SERVICE_INSTANCE_SET, - ACTION_SERVICE_SUBTYPE_ADD, ACTION_SERVICES_CLEAR, ACTION_SEARCH_ADD, ACTION_SEARCH_SEND, @@ -444,10 +443,6 @@ typedef struct { mdns_srv_item_t *service; char *instance; } srv_instance; - struct { - mdns_srv_item_t *service; - char *subtype; - } srv_subtype_add; struct { mdns_search_once_t *search; } search_add; From 643dc6d43b324ab5db24afae462936d64ec3ae03 Mon Sep 17 00:00:00 2001 From: David Cermak Date: Tue, 23 Jul 2024 10:46:26 +0200 Subject: [PATCH 7/9] fix(mdns): Fix API races setting instance name for services Fixes **API race issue** (described in 8a690503) for API mdns_service_instance_name_set_for_host() --- components/mdns/mdns.c | 79 +++++++------------ .../mdns/private_include/mdns_private.h | 5 -- 2 files changed, 28 insertions(+), 56 deletions(-) diff --git a/components/mdns/mdns.c b/components/mdns/mdns.c index d94ed8d80..66c5eeb55 100644 --- a/components/mdns/mdns.c +++ b/components/mdns/mdns.c @@ -5108,9 +5108,6 @@ static void _mdns_free_action(mdns_action_t *action) case ACTION_INSTANCE_SET: free(action->data.instance); break; - case ACTION_SERVICE_INSTANCE_SET: - free(action->data.srv_instance.instance); - break; case ACTION_SEARCH_ADD: //fallthrough case ACTION_SEARCH_SEND: @@ -5172,15 +5169,6 @@ static void _mdns_execute_action(mdns_action_t *action) _mdns_server->instance = action->data.instance; _mdns_restart_all_pcbs_no_instance(); - break; - case ACTION_SERVICE_INSTANCE_SET: - if (action->data.srv_instance.service->service->instance) { - _mdns_send_bye(&action->data.srv_instance.service, 1, false); - free((char *)action->data.srv_instance.service->service->instance); - } - action->data.srv_instance.service->service->instance = action->data.srv_instance.instance; - _mdns_probe_all_pcbs(&action->data.srv_instance.service, 1, false, false); - break; case ACTION_SERVICES_CLEAR: _mdns_send_final_bye(false); @@ -6344,57 +6332,46 @@ esp_err_t mdns_service_subtype_add_for_host(const char *instance_name, const cha mdns_service_t *srv = s->service; mdns_subtype_t *subtype_item = (mdns_subtype_t *)malloc(sizeof(mdns_subtype_t)); - ESP_GOTO_ON_FALSE(subtype_item, ESP_ERR_NO_MEM, err, TAG, "Out of memory"); + ESP_GOTO_ON_FALSE(subtype_item, ESP_ERR_NO_MEM, out_of_mem, TAG, "Out of memory"); subtype_item->subtype = strdup(subtype); - ESP_GOTO_ON_FALSE(subtype_item->subtype, ESP_ERR_NO_MEM, err, TAG, "Out of memory"); + ESP_GOTO_ON_FALSE(subtype_item->subtype, ESP_ERR_NO_MEM, out_of_mem, TAG, "Out of memory"); subtype_item->next = srv->subtype; srv->subtype = subtype_item; err: MDNS_SERVICE_UNLOCK(); - if (ret == ESP_ERR_NO_MEM) { - HOOK_MALLOC_FAILED; - } + return ret; +out_of_mem: + MDNS_SERVICE_UNLOCK(); + HOOK_MALLOC_FAILED; + free(subtype_item); return ret; } -esp_err_t mdns_service_instance_name_set_for_host(const char *instance_old, const char *service, const char *proto, const char *hostname, +esp_err_t mdns_service_instance_name_set_for_host(const char *instance_old, const char *service, const char *proto, const char *host, const char *instance) { MDNS_SERVICE_LOCK(); - if (!_mdns_server || !_mdns_server->services || _str_null_or_empty(service) || _str_null_or_empty(proto)) { - MDNS_SERVICE_UNLOCK(); - return ESP_ERR_INVALID_ARG; - } - if (_str_null_or_empty(instance) || strlen(instance) > (MDNS_NAME_BUF_LEN - 1)) { - MDNS_SERVICE_UNLOCK(); - return ESP_ERR_INVALID_ARG; - } - mdns_srv_item_t *s = _mdns_get_service_item_instance(instance_old, service, proto, hostname); - MDNS_SERVICE_UNLOCK(); - if (!s) { - return ESP_ERR_NOT_FOUND; - } - char *new_instance = strndup(instance, MDNS_NAME_BUF_LEN - 1); - if (!new_instance) { - return ESP_ERR_NO_MEM; - } + esp_err_t ret = ESP_OK; + const char *hostname = host ? host : _mdns_server->hostname; - 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; + ESP_GOTO_ON_FALSE(_mdns_server && _mdns_server->services && !_str_null_or_empty(service) && !_str_null_or_empty(proto) && + !_str_null_or_empty(instance) && strlen(instance) <= (MDNS_NAME_BUF_LEN - 1), ESP_ERR_INVALID_ARG, err, TAG, "Invalid state or arguments"); + + mdns_srv_item_t *s = _mdns_get_service_item_instance(instance_old, service, proto, hostname); + ESP_GOTO_ON_FALSE(s, ESP_ERR_NOT_FOUND, err, TAG, "Service doesn't exist"); + + if (s->service->instance) { + _mdns_send_bye(&s, 1, false); + free((char *)s->service->instance); } - action->type = ACTION_SERVICE_INSTANCE_SET; - action->data.srv_instance.service = s; - action->data.srv_instance.instance = new_instance; - if (xQueueSend(_mdns_server->action_queue, &action, (TickType_t)0) != pdPASS) { - free(new_instance); - free(action); - return ESP_ERR_NO_MEM; - } - return ESP_OK; + s->service->instance = strndup(instance, MDNS_NAME_BUF_LEN - 1); + ESP_GOTO_ON_FALSE(s->service->instance, ESP_ERR_NO_MEM, err, TAG, "Out of memory"); + _mdns_probe_all_pcbs(&s, 1, false, false); + +err: + MDNS_SERVICE_UNLOCK(); + return ret; } esp_err_t mdns_service_instance_name_set(const char *service, const char *proto, const char *instance) @@ -6402,7 +6379,7 @@ esp_err_t mdns_service_instance_name_set(const char *service, const char *proto, if (!_mdns_server) { return ESP_ERR_INVALID_STATE; } - return mdns_service_instance_name_set_for_host(NULL, service, proto, _mdns_server->hostname, instance); + return mdns_service_instance_name_set_for_host(NULL, service, proto, NULL, instance); } esp_err_t mdns_service_remove_for_host(const char *instance, const char *service, const char *proto, const char *host) @@ -6413,7 +6390,7 @@ esp_err_t mdns_service_remove_for_host(const char *instance, const char *service ESP_GOTO_ON_FALSE(_mdns_server && _mdns_server->services && !_str_null_or_empty(service) && !_str_null_or_empty(proto), ESP_ERR_INVALID_ARG, err, TAG, "Invalid state or arguments"); mdns_srv_item_t *s = _mdns_get_service_item_instance(instance, service, proto, hostname); - ESP_GOTO_ON_FALSE(s, ESP_ERR_INVALID_ARG, err, TAG, "Service doesn't exist"); + ESP_GOTO_ON_FALSE(s, ESP_ERR_NOT_FOUND, err, TAG, "Service doesn't exist"); mdns_srv_item_t *a = _mdns_server->services; mdns_srv_item_t *b = a; diff --git a/components/mdns/private_include/mdns_private.h b/components/mdns/private_include/mdns_private.h index 131a55590..9b6554fd9 100644 --- a/components/mdns/private_include/mdns_private.h +++ b/components/mdns/private_include/mdns_private.h @@ -187,7 +187,6 @@ typedef enum { ACTION_SYSTEM_EVENT, ACTION_HOSTNAME_SET, ACTION_INSTANCE_SET, - ACTION_SERVICE_INSTANCE_SET, ACTION_SERVICES_CLEAR, ACTION_SEARCH_ADD, ACTION_SEARCH_SEND, @@ -439,10 +438,6 @@ typedef struct { mdns_if_t interface; mdns_event_actions_t event_action; } sys_event; - struct { - mdns_srv_item_t *service; - char *instance; - } srv_instance; struct { mdns_search_once_t *search; } search_add; From 169405b534d7558cfc562eeb650304cb080af60b Mon Sep 17 00:00:00 2001 From: David Cermak Date: Tue, 23 Jul 2024 11:11:11 +0200 Subject: [PATCH 8/9] fix(mdns): Fix API races when removing all services Fixes **API race issue** (described in 8a690503) for API mdns_service_remove_all() --- components/mdns/mdns.c | 47 +++++++------------ .../mdns/private_include/mdns_private.h | 1 - 2 files changed, 16 insertions(+), 32 deletions(-) diff --git a/components/mdns/mdns.c b/components/mdns/mdns.c index 66c5eeb55..b8062c5ca 100644 --- a/components/mdns/mdns.c +++ b/components/mdns/mdns.c @@ -5148,8 +5148,6 @@ static void _mdns_free_action(mdns_action_t *action) */ static void _mdns_execute_action(mdns_action_t *action) { - mdns_srv_item_t *a = NULL; - switch (action->type) { case ACTION_SYSTEM_EVENT: perform_event_action(action->data.sys_event.interface, action->data.sys_event.event_action); @@ -5169,19 +5167,6 @@ static void _mdns_execute_action(mdns_action_t *action) _mdns_server->instance = action->data.instance; _mdns_restart_all_pcbs_no_instance(); - break; - case ACTION_SERVICES_CLEAR: - _mdns_send_final_bye(false); - a = _mdns_server->services; - _mdns_server->services = NULL; - 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); - } - break; case ACTION_SEARCH_ADD: _mdns_search_add(action->data.search_add.search); @@ -6453,27 +6438,27 @@ esp_err_t mdns_service_remove(const char *service_type, const char *proto) esp_err_t mdns_service_remove_all(void) { - if (!_mdns_server) { - return ESP_ERR_INVALID_ARG; - } MDNS_SERVICE_LOCK(); + esp_err_t ret = ESP_OK; + ESP_GOTO_ON_FALSE(_mdns_server, ESP_ERR_INVALID_ARG, done, TAG, "Invalid state"); if (!_mdns_server->services) { - MDNS_SERVICE_UNLOCK(); - return ESP_OK; + goto done; } - MDNS_SERVICE_UNLOCK(); - mdns_action_t *action = (mdns_action_t *)malloc(sizeof(mdns_action_t)); - if (!action) { - HOOK_MALLOC_FAILED; - return ESP_ERR_NO_MEM; + _mdns_send_final_bye(false); + mdns_srv_item_t *services = _mdns_server->services; + _mdns_server->services = NULL; + while (services) { + mdns_srv_item_t *s = services; + services = services->next; + _mdns_remove_scheduled_service_packets(s->service); + _mdns_free_service(s->service); + free(s); } - action->type = ACTION_SERVICES_CLEAR; - if (xQueueSend(_mdns_server->action_queue, &action, (TickType_t)0) != pdPASS) { - free(action); - return ESP_ERR_NO_MEM; - } - return ESP_OK; + +done: + MDNS_SERVICE_UNLOCK(); + return ret; } /* diff --git a/components/mdns/private_include/mdns_private.h b/components/mdns/private_include/mdns_private.h index 9b6554fd9..381bd4be4 100644 --- a/components/mdns/private_include/mdns_private.h +++ b/components/mdns/private_include/mdns_private.h @@ -187,7 +187,6 @@ typedef enum { ACTION_SYSTEM_EVENT, ACTION_HOSTNAME_SET, ACTION_INSTANCE_SET, - ACTION_SERVICES_CLEAR, ACTION_SEARCH_ADD, ACTION_SEARCH_SEND, ACTION_SEARCH_END, From 2c1b16617eeb45195a6670ebafeb3602a8b42cb0 Mon Sep 17 00:00:00 2001 From: David Cermak Date: Mon, 19 Aug 2024 14:26:13 +0200 Subject: [PATCH 9/9] fix(mdns): Fix mdns_delegate_hostname_add() to block until done Adds action semaphore the same way it's done in mdns_hostname_add() There could still potentially be a minor issue when calling these two APIs simultanously. Will solve the same ways as in 8a690503 (tracked as IDF-10913) --- components/mdns/mdns.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/components/mdns/mdns.c b/components/mdns/mdns.c index b8062c5ca..473750bfc 100644 --- a/components/mdns/mdns.c +++ b/components/mdns/mdns.c @@ -5211,6 +5211,7 @@ static void _mdns_execute_action(mdns_action_t *action) free((char *)action->data.delegate_hostname.hostname); free_address_list(action->data.delegate_hostname.address_list); } + xSemaphoreGive(_mdns_server->action_sema); break; case ACTION_DELEGATE_HOSTNAME_SET_ADDR: if (!_mdns_delegate_hostname_set_address(action->data.delegate_hostname.hostname, @@ -5774,6 +5775,7 @@ esp_err_t mdns_delegate_hostname_add(const char *hostname, const mdns_ip_addr_t free(action); return ESP_ERR_NO_MEM; } + xSemaphoreTake(_mdns_server->action_sema, portMAX_DELAY); return ESP_OK; } @@ -5893,7 +5895,7 @@ esp_err_t mdns_service_add_for_host(const char *instance, const char *service, c ESP_GOTO_ON_FALSE(_mdns_can_add_more_services(), ESP_ERR_NO_MEM, err, TAG, "Cannot add more services"); mdns_srv_item_t *item = _mdns_get_service_item_instance(instance, service, proto, hostname); - ESP_GOTO_ON_FALSE(!item, ESP_ERR_INVALID_ARG, err, TAG, "Already exists"); + ESP_GOTO_ON_FALSE(!item, ESP_ERR_INVALID_ARG, err, TAG, "Service already exists"); s = _mdns_create_service(service, proto, hostname, port, instance, num_items, txt); ESP_GOTO_ON_FALSE(s, ESP_ERR_NO_MEM, err, TAG, "Cannot create service: Out of memory");