Skip to content

Commit 6300509

Browse files
crux161averne
andauthored
Fix use-after-free in UMS Filesystem name/mount_name (#18)
* Fix use-after-free in UMS Filesystem name/mount_name 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. * Defer UMS device-list updates to the main thread 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.) * Fix indentation --------- Co-authored-by: averne <averne381@gmail.com>
1 parent cc4a21f commit 6300509

2 files changed

Lines changed: 48 additions & 7 deletions

File tree

src/fs/fs_common.hpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -165,7 +165,7 @@ class Filesystem {
165165

166166
public:
167167
Type type;
168-
std::string_view name, mount_name;
168+
std::string name, mount_name;
169169

170170
protected:
171171
devoptab_t devoptab = {};

src/main.cpp

Lines changed: 47 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -15,9 +15,12 @@
1515
// You should have received a copy of the GNU General Public License
1616
// along with SwitchWave. If not, see <http://www.gnu.org/licenses/>.
1717

18+
#include <cerrno>
1819
#include <cstdio>
1920
#include <chrono>
2021
#include <filesystem>
22+
#include <mutex>
23+
#include <optional>
2124
#include <string_view>
2225
#include <thread>
2326

@@ -137,23 +140,47 @@ void mpv_presetup() {
137140
std::printf("Dumped standard font\n");
138141
}
139142

143+
// libusbhsfs invokes the populate callback on its USB-event thread.
144+
// Stash the new device list under a mutex; the main thread applies
145+
// the change so context.filesystems / cur_fs stay single-threaded.
146+
std::mutex g_ums_pending_mtx;
147+
std::optional<std::vector<sw::fs::UmsController::Device>> g_ums_pending;
148+
140149
void ums_devices_changed_cb(const std::vector<sw::fs::UmsController::Device> &devices, void *user) {
141-
auto &context = *static_cast<sw::Context *>(user);
150+
auto lk = std::scoped_lock(g_ums_pending_mtx);
151+
g_ums_pending = devices;
152+
}
153+
154+
void apply_pending_ums_changes(sw::Context &context) {
155+
std::vector<sw::fs::UmsController::Device> devices;
156+
{
157+
auto lk = std::scoped_lock(g_ums_pending_mtx);
158+
if (!g_ums_pending)
159+
return;
160+
devices = std::move(*g_ums_pending);
161+
g_ums_pending.reset();
162+
}
142163

143164
// Remove unmounted devices
144-
for (auto &fs: context.filesystems) {
165+
bool removed_cur = false;
166+
std::erase_if(context.filesystems, [&](const auto &fs) {
145167
if (fs->type != sw::fs::Filesystem::Type::Usb)
146-
continue;
168+
return false;
147169

148170
auto it = std::find_if(devices.begin(), devices.end(), [&fs](const auto &dev) {
149171
return fs->mount_name == dev.mount_name;
150172
});
151173

152174
if (it == devices.end()) {
153-
std::erase(context.filesystems, fs);
154-
context.cur_fs = context.filesystems.front();
175+
if (context.cur_fs == fs)
176+
removed_cur = true;
177+
return true;
155178
}
156-
}
179+
return false;
180+
});
181+
182+
if (removed_cur && !context.filesystems.empty())
183+
context.cur_fs = context.filesystems.front();
157184

158185
// Add new devices
159186
for (auto &dev: devices) {
@@ -181,6 +208,8 @@ int menu_loop(sw::Renderer &renderer, sw::Context &context) {
181208
break;
182209
}
183210

211+
apply_pending_ums_changes(context);
212+
184213
padUpdate(&g_pad);
185214
auto has_touches = hidGetTouchScreenStates(&g_touch_state, 1);
186215
ImGui::nx::newFrame(&g_pad, has_touches ? &g_touch_state : nullptr);
@@ -277,6 +306,18 @@ int video_loop(sw::Renderer &renderer, sw::Context &context) {
277306
break;
278307
}
279308

309+
apply_pending_ums_changes(context);
310+
311+
// If the filesystem hosting the playing file was unplugged
312+
// (e.g. USB key removed mid-playback), stop mpv and bail out
313+
// of the player loop so it can't keep reading a dead device.
314+
if (!context.cur_file.empty() &&
315+
!context.get_filesystem(sw::fs::Path::mountpoint(context.cur_file))) {
316+
lmpv.command_async("stop");
317+
context.set_error(-EIO);
318+
break;
319+
}
320+
280321
padUpdate(&g_pad);
281322
auto has_touches = hidGetTouchScreenStates(&g_touch_state, 1);
282323
ImGui::nx::newFrame(&g_pad, has_touches ? &g_touch_state : nullptr);

0 commit comments

Comments
 (0)