Repository navigation
fix(libc): read the stream status flags atomically when keeping a directory read error - #11335
Conversation
…ectory read error PublishStatus updates FileStream's guest status word with atomic operations from the thread that just read, while KeepDirectoryReadError read it with a plain load, a data race that ThreadSanitizer reports in guest_gets when two threads read the same stream. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
I checked the claimed |
thepoppfish
left a comment
There was a problem hiding this comment.
Reviewed exact head 950d9401f1d0d891bad24858971b75fe07e5e9e6. No blocking finding: the relaxed atomic load matches the existing atomic publication of the same 16-bit status word and preserves the directory/error-bit checks and errno behavior.
I independently reproduced the race before discovering this PR, then implemented the same one-line change locally. I am retiring that duplicate implementation rather than opening another fix PR. The old header (blob a97f626f49700171e176920c8bfa7ef76cc33980) reports a two-byte read in KeepDirectoryReadError racing the atomic write in PublishStatus under ThreadSanitizer (exit 66). With your exact fixed header (blob 5747dd3fa7c11a322ee9510fb9080318bde018af), the same regression exits 0 with no ThreadSanitizer warning.
Independent validation:
- Ubuntu 24.04.5 under WSL2, kernel
6.18.33.2-microsoft-standard-WSL2, GCC 13.3. The focused test uses the actual FileStream header: four serial EOF/error combinations plus 20,000 publications concurrent with 20,000 reads on a directory-marked stream view. It checks errno and preservation of unrelated status bits. This is a direct method-level regression, rather than a claim that I reran the complete guest_gets test under ThreadSanitizer. - Source-built the actual complete
libctarget and its declared ZArchive/zstd dependencies, plus three stream test executables, using the original libc CMake source list and root test/unwind helpers in an isolated project: 110 Ninja steps on Windows 11 build 26200 / WinLibs GCC 15.2.0-r7 Release, and 110 on Linux / GCC 13.3 Release. guest_gets,file_streamsand the newfile_stream_statusall pass: 3/3 Windows (0.67 s total), 3/3 Linux (0.26 s total); no skips. These complete SDK-target runs are ordinary Release builds; only the separate focused probe was ThreadSanitizer-instrumented.- Conventions and whitespace checks pass. The public-header declaration audit passes for 368 libraries / 12,224 exports; I did not run the full standalone-header compilation matrix.
The SDK builds used my main-based candidate 3ff5eea1d1add62bc581dec8b6347bd77aba9cf4, not a fresh checkout build of this PR. I checked the compiler dependency manifests: all 128 existing tracked compiler inputs match your exact head; all 129 inputs including the added test match the support commit below. The libc source tree also differs only by that test's CMake registration. Main 9b197b6ce4583e64bf2427e6beb7270bcfa1528e merges cleanly, and the relevant SDK/build inputs are unchanged from the tested main base 943ea8acb23f0a1b2af35d85393fe6fd5afb7743. No full-root product build, macOS, console, game, GPU or hosted-CI verification is claimed. Windows configuration warned about long object paths for two unrelated excluded mutex/cnd test targets; the requested targets built and ran, with no compiler warnings.
An optional test-only commit is available: ba05e8b661fb7e51b54aa115fdb28e94a1680f54. Its parent is your exact head, and it changes only core/libs/tests/FileStreamStatus.cpp and the CMake registration; it contains no competing production change. It links the existing libc runtime and uses the existing Windows unwind helper, so the regression also builds/runs with the supported GCC 15 DWARF exception setup. Adoption is optional for this approval.
Focused sanitizer reproduction from the test-support tree:
g++ -std=c++20 -O1 -g -fno-omit-frame-pointer -fsanitize=thread -pthread -Icore/libs core/libs/tests/FileStreamStatus.cpp -o file_stream_status_tsan
TSAN_OPTIONS=halt_on_error=1:exitcode=66 setarch x86_64 -R ./file_stream_status_tsanFor the negative control, I compiled the same test before applying the header change. The first attempt with default ASLR exited 139 without a sanitizer diagnostic. Process-level setarch -R allowed the sanitizer run; no global VM/sysctl setting was changed. Exit 139 is not counted as the regression failure.
AI-assisted: yes.
|
Thanks for the detailed independent follow-up. The focused |
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed the exact head and the one-line atomic access against PublishStatus, which already uses relaxed atomic operations on _guest.flags. The focused parent-vs-change ThreadSanitizer evidence in the review thread exercises KeepDirectoryReadError on a directory-marked stream view and reports the race only on the parent; the additional full stream-target runs are accurately distinguished from that TSan probe. Hosted relinker, ASan, register-context, write-watch, conventions, intake, eligibility, labels and report checks pass. Linux/Windows product jobs were skipped by path policy; no macOS test is claimed. No blocking issue found.
|
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.
|
|
@xXPBSXx The atomic load matches the existing status publisher. Please correct Tested to distinguish the focused, directory-marked FileStream sanitizer probe from the ordinary guest_gets runs. GuestGets::CheckThreads uses a tmpfile stream, while the only production MarkDirectory call is inside the Windows-only OpenDirectoryStream, so that Linux guest_gets path does not reach this flags read. Please cite the focused probe for the race claim and keep its results separate from the complete-target runs. |
MarcZX10
left a comment
There was a problem hiding this comment.
The new file_stream_status target is a useful focused regression, and the added test exercises KeepDirectoryReadError on a directory-marked FileStream concurrently with PublishStatus. However, the PR description is still stale and materially misstates the evidence: it says “No new test” and claims guest_gets itself reports the race under TSan. As the collaborator noted, guest_gets uses a tmpfile and does not mark that stream as a directory on Linux, so it does not reach this read. Please update the What/Tested text to distinguish the focused file_stream_status TSan probe (and its parent-vs-change result) from ordinary guest_gets/stream-target runs, and report the exact tested head. The hosted conventions and policy checks pass; applicable build/relinker checks are still running, and Linux/Windows product jobs are skipped by path policy. I will re-review the exact head after the report matches the actual test path.
|
I independently built the new test source at head |
|
One additional current-main check: |
|
Thank you, thepoppfish, for the focused probe and for the test commit, which is now the second commit of this branch with your authorship ( AI-assisted (Claude Code). |
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed exact head 47de0d9587624b01bf0383a4e0c89787239fd2f9 after the report correction.
The description now distinguishes the two relevant paths correctly: the Linux guest_gets source path does not set directory, while the focused directory-marked FileStream probe exercises KeepDirectoryReadError against concurrent PublishStatus and reports a parent-only TSan race. The code change matches the existing relaxed atomic publication pattern, and the added CTest target covers status-bit preservation, errno behavior, and concurrent reads/writes. I accept the corrected explanation as resolving my earlier evidence-path concern.
One repository requirement remains: the PR head merges main at 3d2b7c0673, while current upstream/main is 8100e480f7d55e87afb5376b8f784bf17df0f75c. tools/check_main_sync.py --base upstream/main --head upstream/pr/11335 reports 3 commits ahead and 139 behind; the merge preview is clean, and the touched source/test paths have no intervening edits. Please refresh onto current main and rerun file_stream_status on the resulting exact head. Current conventions and git diff --check pass; hosted relinker, ASan, register-context, and write-watch checks pass; Linux and Windows product jobs are skipped by path policy. The TSan details remain author-reported, not independently rerun here.
AI-assisted review.
|
Merged Retested on the exact head: the focused The red arm (the fix header removed, the test kept) was not repeated on this head; it was measured on the previous head Not tested: macOS. AI-assisted (Claude Code). |
MarcZX10
left a comment
There was a problem hiding this comment.
Approved exact head c3ada6c5d76a7e8b2acaef13b8f7a2ae633f021c. The updated description correctly identifies the directory-marked file_stream_status probe as the TSan evidence and explains why Linux guest_gets does not reach that read in the source. The author reports a parent-vs-change TSan result on that focused probe, plus exact-head Linux/Windows Release stream tests.
I independently compiled and ran FileStreamStatus.cpp at this head with GCC 12 (-std=c++20 -Wall -Wextra -Werror -pthread): 4 serial cases, 20,000 publications, 20,000 reads, 0 failures. Current-main conventions and git diff --check pass. Current main is three commits beyond the branch's merged base; the merge preview is clean and those commits do not touch the PR's files. Hosted relinker, ASan, register-context, write-watch, conventions and policy checks pass; automatic Linux/Windows product jobs are skipped by path policy. I did not independently run TSan or Windows.
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed exact head c3ada6c5d76a7e8b2acaef13b8f7a2ae633f021c. The one-line fix makes KeepDirectoryReadError use a relaxed atomic load, matching the publication path; the focused regression exercises both values of EOF/error, preserves unrelated bits, and races 20,000 reads against 20,000 publications. The updated report distinguishes the directory-marked TSan reproducer from ordinary Linux guest_gets, and records parent-vs-change TSan results. git diff --check and tools/check_conventions.py --base upstream/main --head upstream/pr/11335 pass; current-main merge preview is clean. Hosted eligibility, conventions, labels, report, relinker matrix, ASan, register-context, and write-watch checks pass; Linux/Windows product jobs are path-skipped.
MarcZX10
left a comment
There was a problem hiding this comment.
The focused code and test review remains positive, but this head is no longer current with the required base. GitHub compares fix/filestream-flags-race as 4 commits ahead and 505 behind current main 6b16c9c6499a6118fb02444b5d3cfa433f9577db. Since merge base 4e43321bd6, main has also changed core/libs/prx/libc/CMakeLists.txt, the file that registers this test. CONTRIBUTING.md requires keeping the branch up to date with main and resolving conflicts.
Please merge/rebase current main, preserve both the new main-side test registrations and this PR's file_stream_status target, then rerun the focused test (including the focused TSan probe if available) and conventions on the resulting exact head. I am not reopening the earlier functional findings; the current-base and exact-head validation are the remaining merge blockers.
|
Merged Retested on the exact head on Windows 10 (MinGW-w64 GCC 15.2), Release, ninja exit code 0: Not rerun on this head: the focused ThreadSanitizer probe and the Linux Release runs, because the WSL2 virtual machine would not start. On the previous head AI-assisted (Claude Code). |
|
Follow-up to the earlier note: the Linux runs are done on the exact head AI-assisted (Claude Code). |
What
Read the status flags with
__atomic_load_ninKeepDirectoryReadError, the same relaxed orderingPublishStatususes for its own load. One line inFileStream.hpp.The test is the focused
file_stream_statusregression by thepoppfish (second commit): it marks aFileStreamview as a directory stream and runsKeepDirectoryReadErrorconcurrently withPublishStatus. ThreadSanitizer reports the race on that test with the parent'sFileStream.hppand not with this change.guest_getsis not the evidence for the race: its stream comes fromtmpfile, and the only productionMarkDirectory()call is inOpenDirectoryStream, which is Windows-only, so that Linux path does not reach this read in the source. The ThreadSanitizer report that first showed it onguest_getsis explained by the disassembly of the instrumented build onmain5b85225ef3, where the compiler loads the status word before it testsdirectory.Not in this PR:
feofandferrorinStandardStreams.cppalso read the flags word without an atomic load; ThreadSanitizer did not report them; the focused test does not exercise them.Fixes: #11325
Overlap:
git merge-treeof this head with each of the 42 open pull requests that editFileStream.hpp,FileStreamStatus.cpporcore/libs/prx/libc/CMakeLists.txtreports a conflict for 9 of them (measured againstmain4b1a9a3b2f), and each of those 9 already conflicts withmainon its own, so no landing order is needed. #10433 and #10781 touch other stream code; their pairs with the fix commit were clean.Tested
Head
47de0d9587624b01bf0383a4e0c89787239fd2f9: fix950d9401f1, testa45205c738(author thepoppfish) and the merge47de0d9587, which mergedmain3d2b7c0673with no conflicts (FileStream.hppis unchanged onmainsince14eb2cda22), onmain3d2b7c0673:Focused directory-marked probe (the evidence for the race), Ubuntu 24.04 under WSL2 (a virtualised kernel, not bare-metal), GCC 13.3,
g++ -std=c++20 -O1 -g -fno-omit-frame-pointer -fsanitize=thread -pthread -Icore/libs core/libs/tests/FileStreamStatus.cpp, run withTSAN_OPTIONS=halt_on_error=1:exitcode=66 setarch x86_64 -R:FileStream.hpptaken from the parent of the fix commit,14eb2cda22(same checkout, test kept): exit 66 and 1 warning in each of 3 runs (data race ... FileStream.hpp:225 in FileStream::PublishStatus).Complete-target runs:
ctest --repeat until-fail:3:file_stream_status,guest_getsandfile_streamspass.git merge-treeagainstmainis clean.guest_getsunder ThreadSanitizer (Linux, RelWithDebInfo,-fsanitize=thread; measured earlier on the fix commit alone, not re-run on the merge head): 0 warnings in 3 runs with the fix, 1 warning in each of 3 runs with the parent'sFileStream.hpp. That warning is explained by the instrumented build (see What), not by a path the source takes on Linux, so it does not show a malfunction there.check_conventions --base upstream/mainpasses.Checklist
main; no other open PR implements the same changeNotImplemented_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)AI-assisted (Claude Code).
🤖 Generated with Claude Code