From 8a690503ed38e6791714f5453b2ca1d7f8941612 Mon Sep 17 00:00:00 2001 From: David Cermak Date: Mon, 22 Jul 2024 19:16:42 +0200 Subject: [PATCH] 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