VFS: Fix bug which occurs when driver is installed during a select() call

Closes https://github.com/espressif/esp-idf/issues/3554
This commit is contained in:
Roland Dobai 2019-10-14 09:22:55 +02:00
parent 0c7e7817cb
commit 6e25e6a991
3 changed files with 121 additions and 11 deletions

View file

@ -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 :cpp:func:`start_select` is called for setting up the environment for
detection of read/write/error conditions on file descriptors belonging to the 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 given VFS driver.
environment which was setup by :cpp:func:`start_select`. Please refer to the
: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 reference implementation for the UART peripheral in
:component_file:`vfs/vfs_uart.c` and most particularly to functions :component_file:`vfs/vfs_uart.c` and most particularly to functions
:cpp:func:`esp_vfs_dev_uart_register`, :cpp:func:`uart_start_select` and :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 enable the :envvar:`CONFIG_USE_ONLY_LWIP_SELECT` option which can reduce the code
size and improve performance. 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 Paths
----- -----

View file

@ -17,11 +17,11 @@
#include <sys/fcntl.h> #include <sys/fcntl.h>
#include <sys/param.h> #include <sys/param.h>
#include "unity.h" #include "unity.h"
#include "soc/uart_struct.h"
#include "freertos/FreeRTOS.h" #include "freertos/FreeRTOS.h"
#include "driver/uart.h" #include "driver/uart.h"
#include "esp_vfs.h" #include "esp_vfs.h"
#include "esp_vfs_dev.h" #include "esp_vfs_dev.h"
#include "esp_vfs_fat.h"
#include "lwip/sockets.h" #include "lwip/sockets.h"
#include "lwip/netdb.h" #include "lwip/netdb.h"
#include "test_utils.h" #include "test_utils.h"
@ -32,9 +32,19 @@ typedef struct {
xSemaphoreHandle sem; xSemaphoreHandle sem;
} test_task_param_t; } 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 const char message[] = "Hello world!";
static int open_dummy_socket() static int open_dummy_socket(void)
{ {
const struct addrinfo hints = { const struct addrinfo hints = {
.ai_family = AF_INET, .ai_family = AF_INET,
@ -52,7 +62,7 @@ static int open_dummy_socket()
return dummy_socket_fd; return dummy_socket_fd;
} }
static int socket_init() static int socket_init(void)
{ {
const struct addrinfo hints = { const struct addrinfo hints = {
.ai_family = AF_INET, .ai_family = AF_INET,
@ -84,7 +94,7 @@ static int socket_init()
return socket_fd; return socket_fd;
} }
static void uart1_init() static void uart1_init(void)
{ {
uart_config_t uart_config = { uart_config_t uart_config = {
.baud_rate = 115200, .baud_rate = 115200,
@ -341,3 +351,83 @@ TEST_CASE("concurent selects work", "[vfs]")
deinit(uart_fd, socket_fd); deinit(uart_fd, socket_fd);
close(dummy_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);
}

View file

@ -811,8 +811,12 @@ int esp_vfs_select(int nfds, fd_set *readfds, fd_set *writefds, fd_set *errorfds
return -1; 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; 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; __errno_r(r) = ENOMEM;
ESP_LOGD(TAG, "calloc is unsuccessful"); ESP_LOGD(TAG, "calloc is unsuccessful");
return -1; 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); const vfs_entry_t *vfs = get_vfs_for_index(i);
fds_triple_t *item = &vfs_fds_triple[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) { if (err != ESP_OK) {
call_end_selects(i, vfs_fds_triple); 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) { if (select_sem) {
vSemaphoreDelete(select_sem); vSemaphoreDelete(select_sem);
select_sem = NULL; 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); 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) { 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) { if (select_sem) {
vSemaphoreDelete(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 // which has a permanent FD. But in order to avoid to lock
// s_fd_table_lock we go through the VFS table. // s_fd_table_lock we go through the VFS table.
for (int i = 0; i < s_vfs_count; ++i) { 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]; const vfs_entry_t *vfs = s_vfs[i];
if (vfs != NULL && vfs->vfs.stop_socket_select != NULL) { if (vfs != NULL && vfs->vfs.stop_socket_select != NULL) {
vfs->vfs.stop_socket_select(); 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 // which has a permanent FD. But in order to avoid to lock
// s_fd_table_lock we go through the VFS table. // s_fd_table_lock we go through the VFS table.
for (int i = 0; i < s_vfs_count; ++i) { 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]; const vfs_entry_t *vfs = s_vfs[i];
if (vfs != NULL && vfs->vfs.stop_socket_select_isr != NULL) { if (vfs != NULL && vfs->vfs.stop_socket_select_isr != NULL) {
vfs->vfs.stop_socket_select_isr(woken); vfs->vfs.stop_socket_select_isr(woken);