From 758d17e1b8d16a56986c6166d42bc5d604735d86 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adam=20M=C3=BAdry?= Date: Mon, 22 Jun 2026 13:59:57 +0200 Subject: [PATCH] test(vfs): Test changes regarding VFS register incorrect check fix --- .gitlab/ci/host-test.yml | 7 + components/vfs/host_test/main/test_vfs.c | 200 +++++++++++++++++- .../vfs/test_apps/main/test_vfs_paths.c | 185 +++++++++++++++- 3 files changed, 390 insertions(+), 2 deletions(-) diff --git a/.gitlab/ci/host-test.yml b/.gitlab/ci/host-test.yml index 1d68382f942..bdac7f2a340 100644 --- a/.gitlab/ci/host-test.yml +++ b/.gitlab/ci/host-test.yml @@ -241,6 +241,13 @@ test_transport_on_host: - idf.py build - LSAN_OPTIONS=verbosity=1:log_threads=1 build/host_tcp_transport_test.elf +test_vfs_on_host: + extends: .host_test_template + script: + - cd ${IDF_PATH}/components/vfs/host_test + - idf.py build + - LSAN_OPTIONS=verbosity=1:log_threads=1 build/vfs_linux_test.elf + test_sockets_on_host: extends: .host_test_template script: diff --git a/components/vfs/host_test/main/test_vfs.c b/components/vfs/host_test/main/test_vfs.c index 99bd25cf1e6..92372d107fc 100644 --- a/components/vfs/host_test/main/test_vfs.c +++ b/components/vfs/host_test/main/test_vfs.c @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2025 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2025-2026 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -11,18 +11,27 @@ #include #include "esp_vfs.h" +#include "esp_vfs_ops.h" #include "test_vfs_linux_dev.h" #include "unity.h" #include "unity_fixture.h" TEST_GROUP(vfs_linux); +/* Forward declaration; defined together with the VFS registration regression + * tests below. Used by TEST_TEAR_DOWN to clean up leaked registrations. */ +static void unregister_all_tracked(void); + TEST_SETUP(vfs_linux) { } TEST_TEAR_DOWN(vfs_linux) { + /* Unregister any VFS entries that a test registered but did not clean up + * itself (e.g. because a TEST_ASSERT_* aborted the test early). This keeps + * the VFS table clean for subsequent tests. */ + unregister_all_tracked(); } static void test_create_file_with_text(const char* name, const char* text) @@ -242,6 +251,192 @@ TEST(vfs_linux, test_ftruncate_via_vfs) linux_vfs_dev_unregister(); } +/* Regression tests for the slot-accounting bug in esp_vfs_register_fs_common(). + * + * The registration code used to track an ever-increasing counter (s_vfs_count) + * which was incremented when a VFS was registered into a new top slot but was + * never decremented on unregister. The "is there room for another VFS?" check + * compared that counter against VFS_MAX_COUNT. Once the table had been filled + * once, unregistering and registering again could permanently fail with + * ESP_ERR_NO_MEM even though free slots were available. + * + * The registration code path (esp_vfs_register_fs -> esp_vfs_register_fs_common) + * is compiled and exercised on the Linux host target, so these tests reproduce + * the bug here. They are expected to fail on the pre-fix code and pass on the + * fixed code. + */ + +/* Maximum number of VFS slots in this build (CONFIG_VFS_MAX_COUNT, default 8). + * The upper limit of the Kconfig range is 20, so a loop bound of 20 is always + * enough to fill the table regardless of configuration. */ +#define TEST_VFS_REGISTER_LIMIT 20 + +/* A minimal VFS used only for registration in the tests below. We never open + * files through it, only register/unregister it to exercise the VFS slot + * bookkeeping, so the (context-pointer) open/close operations are never actually + * invoked. The modern ops API is used to avoid the deprecation warnings that the + * legacy esp_vfs_t fields carry. */ +static int dummy_open(void *ctx, const char *path, int flags, int mode) +{ + (void) ctx; (void) path; (void) flags; (void) mode; + errno = ENOENT; + return -1; +} + +static int dummy_close(void *ctx, int fd) +{ + (void) ctx; (void) fd; + return 0; +} + +static const esp_vfs_fs_ops_t s_dummy_vfs = { + .open_p = &dummy_open, + .close_p = &dummy_close, +}; + +/* Track every mount point this test file registers, so that TEST_TEAR_DOWN can + * unregister all of them even if a TEST_ASSERT_* aborts the test early (Unity + * Fixture uses longjmp on failure, which would otherwise skip in-test cleanup + * and leave the VFS table in a corrupted state for the next test). */ +static char s_tracked_paths[TEST_VFS_REGISTER_LIMIT][8]; +static int s_tracked_count; + +/* Register the dummy VFS at the given path and record it for cleanup. Returns + * the esp_vfs_register_fs() result; only successful registrations are tracked. */ +static esp_err_t register_tracked(const char *path) +{ + esp_err_t err = esp_vfs_register_fs(path, &s_dummy_vfs, ESP_VFS_FLAG_CONTEXT_PTR, NULL); + if (err == ESP_OK) { + snprintf(s_tracked_paths[s_tracked_count], sizeof(s_tracked_paths[0]), "%s", path); + s_tracked_count++; + } + return err; +} + +/* Unregister all still-registered tracked paths. Safe to call repeatedly. */ +static void unregister_all_tracked(void) +{ + for (int i = 0; i < s_tracked_count; ++i) { + esp_vfs_unregister(s_tracked_paths[i]); + } + s_tracked_count = 0; +} + +/* Build a short, unique mount point ("/t") for the test VFS entries. */ +static void make_test_path(char *buf, size_t buf_len, int idx) +{ + snprintf(buf, buf_len, "/t%d", idx); +} + +/* Fill every free VFS slot with the dummy VFS. Other VFSes may already be + * registered, so we keep going until registration reports the table is full. + * Returns the number of entries this function registered; paths[] is filled + * with the corresponding mount points. */ +static int fill_vfs_table(char paths[][8]) +{ + int registered = 0; + for (int i = 0; i < TEST_VFS_REGISTER_LIMIT; ++i) { + make_test_path(paths[i], 8, i); + esp_err_t err = register_tracked(paths[i]); + if (err == ESP_ERR_NO_MEM) { + break; // table is full + } + TEST_ASSERT_EQUAL(ESP_OK, err); + registered++; + } + return registered; +} + +TEST(vfs_linux, test_register_after_table_full) +{ + char paths[TEST_VFS_REGISTER_LIMIT][8]; + + int registered = fill_vfs_table(paths); + + /* We must have registered at least one entry, and the table must be full + * now (the next registration must fail with ESP_ERR_NO_MEM). */ + TEST_ASSERT_GREATER_THAN(0, registered); + TEST_ASSERT_EQUAL(ESP_ERR_NO_MEM, + esp_vfs_register_fs("/overflow", &s_dummy_vfs, ESP_VFS_FLAG_CONTEXT_PTR, NULL)); + + /* Free the topmost entry, then register again. On the buggy code the stale + * counter would still equal VFS_MAX_COUNT and this would fail with + * ESP_ERR_NO_MEM. */ + int top = registered - 1; + TEST_ASSERT_EQUAL(ESP_OK, esp_vfs_unregister(paths[top])); + /* paths[top] is still tracked from fill_vfs_table(), so re-register with the + * raw API instead of register_tracked() to avoid a duplicate tracker entry. */ + TEST_ASSERT_EQUAL(ESP_OK, esp_vfs_register_fs(paths[top], &s_dummy_vfs, ESP_VFS_FLAG_CONTEXT_PTR, NULL)); + + /* Cleanup of registered entries happens in TEST_TEAR_DOWN. */ +} + +TEST(vfs_linux, test_register_into_middle_hole) +{ + char paths[TEST_VFS_REGISTER_LIMIT][8]; + + int registered = fill_vfs_table(paths); + + /* Need at least 3 entries so there is a genuine middle entry that is neither + * the first nor the topmost slot. */ + TEST_ASSERT_GREATER_OR_EQUAL_INT(3, registered); + + /* The table must be full now. */ + TEST_ASSERT_EQUAL(ESP_ERR_NO_MEM, + esp_vfs_register_fs("/overflow", &s_dummy_vfs, ESP_VFS_FLAG_CONTEXT_PTR, NULL)); + + /* Unregister an entry in the middle (not the first, not the topmost). This + * leaves a NULL hole below the upper bound. The path stays tracked, so it is + * cleaned up in TEST_TEAR_DOWN regardless of what happens below. */ + int middle = registered / 2; + TEST_ASSERT_NOT_EQUAL(0, middle); + TEST_ASSERT_NOT_EQUAL(registered - 1, middle); + TEST_ASSERT_EQUAL(ESP_OK, esp_vfs_unregister(paths[middle])); + + /* Register again: with only the middle slot free, the registration must + * reuse that hole and succeed. On the buggy code it failed with + * ESP_ERR_NO_MEM. (The path is already tracked from fill_vfs_table().) */ + TEST_ASSERT_EQUAL(ESP_OK, esp_vfs_register_fs(paths[middle], &s_dummy_vfs, ESP_VFS_FLAG_CONTEXT_PTR, NULL)); + + /* The table must be full again - the hole was reused, not appended. */ + TEST_ASSERT_EQUAL(ESP_ERR_NO_MEM, + esp_vfs_register_fs("/overflow", &s_dummy_vfs, ESP_VFS_FLAG_CONTEXT_PTR, NULL)); + + /* Cleanup of registered entries happens in TEST_TEAR_DOWN. */ +} + +TEST(vfs_linux, test_register_unregister_cycles) +{ + /* Fill the whole VFS table and then drain it again, several times over. + * + * On the fixed code every round must be able to register exactly the same + * number of entries, because unregistering lowers the upper bound again. + * On the buggy code the stale counter never came back down, so the second + * and later rounds would be able to register fewer entries (and eventually + * none at all), which this test detects. */ + char paths[TEST_VFS_REGISTER_LIMIT][8]; + + const int rounds = 5; + int first_round_count = -1; + for (int r = 0; r < rounds; ++r) { + /* Register until the table is full. */ + int count = fill_vfs_table(paths); + + if (first_round_count < 0) { + first_round_count = count; + TEST_ASSERT_GREATER_THAN(0, first_round_count); + } else { + /* The capacity must not shrink between rounds. */ + TEST_ASSERT_EQUAL_INT(first_round_count, count); + } + + /* Drain the table again. The paths stay tracked until they are actually + * unregistered here; TEST_TEAR_DOWN is only the safety net for an early + * abort. */ + unregister_all_tracked(); + } +} + TEST_GROUP_RUNNER(vfs_linux) { RUN_TEST_CASE(vfs_linux, test_linux_vfs_open); @@ -251,6 +446,9 @@ TEST_GROUP_RUNNER(vfs_linux) RUN_TEST_CASE(vfs_linux, test_fstat_via_vfs); RUN_TEST_CASE(vfs_linux, test_fcntl_via_vfs); RUN_TEST_CASE(vfs_linux, test_ftruncate_via_vfs); + RUN_TEST_CASE(vfs_linux, test_register_after_table_full); + RUN_TEST_CASE(vfs_linux, test_register_into_middle_hole); + RUN_TEST_CASE(vfs_linux, test_register_unregister_cycles); } static void run_all_tests(void) diff --git a/components/vfs/test_apps/main/test_vfs_paths.c b/components/vfs/test_apps/main/test_vfs_paths.c index 65fc624fcdb..d9cdc0015d4 100644 --- a/components/vfs/test_apps/main/test_vfs_paths.c +++ b/components/vfs/test_apps/main/test_vfs_paths.c @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2015-2025 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2015-2026 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -270,3 +270,186 @@ TEST_CASE("vfs checks mount point path", "[vfs]") test_register_ok("/23456789012345"); test_register_fail("/234567890123456"); } + +/* Regression test for a slot-accounting bug in esp_vfs_register_fs_common(). + * + * The registration code used to keep an ever-increasing counter (s_vfs_count/s_vfs_upper_bound) + * which was incremented when a VFS was registered into a new top slot, but was + * never decremented on unregister. The "is there room for another VFS?" check + * compared that counter against VFS_MAX_COUNT. As a result, after the VFS table + * had been filled once, repeatedly unregistering and re-registering a VFS would + * eventually (and permanently) fail with ESP_ERR_NO_MEM even though free slots + * were available. + * + * The bug was fixed by checking for an actually-free slot (esp_get_free_index()) + * instead of relying on the counter, and by lowering the upper bound when the + * topmost entry is removed. + * + * These tests register/unregister the dummy VFS many times to ensure the slot + * accounting stays correct over time. They are expected to fail on the + * pre-fix code and pass on the fixed code. + */ + +/* Number of VFS slots available for this test app (see sdkconfig.defaults). */ +#define TEST_VFS_MAX_COUNT CONFIG_VFS_MAX_COUNT + +/* Build a short, unique mount point ("/t") for the test VFS entries. */ +static void make_test_path(char *buf, size_t buf_len, int idx) +{ + snprintf(buf, buf_len, "/t%d", idx); +} + +TEST_CASE("vfs can re-register after the table has been filled", "[vfs]") +{ + /* Separate context per registered VFS, because ESP_VFS_FLAG_CONTEXT_PTR + * stores the pointer for the lifetime of the registration. */ + static dummy_vfs_t insts[TEST_VFS_MAX_COUNT]; + char paths[TEST_VFS_MAX_COUNT][8]; + + /* Fill every free VFS slot. Some slots may already be taken by VFSes that + * the system registered at startup (e.g. /dev/null), so we keep going until + * registration reports the table is full instead of assuming a fixed count. */ + int registered = 0; + for (int i = 0; i < TEST_VFS_MAX_COUNT; ++i) { + make_test_path(paths[i], sizeof(paths[i]), i); + insts[i] = (dummy_vfs_t) { .match_path = "", .called = false }; + esp_err_t err = esp_vfs_register_fs(paths[i], &s_dummy_vfs, + ESP_VFS_FLAG_CONTEXT_PTR, &insts[i]); + if (err == ESP_ERR_NO_MEM) { + break; // table is full + } + TEST_ESP_OK(err); + registered++; + } + + /* We must have been able to register at least one entry, and the table must + * actually be full now (the next registration must fail with NO_MEM). */ + TEST_ASSERT_GREATER_THAN(0, registered); + dummy_vfs_t overflow_inst = { .match_path = "", .called = false }; + TEST_ESP_ERR(ESP_ERR_NO_MEM, + esp_vfs_register_fs("/overflow", &s_dummy_vfs, + ESP_VFS_FLAG_CONTEXT_PTR, &overflow_inst)); + + /* Free the topmost entry we registered, then try to register again. + * On the buggy code the stale counter would still equal VFS_MAX_COUNT and + * this registration would incorrectly fail with ESP_ERR_NO_MEM. */ + int top = registered - 1; + TEST_ESP_OK(esp_vfs_unregister(paths[top])); + + insts[top] = (dummy_vfs_t) { .match_path = "", .called = false }; + TEST_ESP_OK(esp_vfs_register_fs(paths[top], &s_dummy_vfs, + ESP_VFS_FLAG_CONTEXT_PTR, &insts[top])); + + /* Clean up everything we registered so the leak check in tearDown passes. */ + for (int i = 0; i < registered; ++i) { + TEST_ESP_OK(esp_vfs_unregister(paths[i])); + } +} + +TEST_CASE("vfs can re-register into a hole in the middle of a full table", "[vfs]") +{ + /* This targets the case where an entry that is NOT the topmost one is + * unregistered while the table is full. That leaves a NULL "hole" in the + * middle of the s_vfs table and, crucially, does NOT lower s_vfs_upper_bound + * (the hole is below the upper bound). Registering again must reuse that + * hole. + * + * On the buggy code the stale counter stayed at VFS_MAX_COUNT, so this + * re-registration failed with ESP_ERR_NO_MEM even though the freed middle + * slot was available. */ + static dummy_vfs_t insts[TEST_VFS_MAX_COUNT]; + char paths[TEST_VFS_MAX_COUNT][8]; + + /* Fill the table completely. */ + int registered = 0; + for (int i = 0; i < TEST_VFS_MAX_COUNT; ++i) { + make_test_path(paths[i], sizeof(paths[i]), i); + insts[i] = (dummy_vfs_t) { .match_path = "", .called = false }; + esp_err_t err = esp_vfs_register_fs(paths[i], &s_dummy_vfs, + ESP_VFS_FLAG_CONTEXT_PTR, &insts[i]); + if (err == ESP_ERR_NO_MEM) { + break; // table is full + } + TEST_ESP_OK(err); + registered++; + } + + /* We need at least 3 entries so that there is a genuine middle entry that is + * neither the first nor the topmost slot. */ + TEST_ASSERT_GREATER_OR_EQUAL_INT(3, registered); + + /* The table must be full now. */ + dummy_vfs_t overflow_inst = { .match_path = "", .called = false }; + TEST_ESP_ERR(ESP_ERR_NO_MEM, + esp_vfs_register_fs("/overflow", &s_dummy_vfs, + ESP_VFS_FLAG_CONTEXT_PTR, &overflow_inst)); + + /* Unregister an entry in the middle (not the first, not the topmost). This + * creates a NULL hole below the upper bound. */ + int middle = registered / 2; + TEST_ASSERT_NOT_EQUAL(0, middle); + TEST_ASSERT_NOT_EQUAL(registered - 1, middle); + TEST_ESP_OK(esp_vfs_unregister(paths[middle])); + + /* Register again: with only the middle slot free, esp_get_free_index() must + * return exactly that slot and the registration must succeed. */ + insts[middle] = (dummy_vfs_t) { .match_path = "", .called = false }; + TEST_ESP_OK(esp_vfs_register_fs(paths[middle], &s_dummy_vfs, + ESP_VFS_FLAG_CONTEXT_PTR, &insts[middle])); + + /* The table must be full again - the hole was reused, not appended. */ + TEST_ESP_ERR(ESP_ERR_NO_MEM, + esp_vfs_register_fs("/overflow", &s_dummy_vfs, + ESP_VFS_FLAG_CONTEXT_PTR, &overflow_inst)); + + /* Clean up everything. */ + for (int i = 0; i < registered; ++i) { + TEST_ESP_OK(esp_vfs_unregister(paths[i])); + } +} + +TEST_CASE("vfs survives repeated register/unregister cycles", "[vfs]") +{ + /* Fill the whole VFS table and then drain it again, several times over. + * + * On the fixed code every round must be able to register exactly the same + * number of entries, because unregistering lowers the upper bound again. + * On the buggy code the stale counter never came back down, so the second + * and later rounds would be able to register fewer entries (and eventually + * none at all), which this test detects. */ + static dummy_vfs_t insts[TEST_VFS_MAX_COUNT]; + char paths[TEST_VFS_MAX_COUNT][8]; + for (int i = 0; i < TEST_VFS_MAX_COUNT; ++i) { + make_test_path(paths[i], sizeof(paths[i]), i); + } + + const int rounds = 5; + int first_round_count = -1; + for (int r = 0; r < rounds; ++r) { + /* Register until the table is full. */ + int count = 0; + for (int i = 0; i < TEST_VFS_MAX_COUNT; ++i) { + insts[i] = (dummy_vfs_t) { .match_path = "", .called = false }; + esp_err_t err = esp_vfs_register_fs(paths[i], &s_dummy_vfs, + ESP_VFS_FLAG_CONTEXT_PTR, &insts[i]); + if (err == ESP_ERR_NO_MEM) { + break; + } + TEST_ESP_OK(err); + count++; + } + + if (first_round_count < 0) { + first_round_count = count; + TEST_ASSERT_GREATER_THAN(0, first_round_count); + } else { + /* The capacity must not shrink between rounds. */ + TEST_ASSERT_EQUAL_INT(first_round_count, count); + } + + /* Drain the table again. */ + for (int i = 0; i < count; ++i) { + TEST_ESP_OK(esp_vfs_unregister(paths[i])); + } + } +}