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

@@ -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

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
 -----
 

components/vfs/test/test_vfs_select.c

@@ -17,11 +17,11 @@
 #include <sys/fcntl.h>
 #include <sys/param.h>
 #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(&param);
+    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(&param);
+    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);
+}

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);