Repository navigation
Conversation
added 18 commits
October 11, 2026 15:24
…s vulnerabilities
Registers the retained explanatory comments documenting manual rollback blocks, arithmetic overflow pre-validation, and the _allocations ledger in MemoryPool.cpp to satisfy check_conventions.py requirements.
Guest-controlled offsets and lengths reached the page bitmap and block map without validation, and several mutating paths could leave the two structures inconsistent if an allocation or backing cleanup failed. Validation - Reject null out-pointers, zero lengths, negative starts, unaligned Free/Retype starts, and non power-of-two alignments. - Round lengths up to whole pages and raise alignment to at least a page. - Make all range arithmetic overflow-safe. - Clamp search ends to the pool size; a negative end means unbounded, matching previous Alloc behaviour. - Share one set of normalisation helpers across every entry point. Consistency - Alloc: insert block, create backing, then mark pages; undo on failure. - Free/Retype: split blocks first, roll splits back if a later step throws, then apply non-throwing updates. Pool is unchanged on throw. - Free keeps partial and multi-allocation release semantics; CheckedFree still requires every page in range to be allocated. - Retype retypes allocated pages and skips gaps. Other - Restore DirectMemoryLargestFree, now validated. - Alloc skips past the last used page on collision instead of stepping one alignment unit at a time. - Move the pool into an anonymous namespace; add static_asserts for page and pool size. - Document the exception policy: Free and Retype propagate (void API), all other exports report failure via return value.
DirectMemoryFree and DirectMemoryRetype still propagate exceptions on purpose, but the pool is now left unchanged when they throw. Record the strong guarantee and the remaining gap (uncaught exceptions at the C/syscall boundary).
Guest-controlled offsets and lengths reached the page bitmap and block map without validation, and several mutating paths could leave the two structures inconsistent if an allocation or backing cleanup failed. Validation - Reject null out-pointers, zero lengths, negative starts, unaligned Free/Retype starts, and non power-of-two alignments. - Round lengths up to whole pages and raise alignment to at least a page. - Make all range arithmetic overflow-safe. - Clamp search ends to the pool size; a negative end means unbounded, matching previous Alloc behaviour. - Share one set of normalisation helpers across every entry point. Consistency - Alloc: insert block, create backing, then mark pages; undo on failure. - Free/Retype: split blocks first, roll splits back if a later step throws, then apply non-throwing updates. Pool is unchanged on throw. - Free keeps partial and multi-allocation release semantics; CheckedFree still requires every page in range to be allocated. - Retype retypes allocated pages and skips gaps. Other - Restore DirectMemoryLargestFree, now validated. - Alloc skips past the last used page on collision instead of stepping one alignment unit at a time. - Move the pool into an anonymous namespace; add static_asserts for page and pool size. - Document the exception policy: Free and Retype propagate (void API), all other exports report failure via return value.
Guest-controlled offsets and lengths reached the page bitmap and block map unchecked, and Alloc/Free/Retype could leave the two out of sync when node allocation or backing cleanup failed. - Validate pointers, lengths, alignment and ranges; make all range arithmetic overflow-safe. Search ends past the pool are clamped, and a negative end means unbounded, as before. - Alloc: insert block, create backing, then mark pages; undo on failure. - Free/Retype: split blocks first and roll the splits back if a later step throws, so the pool is unchanged on throw. - Keep partial and multi-allocation Free; CheckedFree still requires every page in range to be allocated. - Retype changes allocated pages and skips gaps. - Restore DirectMemoryLargestFree, now validated. - Only DirectMemoryAlloc catches std::bad_alloc (-> ENOMEM); all other exceptions propagate. A block-map/page-flag mismatch throws std::logic_error.
Add MemoryPoolTest with cases for argument validation, rounding, alignment and clamping, partial and spanning Free, Retype splitting, and rollback after injected bad_alloc and backing/forget failures. Register each case with ctest. docs(techdebt): update MemoryPool exception propagation entry Only DirectMemoryAlloc translates bad_alloc; all other entry points propagate, and mutating paths roll back before throwing.
Only DirectMemoryAlloc translates std::bad_alloc (to ENOMEM); all other entry points, including the void DirectMemoryFree and DirectMemoryRetype, let exceptions propagate. Mutating paths roll back before throwing, so the pool is unchanged. Record that nothing catches at the guest boundary yet.
9 tasks done
adriadam10
requested changes
Oct 11, 2026
adriadam10
left a comment
adriadam10
left a comment
Collaborator
There was a problem hiding this comment.
Thanks for the work. Blocking points:
core/libs/tests/MemoryPoolTest.cppis not registered anywhere: noadd_test_executable/add_testincore/libs/CMakeLists.txtorcore/libs/prx/libkernel/CMakeLists.txtmentions it, so it never runs locally or in CI (the PR body says "registered in ctest"). Please register it with an explicitadd_testand make sure it fails against the oldMemoryPool.cppthrough ctest. Note it definesCreateDirectMemoryBacking,ForgetDirectMemoryand the globaloperator new, so it must compileMemoryPool.cppdirectly rather than linklibkernel.- The
### Testedsection still has template placeholders (<passed / total, skipped; ...>,<result>fortools/check_conventions.py). Fill them with the real results on the final head. docs/dev/TechnicalDebt.md:591: the new line is the last of the "Functional" section, where every PR appends and conflicts. Put it a few lines away from the end.- Behaviour changes need a source (FreeBSD
sys/kern/man pages, KytyPS5, shadPS4):DirectMemoryFree/DirectMemoryRetypenow silently ignore invalid ranges (MemoryPool.cpp:306,321) instead of failing visibly, and a negativesearchEndnow means unbounded inLargestFree. Say which reference each follows, or record it under "Unknown function info" in TechnicalDebt.
Happy to re-review once these are addressed.
added 2 commits
October 11, 2026 17:13
Moves the existing functional inline comment registration to prevent merge conflicts at the end of the block. Documents the silent invalid range handling in DirectMemoryFree/Retype and the unbounded negative searchEnd in LargestFree under the unknown function info section.
adriadam10
requested changes
Oct 11, 2026
adriadam10
left a comment
adriadam10
left a comment
Collaborator
There was a problem hiding this comment.
Thanks for the update. The points from the previous review are still open on the current head (the new commits are merges from main plus a docs change):
core/libs/tests/MemoryPoolTest.cppis still not registered: noadd_test_executable/add_testmentions it incore/libs/CMakeLists.txtorcore/libs/prx/libkernel/CMakeLists.txt, so it never runs locally or in CI. It definesCreateDirectMemoryBacking,ForgetDirectMemoryand the globaloperator new, so compileMemoryPool.cppdirectly into its own target instead of linkinglibkernel, and check through ctest that it fails against the oldMemoryPool.cpp.### Testedstill contains the template placeholders (<passed / total, skipped; ...>,<list any other failures...>,<result>fortools/check_conventions.py). Fill them with the real results on the final head.docs/dev/TechnicalDebt.md:597-598: the entry is still the last line of "Functional" (everyone appends there, so it conflicts), and the last commit added a second line starting with- - [MemoryPool]that is malformed and repeats the first. Keep one line, a few lines away from the end of the section.- Behaviour changes need a source:
DirectMemoryFree/DirectMemoryRetypenow silently ignore invalid ranges, and a negativesearchEndinLargestFreemeans unbounded. Say which reference each follows (FreeBSDsys/vm, KytyPS5, shadPS4) or record it under "Unknown function info" indocs/dev/TechnicalDebt.md.
Happy to re-review once these are addressed.
added 3 commits
October 11, 2026 17:23
Compiles the test directly against MemoryPool.cpp to ensure custom mocked functions are utilized, and registers it to run locally and in CI via ctest.
adriadam10
requested changes
Oct 11, 2026
adriadam10
left a comment
adriadam10
left a comment
Collaborator
There was a problem hiding this comment.
The current head breaks CMake configure for the whole tree, so nothing can be built or tested:
core/libs/CMakeLists.txt:332-339: the block you added ends with an extraendif(), and cmake stops withFlow control statements are not properly nested(line 339). Drop the extraendif()and put the block inside the existingif(BUILD_TESTING), e.g. next to the otheradd_test_executablecalls (indented like them, no blank line beforeendif()). Then check withctest -NthatMemoryPoolTestis listed and withctest -R MemoryPoolTestthat it fails against the oldMemoryPool.cpp.### Testedstill has the template placeholders (<passed / total, skipped; ...>,<list any other failures ...>,<result>). Fill them with the real results on the final head.docs/dev/TechnicalDebt.md:600-601: there are two lines, the second starting with- - [MemoryPool](malformed). Keep one, and not as the last line of "Functional" (everyone appends there and it conflicts).- Behaviour changes need a source:
DirectMemoryFree/DirectMemoryRetypenow ignore invalid ranges, and a negativesearchEndinLargestFreemeans unbounded. Say which reference each follows (FreeBSDsys/vm, KytyPS5, shadPS4) or record it under "Unknown function info". - The
conventionsjob is red; runpython3 tools/check_conventions.pyand fix what belongs to your commits.
Happy to re-review once the build configures again.
Removes a stray markdown list character and updates the unknown function info to accurately reflect that invalid ranges are now rejected, leaving only the unbounded negative searchEnd documentation.
|
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.
|
This was referenced Oct 11, 2026
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
Hardens
PhysicalMemoryPoolincore/libs/prx/libkernel/DirectMemory/MemoryPool.cpp:start/lencan no longer reach_used[].Alloceffectively did before.Alloc,Free/CheckedFreeandRetyperoll back the pool's own metadata (_blocksand_used[]) when node allocation or backing creation throws. Blocks are split in place (_splitAt/_unsplitAtplus a rangeerase) and split first, so every step that can throw runs before any state is committed.DirectMemoryLargestFree, now validated.Free;CheckedFreestill requires every page in range to be allocated;Retypechanges allocated pages and skips gaps.DirectMemoryAlloccatchesstd::bad_alloc(toSCE_KERNEL_ERROR_ENOMEM). Every other entry point lets exceptions propagate. A mismatch between_blocksand_used[]throwsstd::logic_errorinstead of returning a plausible error code.MemoryPoolTest, registered in ctest, and updates theTechnicalDebt.mdentry for MemoryPool.Why
Free,Retypeand the page-marking helpers did no range checks, andcur + len,start + align - 1andstart + lencould overflow, so guest input could write past_used[]or create overlapping allocations.Alloc,FreeandRetypecould throw part-way (bad_alloc, backing cleanup) and leave_used[]and_blocksout of sync.false,0orEFAULT, which hides emulator invariant failures.Behaviour changes
EINVAL/EFAULTstartinFree/CheckedFree/RetypeFreeon a range with nothing allocatedForgetDirectMemoryFree/Retypepropagated, others swallowedbad_allocinAlloc(toENOMEM)LargestFreewith negativesearchEndKnown limitation
The rollback covers the pool's own metadata only.
ForgetDirectMemoryerasesg_physPagesbeforeErasePhysMappings, andErasePhysMappingscan throw while inserting the surviving pieces of a partially overlapped mapping. In that case the pool restores its splits, but the already-erased physical pages and mapping nodes are not restored, so a retry may failValidatePhysicalRange. Making those updates transactional is tracked inTechnicalDebt.mdand is not part of this PR.Tested
New tests (
MemoryPoolTest): 14 cases covering argument validation, rounding/alignment/clamping, partial and spanningFree,Retypesplitting, and rollback of_blocks/_used[]after injectedbad_allocand backing/forget failures. Run per case against the previousMemoryPool.cpp, 9 fail or crash; all 14 pass after the change (ASan + UBSan, C++11 and C++17, mockForgetDirectMemoryandCreateDirectMemoryBacking).Environment: Windows 11 Home (26100), MinGW-w64 GCC 15.2.0 (WinLibs r7), Release.
Full suite: 550 of 554 tests passed, 1 skipped.
agc_driver_prepared_shaders_debug_failurefails on cleanmainon this machine.guest_module_identityfailed once but passed when rerun alone.CTestTestfile.cmakecommands because local antivirus blocksctest.exe.tools/check_conventions.py: Passed with 0 warnings.Checklist
main(rebased, no merge commits); no other open PR implements the same functions