From 2ccfe3da96cba837c2cf0a9695d1595d377b5b7d Mon Sep 17 00:00:00 2001 From: Ivan Grokhotkov Date: Wed, 21 Jun 2017 01:21:14 +0800 Subject: [PATCH 1/3] vfs: fix opendir of a filesystem root directory Previously opendir("/data") would fail if filesystem with "data" prefix was registered in VFS, while opendir("/data/") would succeed. This change fixes handling for the former case and adds relevant tests. --- components/fatfs/test/test_fatfs_common.c | 25 +++ components/fatfs/test/test_fatfs_common.h | 2 + components/fatfs/test/test_fatfs_sdmmc.c | 7 + components/fatfs/test/test_fatfs_spiflash.c | 7 + components/vfs/test/test_vfs_paths.c | 178 ++++++++++++++++++++ components/vfs/vfs.c | 18 +- 6 files changed, 231 insertions(+), 6 deletions(-) create mode 100644 components/vfs/test/test_vfs_paths.c diff --git a/components/fatfs/test/test_fatfs_common.c b/components/fatfs/test/test_fatfs_common.c index 47dca17e93..a2f35c3db7 100644 --- a/components/fatfs/test/test_fatfs_common.c +++ b/components/fatfs/test/test_fatfs_common.c @@ -227,6 +227,31 @@ void test_fatfs_mkdir_rmdir(const char* filename_prefix) TEST_ASSERT_EQUAL(0, rmdir(name_dir2)); } +void test_fatfs_can_opendir(const char* path) +{ + char name_dir_file[64]; + const char * file_name = "test_opd.txt"; + snprintf(name_dir_file, sizeof(name_dir_file), "%s/%s", path, file_name); + unlink(name_dir_file); + test_fatfs_create_file_with_text(name_dir_file, "test_opendir\n"); + DIR* dir = opendir(path); + TEST_ASSERT_NOT_NULL(dir); + bool found = false; + while (true) { + struct dirent* de = readdir(dir); + if (!de) { + break; + } + if (strcasecmp(de->d_name, file_name) == 0) { + found = true; + break; + } + } + TEST_ASSERT_TRUE(found); + TEST_ASSERT_EQUAL(0, closedir(dir)); + unlink(name_dir_file); +} + void test_fatfs_opendir_readdir_rewinddir(const char* dir_prefix) { char name_dir_inner_file[64]; diff --git a/components/fatfs/test/test_fatfs_common.h b/components/fatfs/test/test_fatfs_common.h index 84258c1dab..dcc31e37d4 100644 --- a/components/fatfs/test/test_fatfs_common.h +++ b/components/fatfs/test/test_fatfs_common.h @@ -53,6 +53,8 @@ void test_fatfs_concurrent(const char* filename_prefix); void test_fatfs_mkdir_rmdir(const char* filename_prefix); +void test_fatfs_can_opendir(const char* path); + void test_fatfs_opendir_readdir_rewinddir(const char* dir_prefix); void test_fatfs_rw_speed(const char* filename, void* buf, size_t buf_size, size_t file_size, bool write); diff --git a/components/fatfs/test/test_fatfs_sdmmc.c b/components/fatfs/test/test_fatfs_sdmmc.c index bc77d94c81..0c6d41aff5 100644 --- a/components/fatfs/test/test_fatfs_sdmmc.c +++ b/components/fatfs/test/test_fatfs_sdmmc.c @@ -130,6 +130,13 @@ TEST_CASE("(SD) can create and remove directories", "[fatfs][ignore]") test_teardown(); } +TEST_CASE("(WL) can opendir root directory of FS", "[fatfs][ignore]") +{ + test_setup(); + test_fatfs_can_opendir("/sdcard"); + test_teardown(); +} + TEST_CASE("(SD) opendir, readdir, rewinddir, seekdir work as expected", "[fatfs][ignore]") { test_setup(); diff --git a/components/fatfs/test/test_fatfs_spiflash.c b/components/fatfs/test/test_fatfs_spiflash.c index 971838db96..bcc396b93c 100644 --- a/components/fatfs/test/test_fatfs_spiflash.c +++ b/components/fatfs/test/test_fatfs_spiflash.c @@ -125,6 +125,13 @@ TEST_CASE("(WL) can create and remove directories", "[fatfs][wear_levelling]") test_teardown(); } +TEST_CASE("(WL) can opendir root directory of FS", "[fatfs][wear_levelling]") +{ + test_setup(); + test_fatfs_can_opendir("/spiflash"); + test_teardown(); +} + TEST_CASE("(WL) opendir, readdir, rewinddir, seekdir work as expected", "[fatfs][wear_levelling]") { test_setup(); diff --git a/components/vfs/test/test_vfs_paths.c b/components/vfs/test/test_vfs_paths.c new file mode 100644 index 0000000000..033a376db3 --- /dev/null +++ b/components/vfs/test/test_vfs_paths.c @@ -0,0 +1,178 @@ +// Copyright 2015-2017 Espressif Systems (Shanghai) PTE LTD +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include +#include +#include +#include +#include +#include +#include +#include "esp_vfs.h" +#include "unity.h" +#include "esp_log.h" + +static const char* TAG = "test_vfs_paths"; + +/* Dummy VFS implementation to check if VFS is called or not with expected path + */ +typedef struct { + const char* match_path; + bool called; +} dummy_vfs_t; + +static int dummy_open(void* ctx, const char * path, int flags, int mode) +{ + dummy_vfs_t* dummy = (dummy_vfs_t*) ctx; + dummy->called = true; + if (strcmp(dummy->match_path, path) == 0) { + return 1; + } + errno = ENOENT; + return -1; +} + +static int dummy_close(void* ctx, int fd) +{ + dummy_vfs_t* dummy = (dummy_vfs_t*) ctx; + dummy->called = true; + if (fd == 1) { + return 0; + } + errno = EBADF; + return -1; +} + +static DIR* dummy_opendir(void* ctx, const char* path) +{ + dummy_vfs_t* dummy = (dummy_vfs_t*) ctx; + dummy->called = true; + if (strcmp(dummy->match_path, path) == 0) { + DIR* result = calloc(1, sizeof(DIR)); + TEST_ASSERT_NOT_NULL(result); + return result; + } + errno = ENOENT; + return NULL; +} + +static int dummy_closedir(void* ctx, DIR* pdir) +{ + dummy_vfs_t* dummy = (dummy_vfs_t*) ctx; + dummy->called = true; + free(pdir); + return 0; +} + +/* Initializer for this dummy VFS implementation + */ + +#define DUMMY_VFS() { \ + .flags = ESP_VFS_FLAG_CONTEXT_PTR, \ + .open_p = dummy_open, \ + .close_p = dummy_close, \ + .opendir_p = dummy_opendir, \ + .closedir_p = dummy_closedir \ + } + +/* Helper functions to test VFS behavior + */ + +static void test_open(dummy_vfs_t* instance, const char* path, + bool should_be_called, bool should_be_opened, int line) +{ + const int flags = O_CREAT | O_TRUNC | O_RDWR; + instance->called = false; + int fd = esp_vfs_open(__getreent(), path, flags, 0); + UNITY_TEST_ASSERT_EQUAL_INT(should_be_called, instance->called, line, + "should_be_called check failed"); + if (should_be_called) { + if (should_be_opened) { + UNITY_TEST_ASSERT(fd >= 0, line, "should be opened"); + } else { + UNITY_TEST_ASSERT(fd < 0, line, "should not be opened"); + } + } + esp_vfs_close(__getreent(), fd); +} + +static void test_opendir(dummy_vfs_t* instance, const char* path, + bool should_be_called, bool should_be_opened, int line) +{ + instance->called = false; + DIR* dir = opendir(path); + UNITY_TEST_ASSERT_EQUAL_INT(should_be_called, instance->called, line, + "should_be_called check failed"); + if (should_be_called) { + if (should_be_opened) { + UNITY_TEST_ASSERT(dir != NULL, line, "should be opened"); + } else { + UNITY_TEST_ASSERT(dir == 0, line, "should not be opened"); + } + } + if (dir) { + closedir(dir); + } +} + +/* Helper macros which forward line number to assertion macros inside test_open + * and test_opendir + */ + +#define test_opened(instance, path) test_open(instance, path, true, true, __LINE__) +#define test_not_opened(instance, path) test_open(instance, path, true, false, __LINE__) +#define test_not_called(instance, path) test_open(instance, path, false, false, __LINE__) + +#define test_dir_opened(instance, path) test_opendir(instance, path, true, true, __LINE__) +#define test_dir_not_opened(instance, path) test_opendir(instance, path, true, false, __LINE__) +#define test_dir_not_called(instance, path) test_opendir(instance, path, false, false, __LINE__) + + + +TEST_CASE("vfs parses paths correctly", "[vfs]") +{ + dummy_vfs_t inst_foo = { + .match_path = "", + .called = false + }; + esp_vfs_t desc_foo = DUMMY_VFS(); + TEST_ESP_OK( esp_vfs_register("/foo", &desc_foo, &inst_foo) ); + + dummy_vfs_t inst_foo1 = { + .match_path = "", + .called = false + }; + esp_vfs_t desc_foo1 = DUMMY_VFS(); + TEST_ESP_OK( esp_vfs_register("/foo1", &desc_foo1, &inst_foo1) ); + + inst_foo.match_path = "/file"; + test_opened(&inst_foo, "/foo/file"); + test_not_opened(&inst_foo, "/foo/file1"); + test_not_called(&inst_foo, "/foo1/file"); + test_not_called(&inst_foo, "/foo1"); + test_not_opened(&inst_foo, "/foo"); + inst_foo.match_path = "/junk"; + test_dir_opened(&inst_foo, "/foo/junk"); + inst_foo.match_path = "/"; + test_dir_opened(&inst_foo, "/foo/"); + test_dir_opened(&inst_foo, "/foo"); + test_dir_not_called(&inst_foo1, "/foo"); + test_dir_not_opened(&inst_foo, "/foo/1"); + test_dir_not_called(&inst_foo, "/foo1"); + + inst_foo1.match_path = "/file1"; + test_not_called(&inst_foo1, "/foo/file1"); + test_opened(&inst_foo1, "/foo1/file1"); + test_not_opened(&inst_foo1, "/foo1/file"); +} diff --git a/components/vfs/vfs.c b/components/vfs/vfs.c index fff3a93a1d..b656cb22b9 100644 --- a/components/vfs/vfs.c +++ b/components/vfs/vfs.c @@ -1,4 +1,4 @@ -// Copyright 2015-2016 Espressif Systems (Shanghai) PTE LTD +// Copyright 2015-2017 Espressif Systems (Shanghai) PTE LTD // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -117,6 +117,10 @@ static int translate_fd(const vfs_entry_t* vfs, int fd) static const char* translate_path(const vfs_entry_t* vfs, const char* src_path) { assert(strncmp(src_path, vfs->path_prefix, vfs->path_prefix_len) == 0); + if (strlen(src_path) == vfs->path_prefix_len) { + // special case when src_path matches the path prefix exactly + return "/"; + } return src_path + vfs->path_prefix_len; } @@ -128,13 +132,15 @@ static const vfs_entry_t* get_vfs_for_path(const char* path) if (!vfs) { continue; } - if (len < vfs->path_prefix_len + 1) { // +1 is for the trailing slash after base path + // match path prefix + if (len < vfs->path_prefix_len || + memcmp(path, vfs->path_prefix, vfs->path_prefix_len) != 0) { continue; } - if (memcmp(path, vfs->path_prefix, vfs->path_prefix_len) != 0) { // match prefix - continue; - } - if (path[vfs->path_prefix_len] != '/') { // don't match "/data" prefix for "/data1/foo.txt" + // if path is not equal to the prefix, expect to see a path separator + // i.e. don't match "/data" prefix for "/data1/foo.txt" path + if (len > vfs->path_prefix_len && + path[vfs->path_prefix_len] != '/') { continue; } return vfs; From 4f71b4574a9ca0833818b2a183f68e163d41a2f4 Mon Sep 17 00:00:00 2001 From: Ivan Grokhotkov Date: Wed, 21 Jun 2017 13:54:04 +0800 Subject: [PATCH 2/3] vfs: fix NULL pointer dereference in esp_vfs_unregister --- components/vfs/test/test_vfs_paths.c | 4 ++-- components/vfs/vfs.c | 3 +++ 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/components/vfs/test/test_vfs_paths.c b/components/vfs/test/test_vfs_paths.c index 033a376db3..c0a863e188 100644 --- a/components/vfs/test/test_vfs_paths.c +++ b/components/vfs/test/test_vfs_paths.c @@ -23,8 +23,6 @@ #include "unity.h" #include "esp_log.h" -static const char* TAG = "test_vfs_paths"; - /* Dummy VFS implementation to check if VFS is called or not with expected path */ typedef struct { @@ -175,4 +173,6 @@ TEST_CASE("vfs parses paths correctly", "[vfs]") test_not_called(&inst_foo1, "/foo/file1"); test_opened(&inst_foo1, "/foo1/file1"); test_not_opened(&inst_foo1, "/foo1/file"); + TEST_ESP_OK( esp_vfs_unregister("/foo") ); + TEST_ESP_OK( esp_vfs_unregister("/foo1") ); } diff --git a/components/vfs/vfs.c b/components/vfs/vfs.c index b656cb22b9..6501359bb1 100644 --- a/components/vfs/vfs.c +++ b/components/vfs/vfs.c @@ -91,6 +91,9 @@ esp_err_t esp_vfs_unregister(const char* base_path) { for (size_t i = 0; i < s_vfs_count; ++i) { vfs_entry_t* vfs = s_vfs[i]; + if (vfs == NULL) { + continue; + } if (memcmp(base_path, vfs->path_prefix, vfs->path_prefix_len) == 0) { free(vfs); s_vfs[i] = NULL; From 5b678eed0d6d79dd4ee24813b511d502a7dc2420 Mon Sep 17 00:00:00 2001 From: Ivan Grokhotkov Date: Wed, 21 Jun 2017 14:17:14 +0800 Subject: [PATCH 3/3] vfs: add support for nested mount points Fixes https://github.com/espressif/esp-idf/issues/135 --- components/vfs/README.rst | 19 +++++---- components/vfs/test/test_vfs_paths.c | 63 ++++++++++++++++++++++++++++ components/vfs/vfs.c | 19 +++++++-- 3 files changed, 90 insertions(+), 11 deletions(-) diff --git a/components/vfs/README.rst b/components/vfs/README.rst index 318004c250..d0a34b7de6 100644 --- a/components/vfs/README.rst +++ b/components/vfs/README.rst @@ -70,15 +70,20 @@ Paths Each registered FS has a path prefix associated with it. This prefix may be considered a "mount point" of this partition. -Registering mount points which have another mount point as a prefix is not supported and results in undefined behavior. For instance, the following is correct and supported: - -- FS 1 on /data/fs1 -- FS 2 on /data/fs2 - -This **will not work** as expected: +In case when mount points are nested, the mount point with the longest matching path prefix is used when opening the file. For instance, suppose that the following filesystems are registered in VFS: - FS 1 on /data -- FS 2 on /data/fs2 +- FS 2 on /data/static + +Then: + +- FS 1 will be used when opening a file called ``/data/log.txt`` +- FS 2 will be used when opening a file called ``/data/static/index.html`` +- Even if ``/index.html"`` doesn't exist in FS 2, FS 1 will *not* be searched for ``/static/index.html``. + +As a general rule, mount point names must start with the path separator (``/``) and must contain at least one character after path separator. However an empty mount point name is also supported, and may be used in cases when application needs to provide "fallback" filesystem, or override VFS functionality altogether. Such filesystem will be used if no prefix matches the path given. + +VFS does not handle dots (``.``) in path names in any special way. VFS does not treat ``..`` as a reference to the parent directory. I.e. in the above example, using a path ``/data/static/../log.txt`` will not result in a call to FS 1 to open ``/log.txt``. Specific FS drivers (such as FATFS) may handle dots in file names differently. When opening files, FS driver will only be given relative path to files. For example: diff --git a/components/vfs/test/test_vfs_paths.c b/components/vfs/test/test_vfs_paths.c index c0a863e188..fa00dc4e90 100644 --- a/components/vfs/test/test_vfs_paths.c +++ b/components/vfs/test/test_vfs_paths.c @@ -173,6 +173,69 @@ TEST_CASE("vfs parses paths correctly", "[vfs]") test_not_called(&inst_foo1, "/foo/file1"); test_opened(&inst_foo1, "/foo1/file1"); test_not_opened(&inst_foo1, "/foo1/file"); + + // Test nested VFS entries + dummy_vfs_t inst_foobar = { + .match_path = "", + .called = false + }; + esp_vfs_t desc_foobar = DUMMY_VFS(); + TEST_ESP_OK( esp_vfs_register("/foo/bar", &desc_foobar, &inst_foobar) ); + + dummy_vfs_t inst_toplevel = { + .match_path = "", + .called = false + }; + esp_vfs_t desc_toplevel = DUMMY_VFS(); + TEST_ESP_OK( esp_vfs_register("", &desc_toplevel, &inst_toplevel) ); + + inst_foo.match_path = "/bar/file"; + inst_foobar.match_path = "/file"; + test_not_called(&inst_foo, "/foo/bar/file"); + test_opened(&inst_foobar, "/foo/bar/file"); + test_dir_not_called(&inst_foo, "/foo/bar/file"); + test_dir_opened(&inst_foobar, "/foo/bar/file"); + inst_toplevel.match_path = "/tmp/foo"; + test_opened(&inst_toplevel, "/tmp/foo"); + TEST_ESP_OK( esp_vfs_unregister("/foo") ); TEST_ESP_OK( esp_vfs_unregister("/foo1") ); + TEST_ESP_OK( esp_vfs_unregister("/foo/bar") ); + TEST_ESP_OK( esp_vfs_unregister("") ); +} + + +void test_vfs_register(const char* prefix, bool expect_success, int line) +{ + dummy_vfs_t inst; + esp_vfs_t desc = DUMMY_VFS(); + esp_err_t err = esp_vfs_register(prefix, &desc, &inst); + if (expect_success) { + UNITY_TEST_ASSERT_EQUAL_INT(ESP_OK, err, line, "esp_vfs_register should succeed"); + } else { + UNITY_TEST_ASSERT_EQUAL_INT(ESP_ERR_INVALID_ARG, + err, line, "esp_vfs_register should fail"); + } + if (err == ESP_OK) { + TEST_ESP_OK( esp_vfs_unregister(prefix) ); + } +} + +#define test_register_ok(prefix) test_vfs_register(prefix, true, __LINE__) +#define test_register_fail(prefix) test_vfs_register(prefix, false, __LINE__) + +TEST_CASE("vfs checks mount point path", "[vfs]") +{ + test_register_ok(""); + test_register_fail("/"); + test_register_fail("a"); + test_register_fail("aa"); + test_register_fail("aaa"); + test_register_ok("/a"); + test_register_ok("/aa"); + test_register_ok("/aaa/bbb"); + test_register_fail("/aaa/"); + test_register_fail("/aaa/bbb/"); + test_register_ok("/23456789012345"); + test_register_fail("/234567890123456"); } diff --git a/components/vfs/vfs.c b/components/vfs/vfs.c index 6501359bb1..226023d5a3 100644 --- a/components/vfs/vfs.c +++ b/components/vfs/vfs.c @@ -55,10 +55,10 @@ static size_t s_vfs_count = 0; esp_err_t esp_vfs_register(const char* base_path, const esp_vfs_t* vfs, void* ctx) { size_t len = strlen(base_path); - if (len < 2 || len > ESP_VFS_PATH_MAX) { + if ((len != 0 && len < 2)|| len > ESP_VFS_PATH_MAX) { return ESP_ERR_INVALID_ARG; } - if (base_path[0] != '/' || base_path[len - 1] == '/') { + if ((len > 0 && base_path[0] != '/') || base_path[len - 1] == '/') { return ESP_ERR_INVALID_ARG; } vfs_entry_t *entry = (vfs_entry_t*) malloc(sizeof(vfs_entry_t)); @@ -129,6 +129,8 @@ static const char* translate_path(const vfs_entry_t* vfs, const char* src_path) static const vfs_entry_t* get_vfs_for_path(const char* path) { + const vfs_entry_t* best_match = NULL; + ssize_t best_match_prefix_len = -1; size_t len = strlen(path); for (size_t i = 0; i < s_vfs_count; ++i) { const vfs_entry_t* vfs = s_vfs[i]; @@ -146,9 +148,18 @@ static const vfs_entry_t* get_vfs_for_path(const char* path) path[vfs->path_prefix_len] != '/') { continue; } - return vfs; + // Out of all matching path prefixes, select the longest one; + // i.e. if "/dev" and "/dev/uart" both match, for "/dev/uart/1" path, + // choose "/dev/uart", + // This causes all s_vfs_count VFS entries to be scanned when opening + // a file by name. This can be optimized by introducing a table for + // FS search order, sorted so that longer prefixes are checked first. + if (best_match_prefix_len < (ssize_t) vfs->path_prefix_len) { + best_match_prefix_len = (ssize_t) vfs->path_prefix_len; + best_match = vfs; + } } - return NULL; + return best_match; } /*