From e5ee10e89f23c82b366d20ba330fe6cd9a86ad98 Mon Sep 17 00:00:00 2001 From: Roland Dobai Date: Mon, 14 Oct 2019 09:22:55 +0200 Subject: [PATCH] VFS: Fix bug which occurs when driver is installed during a select() call Closes https://github.com/espressif/esp-idf/issues/3554 --- .gitlab-ci.yml | 7 ++ components/vfs/README.rst | 16 ++++- components/vfs/test/test_vfs_select.c | 98 +++++++++++++++++++++++++-- components/vfs/vfs.c | 18 +++-- 4 files changed, 128 insertions(+), 11 deletions(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 15c9a8bf4..b7dd932d4 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -1193,6 +1193,13 @@ UT_031: - ESP32_IDF - UT_T1_1 +UT_032: + <<: *unit_test_template + tags: + - ESP32_IDF + - UT_T1_1 + - psram + IT_001: <<: *test_template parallel: 3 diff --git a/components/vfs/README.rst b/components/vfs/README.rst index e7e50fd67..bb398a171 100644 --- a/components/vfs/README.rst +++ b/components/vfs/README.rst @@ -82,8 +82,16 @@ then you need to register the VFS with :cpp:func:`start_select` and :cpp:func:`start_select` is called for setting up the environment for detection of read/write/error conditions on file descriptors belonging to the -given VFS. :cpp:func:`end_select` is called to stop/deinitialize/free the -environment which was setup by :cpp:func:`start_select`. Please refer to the +given VFS driver. + +:cpp:func:`end_select` is called to stop/deinitialize/free the +environment which was setup by :cpp:func:`start_select`. + +.. note:: + :cpp:func:`end_select` might be called without a previous :cpp:func:`start_select` call in some rare + circumstances. :cpp:func:`end_select` should fail gracefully if this is the case. + +Please refer to the reference implementation for the UART peripheral in :component_file:`vfs/vfs_uart.c` and most particularly to functions :cpp:func:`esp_vfs_dev_uart_register`, :cpp:func:`uart_start_select` and @@ -97,6 +105,10 @@ If :cpp:func:`select` is used for socket file descriptors only then one can enable the :envvar:`CONFIG_USE_ONLY_LWIP_SELECT` option which can reduce the code size and improve performance. +.. note:: + Don't change the socket driver during an active :cpp:func:`select` call or you might experience some undefined + behavior. + Paths ----- diff --git a/components/vfs/test/test_vfs_select.c b/components/vfs/test/test_vfs_select.c index 341dec1c5..4c627fb74 100644 --- a/components/vfs/test/test_vfs_select.c +++ b/components/vfs/test/test_vfs_select.c @@ -17,11 +17,11 @@ #include #include #include "unity.h" -#include "soc/uart_struct.h" #include "freertos/FreeRTOS.h" #include "driver/uart.h" #include "esp_vfs.h" #include "esp_vfs_dev.h" +#include "esp_vfs_fat.h" #include "lwip/sockets.h" #include "lwip/netdb.h" #include "test_utils.h" @@ -32,9 +32,19 @@ typedef struct { xSemaphoreHandle sem; } test_task_param_t; +typedef struct { + fd_set *rdfds; + fd_set *wrfds; + fd_set *errfds; + int maxfds; + struct timeval *tv; + int select_ret; + xSemaphoreHandle sem; +} test_select_task_param_t; + static const char message[] = "Hello world!"; -static int open_dummy_socket() +static int open_dummy_socket(void) { const struct addrinfo hints = { .ai_family = AF_INET, @@ -52,7 +62,7 @@ static int open_dummy_socket() return dummy_socket_fd; } -static int socket_init() +static int socket_init(void) { const struct addrinfo hints = { .ai_family = AF_INET, @@ -84,7 +94,7 @@ static int socket_init() return socket_fd; } -static void uart1_init() +static void uart1_init(void) { uart_config_t uart_config = { .baud_rate = 115200, @@ -492,3 +502,83 @@ TEST_CASE("concurent selects work", "[vfs]") deinit(uart_fd, socket_fd); close(dummy_socket_fd); } + +static void select_task2(void *task_param) +{ + const test_select_task_param_t *param = task_param; + + int s = select(param->maxfds, param->rdfds, param->wrfds, param->errfds, param->tv); + TEST_ASSERT_EQUAL(param->select_ret, s); + + if (param->sem) { + xSemaphoreGive(param->sem); + } + vTaskDelete(NULL); +} + +static void inline start_select_task2(test_select_task_param_t *param) +{ + xTaskCreate(select_task2, "select_task2", 4*1024, (void *) param, 5, NULL); +} + +TEST_CASE("select() works with concurrent mount", "[vfs][fatfs]") +{ + wl_handle_t test_wl_handle; + int uart_fd, socket_fd; + + init(&uart_fd, &socket_fd); + const int dummy_socket_fd = open_dummy_socket(); + + esp_vfs_fat_sdmmc_mount_config_t mount_config = { + .format_if_mount_failed = true, + .max_files = 2 + }; + + // select() will be waiting for a socket & UART and FATFS mount will occur in parallel + + struct timeval tv = { + .tv_sec = 1, + .tv_usec = 0, + }; + + fd_set rdfds; + FD_ZERO(&rdfds); + FD_SET(uart_fd, &rdfds); + FD_SET(dummy_socket_fd, &rdfds); + + test_select_task_param_t param = { + .rdfds = &rdfds, + .wrfds = NULL, + .errfds = NULL, + .maxfds = MAX(uart_fd, dummy_socket_fd) + 1, + .tv = &tv, + .select_ret = 0, // expected timeout + .sem = xSemaphoreCreateBinary(), + }; + TEST_ASSERT_NOT_NULL(param.sem); + + start_select_task2(¶m); + vTaskDelay(10 / portTICK_PERIOD_MS); //make sure the task has started and waits in select() + + TEST_ESP_OK(esp_vfs_fat_spiflash_mount("/spiflash", NULL, &mount_config, &test_wl_handle)); + + TEST_ASSERT_EQUAL(pdTRUE, xSemaphoreTake(param.sem, 1500 / portTICK_PERIOD_MS)); + + // select() will be waiting for a socket & UART and FATFS unmount will occur in parallel + + FD_ZERO(&rdfds); + FD_SET(uart_fd, &rdfds); + FD_SET(dummy_socket_fd, &rdfds); + + start_select_task2(¶m); + vTaskDelay(10 / portTICK_PERIOD_MS); //make sure the task has started and waits in select() + + TEST_ESP_OK(esp_vfs_fat_spiflash_unmount("/spiflash", test_wl_handle)); + + TEST_ASSERT_EQUAL(pdTRUE, xSemaphoreTake(param.sem, 1500 / portTICK_PERIOD_MS)); + + vSemaphoreDelete(param.sem); + + deinit(uart_fd, socket_fd); + close(dummy_socket_fd); +} diff --git a/components/vfs/vfs.c b/components/vfs/vfs.c index 19d9dadee..3851ee11b 100644 --- a/components/vfs/vfs.c +++ b/components/vfs/vfs.c @@ -811,8 +811,12 @@ int esp_vfs_select(int nfds, fd_set *readfds, fd_set *writefds, fd_set *errorfds return -1; } + // Capture s_vfs_count to a local variable in case a new driver is registered or removed during this actual select() + // call. s_vfs_count cannot be protected with a mutex during a select() call (which can be one without a timeout) + // because that could block the registration of new driver. + const size_t vfs_count = s_vfs_count; fds_triple_t *vfs_fds_triple; - if ((vfs_fds_triple = calloc(s_vfs_count, sizeof(fds_triple_t))) == NULL) { + if ((vfs_fds_triple = calloc(vfs_count, sizeof(fds_triple_t))) == NULL) { __errno_r(r) = ENOMEM; ESP_LOGD(TAG, "calloc is unsuccessful"); return -1; @@ -895,7 +899,7 @@ int esp_vfs_select(int nfds, fd_set *readfds, fd_set *writefds, fd_set *errorfds } } - for (int i = 0; i < s_vfs_count; ++i) { + for (int i = 0; i < vfs_count; ++i) { const vfs_entry_t *vfs = get_vfs_for_index(i); fds_triple_t *item = &vfs_fds_triple[i]; @@ -910,7 +914,7 @@ int esp_vfs_select(int nfds, fd_set *readfds, fd_set *writefds, fd_set *errorfds if (err != ESP_OK) { call_end_selects(i, vfs_fds_triple); - (void) set_global_fd_sets(vfs_fds_triple, s_vfs_count, readfds, writefds, errorfds); + (void) set_global_fd_sets(vfs_fds_triple, vfs_count, readfds, writefds, errorfds); if (select_sem) { vSemaphoreDelete(select_sem); select_sem = NULL; @@ -954,9 +958,9 @@ int esp_vfs_select(int nfds, fd_set *readfds, fd_set *writefds, fd_set *errorfds xSemaphoreTake(select_sem, ticks_to_wait); } - call_end_selects(s_vfs_count, vfs_fds_triple); // for VFSs for start_select was called before + call_end_selects(vfs_count, vfs_fds_triple); // for VFSs for start_select was called before if (ret >= 0) { - ret += set_global_fd_sets(vfs_fds_triple, s_vfs_count, readfds, writefds, errorfds); + ret += set_global_fd_sets(vfs_fds_triple, vfs_count, readfds, writefds, errorfds); } if (select_sem) { vSemaphoreDelete(select_sem); @@ -980,6 +984,8 @@ void esp_vfs_select_triggered(SemaphoreHandle_t *signal_sem) // which has a permanent FD. But in order to avoid to lock // s_fd_table_lock we go through the VFS table. for (int i = 0; i < s_vfs_count; ++i) { + // Note: s_vfs_count could have changed since the start of vfs_select() call. However, that change doesn't + // matter here stop_socket_select() will be called for only valid VFS drivers. const vfs_entry_t *vfs = s_vfs[i]; if (vfs != NULL && vfs->vfs.stop_socket_select != NULL) { vfs->vfs.stop_socket_select(); @@ -998,6 +1004,8 @@ void esp_vfs_select_triggered_isr(SemaphoreHandle_t *signal_sem, BaseType_t *wok // which has a permanent FD. But in order to avoid to lock // s_fd_table_lock we go through the VFS table. for (int i = 0; i < s_vfs_count; ++i) { + // Note: s_vfs_count could have changed since the start of vfs_select() call. However, that change doesn't + // matter here stop_socket_select() will be called for only valid VFS drivers. const vfs_entry_t *vfs = s_vfs[i]; if (vfs != NULL && vfs->vfs.stop_socket_select_isr != NULL) { vfs->vfs.stop_socket_select_isr(woken);