From 586baf29603d2a07d4d14ce74eebff21cdfa40cc Mon Sep 17 00:00:00 2001 From: Cloud_Yun Date: Sat, 17 Oct 2026 15:00:58 +0900 Subject: [PATCH] End a directory's unfinished enumerations when the directory is closed kal_fs_list_begin opens a descriptor of its own for each enumeration, and only kal_fs_list_next released it, on reaching the end. kal_fs_close_dir closed the directory's descriptor and nothing else, so an enumeration abandoned part-way kept its descriptor and buffer until the program exited, although fs.h says that closing the directory that produced an iterator releases it. Keep the listings not yet read to the end on a list, with the directory each was begun on. kal_fs_close_dir ends that directory's listings before the descriptor is closed, and kal_fs_list_next takes a listing off the list when it reaches the end or fails. The close now frees the listing, so an iterator used after its directory is closed uses freed memory instead of going on with the directory. The test begins and abandons 64 enumerations and compares the number of open descriptors before and after. It fails without the change. --- src/fs.cpp | 50 ++++++++++++++++++++++++++++++++++++---- tests/conformance_fs.cpp | 42 +++++++++++++++++++++++++++++++++ 2 files changed, 88 insertions(+), 4 deletions(-) diff --git a/src/fs.cpp b/src/fs.cpp index 01ebe71..7632482 100644 --- a/src/fs.cpp +++ b/src/fs.cpp @@ -254,11 +254,49 @@ kal_uintptr put_name(const char* src, kal_uintptr n, // calling. struct listing { int fd; + int dir; // the descriptor of the directory it was begun upon + listing* next; okl_uptr used; okl_uptr pos; char buf[8192]; }; +// THE ENUMERATIONS NOT YET READ TO THE END, BECAUSE CLOSING THE DIRECTORY MUST +// END THEM. fs.h releases an iterator "by reading past the end or by closing +// the directory that produced it", and the descriptor a listing holds is its +// own, so closing the directory's descriptor does not reach it. Measured before +// this list existed: a C library above that opened a directory larger than its +// buffer, read one entry and closed it, 64 times, was left holding 64 more +// descriptors. +// +// The lock spins rather than suspends, because suspending would make openkal.fs +// depend upon openkal.task, as src/memory.cpp says of its own. +listing* g_listings = nullptr; +volatile int g_listings_held = 0; + +void listings_lock() { + while (__atomic_exchange_n(&g_listings_held, 1, __ATOMIC_ACQUIRE)) okl::sys(okl::nr_sched_yield); +} +void listings_unlock() { __atomic_store_n(&g_listings_held, 0, __ATOMIC_RELEASE); } + +// Ends every listing begun upon `dir', or `one' alone where `dir' is negative. +// What is ended is unlinked under the lock and released after it. +void listings_end(int dir, listing* one) { + listing* done = nullptr; + listings_lock(); + for (listing** p = &g_listings; *p != nullptr;) { + listing* s = *p; + if (dir >= 0 ? s->dir == dir : s == one) { *p = s->next; s->next = done; done = s; } + else p = &s->next; + } + listings_unlock(); + while (done != nullptr) { + listing* s = done; done = s->next; + okl::sys(okl::nr_close, s->fd); + kal_free(s, sizeof(listing), alignof(listing)); + } +} + } // namespace extern "C" { @@ -312,7 +350,11 @@ int kal_fs_open(kal_dir base, const char* name, kal_uintptr len, void kal_fs_close_dir(kal_dir d) { const int fd = okl::unpack(d.h); - if (fd >= 0) { okl::retire(d.h); okl::sys(okl::nr_close, fd); } + if (fd < 0) return; + // Before the close, because after it the number may already name another + // directory whose enumerations are not this one's. + listings_end(fd, nullptr); + okl::retire(d.h); okl::sys(okl::nr_close, fd); } void kal_fs_close_file(kal_file f) { @@ -515,7 +557,8 @@ int kal_fs_list_begin(kal_dir d, kal_uintptr* iter) { if (okl::failed(own)) return okl::translate(own); auto* s = static_cast(kal_alloc(sizeof(listing), alignof(listing))); if (s == nullptr) { okl::sys(okl::nr_close, own); return kal_err_no_memory; } - s->fd = static_cast(own); s->used = 0; s->pos = 0; + s->fd = static_cast(own); s->dir = fd; s->used = 0; s->pos = 0; + listings_lock(); s->next = g_listings; g_listings = s; listings_unlock(); *iter = reinterpret_cast(s); return kal_ok; } @@ -532,8 +575,7 @@ int kal_fs_list_next(kal_dir, kal_uintptr* iter, static_cast(sizeof s->buf)); if (okl::interrupted(r)) continue; if (okl::failed(r) || r == 0) { - okl::sys(okl::nr_close, s->fd); - kal_free(s, sizeof(listing), alignof(listing)); + listings_end(-1, s); *iter = 0; if (name_len) *name_len = 0; return okl::failed(r) ? okl::translate(r) : kal_ok; diff --git a/tests/conformance_fs.cpp b/tests/conformance_fs.cpp index 0213f32..db9e587 100644 --- a/tests/conformance_fs.cpp +++ b/tests/conformance_fs.cpp @@ -95,6 +95,48 @@ int main() { check(kal_fs_open(d, "inner", 5, kal::fs::open::read.bits, &after) != kal_ok, "a released handle is not treated as valid"); + // An enumeration left part-way is ended by closing the directory that + // produced it, which fs.h states. Counted in the kernel's own table, through + // the whole file system this implementation supplies as a directory. + { + kal_dir slash{}; bool have_root = false; + for (kal_uintptr i = 0; i < kal::fs::preopen_count(); ++i) { + char nm[512]; kal_uintptr nl = 0; kal_dir p{}; + if (kal_fs_preopen(i, &p, nm, sizeof nm, &nl) == kal_ok && nl == 1 && nm[0] == '/') { + slash = p; have_root = true; + } + } + auto open_now = [&]() -> int { + kal_dir fds{}; kal_uintptr it = 0; int n = 0; + if (kal_fs_open_dir(slash, "proc/self/fd", 12, &fds) != kal_ok) return -1; + if (kal_fs_list_begin(fds, &it) == kal_ok) { + for (;;) { + char name[512]; kal_uintptr len = 0; int kind = 0; + if (kal_fs_list_next(fds, &it, name, sizeof name, &len, &kind) != kal_ok || it == 0) break; + ++n; + } + } + kal_fs_close_dir(fds); + return n; + }; + if (have_root) { + const int before = open_now(); + check(before >= 0, "the open descriptors are counted through proc/self/fd"); + if (before >= 0) { + for (int i = 0; i < 64; ++i) { + kal_dir again{}; kal_uintptr it = 0; + if (kal_fs_open_dir(root, "okl_dir", 7, &again) != kal_ok) break; + if (kal_fs_list_begin(again, &it) == kal_ok) { + char name[512]; kal_uintptr len = 0; int kind = 0; + kal_fs_list_next(again, &it, name, sizeof name, &len, &kind); + } + kal_fs_close_dir(again); + } + check(open_now() == before, "closing a directory ends an enumeration left part-way"); + } + } + } + check(kal_fs_remove(root, "okl_dir/inner", 13) == kal_ok, "the inner file is removed"); check(kal_fs_remove(root, "okl_dir", 7) == kal_ok, "the directory is removed"); check(kal_fs_remove(root, "okl_probe.txt", 13) == kal_ok, "the file is removed");