Repository navigation
Conversation
The wide scanner pushed back a rejected byte as a signed char. A 0xff byte became -1, which is EOF, and PutBack ignores EOF, so the byte was consumed and never returned to the stream. This affects a lone 0xff as well as one that follows a lead byte. Pass the byte as unsigned so every value survives the pushback.
A stream whose first byte is 0xff must still hold that byte after %ls rejects it.
fdopen returned NULL for a negative descriptor without setting errno, so the caller saw whatever error was left over. The Windows path already reports EBADF for an invalid descriptor, and fdopendir reports 9 for the same input. Set errno to EBADF on that path.
fdopen with descriptor -1 must return NULL and set EBADF.
MarcZX10
commented
Nov 3, 2026
MarcZX10
left a comment
MarcZX10
left a comment
Contributor
Author
There was a problem hiding this comment.
Reviewed — no blocking findings
The reviewed source changes appear correct; hosted build jobs are still pending, so this is a comment rather than approval.
What I ran
Reviewed PR #15329 at head 4fb73821a711840e27fdb808e7a89f275406949c, based on 5cb015caaaaf83f8ee431414ee6a457b5b307d49. The stack checker previewed the head against current main tip 7feb37ee3e30e10139b483fb4df689d5b5b69ab8 and found a clean merge. Environment: Debian Linux x86_64, Python 3.11.2, GCC 12.2.0. I did not build or run CTest.
git diff --check 5cb015caaaaf83f8ee431414ee6a457b5b307d49...4fb73821a711840e27fdb808e7a89f275406949c
python3 tools/pr_precheck.py --repo boykopovar/AnyPS5 --pr 15329
python3 tools/pr_test_quality.py --root /tmp/anyps5-pr-precheck/boykopovar-AnyPS5-pr-15329-4fb73821a711/head-4fb73821a711-b16333c3 --json core/libs/tests/FilePosition.cpp
python3 tools/pr_overlap.py --branch refs/review/pr-15329-20261103 --repo boykopovar/AnyPS5
gh pr checks 15329 --repo boykopovar/AnyPS5
git grep -n 'file_position_tests\|FilePosition.cpp' refs/review/pr-15329-20261103 -- core/libs
for p in core/libs/prx/libc/src/FileAndHeap.cpp core/libs/prx/libc/src/Formatting.cpp core/libs/tests/FilePosition.cpp; do gh pr list --repo boykopovar/AnyPS5 --state open --search "path:$p" --limit 100 --json number,title,headRefOid --jq '.[] | [.number,.headRefOid,.title] | @tsv'; doneResults
| Check | Result | Note |
|---|---|---|
| Diff and conventions | passed | Patch and required PR fields pass. |
| Test registration | passed | CMake registers FilePosition.cpp; heuristic warning was a false positive. |
| Current-main merge preview | passed | Exact head merges cleanly; base has advanced. |
| Overlap helper | failed | The helper could not parse its GitHub response; path searches found no open PRs changing these files. |
| Hosted policy checks | passed | Conventions, intake, eligibility, labels, report, and portability passed. |
| Hosted build tests | not run | Build matrix remains pending; full Linux job was skipped. |
Precheck table and raw output
Precheck table, copied from its report:
| Check | Status | Command / reason |
|---|---|---|
| build_and_compile | not_applicable | The precheck deliberately does not configure, build, or compile AnyPS5. |
| diff_check | passed | git diff --check 5cb015caaaaf83f8ee431414ee6a457b5b307d49...4fb73821a711840e27fdb808e7a89f275406949c |
| conventions_and_pr_template | passed | /usr/bin/python3 tools/check_conventions.py --base 5cb015caaaaf83f8ee431414ee6a457b5b307d49 --head 4fb73821a711840e27fdb808e7a89f275406949c --title fix(libc): keep rejected wide scan bytes and set EBADF for a negative fdopen --body /tmp/anyps5-pr-precheck/boykopovar-AnyPS5-pr-15329-4fb73821a711/pr-body.md |
| changed_export_abi_audit | failed | /usr/bin/python3 tools/pr_abi_audit.py --base 5cb015caaaaf83f8ee431414ee6a457b5b307d49 --head 4fb73821a711840e27fdb808e7a89f275406949c --json |
| python_syntax | not_applicable | No changed Python implementation files. |
| related_python_unit_tests | not_applicable | No changed tools/ module has an existing sibling tools/tests/test_<module>.py suite. |
| python_test_registration | not_applicable | No changed test_*.py files. |
| live_dependency_and_mergeability_check | passed | /usr/bin/python3 /marc/AnyPS5/tools/pr_stack_check.py 15329 --repo boykopovar/AnyPS5 --output-dir /tmp/anyps5-pr-precheck/boykopovar-AnyPS5-pr-15329-4fb73821a711/stack |
| build_dependent_cpp_tests | not_applicable | Not run: C++ tests need exact-head binaries, which require a build; the precheck does not build. |
| test_quality_heuristics | failed | /usr/bin/python3 /tmp/anyps5-pr-precheck/boykopovar-AnyPS5-pr-15329-4fb73821a711/head-4fb73821a711-b16333c3/tools/pr_test_quality.py --root /tmp/anyps5-pr-precheck/boykopovar-AnyPS5-pr-15329-4fb73821a711/head-4fb73821a711-b16333c3 --json core/libs/prx/libc/src/FileAndHeap.cpp core/libs/prx/libc/src/Formatting.cpp core/libs/tests/FilePosition.cpp |
| negative_controls | not_applicable | No changed Python test module in tools/ can be run as a no-build base-source control. |
All conventions checks passed.
{"findings":[{"path":"core/libs/tests/FilePosition.cpp","line":1,"kind":"possible-unregistered-cpp-test","message":"Changed C++ test source path was not found literally in CMake files; inspect registration, generated source lists, and workflow coverage."}]}
pr_overlap: Invalid \\uXXXX escape: line 1 column 1807715 (char 1807714)
The CMake search found add_test_executable(file_position_tests ${LIBS_INCLUDE_DIR}/tests/FilePosition.cpp) and add_test(NAME file_position COMMAND file_position_tests), so the heuristic registration warning is a false positive. The precheck's ABI and test-quality subprocesses could not run because those helper files are absent from the isolated PR snapshot; the local test-quality helper produced the warning above.
Raw hosted CI output
edge-watch (macos-15) pending 0 https://github.com/boykopovar/AnyPS5/actions/runs/41550523808/job/125654429040
edge-watch (macos-15-intel) pending 0 https://github.com/boykopovar/AnyPS5/actions/runs/41550523808/job/125654429236
register-context (macos-15-intel, Debug) pending 0 https://github.com/boykopovar/AnyPS5/actions/runs/41550523808/job/125654429195
register-context (macos-15-intel, Release) pending 0 https://github.com/boykopovar/AnyPS5/actions/runs/41550523808/job/125654429087
register-context (ubuntu-latest, Debug) pending 0 https://github.com/boykopovar/AnyPS5/actions/runs/41550523808/job/125654429126
register-context (ubuntu-latest, Release) pending 0 https://github.com/boykopovar/AnyPS5/actions/runs/41550523808/job/125654429155
relinker (macos-latest, clang++) pending 0 https://github.com/boykopovar/AnyPS5/actions/runs/41550523808/job/125654429134
relinker (macos-latest, clang++, x86_64) pending 0 https://github.com/boykopovar/AnyPS5/actions/runs/41550523808/job/125654429061
relinker (ubuntu-latest, g++) pending 0 https://github.com/boykopovar/AnyPS5/actions/runs/41550523808/job/125654429113
relinker (windows-latest, g++) pending 0 https://github.com/boykopovar/AnyPS5/actions/runs/41550523808/job/125654429193
relinker-asan pending 0 https://github.com/boykopovar/AnyPS5/actions/runs/41550523808/job/125654429063
conventions pass 19s https://github.com/boykopovar/AnyPS5/actions/runs/41550523461/job/125636677244
intake pass 5s https://github.com/boykopovar/AnyPS5/actions/runs/41550523475/job/125636676965
library-portability pass 1m18s https://github.com/boykopovar/AnyPS5/actions/runs/41550523744/job/125636675882
write-watch (macos-15-intel) pending 0 https://github.com/boykopovar/AnyPS5/actions/runs/41550523808/job/125654429104
labels pass 8s https://github.com/boykopovar/AnyPS5/actions/runs/41550523463/job/125636677342
write-watch (macos-15) pending 0 https://github.com/boykopovar/AnyPS5/actions/runs/41550523808/job/125654429071
linux skipping 0 https://github.com/boykopovar/AnyPS5/actions/runs/41550523808/job/125654431163
windows pending 0 https://github.com/boykopovar/AnyPS5/actions/runs/41550523808/job/125654428980
eligibility pass 6s https://github.com/boykopovar/AnyPS5/actions/runs/41550523808/job/125636676475
report pass 48s https://github.com/boykopovar/AnyPS5/actions/runs/41550523705/job/125636675586
Negative control
No code mutation or negative-control test was run. The changed C++ tests require an exact-head build; the existing test asserts that a negative descriptor returns null with errno == EBADF and that an invalid wide-scan byte remains readable.
Findings
Blocking
None found in the three changed files.
Non-blocking
- The new wide-scan test covers an invalid
0xFFas the first byte, which exercises the firstPutBackcall. A two-byte input such as0xC2 0xFFwould also exercise restoration of a previously held lead byte and make the second pushback path explicit. This is a test-coverage suggestion; the current code's unsigned-byte conversions are consistent withStreamSource::PutBackandungetc'sEOFsentinel.
Sources
- Wide scanner restores rejected input bytes at this head
- Stream pushback passes the byte to
ungetc - Negative descriptor handling and errno
- New regression tests
- CMake test registration
Not verified
No local build, CTest, Windows or macOS runtime test, or PS5 console behavior was run. Hosted Windows, relinker, register-context, edge-watch, and write-watch jobs were pending at the last check; Linux was skipped. No independent review was performed; this PR is authored by MarcZX10. The full overlap helper failed, and path searches are not a complete overlap audit.
To get this to a good state
No code change is required by this review. Let the hosted jobs finish and obtain an independent review before relying on approval. If the optional two-byte regression is added, rerun the precheck and file_position CTest on the new head; I will recheck that head and its hosted results.
Contributor
Author
Reviewed — no blocking findingsThe two changes fix the reported byte-pushback and What I ranReviewed all three changed files at head python3 tools/pr_precheck.py --repo boykopovar/AnyPS5 --pr 15329
git diff --check 5cb015caaaaf83f8ee431414ee6a457b5b307d49...4fb73821a711840e27fdb808e7a89f275406949c
gh pr checks 15329 --repo boykopovar/AnyPS5Results
Precheck table and hosted outputThe ABI and test-quality failures were launch errors because those helper scripts are not present in the isolated PR snapshot. The test file already belongs to the existing Exact-head hosted checks: Negative controlI did not run a negative control. The PR description reports that removing each fix makes its corresponding test fail, but those runs are author-reported. The new tests assert that FindingsBlockingNone found in the changed behavior. Non-blockingWhile tracing the modified wide-scan path, I confirmed the PR description's separate multibyte concern: after Sources
Not verifiedI did not run a local build, CTest, the author's negative controls, or console behavior. The hosted Linux workflow job was skipped; the Linux CTest result in the PR description was not independently reproduced. To get this to a good stateNo change is required for this diff based on the checks above. Consider a separate follow-up for valid multibyte wide scanning. I would rerun the same precheck and relevant tests on a new head; this is a MarcZX10-authored PR, so my result is not independent approval. |
Contributor
|
A Windows run with GCC 16: Windows 10.0.22631 (Windows 11), i9-9900K, MSYS2 UCRT64 GCC 16.1.0 (Rev5), Release, this PR (
The test stops at line 45, so the old side didn't reach the AI-assisted: yes (Claude Code) |
XairRrR
suggested changes
Nov 3, 2026
XairRrR
left a comment
XairRrR
left a comment
Contributor
There was a problem hiding this comment.
Reviewed — changes requested
Both fixes change behaviour that main records as the console's, and each one makes an existing test fail: fdopen breaks guest_standard_streams and the 0xff pushback breaks guest_wide_stream_scan.
What I ran
Head 4fb73821a711840e27fdb808e7a89f275406949c merged into main 4f41f4c375f5abb487dbd184fbc150f31994d762 (merged tree 7acfdc3af15da4d0b228fa815430f9b1c301091e). Debian 13 VM, GCC 14.2.0, Release, Ninja.
git diff --check 4f41f4c375f5abb487dbd184fbc150f31994d762...4fb73821a711840e27fdb808e7a89f275406949c
python3 tools/check_conventions.py --base 4f41f4c375f5abb487dbd184fbc150f31994d762
ctest --test-dir build --output-on-failure -R '^(guest_standard_streams|guest_wide_stream_scan|file_position|guest_formatting)$'
# attribution: Formatting.cpp from main only, then FileAndHeap.cpp from main too
git show 4f41f4c375f5:core/libs/prx/libc/src/Formatting.cpp > core/libs/prx/libc/src/Formatting.cpp
ninja -C build guest_standard_stream_tests guest_wide_stream_scan_tests file_position_tests
ctest --test-dir build --output-on-failure -R '^(guest_standard_streams|guest_wide_stream_scan|file_position)$'
git show 4f41f4c375f5:core/libs/prx/libc/src/FileAndHeap.cpp > core/libs/prx/libc/src/FileAndHeap.cpp
ninja -C build guest_standard_stream_tests guest_wide_stream_scan_tests file_position_tests
ctest --test-dir build --output-on-failure -R '^(guest_standard_streams|guest_wide_stream_scan|file_position)$'
# restore the merged files, rebuild, git status --porcelain (clean)Results
| Check | Result |
|---|---|
git diff --check |
clean |
check_conventions.py |
passed |
| Build (selected tests) | passed |
file_position |
passed |
guest_formatting |
passed |
guest_wide_stream_scan |
failed: new failure compared with the baseline of the same main SHA |
guest_standard_streams |
failed: new failure compared with the baseline of the same main SHA |
| Other tests in the selected run | 9 known failures that also fail on main. guest_net_ctl was skipped on main too. None of these is caused by this PR |
Merged tree
2/4 Test #829: guest_wide_stream_scan ...........Subprocess aborted***Exception: 0.01 sec
failed: wide field failure keeps the measured stream position
3/4 Test #889: guest_standard_streams ...........Subprocess aborted***Exception: 0.01 sec
Attribution
- With only
Formatting.cpprestored frommain,guest_wide_stream_scanpasses andguest_standard_streamsstill fails. - With
FileAndHeap.cppalso restored,guest_standard_streamspasses.
So each failure comes from one of the two fixes in this PR. The baseline SHA is the same as main, so neither failure comes from main.
Negative control
I restored both production files from main and rebuilt. file_position then fails, so the new checks do catch the old behaviour. This agrees with the author's report and with the earlier Windows run in the discussion. I then restored the merged files, rebuilt, and confirmed that git status --porcelain is empty.
Findings
Blocking
-
fdopenwith a negative descriptor contradicts the measured console behaviour and failsguest_standard_streams.- The change is at
core/libs/prx/libc/src/FileAndHeap.cpp:443. - Commit
3dac7eb76c("…skip the access check as measured on a PS5") deliberately leaveserrnounchanged on this path. GuestStandardStreams.cpp:827-829pins thaterrnostays 1234 for-1,-2andINT_MIN.- The libc debt entry says the same: "a negative descriptor then returns null without setting
errno" (docs/dev/debt/libc.md:24). - The PR's reasoning compares with the Windows path and with
fdopendir. Neither of those is evidence of what the console'sfdopendoes. - Fix: drop this change. Or, if a PS5 measurement shows
EBADF, cite it, update the existing test and the debt entry, and remove the newCheckFdopenNegativeDescriptor.
- The change is at
-
The
0xffpushback change breaks the pinned wide-scan results inguest_wide_stream_scan.- The change is at
core/libs/prx/libc/src/Formatting.cpp:351-352. - The
signed charcast was added in8a0ba60209together with cases that expect the rejected0xffto be consumed. For example,"\xff\x7a"with%lsexpects position 1 and next byte0x7a. SeeGuestWideStreamScan.cpp:270-272. - The assertions there are labelled "measured" (L290-L294). This matches a console
ungetc((char)0xff)colliding withEOF. - With this PR, the test aborts at "wide field failure keeps the measured stream position".
- Note that the scanf debt entry (
libc.md:246) says the stream position after a failed wide field "was not measured". That contradicts the test labels, so the source of truth is unclear onmainitself. - Fix: either keep the current behaviour, or provide a console measurement showing the byte stays in the stream. In the second case, update the
GuestWideStreamScan.cpprows (all\xffcases,%Sincluded) and the debt entry in the same PR.
- The change is at
-
The description's test claims leave out the failing tests.
- The description reports only
file_positionandguest_formatting. - The two tests that cover these exact paths (
guest_standard_streams,guest_wide_stream_scan) fail. - The two earlier review comments and the Windows run also checked only
file_position, so none of them caught this.
- The description reports only
Non-blocking
None.
Not verified
- Console behaviour of either path: no PS5 is available here.
- Windows and macOS builds.
- The full test suite was not run. The selected tests the change reaches were run by the script, and the four above were rerun by hand.
To get this to a good state
- Drop the
fdopenchange unless a PS5 measurement supportsEBADF. If one does, change the existingGuestStandardStreams.cppexpectation and the debt entry in the same PR, with the measurement cited. - Do the same for the wide-scan pushback: keep the existing behaviour, or measure on a console and update
GuestWideStreamScan.cppanddocs/dev/debt/libc.mdtogether. - Rerun
guest_standard_streams,guest_wide_stream_scan,file_positionandguest_formatting, and report all four in the description.
Contributor
❌ AMD GPU test: failedPR head
Failing testsThis PR changes test files, so each failing test was also built and run on main with only the PR's test changes applied. ✅ there means the failure comes from the PR's other changes; ❌ means the test fails on main too, so this run does not show the PR caused it.
guest_standard_streams: Subprocess aborted (also failed when re-run alone), assertion and diagnosticsguest_wide_stream_scan: Subprocess aborted (also failed when re-run alone), assertion and diagnosticsUnstable on main (2, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (12, not counted against this PR)Validated but not compared (3, not in the main baseline's validation pass)Validation IDs already present on main (4 IDs in 764 tests)Skipped (60)ReproduceAutomated build and test on a contributor's AMD machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
Collaborator
|
Hi MarcZX10, A heads-up from our Windows test run, in case it helps. With this PR at We didn't get a Linux result for it in that run. Since the change is in the wide scan path ( |
This branch has not been deployed
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.
What
Two libc fixes:
0xffbecame EOF and was dropped from the stream. The byte is now kept.fdopenwith a negative descriptor returned NULL without settingerrno. It now setsEBADF.Commits:
Tested
ctest -R '^(file_position|guest_formatting)$': 2 of 2 passed (file_positionTest feat(relinker): relink needed modules outside the module directories #716,guest_formattingTest build(windows): guard NOMINMAX before including windows.h #872).fdopenfix failsFilePosition.cpp:50; removing the0xfffix failsFilePosition.cpp:45.tools/check_conventions.py --base upstream/mainandgit diff --check upstream/main: clean.Not verified
Formatting.cpp) looks like a real defect, but existing tests pin the current rejection. It is left for a maintainer decision.Checklist
mainat the time of testing; fix(libc): retain short wide scanf character fields #9863 covers different fields, not the pushback