Skip to content

End a directory's unfinished enumerations when the directory is closed - #35

Open
yspbwx2010 wants to merge 1 commit into
mcpplibs:mainfrom
yspbwx2010:fix/close-dir-ends-enumeration
Open

yspbwx2010 wants to merge 1 commit into
mcpplibs:mainfrom
yspbwx2010:fix/close-dir-ends-enumeration

Conversation

@yspbwx2010

Copy link
Copy Markdown
Contributor

Fixes #34

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 its buffer until the program exited, although fs.h says that closing the directory that produced an iterator releases it.

This keeps the listings that have not reached their end on a list, each with the descriptor of the directory it was begun on. kal_fs_close_dir ends that directory's listings before it closes the descriptor, because after the close the same number may already name another directory. kal_fs_list_next takes a listing off the list when it reaches the end or fails.

The list needs a lock, because two threads may enumerate at the same time. It spins like the allocator's lock in memory.cpp, because a lock that suspends would make openkal.fs depend on openkal.task. It is held while the list is walked or changed, and the close and the free happen after it is released. Like the allocator's lock, it is copied into a context started with kal_space_start, so if another thread holds it at the start, that context can never take it. The context also gets its own copy of the list along with the descriptors, so closing a directory there does not touch the parent's enumerations.

Using an iterator after its directory is closed was already a caller error, but it used to keep working, because the listing owned its descriptor. Now the close frees the listing, so such a use, or a kal_fs_list_next on another thread racing the close, uses freed memory, and getdents64 may write into it. openkal-musl clears its iterator when it closes the directory, so it makes no such use.

Reading the rest of the directory when openkal-musl releases a directory descriptor would also work, over any implementation, but every early close would then read the whole directory, and programs that call openkal directly would still leak. The change is here, where fs.h puts the release.

I checked the following.

mcpp test with llvm@23.1.3 on x86_64 Linux: test result ok. 9 passed; 0 failed. The new check in tests/conformance_fs.cpp begins an enumeration of a directory 64 times, reads one entry and closes the directory, and compares the number of open descriptors before and after, counted through /proc/self/fd under the / grant; without that grant it is skipped. Without the change it fails with "closing a directory ends an enumeration left part-way" and the other eight suites pass.

The conformance suite from the specification repository's current main (f5bc061), through tools/run-conformance.sh openkal-linux <this tree> full,optional: 197 held, 0 did not hold, 3 not observed, the same as without the change.

The exported-surface, provides-interfaces and C++ declarations steps of CI, run by hand with the same commands against the specification at f5bc061, pass. The independence check reports one symbol, __tls_get_addr from task.o, and reports it for the unchanged tree as well; fs.o references nothing it did not before. The reference is made by the thread_local variables in task.cpp, which the build I used compiles with -fPIC; I did not check whether a build with the engine and toolchains CI uses reports it.

With openkal-musl 0.20.1 as a path dependency and its openkal-linux dependency pointed at this tree, the program from #34 gives before=7 after=7; the same arrangement over openkal-linux 0.16.1 gives before=7 after=71.

I did not build with gcc 16.1.0 or llvm@22.1.8, which CI uses, and CI pins mcpp 2026.9.11.3 where I used 2026.10.8.1. I did not run the Android object checks or anything on aarch64, macOS or Windows.

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant