Repository navigation
End a directory's unfinished enumerations when the directory is closed - #35
Open
yspbwx2010 wants to merge 1 commit into
Open
yspbwx2010 wants to merge 1 commit into
yspbwx2010 wants to merge 1 commit into
Conversation
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #34
kal_fs_list_beginopens a descriptor of its own for each enumeration, and onlykal_fs_list_nextreleased it, on reaching the end.kal_fs_close_dirclosed 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_dirends that directory's listings before it closes the descriptor, because after the close the same number may already name another directory.kal_fs_list_nexttakes 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_nexton another thread racing the close, uses freed memory, andgetdents64may 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 testwith llvm@23.1.3 on x86_64 Linux:test result ok. 9 passed; 0 failed. The new check intests/conformance_fs.cppbegins 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/fdunder 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_addrfrom task.o, and reports it for the unchanged tree as well; fs.o references nothing it did not before. The reference is made by thethread_localvariables 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 givesbefore=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.