Repository navigation
fix(libc): drop the kernel state of a descriptor that fclose or freopen closes - #10755
Conversation
MarcZX10
left a comment
There was a problem hiding this comment.
The descriptor-state cleanup is needed, and the regression test covers the stale flags and reused descriptor cases. I found a Windows synchronization gap in the close path:
FileStream::Close() calls ForgetClosedDescriptor_nid_no_patch() and then calls std::fclose() (core/libs/prx/libc/include/FileStream.hpp:306-310). The registered callback acquires File::LockPositionedTransfer(descriptor) at core/libs/prx/libkernel/File/src/Open.cpp:275-280, but that lock is destroyed when the callback returns, before fclose actually closes the CRT descriptor. The Windows sceKernelClose/close paths keep the same lock alive across both state cleanup and the native close (Open.cpp:330-341, Stdio.cpp:728-749). Thus a concurrent positioned read/write can acquire the descriptor lock in the gap and race the stream close or descriptor reuse; the cleanup callback currently does not provide the same serialization guarantee.
Could the libc/libkernel handoff keep the positioned-transfer lock held until the underlying close/replacement has completed (for example, through a close wrapper/callback whose scope includes fclose/freopen), and add a Windows regression that coordinates a positioned transfer with fclose/freopen? The current sequential test does not exercise this interleaving. I have not run a local Windows concurrency reproduction, so this is based on the lock scopes and the existing close path.
|
Other open pull requests touch the same code. Merging one will break the other, or both implement the same thing. If this one builds on them, list them in Depends on.
Some comparisons could not be completed because a pull request conflicts with the base branch. Review these shared files manually before concluding that there is no overlap.
|
|
One additional integration point surfaced while checking the overlap report. #8520 moves |
|
@MarcZX10 Thanks, both points taken. Lock across the close (5129671): libkernel now hands libc a begin/end pair instead of a single drop callback. Begin drops the equeue registrations, takes
All pass 10 of 10 on Windows and Linux; full runs are in the description. Directory descriptors and #8520: on |
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed exact head 512967154daeaa0a830cf55f8c9e6a661deee46b. The begin/end close handoff now keeps the Windows positioned-transfer lock alive through the native fclose/replacement, and the stream lock is acquired before it, matching the flockfile then lseek order. The added regression exercises the close/write race, stream-lock/lseek interaction, descriptor reuse and freopen concurrent with stream creation. The author reports the focused test passing 10/10 on Windows and Linux; the applicable hosted relinker, ASan, register-context and write-watch checks are green. Product linux and windows jobs are skipped by policy, so I did not independently verify the platform runtime here.
The known #8520 integration is recorded in the PR description and discussed with the author: if POSIX directory-descriptor tracking lands first, the shared cleanup must be moved out of the Windows-only guard and the stream-close test extended. The current head is otherwise clean against main. I approve this exact head, with that follow-up integration point retained for the eventual combined branch.
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed the descriptor-state cleanup path, the libc/libkernel callback registration, and the close/reopen locking changes. Cleanup is performed before the host descriptor is closed, while the Windows positioned-transfer lock remains held; the new tests cover descriptor reuse, append and close-on-exec state, random-device reuse, freopen, and close races. Hosted relinker and write-watch suites pass on Linux, Windows, and macOS, along with conventions and intake.
|
@Ragbie Please replace GuestStreamClose.cpp’s local export declarations and FileStream forward declaration with the canonical public headers, preserving current main signatures. The clean main integration fails public_headers on 13 duplicate exports. Keep the lock-scope/race regressions and the recorded #8520 second-lander directory-cleanup condition, then rerun public_headers and the Windows/Linux stream-close bar. |
5129671 to
be96d03
Compare
|
@oneandonlydean Done. The branch is rebased onto
|
be96d03 to
5b5a8f0
Compare
|
@Ragbie #8520 merged at 13:23Z (1061954), so the second-lander condition from my earlier comment (6390609564) now applies. Please:
|
5b5a8f0 to
92f4276
Compare
|
@MarcZX10 #8520 has landed, so as agreed
|
|
@oneandonlydean Done in 92f4276, rebased onto
|
|
@Ragbie For the shared FileStream close/reopen lifecycle, I propose #10755 lands before my draft #11831 ( AI-assisted: yes (Codex). October 29 follow-through: focused private combined results are now available: 39 passes on each of Windows and Linux, with identical-fixture cleanup controls and independent review. Neither public branch changed. This does not imply agreement to the proposed order or public integration readiness. |
MarcZX10
left a comment
There was a problem hiding this comment.
Re-checked — new head, looks good
The new POSIX directory-descriptor regression is present on head 92f4276f28e9643fe7e336e1cc40ee02f30277da, and the reported cross-platform focused tests cover it.
What I ran
Reviewed head 92f4276f28e9643fe7e336e1cc40ee02f30277da against PR base 3ead2d72517499985692df803ab72ff92ac82ecc; the merge preview against current main dc38224141e0b6096007b876b4c42fc9144d11b8 is clean. I inspected the eight PR files, including the portable descriptor cleanup and new POSIX test. Local checks ran in Linux; no compiler or build was invoked.
git diff --check refs/remotes/upstream/main...refs/review/pr-10755-current
python3 tools/check_conventions.py --base refs/remotes/upstream/main --head refs/review/pr-10755-current
git merge-tree --write-tree refs/remotes/upstream/main refs/review/pr-10755-current
gh pr checks 10755 --repo boykopovar/AnyPS5Results
| Check | Result |
|---|---|
| Diff check | Passed; no output |
| Repository conventions | Passed |
Merge preview with current main |
Clean; tree cce419e41f5934a32bb710eaad8a9ad4989b7579 |
| Hosted checks | Conventions, eligibility, labels, report, register-context, relinker, relinker-asan, Windows relinker and write-watch passed on the exact head; product Linux/Windows jobs skipped |
| Focused tests | Author reports 88 matching tests passed on Windows and 84 on Linux, repeated three times, including the directory-stream regression; not independently run here |
Unedited local check output
All conventions checks passed.
cce419e41f5934a32bb710eaad8a9ad4989b7579
Findings
Blocking
None found in this re-check. The added POSIX case closes an app archive directory through fdopen/fclose, verifies descriptor reuse by opening a regular file, and reads that file successfully.
Non-blocking
The focused tests are author-reported; I did not build or run them locally. Hosted product Linux and Windows jobs were skipped.
Sources
- Directory-descriptor regression at this head
- Stream-close regressions at this head
- Author's exact-head test report
Not verified
No local build or tests, console behavior, or full ctest run was performed. The author states the full test suite was not rerun for this rebase.
What
fcloseandfreopenclose the stream's descriptor through the host C library and leave libkernel's state for that descriptor number behind: its status flags, close-on-exec and pipe flags, the random-device set, archived files, JIT shared memory, equeue registrations and, on Windows, directory descriptors and file locks.sceKernelCloseandclosedrop all of it. The next descriptor that gets the same number, usually from the nextfopen, inherits it:sceKernelOpen(O_WRONLY | O_CREAT | O_APPEND | O_CLOEXEC),fdopen(fd, "a"),fclose: the nextfopen(path, "r+")gets the same number,fcntl(F_GETFL)returnsO_RDWR | O_APPENDandfcntl(F_GETFD)returnsFD_CLOEXEC. On Windows,sceKernelPwriteandsceKernelWriteon it append instead of writing at the offset.sceKernelOpen("/dev/urandom"),fdopen(fd, "r"),fclose:sceKernelReadon the next file with that number returns random bytes instead of the file's contents.freopen(path, "r+", stream)on such a stream behaves the same way.FreeBSD 11.4 closes the descriptor in both:
fclosecallscleanfile, which calls the stream's close function,__sclose, that is_close(fp->_file).freopenreplaces the old descriptor withdup2onto its number, or closes it. Either way the number comes back with the new file's flags andFD_CLOEXECclear.The change:
File::ForgetClosedDescriptorholds the drops thatsceKernelCloseandclosemade inline. Both call it, in the same order, and on Windows still under the positioned-transfer lock.LibcRegisterClosedDescriptors_nid_no_patch, asLibcRegisterDirectoryStreams_nid_no_patchdoes for directory streams). Begin drops the equeue registrations, takes the positioned-transfer lock on Windows and callsFile::ForgetClosedDescriptor; end releases the lock.FileStream::CloseandFileStream::Reopenhold it throughClosingDescriptor, so the host close runs under the same lock as insceKernelClose, and a concurrentpread/pwriteeither finishes before the close or sees the closed number.freopenreleases it right after closing the old stream, before it opens the new file: UCRT's stream allocation locks the free streams it scans, so holding the old stream's lock into the open can deadlock against another thread'sfopen. Without libkernel nothing is registered and there is no state to drop.ClosingDescriptortakes the stream lock before the positioned-transfer lock, the order a guestflockfilefollowed bylseekuses.File::LockPositionedTransferreturns a lock that co-owns its mutex, so adupthat reuses the number while the lock is held cannot free the mutex.FileStream::Closeclears the stream only after the hostfclosereturns, so a thread that holds the stream withflockfilecan stillfunlockfileit whilefclosewaits.Tested
Windows 11, WinLibs GCC 15.2.0:
guest_stream_close:main's libc and libkernel it fails on the status flags, close-on-exec, the positioned and plain writes, the random-device read and thefreopenstatus flags, close-on-exec and write.sceKernelPwriteon another thread while the main thread callsfclose, orfreopenthenfclose, 16 rounds; the write must complete or fail withEBADF, and nothing may land in the reopened file. When the lock is released beforefclose, it fails 3 of 3 runs:pwritegetsESPIPEbecause its handle is closed between_get_osfhandleandGetFileType.flockfileby a thread that then callslseekwhile another threadfcloses it. With the positioned-transfer lock taken before the stream lock it hangs until the ctest timeout.freopen500 times while another threadfopens andfcloses a second file. With the stream lock held into the open it hangs until the ctest timeout in 3 of 3 runs.--repeat until-fail:10(longest run 0.40 s).ctestafter rebasing ontomain8a29e0883:99% tests passed, 5 tests failed out of 2379.agc_write_tracking_metadata_commit,agc_host_import_cut_lifetime,agc_driver_depth_plane_partial_storeandagc_driver_image_plane_address_on_linefail onmainon this machine too, andguest_wide_formatting(wcsftime length matches strftime) fails the same way onmainplus an unrelated socket change.check_skips.py: all skips expected.public_headersand its compile batches pass: the test now includesprx/libkernel/include/Api.hpp,prx/libkernel/File/include/File.hpp,prx/libc/include/Api.hppandprx/libc/include/FileStream.hppinstead of declaring the 13 exports itself.mainbf77a9e9c, after feat(libkernel): enumerate archived app directories #8520:File::ForgetClosedDescriptornow drops the directory-descriptor entry on every host, assceKernelCloseandclosedo since feat(libkernel): enumerate archived app directories #8520, sofcloseandfreopendrop it too.guest_stream_closeadds a host directory closed throughfdopenandfclosewhose number the next file reuses (Windows tracks every directory). Full build, then the 134 tests matchingstream_close|public_headers|kernel_fcntl|append_writes|file_lock|freopen|fclose|stdio|stream|app_archive|directorypass, andguest_stream_closeandguest_kernel_app_archive_archivepass with--repeat until-fail:10. Fullctestwas not rerun for this rebase.Linux (Ubuntu 24.04 container, GCC 13.3.0):
guest_stream_closefails onmainand passes with the change, with--repeat until-fail:10. After the rebase,guest_stream_closeandpublic_headerspass with--repeat until-fail:10, and fullctestgives99% tests passed, 24 tests failed out of 2286; the same 24 fail onmainin this container (22agc_*on its lavapipe and kernel withoutUFFD_FEATURE_WP_ASYNC,pr_overlap_encodingandguest_system_load_exec_standard_error). On POSIX only app0.zar directories are tracked, soguest_kernel_app_archive_archiveadds one closed throughfdopenandfclose: with the call still under_WIN32it fails at the read of the file that reuses the number (EISDIR), and with this head it passes, withguest_stream_closeand the other twoguest_kernel_app_archive_*tests, with--repeat until-fail:3. Full Linuxctestwas not rerun after #8520.Overlaps:
FileStream::Close(archive detach); the two changes are independent.File::ForgetDirectoryDescriptorout of the_WIN32block insceKernelCloseandclose;File::ForgetClosedDescriptornow does the same.FileStreamclose and reopen lifecycle; agreed order: this PR first, feat(libc): implement fcloseall with tracked stream lifetimes #11831 rebases after it.Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file.mdfiles or images added to the repository (attach them to this PR)