[Scummvm-git-logs] scummvm master -> 5e725d639c18badcb183a0843db61c3d84434c8a

[sluicebox]

This automated email contains information about 1 new commit which have been
pushed to the 'scummvm' repo located at https://github.com/scummvm/scummvm .

Summary:
5e725d639c SCI: SavegameDesc::name is now always terminated


Commit: 5e725d639c18badcb183a0843db61c3d84434c8a
    https://github.com/scummvm/scummvm/commit/5e725d639c18badcb183a0843db61c3d84434c8a
Author: sluicebox (22204938+sluicebox at users.noreply.github.com)
Date: 2021-12-21T14:38:19-05:00

Commit Message:
SCI: SavegameDesc::name is now always terminated

`SavegameDesc::name` is now easier to understand and safer to use.
Fixes Phant2 regression from 913d943a090746cf381d7227c0e0e28f053a0cb8
Fixes Phant2 out-of-bounds read in `Console::cmdListSaves`

This was originally always a null terminated string but when SCI32
was added the terminator became optional, leading to a confusing
situation. `name` is now always terminated internally. SCI16 interfaces
continue to terminate the 36th element and SCI32 interfaces continue
to use the first 36 elements as-is.

See: 4357809ce8238cfd0bc5d8c92b6da28068607d55

Changed paths:
    engines/sci/engine/file.cpp
    engines/sci/engine/file.h
    engines/sci/engine/guest_additions.cpp
    engines/sci/engine/kfile.cpp


diff --git a/engines/sci/engine/file.cpp b/engines/sci/engine/file.cpp
index 01016e313a..7f10801ddc 100644
--- a/engines/sci/engine/file.cpp
+++ b/engines/sci/engine/file.cpp
@@ -343,9 +343,7 @@ bool fillSavegameDesc(const Common::String &filename, SavegameDesc &desc) {
 		nameString = nameU32String.encode(Common::kWindows1255);
 	}
 
-	// At least Phant2 requires use of strncpy, since it creates save game
-	// names of exactly kMaxSaveNameLength
-	Common::strlcpy(desc.name, nameString.c_str(), kMaxSaveNameLength);
+	Common::strlcpy(desc.name, nameString.c_str(), sizeof(desc.name));
 
 	return true;
 }
diff --git a/engines/sci/engine/file.h b/engines/sci/engine/file.h
index 9938b7b42c..0f3d8043c9 100644
--- a/engines/sci/engine/file.h
+++ b/engines/sci/engine/file.h
@@ -36,7 +36,7 @@ enum kFileOpenMode {
 };
 
 enum {
-	kMaxSaveNameLength = 36, ///< Maximum length of a savegame name (including optional terminator character).
+	kMaxSaveNameLength = 36, ///< Maximum length of a savegame name (excluding terminator character)
 	kMaxNumSaveGames = 20 ///< Maximum number of savegames
 };
 
@@ -66,7 +66,11 @@ struct SavegameDesc {
 	int date;
 	int time;
 	int version;
-	char name[kMaxSaveNameLength];
+	// name is a null-terminated 36 character array in SCI16,
+	// but in SCI32 the 36th character can be used for text.
+	// At least Phant2 makes use of this. We add an element
+	// so that this string is always terminated internally.
+	char name[kMaxSaveNameLength + 1];
 	Common::String gameVersion;
 	uint32 script0Size;
 	uint32 gameObjectOffset;
diff --git a/engines/sci/engine/guest_additions.cpp b/engines/sci/engine/guest_additions.cpp
index 2c4e67d7d0..5db8e2c9a2 100644
--- a/engines/sci/engine/guest_additions.cpp
+++ b/engines/sci/engine/guest_additions.cpp
@@ -625,7 +625,7 @@ reg_t GuestAdditions::promptSaveRestoreRama(EngineState *s, int argc, reg_t *arg
 					// actually save into the new save
 					resetCatalogFile = true;
 				}
-			} else if (strncmp(saveGameName.c_str(), saves[saveIndex].name, kMaxSaveNameLength) != 0) {
+			} else if (strcmp(saveGameName.c_str(), saves[saveIndex].name) != 0) {
 				// The game doesn't let the save game name change for the same
 				// slot, but ScummVM's GUI does, so force the new name into the
 				// save file metadata if it has changed so it actually makes it
diff --git a/engines/sci/engine/kfile.cpp b/engines/sci/engine/kfile.cpp
index 9a4d0cdd69..bcb1b52a9a 100644
--- a/engines/sci/engine/kfile.cpp
+++ b/engines/sci/engine/kfile.cpp
@@ -1322,7 +1322,7 @@ reg_t kGetSaveFiles(EngineState *s, int argc, reg_t *argv) {
 
 	for (uint i = 0; i < totalSaves; i++) {
 		*slot++ = make_reg(0, saves[i].id + SAVEGAMEID_OFFICIALRANGE_START); // Store the virtual savegame ID (see above)
-		strcpy(saveNamePtr, saves[i].name);
+		Common::strlcpy(saveNamePtr, saves[i].name, kMaxSaveNameLength);
 		saveNamePtr += kMaxSaveNameLength;
 	}