Merge pull request #22642 from hrydgard/cwcheat-malformed

CwCheat: Ignore invisible characters, and show parse errors

AGENTS.md

@@ -358,6 +358,10 @@ private:
 
 But generally follow the surrounding style. Braces are preferred on the same line. Braces are always used even when they could be omitted due the inner part being just a single line.
 
+If a large `else {}` block can be avoided with an early return when writing new code, do that
+instead. Don't wrap existing code in a new `else` and reindent it, either - it makes the diff
+much bigger than the change.
+
 ### Comments
 
 Keep comments tight. Say the thing once; don't restate what the code already shows, and don't

Common/StringUtils.cpp

@@ -38,6 +38,7 @@
 #include <cctype>
 
 #include "Common/Buffer.h"
+#include "Common/Data/Encoding/Utf8.h"
 #include "Common/StringUtils.h"
 
 size_t truncate_cpy(char *dest, size_t destSize, const char *src) {
@@ -105,9 +106,44 @@ int CountChar(std::string_view haystack, char needle) {
 	return count;
 }
 
+static void ApplyLengthLimits(std::string *str, int minLength, int maxLength) {
+	if (minLength > 0) {
+		if ((int)str->size() < minLength) {
+			// Just reject it by returning an empty string, as we can't really
+			// conjure up new characters here.
+			str->clear();
+			return;
+		}
+	}
+
+	if (maxLength >= 0) {
+		// TODO: Cut at whole UTF-8 chars!
+		if ((int)str->size() > maxLength) {
+			str->resize(maxLength);
+		}
+	}
+}
+
 std::string SanitizeString(std::string_view input, StringRestriction restriction, int minLength, int maxLength) {
 	if (restriction == StringRestriction::None) {
 		return std::string(input);
+	} else if (restriction == StringRestriction::NoInvisibleFormatting) {
+		std::string out;
+		out.reserve(input.size());
+		UTF8 utf(input);
+		while (!utf.end()) {
+			const int start = utf.byteIndex();
+			const uint32_t c = utf.next();
+			if (c == 0x00A0 || c == 0x202F) {
+				out.push_back(' ');
+			} else if (c == 0x00AD || (c >= 0x200B && c <= 0x200F) || (c >= 0x202A && c <= 0x202E) || (c >= 0x2060 && c <= 0x206F) || c == 0xFEFF) {
+				// Soft hyphen, zero-width characters, direction marks, embeddings and isolates, BOM.
+			} else {
+				out.append(input.data() + start, utf.byteIndex() - start);
+			}
+		}
+		ApplyLengthLimits(&out, minLength, maxLength);
+		return out;
 	}
 	// First, remove any chars not in A-Za-z0-9_-. This will effectively get rid of any Unicode char, emojis etc too.
 	std::string sanitized;
@@ -116,6 +152,7 @@ std::string SanitizeString(std::string_view input, StringRestriction restriction
 	for (char c : input) {
 		switch (restriction) {
 		case StringRestriction::None:
+		case StringRestriction::NoInvisibleFormatting:
 			sanitized.push_back(c);
 			break;
 		case StringRestriction::AlphaNumDashUnderscore:
@@ -155,20 +192,7 @@ std::string SanitizeString(std::string_view input, StringRestriction restriction
 		}
 	}
 
-	if (minLength > 0) {
-		if ((int)sanitized.size() < minLength) {
-			// Just reject it by returning an empty string, as we can't really
-			// conjure up new characters here.
-			return std::string();
-		}
-	}
-
-	if (maxLength >= 0) {
-		// TODO: Cut at whole UTF-8 chars!
-		if ((int)sanitized.size() > maxLength) {
-			sanitized.resize(maxLength);
-		}
-	}
+	ApplyLengthLimits(&sanitized, minLength, maxLength);
 
 	if (restriction == StringRestriction::NoLineBreaksOrSpecials) {
 		// Additionally, cut off the string if we find an overlong UTF-8 character, such as in Jak & Daxter's title.

Common/StringUtils.h

@@ -91,6 +91,7 @@ enum class StringRestriction {
 	FileName,
 	NoLineBreaksOrSpecials,  // Used for savedata UI. Removes line breaks, backslashes and similar.
 	ConvertToUnixEndings,
+	NoInvisibleFormatting,  // Drops zero-width characters, direction marks and similar, turns no-break spaces into spaces.
 };
 
 std::string SanitizeString(std::string_view username, StringRestriction restriction, int minLength = 0, int maxLength = -1);

Core/CwCheat.cpp

@@ -68,10 +68,9 @@ bool CheatFileParser::Parse() {
 		if (!tempLine)
 			continue;
 
-		// Detect UTF-8 BOM sequence, and ignore it.
-		if (lineNumber == 1 && memcmp(tempLine, "\xEF\xBB\xBF", 3) == 0)
-			tempLine += 3;
-		std::string line = TrimString(tempLine);
+		// Cheats copied from websites can carry invisible characters (like direction marks) that would
+		// break the hex values. This also drops the UTF-8 BOM.
+		std::string line = TrimString(SanitizeString(tempLine, StringRestriction::NoInvisibleFormatting));
 
 		// Minimum length 5 is shortest possible _ lines name of the game "_G N+"
 		// and a minimum of 1 displayable character in cheat name string "_C0 1"
@@ -351,9 +350,12 @@ void CWCheatEngine::ParseCheats() {
 	cheats_ = parser.GetCheats();
 }
 
-std::vector<CheatFileInfo> CWCheatEngine::FileInfo() const {
+std::vector<CheatFileInfo> CWCheatEngine::FileInfo(std::vector<std::string> *errors) const {
 	CheatFileParser parser(filename_, gameID_);
 	parser.Parse();
+	if (errors) {
+		*errors = parser.GetErrors();
+	}
 	return parser.GetFileInfo();
 }
 

Core/CwCheat.h

@@ -57,7 +57,8 @@ struct CheatOperation;
 class CWCheatEngine {
 public:
 	CWCheatEngine(std::string_view gameID);
-	std::vector<CheatFileInfo> FileInfo() const;
+	// errors, if given, gets the lines that didn't parse.
+	std::vector<CheatFileInfo> FileInfo(std::vector<std::string> *errors = nullptr) const;
 	void ParseCheats();
 	void CreateCheatFile();
 	const Path &CheatFilename() const {

UI/CwCheatScreen.cpp

@@ -183,7 +183,7 @@ bool CwCheatScreen::TryLoadCheatInfo() {
 	}
 	fileCheckCounter_ = 0;
 
-	fileInfo_ = engine_->FileInfo();
+	fileInfo_ = engine_->FileInfo(&parseErrors_);
 
 	// Let's also trigger a reload, in case it changed.
 	g_Config.bReloadCheats = true;
@@ -286,6 +286,23 @@ void CwCheatScreen::CreateContentViews(UI::ViewGroup *parent) {
 		rightColumn->Add(new Spacer(8.0f));
 	}
 
+	if (!parseErrors_.empty()) {
+		// Cheats with a bad line are left out of the list, so say why.
+		const size_t maxShown = 5;
+		std::string details;
+		for (size_t i = 0; i < parseErrors_.size() && i < maxShown; i++) {
+			if (i > 0) {
+				details += '\n';
+			}
+			details += parseErrors_[i];
+		}
+		if (parseErrors_.size() > maxShown) {
+			details += "\n...";
+		}
+		rightColumn->Add(new NoticeView(NoticeLevel::WARN, cw->T("Some lines in the cheat file could not be read"), details));
+		rightColumn->Add(new Spacer(8.0f));
+	}
+
 	rightColumn->Add(new ItemHeader(cw->T("Cheats")));
 
 	bool prevIsTitle = false;

UI/CwCheatScreen.h

@@ -73,6 +73,7 @@ private:
 
 	CWCheatEngine *engine_ = nullptr;
 	std::vector<CheatFileInfo> fileInfo_;
+	std::vector<std::string> parseErrors_;
 	std::string gameID_;
 	int fileCheckCounter_ = 0;
 	uint64_t fileCheckHash_ = 0;

unittest/TestCwCheat.cpp

@@ -21,6 +21,7 @@
 
 #include "Common/File/FileUtil.h"
 #include "Common/File/Path.h"
+#include "Common/StringUtils.h"
 #include "Core/CwCheat.h"
 #include "Core/MemMap.h"
 #include "Core/MemMapHelpers.h"
@@ -213,6 +214,38 @@ static bool TestCheatParseErrors() {
 	return true;
 }
 
+// Cheats pasted from websites, with a BOM, no _S line and a U+202C (pop directional formatting)
+// after every address (#14955).
+static bool TestCheatInvisibleCharacters() {
+	const Path filename = UnitTestTempPath("unittest_cheats_invisible.ini");
+	const char *content = "\xEF\xBB\xBF\n_C1 Blade Change\xE2\x80\xAC\n_L 0x002F614A\xE2\x80\xAC 0x0000003D\n_L\xC2\xA0" "0x002F614B 0x00000018\n";
+	if (!File::WriteStringToFile(true, content, filename)) {
+		return false;
+	}
+	bool parsed;
+	std::vector<CheatCode> cheats;
+	std::vector<CheatFileInfo> info;
+	{
+		CheatFileParser parser(filename, "TEST00000");
+		parsed = parser.Parse();
+		cheats = parser.GetCheats();
+		info = parser.GetFileInfo();
+	}
+	File::Delete(filename);
+	EXPECT_TRUE(parsed);
+	EXPECT_EQ_INT((int)cheats.size(), 1);
+	EXPECT_EQ_INT((int)cheats[0].lines.size(), 2);
+	EXPECT_EQ_HEX(cheats[0].lines[0].part1, 0x002F614A);
+	EXPECT_EQ_HEX(cheats[0].lines[1].part2, 0x00000018);
+	EXPECT_EQ_INT((int)info.size(), 1);
+	EXPECT_EQ_STR(info[0].name, std::string("Blade Change"));
+
+	// The length limits still apply, after the characters are dropped.
+	EXPECT_EQ_STR(SanitizeString("a\xE2\x80\x8B" "bcd", StringRestriction::NoInvisibleFormatting, 0, 3), std::string("abc"));
+	EXPECT_EQ_STR(SanitizeString("a\xE2\x80\x8B" "b", StringRestriction::NoInvisibleFormatting, 3), std::string());
+	return true;
+}
+
 bool TestCwCheat() {
 	// Other tests may leave currentMIPS pointing elsewhere; the cheat engine needs the real one.
 	currentMIPS = &mipsr4k;
@@ -221,7 +254,7 @@ bool TestCwCheat() {
 		printf("Memory::Init failed\n");
 		return false;
 	}
-	const bool success = RunCheatTests() && TestCheatParseErrors();
+	const bool success = RunCheatTests() && TestCheatParseErrors() && TestCheatInvisibleCharacters();
 	Memory::Shutdown();
 	return success;
 }