Merge pull request #22174 from hrydgard/misc-correctness-fixes

Claude code review: Misc minor correctness fixes

Common/SysError.cpp

@@ -25,6 +25,15 @@
 #include "CommonWindows.h"
 #else
 #include <errno.h>
+
+// 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
 }

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();

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<std::mutex> guard(mutex_);
+	listeners_.push_back(listener);
+}
+
 void ControlMapper::RemoveListener(ControlListener *listener) {
 	std::lock_guard<std::mutex> guard(mutex_);
 	auto it = std::find(listeners_.begin(), listeners_.end(), listener);

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<InputMapping, InputSample> curInput_;

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[] = {

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)]() {

Core/WebServer.cpp

@@ -712,6 +712,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<std::mutex> guard(g_webServerLock);