Repository navigation
fix(libkernel): protect app metadata state across threads and validate icon stream read - #5704
Conversation
adriadam10
left a comment
There was a problem hiding this comment.
Thanks, the locking in ensureTitleLoaded/ensureIconLoaded is right: every reader goes through them, so the mutex covers the globals. Points before this can merge:
- No test for any of the changes. The icon checks (negative/
tellgfailure, short read, oversize file) are reachable throughHasAppIcon/GetAppIconDatawith a fixtureicon0.pngin a guest test registered in ctest; each check must fail without the change. "Tested" lists onlyparam_json_utf8_tests, which doesn't touch this file. AppMetadata.cpp:31: the!destination || destinationSize == 0guard can't trigger; both callers passg_title/g_titleIdwithsizeof. Drop it or show the call that reaches it.AppMetadata.cpp:85: the 64 MiB cap has no source. Say where the limit comes from (real icon0.png sizes, FreeBSD/PS5 reference), or drop it: thetellg() <= 0check already removes the wrap intobad_alloc, which is the case described in the body.
Welcome to push these and I'll look again.
…e icon stream read
8b5ba55 to
c836b3b
Compare
|
@adriadam10 Thanks for the review! Addressed all points in the latest push:
Rebased on latest |
|
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.
|
adriadam10
left a comment
There was a problem hiding this comment.
Checked the re-push: the mutex covers every reader of the title/icon globals, the unreachable guard and the unsourced cap are gone, and guest_app_icon is registered in ctest. It passes here (build + ctest, 174 tests) and fails when AppMetadata.cpp is reverted to main. Merging once CI is green.
What
ensureTitleLoadedandensureIconLoadedinAppMetadata.cppwithstd::mutex g_metadataMutexto prevent concurrent guest threads from racing on title/titleId/param initialization and icon buffer allocation.file.tellg()results on error or unseekable streams (file.fail() || fileSize <= 0), preventing integer wrap intostd::bad_alloc.file.gcount()and stream state after reading icon data to ensure the buffer is fully read before PNG header validation.guest_app_icon_tests(core/libs/tests/GuestAppIcon.cpp) registered in ctest, verifying:HasAppIconand throws onGetAppIconDatatellg() <= 0) throws the expected errorcopyToFixedBufferand dropped unsourced 64 MiB size cap per review feedback.Tested
guest_app_icon_testspassing all sub-cases (--no-icon,--valid-icon,--empty-icon,--corrupt-png,--concurrent).python tools/check_conventions.py --base origin/main.git diff --checkpassed cleanly with 0 errors.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)