Merge pull request #22191 from hrydgard/memarena-and-bounds-fixes

MemArena and bounds fixes

AGENTS.md

@@ -15,10 +15,28 @@ Ignore the folder ai_instructions in the root directory, it's old stuff from con
    `newline=''` silently converts the whole file, turning a two-line addition into a 5000-line diff. Check
    `git diff --stat` before committing - a whole-file rewrite is obvious there and invisible in the editor.
    Prefer the Edit tool, which does exact string replacement and can't do this.
-5. **Don't feed Python to `bash -c` via a heredoc when the code contains backslashes.** The quoting mangles them,
-   and an anchor string like `'...MemBlockInfo.cpp \\\r\n'` silently fails to match, so the patch reports
-   "anchor missing" for reasons that aren't visible. Write the script to a file and run that instead, building
-   separators with `chr(92)` if need be.
+5. **Don't feed Python to `bash -c` via a heredoc when the code contains backslashes.** The Git Bash / MinGW
+   layer strips one level of backslash escaping on the way in, *even with a quoted delimiter* (`<<'PY'`), which
+   normally suppresses all substitution. So the script Python receives is not the one you wrote:
+
+   | You write in the heredoc | Python actually sees | Result |
+   |---|---|---|
+   | `"\r\n"` | `"\r\n"` | fine - survives, because you *want* Python to interpret it |
+   | `"\\n"` (intending a literal `\n` in the output) | `"\n"` | **a real newline is written into the file** |
+   | `'foo \\\r\n'` as a match anchor | `'foo \<CR><LF>'` | **anchor silently doesn't match**, reported as "anchor missing" |
+
+   The tell for the first case is a compiler error like `C2001: newline in string literal`; the second case
+   produces no error at all, just a patch that quietly did nothing. Both are invisible in the heredoc you wrote.
+
+   The rule: **a heredoc is fine as long as every backslash in the script is one you want Python to interpret.
+   The moment you need a literal backslash in the *output*, stop.** Then either:
+   - use the Edit tool instead (exact string replacement, no shell in the path - the best option for the common
+     case of "insert a few lines of C++ that contain `\n`"), or
+   - write the script to a file with the Write tool and run `python thescript.py`, or
+   - build the backslash as `chr(92)` so no literal backslash appears in the heredoc at all.
+
+   Note this is about the *Python source*, not the data: reading and rewriting a CRLF file with `newline=''` and
+   `\r\n` anchors works fine in a heredoc, and is the normal way to patch files here (see rule 4).
 
 ## Core Safety Checks
 

Common/MemArenaHorizon.cpp

@@ -50,11 +50,13 @@ void *MemArena::CreateView(s64 offset, size_t size, void *base) {
 	if (R_FAILED(rc)) {
 		printf("Fatal error creating the view... base: %p offset: %p size: %p src: %p err: %d\n",
 			   (void *)base, (void *)offset, (void *)size, (void *)(memoryCodeBase + offset), rc);
-	} else {
-		printf("Created the view... base: %p offset: %p size: %p src: %p err: %d\n",
-			   (void *)base, (void *)offset, (void *)size, (void *)(memoryCodeBase + offset), rc);
+		// Returning base here reports success, so the caller happily uses an unmapped address
+		// and we take a fault later with nothing pointing back at this.
+		return nullptr;
 	}
 
+	printf("Created the view... base: %p offset: %p size: %p src: %p err: %d\n",
+		   (void *)base, (void *)offset, (void *)size, (void *)(memoryCodeBase + offset), rc);
 	return base;
 }
 

Common/MemArenaPosix.cpp

@@ -97,7 +97,11 @@ bool MemArena::GrabMemSpace(size_t size) {
 	}
 	if (ftruncate(fd, size) != 0) {
 		ERROR_LOG(Log::MemMap, "Failed to ftruncate %d (%s) to size %08x", (int)fd, ram_temp_file.c_str(), (int)size);
-		// Should this be a failure?
+		// This is a failure: the mmaps below succeed against a short file, and touching a page past
+		// its end raises SIGBUS - which is only hooked on __APPLE__, so elsewhere it's a bare crash.
+		close(fd);
+		fd = -1;
+		return false;
 	}
 #endif
 	return true;

Common/MemArenaWin32.cpp

@@ -52,6 +52,16 @@ void *MemArena::CreateView(s64 offset, size_t size, void *viewbase) {
 	size = roundup(size);
 #if PPSSPP_PLATFORM(UWP)
 	// We just grabbed some RAM before using RESERVE. This commits it.
+	//
+	// NOTE: This ignores offset, so views don't alias - each gets its own storage. UWP defines
+	// MASKED_PSP_MEMORY, which folds the uncached and kernel address bits away before they get
+	// here, so the only casualties are the three VRAM mirrors at 0x04200000/0x04400000/0x04600000,
+	// which practically nothing depends on.
+	//
+	// Don't try to fix this with placeholders: CreateFileMappingFromApp + VirtualAlloc2FromApp
+	// (MEM_RESERVE_PLACEHOLDER) + MapViewOfFile3FromApp compiles and is available on the targeted
+	// SDK, but the very first reservation fails at runtime with ERROR_INVALID_ADDRESS (487) - not
+	// a per-view problem, the app container just won't hand out placeholders. Tried and reverted.
 	void *ptr = VirtualAllocFromApp(viewbase, size, MEM_COMMIT, PAGE_READWRITE);
 #else
 	void *ptr = MapViewOfFileEx(hMemoryMapping, FILE_MAP_ALL_ACCESS, 0, (DWORD)((u64)offset), size, viewbase);

Core/CmdLine.cpp

@@ -72,11 +72,26 @@ ParseParamResult SetValue(CommandLineOptions *options, const CommandLineParam &p
 		break;
 	}
 	case CmdParamType::Int:
-		*reinterpret_cast<std::optional<int> *>(optionsPtr + param.offsetInStruct) = std::stoi(value);
+	{
+		// Not std::stoi - that throws on junk, which takes the process down with no diagnostic.
+		int v;
+		if (sscanf(value.c_str(), "%d", &v) != 1) {
+			PRINT_STDERR("Error: Invalid value for integer parameter --%s: '%s'.\n", param.longName, value.c_str());
+			return ParseParamResult::BadValue;
+		}
+		*reinterpret_cast<std::optional<int> *>(optionsPtr + param.offsetInStruct) = v;
 		break;
+	}
 	case CmdParamType::Double:
-		*reinterpret_cast<std::optional<double> *>(optionsPtr + param.offsetInStruct) = std::stod(value);
+	{
+		double v;
+		if (sscanf(value.c_str(), "%lf", &v) != 1) {
+			PRINT_STDERR("Error: Invalid value for number parameter --%s: '%s'.\n", param.longName, value.c_str());
+			return ParseParamResult::BadValue;
+		}
+		*reinterpret_cast<std::optional<double> *>(optionsPtr + param.offsetInStruct) = v;
 		break;
+	}
 	case CmdParamType::String:
 		*reinterpret_cast<std::optional<std::string> *>(optionsPtr + param.offsetInStruct) = value;
 		break;

Core/Compatibility.cpp

@@ -213,7 +213,13 @@ void Compatibility::CheckSetting(IniFile &iniFile, const std::string &gameID, co
 	std::string value;
 	Section *section = iniFile.GetSection(option);
 	if (section && section->Get(gameID.c_str(), &value)) {
-		*flag = stof(value);
+		// Not stof - it throws on a malformed entry, and compat.ini is user-editable.
+		float parsed;
+		if (sscanf(value.c_str(), "%f", &parsed) != 1) {
+			WARN_LOG(Log::Loader, "compat.ini: [%s] %s is not a number: '%s'", option, gameID.c_str(), value.c_str());
+			return;
+		}
+		*flag = parsed;
 
 		if (!activeList_.empty()) {
 			activeList_ += "\n";
@@ -226,7 +232,12 @@ void Compatibility::CheckSetting(IniFile &iniFile, const std::string &gameID, co
 	std::string value;
 	Section *section = iniFile.GetSection(option);
 	if (section && section->Get(gameID.c_str(), &value)) {
-		*flag = stoi(value);
+		int parsed;
+		if (sscanf(value.c_str(), "%d", &parsed) != 1) {
+			WARN_LOG(Log::Loader, "compat.ini: [%s] %s is not an integer: '%s'", option, gameID.c_str(), value.c_str());
+			return;
+		}
+		*flag = parsed;
 
 		if (!activeList_.empty()) {
 			activeList_ += ":" + std::to_string(*flag) + "\n";

GPU/Common/DrawEngineCommon.cpp

@@ -228,6 +228,13 @@ bool DrawEngineCommon::TestBoundingBox(const void *vdata, const void *inds, int
 
 			if (vertexCount > 0 && inds) {
 				GetIndexBounds(inds, vertexCount, vertType, &indexLowerBound, &indexUpperBound);
+				if (indexUpperBound > 1024) {
+					// NormalizeVertices below writes indexUpperBound - indexLowerBound + 1 vertices
+					// into corners, which only has room until the verts region above it. The index
+					// values are the game's, so the vertexCount cap doesn't bound them. A bbox test
+					// over this many verts is counter-productive anyway - say it's visible.
+					return true;
+				}
 			}
 			// TODO: Avoid normalization if just plain skinning.
 			const u32 vertTypeID = GetVertTypeID(vertType, gstate.getUVGenMode());

GPU/Common/PostShader.cpp

@@ -163,6 +163,12 @@ void LoadPostShaderInfo(Draw::DrawContext *draw, const std::vector<Path> &direct
 					section.Get("OutputResolution", &info.outputResolution);
 					section.Get("Upscaling", &info.isUpscalingFilter);
 					section.Get("SSAA", &info.SSAAFilterLevel);
+					if (info.SSAAFilterLevel < 0 || info.SSAAFilterLevel > 8) {
+						// It multiplies the render resolution, and shader inis come from downloads -
+						// the neighbouring texture-shader "Scale" is bounded for the same reason.
+						WARN_LOG(Log::G3D, "Ignoring out-of-range SSAA level %d in shader '%s'", info.SSAAFilterLevel, info.section.c_str());
+						info.SSAAFilterLevel = 0;
+					}
 					section.Get("60fps", &info.requires60fps);
 					section.Get("UsePreviousFrame", &info.usePreviousFrame);
 

GPU/Common/TextureReplacer.cpp

@@ -490,6 +490,12 @@ void TextureReplacer::ParseHashRange(const std::string &key, const std::string &
 		return;
 	}
 
+	if (toW == 0 || toH == 0) {
+		// These end up as desc_.newW/newH, which ReplacedTexture::Prepare divides by.
+		ERROR_LOG(Log::TexReplacement, "Ignoring invalid hashrange %s = %s, range is empty", key.c_str(), value.c_str());
+		return;
+	}
+
 	const u64 rangeKey = ((u64)addr << 32) | ((u64)fromW << 16) | fromH;
 	hashranges_[rangeKey] = WidthHeightPair(toW, toH);
 }

UI/CustomButtonMappingScreen.cpp

@@ -138,6 +138,9 @@ void CustomButtonMappingScreen::CreateDialogViews(UI::ViewGroup *parent) {
 	bool *show = nullptr;
 	memset(array, 0, sizeof(array));
 	cfg = &g_Config.CustomButton[id_];
+	// This screen is reachable from the main menu, so neither GamepadEmu nor the layout screen
+	// need have sanitized these yet - and everything below indexes the tables with them.
+	CustomKeyData::Sanitize(*cfg);
 	show = &touch.touchCustom[id_].show;
 	for (int i = 0; i < ARRAY_SIZE(g_customKeyList); i++)
 		array[i] = (0x01 == ((g_Config.CustomButton[id_].key >> i) & 0x01));

UI/GamepadEmu.cpp

@@ -1106,12 +1106,7 @@ GamepadEmuView::GamepadEmuView(const TouchControlConfig &config, float xres, flo
 
 	// Sanitize custom button images, while adding them.
 	for (int i = 0; i < TouchControlConfig::CUSTOM_BUTTON_COUNT; i++) {
-		if (g_Config.CustomButton[i].shape >= ARRAY_SIZE(CustomKeyData::customKeyShapes)) {
-			g_Config.CustomButton[i].shape = 0;
-		}
-		if (g_Config.CustomButton[i].image >= ARRAY_SIZE(CustomKeyData::customKeyImages)) {
-			g_Config.CustomButton[i].image = 0;
-		}
+		CustomKeyData::Sanitize(g_Config.CustomButton[i]);
 
 		char temp[64];
 		snprintf(temp, sizeof(temp), "Custom %d button", i + 1);

UI/GamepadEmu.h

@@ -22,6 +22,7 @@
 
 #include "Common/UI/View.h"
 #include "Common/UI/ViewGroup.h"
+#include "Core/ConfigValues.h"
 #include "Core/CoreParameter.h"
 #include "Core/HLE/sceCtrl.h"
 #include "UI/EmuScreen.h"
@@ -366,6 +367,17 @@ namespace CustomKeyData {
 		// IMPORTANT: Only add at the end!
 	};
 	static_assert(ARRAY_SIZE(g_customKeyList) <= 64, "Too many key for a uint64_t bit mask");
+
+	// image and shape come straight from the ini and index the tables above, so anything that
+	// reads them has to run this first.
+	inline void Sanitize(ConfigCustomButton &cfg) {
+		if (cfg.image < 0 || cfg.image >= (int)ARRAY_SIZE(customKeyImages)) {
+			cfg.image = 0;
+		}
+		if (cfg.shape < 0 || cfg.shape >= (int)ARRAY_SIZE(customKeyShapes)) {
+			cfg.shape = 0;
+		}
+	}
 };
 
 // Gesture key only have virtual button that can work without constant press

UI/TouchControlLayoutScreen.cpp

@@ -537,12 +537,7 @@ void ControlLayoutView::CreateViews() {
 
 	for (int i = 0; i < TouchControlConfig::CUSTOM_BUTTON_COUNT; i++) {
 		// Similar to GamepadEmu, we sanitize the images for valid values.
-		if (g_Config.CustomButton[i].shape >= ARRAY_SIZE(CustomKeyData::customKeyShapes)) {
-			g_Config.CustomButton[i].shape = 0;
-		}
-		if (g_Config.CustomButton[i].image >= ARRAY_SIZE(CustomKeyData::customKeyImages)) {
-			g_Config.CustomButton[i].image = 0;
-		}
+		CustomKeyData::Sanitize(g_Config.CustomButton[i]);
 
 		char temp[64];
 		snprintf(temp, sizeof(temp), "Custom %d button", i);