Skip to content

Fix use-after-free in UMS Filesystem name/mount_name#18

Open
crux161 wants to merge 2 commits into
averne:masterfrom
crux161:fix/ums-dangling-string-view
Open

Fix use-after-free in UMS Filesystem name/mount_name#18
crux161 wants to merge 2 commits into
averne:masterfrom
crux161:fix/ums-dangling-string-view

Conversation

@crux161
Copy link
Copy Markdown

@crux161 crux161 commented May 5, 2026

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::name and Filesystem::mount_name (src/fs/fs_common.hpp) are std::string_view. For USB devices, those views point into std::string members of UmsController::Device.

libusbhsfs's populate callback (UmsController::usbhsfs_populate_cb) does self->devices.clear() then re-populates on every event — partition discovery, mount, unmount. That destroys the strings the existing Filesystem views reference, leaving them dangling. Subsequent file access via mount_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_name now own their strings (std::string instead of std::string_view). USB Filesystem instances survive UmsController::devices.clear(). The C++23 build (gnu++23) makes the existing this->name = sv assignments in network FS subclasses still compile unchanged.
  • ums_devices_changed_cb no longer calls std::erase on context.filesystems while range-iterating it (was UB). Switched to std::erase_if, and cur_fs is only reset when it actually pointed to a removed device.

2. Defer UMS updates to the main thread (commit 2)

  • libusbhsfs invokes the populate callback on its USB-event thread, which previously mutated context.filesystems / cur_fs while main-thread UI code was reading them. The callback now only stashes a pending device list under a mutex; apply_pending_ums_changes drains and applies it once per frame from menu_loop and video_loop, keeping context.filesystems single-threaded for readers.
  • When the filesystem hosting the currently-playing file disappears (USB unplugged mid-playback), async-stop mpv and break out of video_loop with EIO so the user falls back to the menu cleanly.

Test plan

  • Built and tested on real Switch hardware.
  • USB-key file access no longer crashes.
  • Unplug while in the menu: device cleanly disappears from the list, currently-listed files become inaccessible with a clean error rather than a crash.
  • Unplug while paused / between reads in playback: drops back to menu cleanly.
  • Network filesystems and SD card unaffected (regression check).

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.

crux161 added 2 commits May 5, 2026 10:38
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.)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant