Fix use-after-free in UMS Filesystem name/mount_name#18
Open
crux161 wants to merge 2 commits into
Open
Conversation
Filesystem stored name/mount_name as std::string_view referencing strings owned by UmsController::Device. libusbhsfs's populate callback clears the devices vector on every event (partition discovery, mount/unmount), invalidating those views. Subsequent file access via mount_name.data() triggered a use-after-free and crashed when accessing USB storage. Make Filesystem own its strings (std::string). Also fix iterator invalidation in ums_devices_changed_cb where std::erase ran on the vector being range-iterated; switch to std::erase_if and only reset cur_fs if it actually pointed to the removed device.
libusbhsfs's populate callback runs on its USB-event thread. The previous code mutated context.filesystems / cur_fs directly from that thread, racing with main-thread reads from UI code. Stash the new device list under a mutex; have the main loops (menu_loop, video_loop) drain and apply pending changes once per frame so context.filesystems stays single-threaded for readers. Also handle USB unplug during playback: when the filesystem hosting the currently-playing file disappears, async-stop mpv and break out of video_loop with EIO so the user falls back to the menu cleanly. (Note: this does not eliminate crashes when mpv's demuxer thread is mid-read at the moment of physical removal -- that race is inside libusbhsfs and would require upstream changes to fix.)
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.
Summary
Fixes a crash when accessing files from USB storage (USB key, external drive) on Switch, plus a follow-up thread-safety hardening of the UMS device callback.
Crash root cause
Filesystem::nameandFilesystem::mount_name(src/fs/fs_common.hpp) arestd::string_view. For USB devices, those views point intostd::stringmembers ofUmsController::Device.libusbhsfs's populate callback (
UmsController::usbhsfs_populate_cb) doesself->devices.clear()then re-populates on every event — partition discovery, mount, unmount. That destroys the strings the existingFilesystemviews reference, leaving them dangling. Subsequent file access viamount_name.data()(e.g. through devoptab lookup) triggers a use-after-free and crashes.Fixes in this PR
1. Own the strings (commit 1)
Filesystem::name/mount_namenow own their strings (std::stringinstead ofstd::string_view). USBFilesysteminstances surviveUmsController::devices.clear(). The C++23 build (gnu++23) makes the existingthis->name = svassignments in network FS subclasses still compile unchanged.ums_devices_changed_cbno longer callsstd::eraseoncontext.filesystemswhile range-iterating it (was UB). Switched tostd::erase_if, andcur_fsis only reset when it actually pointed to a removed device.2. Defer UMS updates to the main thread (commit 2)
context.filesystems/cur_fswhile main-thread UI code was reading them. The callback now only stashes a pending device list under a mutex;apply_pending_ums_changesdrains and applies it once per frame frommenu_loopandvideo_loop, keepingcontext.filesystemssingle-threaded for readers.video_loopwithEIOso the user falls back to the menu cleanly.Test plan
Known remaining limitation
Physical USB removal while mpv's demuxer thread is mid-read still crashes inside libusbhsfs's internal teardown — that race lives below SwitchWave and would require upstream changes in libusbhsfs to defer cleanup until in-flight reads drain. Not addressed here.