AGI: Add bounds checking to strings

Fan games are known to use out of bounds string numbers
scummvm · Apr 16, 2024 · 2a66a9d · 2a66a9d
1 parent d907c5d
commit 2a66a9d
Show file tree

Hide file tree

Showing 6 changed files with 35 additions and 18 deletions.
diff --git a/engines/agi/agi.cpp b/engines/agi/agi.cpp
@@ -662,6 +662,25 @@ void AgiEngine::artificialDelayTrigger_DrawPicture(int16 newPictureNr) {
 	_artificialDelayCurrentPicture = newPictureNr;
 }
 
+const char *AgiGame::getString(int number) {
+	if (0 <= number && number <= MAX_STRINGS) {
+		return strings[number];
+	} else {
+		// Occurs in Flag Quest during startup
+		warning("invalid string number: %d", number);
+		return "";
+	}
+}
+
+void AgiGame::setString(int number, const char *str) {
+	if (0 <= number && number <= MAX_STRINGS) {
+		Common::strlcpy(strings[number], str, MAX_STRINGLEN);
+	} else {
+		// Occurs in Groza, number = 150
+		warning("invalid string number: %d, '%s'", number, str);
+	}
+}
+
 void AgiGame::setAppleIIgsSpeedLevel(int i) {
 	appleIIgsSpeedLevel = i;
 	_vm->setVar(VM_VAR_WINDOW_AUTO_CLOSE_TIMER, 6);

diff --git a/engines/agi/agi.h b/engines/agi/agi.h
@@ -474,6 +474,8 @@ struct AgiGame {
 	uint16 appleIIgsSpeedControllerSlot;
 	int appleIIgsSpeedLevel;
 
+	const char *getString(int number);
+	void setString(int number, const char *str);
 	void setAppleIIgsSpeedLevel(int appleIIgsSpeedLevel);
 
 	AgiGame() {

diff --git a/engines/agi/op_cmd.cpp b/engines/agi/op_cmd.cpp
@@ -658,7 +658,7 @@ void cmdWordToString(AgiGame *state, AgiEngine *vm, uint8 *parameter) {
 	uint16 stringNr = parameter[0];
 	uint16 wordNr = parameter[1];
 
-	Common::strlcpy(state->strings[stringNr], vm->_words->getEgoWord(wordNr), MAX_STRINGLEN);
+	state->setString(stringNr, vm->_words->getEgoWord(wordNr));
 }
 
 void cmdOpenDialogue(AgiGame *state, AgiEngine *vm, uint8 *parameter) {
@@ -955,7 +955,7 @@ void cmdSetSimple(AgiGame *state, AgiEngine *vm, uint8 *parameter) {
 		state->automaticSave = false;
 
 		// Try to get description for automatic saves
-		const char *textPtr = state->strings[stringNr];
+		const char *textPtr = state->getString(stringNr);
 
 		strncpy(state->automaticSaveDescription, textPtr, sizeof(state->automaticSaveDescription));
 		state->automaticSaveDescription[sizeof(state->automaticSaveDescription) - 1] = 0;
@@ -1133,7 +1133,7 @@ void cmdParse(AgiGame *state, AgiEngine *vm, uint8 *parameter) {
 	vm->setFlag(VM_FLAG_ENTERED_CLI, false);
 	vm->setFlag(VM_FLAG_SAID_ACCEPTED_INPUT, false);
 
-	vm->_words->parseUsingDictionary(vm->_text->stringPrintf(state->strings[stringNr]));
+	vm->_words->parseUsingDictionary(vm->_text->stringPrintf(state->getString(stringNr)));
 }
 
 void cmdCall(AgiGame *state, AgiEngine *vm, uint8 *parameter) {
@@ -2051,7 +2051,7 @@ void cmdGetString(AgiGame *state, AgiEngine *vm, uint8 *parameter) {
 
 	// copy string to destination
 	// TODO: not sure if set all the time or only when ENTER is pressed
-	Common::strlcpy(&vm->_game.strings[stringDestNr][0], (char *)textMgr->_inputString, MAX_STRINGLEN);
+	vm->_game.setString(stringDestNr, (char *)textMgr->_inputString);
 
 	textMgr->charPos_Pop();
 
@@ -2093,7 +2093,7 @@ void cmdGetNum(AgiGame *state, AgiEngine *vm, uint8 *parameter) {
 	number = atoi((char *)textMgr->_inputString);
 	vm->setVar(numberDestVarNr, number);
 
-	debugC(4, kDebugLevelScripts, "[%s] -> %d", state->strings[MAX_STRINGS], number);
+	debugC(4, kDebugLevelScripts, "[%s] -> %d", state->getString(MAX_STRINGS), number);
 }
 
 void cmdSetCursorChar(AgiGame *state, AgiEngine *vm, uint8 *parameter) {
@@ -2136,10 +2136,8 @@ void cmdSetKey(AgiGame *state, AgiEngine *vm, uint8 *parameter) {
 void cmdSetString(AgiGame *state, AgiEngine *vm, uint8 *parameter) {
 	uint16 stringNr = parameter[0];
 	uint16 textNr = parameter[1] - 1;
-	// CM: to avoid crash in Groza (str = 150)
-	if (stringNr > MAX_STRINGS)
-		return;
-	Common::strlcpy(state->strings[stringNr], state->_curLogic->texts[textNr], MAX_STRINGLEN);
+
+	state->setString(stringNr, state->_curLogic->texts[textNr]);
 }
 
 void cmdDisplay(AgiGame *state, AgiEngine *vm, uint8 *parameter) {

diff --git a/engines/agi/op_test.cpp b/engines/agi/op_test.cpp
@@ -191,7 +191,7 @@ void condBit(AgiGame *state, AgiEngine *vm, uint8 *p) {
 }
 
 void condCompareStrings(AgiGame *state, AgiEngine *vm, uint8 *p) {
-	debugC(7, kDebugLevelScripts, "comparing [%s], [%s]", state->strings[p[0]], state->strings[p[1]]);
+	debugC(7, kDebugLevelScripts, "comparing [%s], [%s]", state->getString(p[0]), state->getString(p[1]));
 	state->testResult = vm->testCompareStrings(p[0], p[1]);
 }
 
@@ -229,8 +229,8 @@ bool AgiEngine::testCompareStrings(uint8 s1, uint8 s2) {
 	char ms2[MAX_STRINGLEN];
 	int j, k, l;
 
-	Common::strlcpy(ms1, _game.strings[s1], MAX_STRINGLEN);
-	Common::strlcpy(ms2, _game.strings[s2], MAX_STRINGLEN);
+	Common::strlcpy(ms1, _game.getString(s1), MAX_STRINGLEN);
+	Common::strlcpy(ms2, _game.getString(s2), MAX_STRINGLEN);
 
 	l = strlen(ms1);
 	for (k = 0, j = 0; k < l; k++) {

diff --git a/engines/agi/saveload.cpp b/engines/agi/saveload.cpp
@@ -217,7 +217,7 @@ int AgiEngine::saveGame(const Common::String &fileName, const Common::String &de
 
 	// game.ev_keyp
 	for (i = 0; i < MAX_STRINGS; i++)
-		out->write(_game.strings[i], MAX_STRINGLEN);
+		out->write(_game.getString(i), MAX_STRINGLEN);
 
 	// record info about loaded resources
 	for (i = 0; i < MAX_DIRECTORY_ENTRIES; i++) {

diff --git a/engines/agi/text.cpp b/engines/agi/text.cpp
@@ -731,7 +731,7 @@ void TextMgr::promptKeyPress(uint16 newKey) {
 	if (_messageState.dialogue_Open) {
 		maxChars = TEXT_STRING_MAX_SIZE - 4;
 	} else {
-		maxChars = TEXT_STRING_MAX_SIZE - strlen(_vm->_game.strings[0]); // string 0 is the prompt string prefix
+		maxChars = TEXT_STRING_MAX_SIZE - strlen(_vm->_game.getString(0)); // string 0 is the prompt string prefix
 	}
 
 	if (_promptCursorPos)
@@ -824,8 +824,6 @@ void TextMgr::promptEchoLine() {
 }
 
 void TextMgr::promptRedraw() {
-	char *textPtr = nullptr;
-
 	if (_promptEnabled) {
 		if (_optionCommandPromptWindow) {
 			// Abort, in case command prompt window is active
@@ -837,7 +835,7 @@ void TextMgr::promptRedraw() {
 		charPos_Set(_promptRow, 0);
 		// agi_printf(str_wordwrap(msg, state.string[0], 40) );
 
-		textPtr = _vm->_game.strings[0];
+		const char *textPtr = _vm->_game.getString(0);
 		textPtr = stringPrintf(textPtr);
 		textPtr = stringWordWrap(textPtr, 40);
 
@@ -1245,7 +1243,7 @@ char *TextMgr::stringPrintf(const char *originalText) {
 				break;
 			case 's':
 				i = strtoul(originalText, nullptr, 10);
-				safeStrcat(resultString, stringPrintf(_vm->_game.strings[i]));
+				safeStrcat(resultString, stringPrintf(_vm->_game.getString(i)));
 				break;
 			case 'm':
 				i = strtoul(originalText, nullptr, 10) - 1;