From 375fa0fc113011242a988feea26d79cba62e9489 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Sun, 30 Aug 2026 22:49:33 +0200 Subject: [PATCH] Misc correctness fixes GetStringErrorMsg had the strerror_r result test backwards. The XSI variant returns 0 on success, so every successful lookup returned "Unknown error"; and under glibc with _GNU_SOURCE the GNU variant is selected instead, which returns the message by pointer and typically leaves the buffer untouched, so it returned an empty string. Either way GetLastErrorMsg() was useless on Linux, Android and macOS. Pick the right handling by overload resolution rather than guessing which signature we got. KeyMap's "no gamepad button mapped to cancel" fallback pushed into confirmKeys instead of cancelKeys - and pushed the confirm button. So unmapping cancel left no gamepad way out of menus, and duplicated an entry in the confirm list. ControlMapper::AddListener mutated listeners_ without taking mutex_, while RemoveListener takes it and the input thread iterates the vector under it. Opening a screen while an axis is moving could reallocate it mid-iteration. The comment about piggybacking on a screenmanager mutex was stale - there isn't one. Config's two std::stof calls on PostShaderSetting values ran on user-editable ini text with no try/catch, so a malformed entry called std::terminate during startup config load. Use the same checked sscanf that LoadGameConfig already uses. (CmdLine.cpp and Compatibility.cpp have the same pattern; not touched here.) The screenshot downscale path leaked its final buffer on every downscaled shot, which savestate thumbnails hit on every save at 3x and above. HandleUploadPost is registered unconditionally, so closing the Upload screen left an unauthenticated file-write endpoint live for as long as anything else kept the server up. Check the flag in the handler. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01DCPmm7FoQUoqrbMdhfqhQ2 --- Common/SysError.cpp | 18 +++++++++++++----- Core/Config.cpp | 16 ++++++++++++++-- Core/ControlMapper.cpp | 5 +++++ Core/ControlMapper.h | 11 +++++------ Core/KeyMap.cpp | 3 ++- Core/Screenshot.cpp | 1 + Core/WebServer.cpp | 8 ++++++++ 7 files changed, 48 insertions(+), 14 deletions(-) diff --git a/Common/SysError.cpp b/Common/SysError.cpp index 993e6ca307..cdeea6a2cb 100644 --- a/Common/SysError.cpp +++ b/Common/SysError.cpp @@ -25,6 +25,15 @@ #include "CommonWindows.h" #else #include + +// See the comment at the call site. +static std::string StrErrorResult(char *result, const char *buf) { + return result ? result : "Unknown error"; +} + +static std::string StrErrorResult(int result, const char *buf) { + return result == 0 ? buf : "Unknown error"; +} #endif // Generic function to get last error message. @@ -60,10 +69,9 @@ std::string GetStringErrorMsg(int errCode) { #else char err_str[buff_size] = {}; - // Thread safe (XSI-compliant) - if (strerror_r(errCode, err_str, buff_size) == 0) { - return "Unknown error"; - } - return err_str; + // strerror_r has two incompatible signatures: the XSI one returns int (0 on success and the + // message is in the buffer), the GNU one returns a char * that may not be the buffer at all. + // Which one we get depends on _GNU_SOURCE, so let overload resolution sort it out. + return StrErrorResult(strerror_r(errCode, err_str, buff_size), err_str); #endif } diff --git a/Core/Config.cpp b/Core/Config.cpp index 2fd6932c93..48e3fb656f 100644 --- a/Core/Config.cpp +++ b/Core/Config.cpp @@ -1382,7 +1382,14 @@ void Config::Load(const char *iniFileName, const char *controllerIniFilename) { // Load post process shader values mPostShaderSetting.clear(); for (const auto &[key, value] : postShaderSetting->ToMap()) { - mPostShaderSetting[key] = std::stof(value); + // The ini is user-editable, and std::stof throws - which would take the process down + // during startup config load. LoadGameConfig already parses this section this way. + float f = 0.0f; + if (sscanf(value.c_str(), "%f", &f) == 1) { + mPostShaderSetting[key] = f; + } else { + WARN_LOG(Log::Config, "Invalid float value string for param %s: '%s'", key.c_str(), value.c_str()); + } } const Section *hostOverrideSetting = iniFile.GetOrCreateSection("HostAliases"); @@ -1955,7 +1962,12 @@ void Config::UnloadGameConfig() { auto postShaderSetting = iniFile.GetOrCreateSection("PostShaderSetting")->ToMap(); mPostShaderSetting.clear(); for (const auto &[k, v] : postShaderSetting) { - mPostShaderSetting[k] = std::stof(v); + float f = 0.0f; + if (sscanf(v.c_str(), "%f", &f) == 1) { + mPostShaderSetting[k] = f; + } else { + WARN_LOG(Log::Config, "Invalid float value string for param %s: '%s'", k.c_str(), v.c_str()); + } } auto postShaderChain = iniFile.GetOrCreateSection("PostShaderList")->ToMap(); diff --git a/Core/ControlMapper.cpp b/Core/ControlMapper.cpp index d939b3c64f..3da17c8c6d 100644 --- a/Core/ControlMapper.cpp +++ b/Core/ControlMapper.cpp @@ -833,6 +833,11 @@ void ControlMapper::GetDebugString(StringWriter &w) const { w.F("Rstick: %f, %f\n", converted_[1][0], converted_[1][1]); } +void ControlMapper::AddListener(ControlListener *listener) { + std::lock_guard guard(mutex_); + listeners_.push_back(listener); +} + void ControlMapper::RemoveListener(ControlListener *listener) { std::lock_guard guard(mutex_); auto it = std::find(listeners_.begin(), listeners_.end(), listener); diff --git a/Core/ControlMapper.h b/Core/ControlMapper.h index 70a65fae04..d28f526b45 100644 --- a/Core/ControlMapper.h +++ b/Core/ControlMapper.h @@ -38,9 +38,9 @@ public: // Required callbacks. // TODO: These are so many now that a virtual interface might be more appropriate.. - void AddListener(ControlListener *listener) { - listeners_.push_back(listener); - } + // Both of these take mutex_ - listeners_ is iterated on the input thread, and screens add and + // remove themselves from another one. + void AddListener(ControlListener *listener); void RemoveListener(ControlListener *listener); // Inject raw PSP key input directly, such as from touch screen controls. @@ -118,9 +118,8 @@ private: int iInternalScreenRotationCached_ = 0; - // Protects basically all the state. - // TODO: Maybe we should piggyback on the screenmanager mutex - it's always locked - // when events come in here. + // Protects basically all the state. (There is no screenmanager mutex to piggyback on, despite + // what a previous comment here claimed - input arrives on its own thread.) std::mutex mutex_; std::map curInput_; diff --git a/Core/KeyMap.cpp b/Core/KeyMap.cpp index 7e3140948f..b9b060a3bd 100644 --- a/Core/KeyMap.cpp +++ b/Core/KeyMap.cpp @@ -185,7 +185,8 @@ void UpdateNativeMenuKeys() { InsertIntoVector(&cancelKeys, hardcodedCancelKeys[i]); } if (!HasMainButtonMapping(cancelKeys)) { - confirmKeys.push_back(InputMapping(DEVICE_ID_ANY, confirmWithCross ? NKCODE_BUTTON_A : NKCODE_BUTTON_B)); + // This used to push the confirm button into confirmKeys - wrong list, wrong button. + cancelKeys.push_back(InputMapping(DEVICE_ID_ANY, confirmWithCross ? NKCODE_BUTTON_B : NKCODE_BUTTON_A)); } const InputMapping hardcodedInfoKeys[] = { diff --git a/Core/Screenshot.cpp b/Core/Screenshot.cpp index 86f1b12bd7..38662b2a72 100644 --- a/Core/Screenshot.cpp +++ b/Core/Screenshot.cpp @@ -427,6 +427,7 @@ static void SaveScreenshotAsync(GPUDebugBuffer &&buf, int w, int h, int maxRes) height /= 2; } result = Save888RGBScreenshot(filename, fmt, shrinkBuffer, width, height) ? ScreenshotResult::Success : ScreenshotResult::FailedToWriteFile; + delete[] shrinkBuffer; } System_RunOnMainThread([result, callback = std::move(callback)]() { diff --git a/Core/WebServer.cpp b/Core/WebServer.cpp index e2b99c5f35..0f5439e8b5 100644 --- a/Core/WebServer.cpp +++ b/Core/WebServer.cpp @@ -702,6 +702,14 @@ static void HandleUploadPost(const http::ServerRequest &request) { return; } + // This handler is registered unconditionally, so it has to check the flag itself - otherwise + // closing the Upload screen leaves an unauthenticated write endpoint live for as long as + // anything else (remote ISO, the debugger) keeps the server up. + if (!(serverFlags & WebServerFlags::FILE_UPLOAD)) { + ERROR_LOG(Log::HTTP, "Upload requested, but uploading isn't enabled"); + return; + } + Path uploadPath; { std::lock_guard guard(g_webServerLock);