Repository navigation
fix(libc): reject a malloc_stats_fast block that is not size 0x28 version 1 - #11870
ionut27001 wants to merge 2 commits into
Conversation
MarcZX10
left a comment
There was a problem hiding this comment.
The measured size/version cases are implemented and covered well. I built the exact head on Linux with the repository-pinned submodules and ran all three registered application_heap_stats tests; all 3 passed. Hosted eligibility, conventions, intake, labels, report, Ubuntu/Windows relinker, ASan and register-context checks also pass; macOS and write-watch jobs are still queued, and the full Linux product build is skipped by path policy.
One unresolved behavior is explicitly left unmeasured: a null stats pointer. The PR says the console would fault, but the implementation and regression test return 22. That creates a defined EINVAL-like behavior without console evidence, contrary to AGENTS.md's rule to throw and record behavior that is unknown. Please either provide a probe establishing the null behavior and implement it, or leave the unmeasured case as an explicit unsupported path.
The PR also identifies debt-file merge conflicts with open #11846, #11848 and #11333. Please coordinate merge order with those authors and record any required dependency in the checklist as CONTRIBUTING.md asks.
|
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.
|
|
@MarcZX10 I took the unsupported-path option for the null pointer:
|
3670a15 to
75ab49f
Compare
|
Windows run of this head (
AI-assisted: yes (Claude Code). |
MarcZX10
left a comment
There was a problem hiding this comment.
Re-checked — looks good
The author changed the unmeasured null-pointer path to throw, records it as unsupported debt, and reports exact-head focused tests with the null case and negative controls; the earlier findings are resolved.
What I ran
- Reviewed exact head
75ab49fb0e23f222c18d0dbdb372de906c01a62eagainst currentmain52c09d567d2fa453494dd720fcf53a98149468e4. - Ran
git merge-tree --write-treeandgit diff --checkagainst currentmain. - Re-read the null guard, allocator calls, boundary test, technical-debt note, latest Windows report, and hosted checks.
Results
| Check | Result |
|---|---|
| Current-main merge preview | Clean; tree 5c335ee9eea360e05578b5c81a5e933c3477c367 |
| Whitespace check | Passed; exit 0 |
| Focused tests | Author reports application_heap_stats, application_heap_stats_default, and application_heap_stats_missing pass twice on Windows; the default test covers size/version cases and null throws before reaching the allocator |
| Negative control | Author reports reverting GuestHeap.cpp makes application_heap_stats_default abort; changing only null back to return 22 also aborts |
| Hosted checks | Windows, conventions, eligibility, labels, report, Ubuntu/Windows/macOS relinker, ASan, register-context and write-watch pass; Linux product test job skipped |
Raw output
$ git merge-tree --write-tree refs/review/main-20261029b refs/review/pr-11870-20261029
5c335ee9eea360e05578b5c81a5e933c3477c367
exit=0
$ git diff --check refs/review/main-20261029b...refs/review/pr-11870-20261029
(no output)
DIFF_CHECK:0
The focused tests and negative controls above are author-reported, not rerun by me on this head.
Negative control
The PR author reports both revert controls above. I did not independently repeat them on this head.
Findings
Blocking
- None found.
Non-blocking
- Linux product tests were skipped. The exact-head focused test and Windows check provide direct coverage of the changed path; the current-main merge preview is clean.
Sources
- Implementation at reviewed head
- Focused test at reviewed head
- Debt entry at reviewed head
- Contribution rules at current main
Not verified
- I did not rerun the focused Windows tests or remeasure on a console. The tests and title report are author-provided.
What
With the default allocator,
malloc_stats_fastreturned 22 for a block size below 0x28, accepted any larger size and any version, and filled the block.In a payload process on a PS5 (firmware 12.70),
malloc_stats_fastexported bylibSceLibcInternal.sprxwas called with a 0x40-byte buffer filled with 0x5a and the size and version fields set per case. Only size 0x28 with version 1 returned 0 and wrote the four 64-bit fields; every other case returned 1 and left all four fields at 0x5a5a5a5a5a5a5a5a. In every case the reserved bytes 4..8 and the 8 bytes at 0x28 stayed 0x5a.Measured log (case lines, payload process, firmware 12.70):
This change returns 1 without writing anything when the size is not exactly 0x28 or the version is not 1. A null pointer, which returned 22 on
main, now throws: it was not probed, as the console would likely fault, so its behaviour is unknown.sceLibcMspaceMallocStatsFastis left alone, as it was not measured. The debt entry now records that the null case throws because it is unmeasured. Ship of Harkinian (PPSA99320), which callsmalloc_stats_fast, passes a valid 0x28 / version 1 block, so its behaviour is unchanged.Overlaps: no open PR changes
malloc_stats_fast(searchedmalloc_stats_fast,MallocStatsFast,GuestHeapStatsFast,malloc stats). #11850 and #11468 also changeGuestHeap.cpp(and #11850docs/dev/debt/libc.md), but not this function;git merge-treeshows no conflict between this head and either. #11846, #11848 and #11333 change the neighbouring lines ofdocs/dev/debt/libc.md, so whichever PR merges second has a one-line debt conflict to resolve; none of them changesmalloc_stats_fast, so there is no dependency and any merge order works. If this merges after them, I will rebase and resolve the debt line here.Tested
Windows 11, WinLibs GCC 15.2.0, Release, on
main3fea2a9 (the head also merges cleanly ontomain431a515):application_heap_stats,application_heap_stats_defaultandapplication_heap_stats_missingpass twice. The default-allocator test now checks that size 0x28 / version 1 returns 0 and that 0x28 / 0, 0x28 / 2, 0x27 / 1 and 0x30 / 1 return 1 with the 0x5a-filled block byte-identical afterwards; null throws and does not reach the allocator.GuestHeap.cppfrommain,application_heap_stats_defaultaborts (0xc0000409), becausemain's code returns 0 and fills the block for version 0 and 2 and for size 0x30, and returns 22 for size 0x27. With only the null check put back toreturn 22, it aborts the same way.Not tested on Linux, nor with a title.
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)