Skip to content

Commit 8a69050

Browse files
committed
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
1 parent 7e5ac87 commit 8a69050

File tree

5 files changed

+92
-180
lines changed

5 files changed

+92
-180
lines changed

components/mdns/mdns.c

Lines changed: 78 additions & 161 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
#include "mdns_networking.h"
1818
#include "esp_log.h"
1919
#include "esp_random.h"
20+
#include "esp_check.h"
2021

2122
static void _mdns_browse_item_free(mdns_browse_t *browse);
2223
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)
51075108
case ACTION_INSTANCE_SET:
51085109
free(action->data.instance);
51095110
break;
5110-
case ACTION_SERVICE_ADD:
5111-
_mdns_free_service(action->data.srv_add.service->service);
5112-
free(action->data.srv_add.service);
5113-
break;
51145111
case ACTION_SERVICE_INSTANCE_SET:
51155112
free(action->data.srv_instance.instance);
51165113
break;
@@ -5194,11 +5191,6 @@ static void _mdns_execute_action(mdns_action_t *action)
51945191
_mdns_server->instance = action->data.instance;
51955192
_mdns_restart_all_pcbs_no_instance();
51965193

5197-
break;
5198-
case ACTION_SERVICE_ADD:
5199-
action->data.srv_add.service->next = _mdns_server->services;
5200-
_mdns_server->services = action->data.srv_add.service;
5201-
_mdns_probe_all_pcbs(&action->data.srv_add.service, 1, false, false);
52025194
break;
52035195
case ACTION_SERVICE_INSTANCE_SET:
52045196
if (action->data.srv_instance.service->service->instance) {
@@ -5299,60 +5291,6 @@ static void _mdns_execute_action(mdns_action_t *action)
52995291
subtype_item->next = service->subtype;
53005292
service->subtype = subtype_item;
53015293
break;
5302-
case ACTION_SERVICE_DEL:
5303-
a = _mdns_server->services;
5304-
mdns_srv_item_t *b = a;
5305-
if (action->data.srv_del.instance) {
5306-
while (a) {
5307-
if (_mdns_service_match_instance(a->service, action->data.srv_del.instance,
5308-
action->data.srv_del.service, action->data.srv_del.proto,
5309-
action->data.srv_del.hostname)) {
5310-
if (_mdns_server->services != a) {
5311-
b->next = a->next;
5312-
} else {
5313-
_mdns_server->services = a->next;
5314-
}
5315-
_mdns_send_bye(&a, 1, false);
5316-
_mdns_remove_scheduled_service_packets(a->service);
5317-
_mdns_free_service(a->service);
5318-
free(a);
5319-
break;
5320-
}
5321-
b = a;
5322-
a = a->next;
5323-
}
5324-
} else {
5325-
while (a) {
5326-
if (_mdns_service_match(a->service, action->data.srv_del.service, action->data.srv_del.proto,
5327-
action->data.srv_del.hostname)) {
5328-
if (_mdns_server->services != a) {
5329-
b->next = a->next;
5330-
_mdns_send_bye(&a, 1, false);
5331-
_mdns_remove_scheduled_service_packets(a->service);
5332-
_mdns_free_service(a->service);
5333-
free(a);
5334-
a = b->next;
5335-
continue;
5336-
} else {
5337-
_mdns_server->services = a->next;
5338-
_mdns_send_bye(&a, 1, false);
5339-
_mdns_remove_scheduled_service_packets(a->service);
5340-
_mdns_free_service(a->service);
5341-
free(a);
5342-
a = _mdns_server->services;
5343-
b = a;
5344-
continue;
5345-
}
5346-
}
5347-
b = a;
5348-
a = a->next;
5349-
}
5350-
}
5351-
free((char *)action->data.srv_del.instance);
5352-
free((char *)action->data.srv_del.service);
5353-
free((char *)action->data.srv_del.proto);
5354-
free((char *)action->data.srv_del.hostname);
5355-
break;
53565294
case ACTION_SERVICES_CLEAR:
53575295
_mdns_send_final_bye(false);
53585296
a = _mdns_server->services;
@@ -6076,77 +6014,45 @@ esp_err_t mdns_instance_name_set(const char *instance)
60766014
* MDNS SERVICES
60776015
* */
60786016

6079-
esp_err_t mdns_service_add_for_host(const char *instance, const char *service, const char *proto, const char *hostname,
6017+
esp_err_t mdns_service_add_for_host(const char *instance, const char *service, const char *proto, const char *host,
60806018
uint16_t port, mdns_txt_item_t txt[], size_t num_items)
60816019
{
60826020
if (!_mdns_server || _str_null_or_empty(service) || _str_null_or_empty(proto) || !port || !_mdns_server->hostname) {
60836021
return ESP_ERR_INVALID_ARG;
60846022
}
60856023

60866024
MDNS_SERVICE_LOCK();
6087-
if (!_mdns_can_add_more_services()) {
6088-
MDNS_SERVICE_UNLOCK();
6089-
return ESP_ERR_NO_MEM;
6090-
}
6025+
esp_err_t ret = ESP_OK;
6026+
const char *hostname = host ? host : _mdns_server->hostname;
6027+
mdns_service_t *s = NULL;
60916028

6092-
if (!hostname) {
6093-
hostname = _mdns_server->hostname;
6094-
}
6029+
ESP_GOTO_ON_FALSE(_mdns_can_add_more_services(), ESP_ERR_NO_MEM, err, TAG, "Cannot add more services");
60956030

60966031
mdns_srv_item_t *item = _mdns_get_service_item_instance(instance, service, proto, hostname);
6097-
MDNS_SERVICE_UNLOCK();
6098-
if (item) {
6099-
return ESP_ERR_INVALID_ARG;
6100-
}
6032+
ESP_GOTO_ON_FALSE(!item, ESP_ERR_INVALID_ARG, err, TAG, "Already exists");
61016033

6102-
mdns_service_t *s = _mdns_create_service(service, proto, hostname, port, instance, num_items, txt);
6103-
if (!s) {
6104-
return ESP_ERR_NO_MEM;
6105-
}
6034+
s = _mdns_create_service(service, proto, hostname, port, instance, num_items, txt);
6035+
ESP_GOTO_ON_FALSE(s, ESP_ERR_NO_MEM, err, TAG, "Cannot create service: Out of memory");
61066036

61076037
item = (mdns_srv_item_t *)malloc(sizeof(mdns_srv_item_t));
6108-
if (!item) {
6109-
HOOK_MALLOC_FAILED;
6110-
_mdns_free_service(s);
6111-
return ESP_ERR_NO_MEM;
6112-
}
6038+
ESP_GOTO_ON_FALSE(item, ESP_ERR_NO_MEM, err, TAG, "Cannot create service: Out of memory");
61136039

61146040
item->service = s;
61156041
item->next = NULL;
61166042

6117-
mdns_action_t *action = (mdns_action_t *)malloc(sizeof(mdns_action_t));
6118-
if (!action) {
6119-
HOOK_MALLOC_FAILED;
6120-
_mdns_free_service(s);
6121-
free(item);
6122-
return ESP_ERR_NO_MEM;
6123-
}
6124-
action->type = ACTION_SERVICE_ADD;
6125-
action->data.srv_add.service = item;
6126-
if (xQueueSend(_mdns_server->action_queue, &action, (TickType_t)0) != pdPASS) {
6127-
_mdns_free_service(s);
6128-
free(item);
6129-
free(action);
6130-
return ESP_ERR_NO_MEM;
6131-
}
6043+
item->next = _mdns_server->services;
6044+
_mdns_server->services = item;
6045+
_mdns_probe_all_pcbs(&item, 1, false, false);
6046+
MDNS_SERVICE_UNLOCK();
6047+
return ESP_OK;
61326048

6133-
size_t start = xTaskGetTickCount();
6134-
size_t timeout_ticks = pdMS_TO_TICKS(MDNS_SERVICE_ADD_TIMEOUT_MS);
6135-
MDNS_SERVICE_LOCK();
6136-
mdns_srv_item_t *target = _mdns_get_service_item_instance(instance, service, proto, hostname);
6049+
err:
61376050
MDNS_SERVICE_UNLOCK();
6138-
while (target == NULL) {
6139-
uint32_t expired = xTaskGetTickCount() - start;
6140-
if (expired >= timeout_ticks) {
6141-
return ESP_FAIL; // Timeout
6142-
}
6143-
vTaskDelay(MIN(10 / portTICK_PERIOD_MS, timeout_ticks - expired));
6144-
MDNS_SERVICE_LOCK();
6145-
target = _mdns_get_service_item_instance(instance, service, proto, hostname);
6146-
MDNS_SERVICE_UNLOCK();
6051+
_mdns_free_service(s);
6052+
if (ret == ESP_ERR_NO_MEM) {
6053+
HOOK_MALLOC_FAILED;
61476054
}
6148-
6149-
return ESP_OK;
6055+
return ret;
61506056
}
61516057

61526058
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
61556061
if (!_mdns_server) {
61566062
return ESP_ERR_INVALID_STATE;
61576063
}
6158-
return mdns_service_add_for_host(instance, service, proto, _mdns_server->hostname, port, txt, num_items);
6064+
return mdns_service_add_for_host(instance, service, proto, NULL, port, txt, num_items);
61596065
}
61606066

61616067
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
61846090
for (mdns_txt_linked_item_t *tmp = items; tmp != NULL; tmp = tmp->next) {
61856091
ret_index++;
61866092
}
6093+
if (ret_index == 0) {
6094+
*txt_count = 0;
6095+
*txt_value_len = NULL;
6096+
return NULL;
6097+
}
61876098
*txt_count = ret_index;
61886099
if (ret_index == 0) { // handle empty TXT
61896100
*txt_value_len = NULL;
@@ -6618,67 +6529,73 @@ esp_err_t mdns_service_instance_name_set(const char *service, const char *proto,
66186529
return mdns_service_instance_name_set_for_host(NULL, service, proto, _mdns_server->hostname, instance);
66196530
}
66206531

6621-
esp_err_t mdns_service_remove_for_host(const char *instance, const char *service, const char *proto, const char *hostname)
6532+
esp_err_t mdns_service_remove_for_host(const char *instance, const char *service, const char *proto, const char *host)
66226533
{
66236534
MDNS_SERVICE_LOCK();
6624-
if (!_mdns_server || !_mdns_server->services || _str_null_or_empty(service) || _str_null_or_empty(proto)) {
6625-
MDNS_SERVICE_UNLOCK();
6626-
return ESP_ERR_INVALID_ARG;
6627-
}
6535+
esp_err_t ret = ESP_OK;
6536+
const char *hostname = host ? host : _mdns_server->hostname;
6537+
ESP_GOTO_ON_FALSE(_mdns_server && _mdns_server->services && !_str_null_or_empty(service) && !_str_null_or_empty(proto),
6538+
ESP_ERR_INVALID_ARG, err, TAG, "Invalid state or arguments");
66286539
mdns_srv_item_t *s = _mdns_get_service_item_instance(instance, service, proto, hostname);
6629-
MDNS_SERVICE_UNLOCK();
6630-
if (!s) {
6631-
return ESP_ERR_NOT_FOUND;
6632-
}
6540+
ESP_GOTO_ON_FALSE(s, ESP_ERR_INVALID_ARG, err, TAG, "Service doesn't exist");
66336541

6634-
mdns_action_t *action = (mdns_action_t *)malloc(sizeof(mdns_action_t));
6635-
if (!action) {
6636-
HOOK_MALLOC_FAILED;
6637-
return ESP_ERR_NO_MEM;
6638-
}
6639-
action->type = ACTION_SERVICE_DEL;
6640-
action->data.srv_del.instance = NULL;
6641-
action->data.srv_del.hostname = NULL;
6642-
if (!_str_null_or_empty(instance)) {
6643-
action->data.srv_del.instance = strndup(instance, MDNS_NAME_BUF_LEN - 1);
6644-
if (!action->data.srv_del.instance) {
6645-
goto fail;
6542+
mdns_srv_item_t *a = _mdns_server->services;
6543+
mdns_srv_item_t *b = a;
6544+
if (instance) {
6545+
while (a) {
6546+
if (_mdns_service_match_instance(a->service, instance, service, proto, hostname)) {
6547+
if (_mdns_server->services != a) {
6548+
b->next = a->next;
6549+
} else {
6550+
_mdns_server->services = a->next;
6551+
}
6552+
_mdns_send_bye(&a, 1, false);
6553+
_mdns_remove_scheduled_service_packets(a->service);
6554+
_mdns_free_service(a->service);
6555+
free(a);
6556+
break;
6557+
}
6558+
b = a;
6559+
a = a->next;
66466560
}
6647-
}
6648-
6649-
if (!_str_null_or_empty(hostname)) {
6650-
action->data.srv_del.hostname = strndup(hostname, MDNS_NAME_BUF_LEN - 1);
6651-
if (!action->data.srv_del.hostname) {
6652-
goto fail;
6561+
} else {
6562+
while (a) {
6563+
if (_mdns_service_match(a->service, service, proto, hostname)) {
6564+
if (_mdns_server->services != a) {
6565+
b->next = a->next;
6566+
_mdns_send_bye(&a, 1, false);
6567+
_mdns_remove_scheduled_service_packets(a->service);
6568+
_mdns_free_service(a->service);
6569+
free(a);
6570+
a = b->next;
6571+
continue;
6572+
} else {
6573+
_mdns_server->services = a->next;
6574+
_mdns_send_bye(&a, 1, false);
6575+
_mdns_remove_scheduled_service_packets(a->service);
6576+
_mdns_free_service(a->service);
6577+
free(a);
6578+
a = _mdns_server->services;
6579+
b = a;
6580+
continue;
6581+
}
6582+
}
6583+
b = a;
6584+
a = a->next;
66536585
}
66546586
}
66556587

6656-
action->data.srv_del.service = strndup(service, MDNS_NAME_BUF_LEN - 1);
6657-
action->data.srv_del.proto = strndup(proto, MDNS_NAME_BUF_LEN - 1);
6658-
if (!action->data.srv_del.service || !action->data.srv_del.proto) {
6659-
goto fail;
6660-
}
6661-
6662-
if (xQueueSend(_mdns_server->action_queue, &action, (TickType_t)0) != pdPASS) {
6663-
goto fail;
6664-
}
6665-
return ESP_OK;
6666-
6667-
fail:
6668-
free((char *)action->data.srv_del.instance);
6669-
free((char *)action->data.srv_del.service);
6670-
free((char *)action->data.srv_del.proto);
6671-
free((char *)action->data.srv_del.hostname);
6672-
free(action);
6673-
return ESP_ERR_NO_MEM;
6588+
err:
6589+
MDNS_SERVICE_UNLOCK();
6590+
return ret;
66746591
}
66756592

66766593
esp_err_t mdns_service_remove(const char *service_type, const char *proto)
66776594
{
66786595
if (!_mdns_server) {
66796596
return ESP_ERR_INVALID_STATE;
66806597
}
6681-
return mdns_service_remove_for_host(NULL, service_type, proto, _mdns_server->hostname);
6598+
return mdns_service_remove_for_host(NULL, service_type, proto, NULL);
66826599
}
66836600

66846601
esp_err_t mdns_service_remove_all(void)

components/mdns/private_include/mdns_private.h

Lines changed: 1 addition & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* SPDX-FileCopyrightText: 2015-2023 Espressif Systems (Shanghai) CO LTD
2+
* SPDX-FileCopyrightText: 2015-2024 Espressif Systems (Shanghai) CO LTD
33
*
44
* SPDX-License-Identifier: Apache-2.0
55
*/
@@ -187,8 +187,6 @@ typedef enum {
187187
ACTION_SYSTEM_EVENT,
188188
ACTION_HOSTNAME_SET,
189189
ACTION_INSTANCE_SET,
190-
ACTION_SERVICE_ADD,
191-
ACTION_SERVICE_DEL,
192190
ACTION_SERVICE_INSTANCE_SET,
193191
ACTION_SERVICE_PORT_SET,
194192
ACTION_SERVICE_TXT_REPLACE,
@@ -446,15 +444,6 @@ typedef struct {
446444
mdns_if_t interface;
447445
mdns_event_actions_t event_action;
448446
} sys_event;
449-
struct {
450-
mdns_srv_item_t *service;
451-
} srv_add;
452-
struct {
453-
char *instance;
454-
char *service;
455-
char *proto;
456-
char *hostname;
457-
} srv_del;
458447
struct {
459448
mdns_srv_item_t *service;
460449
char *instance;

components/mdns/tests/test_afl_fuzz_host/esp32_mock.c

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* SPDX-FileCopyrightText: 2022 Espressif Systems (Shanghai) CO LTD
2+
* SPDX-FileCopyrightText: 2022-2024 Espressif Systems (Shanghai) CO LTD
33
*
44
* SPDX-License-Identifier: Unlicense OR CC0-1.0
55
*/
@@ -9,6 +9,7 @@
99
#include <stdlib.h>
1010
#include <unistd.h>
1111
#include "esp32_mock.h"
12+
#include "esp_log.h"
1213

1314
void *g_queue;
1415
int g_queue_send_shall_fail = 0;
@@ -111,3 +112,12 @@ BaseType_t xTaskNotifyWait(uint32_t bits_entry_clear, uint32_t bits_exit_clear,
111112
{
112113
return pdTRUE;
113114
}
115+
116+
void esp_log_write(esp_log_level_t level, const char *tag, const char *format, ...)
117+
{
118+
}
119+
120+
uint32_t esp_log_timestamp(void)
121+
{
122+
return 0;
123+
}

0 commit comments

Comments
 (0)