SCI: Fix more unsafe C-string usage

engines/sci/console.cpp

@@ -1264,7 +1264,7 @@ bool Console::cmdMapInstrument(int argc, const char **argv) {
 			char *instrumentName = new char[11];
 			Common::strlcpy(instrumentName, argv[1], 11);
 
-			for (uint16 i = 0; i < strlen(instrumentName); i++)
+			for (uint16 i = 0; i < Common::strnlen(instrumentName, 11); i++)
 				if (instrumentName[i] == '_')
 					instrumentName[i] = ' ';
 

engines/sci/engine/file.cpp

@@ -313,7 +313,7 @@ int fgets_wrapper(EngineState *s, char *dest, int maxsize, int handle) {
 	if (maxsize > 1) {
 		memset(dest, 0, maxsize);
 		f->_in->readLine(dest, maxsize);
-		readBytes = strlen(dest); // FIXME: sierra sci returned byte count and didn't react on NUL characters
+		readBytes = Common::strnlen(dest, maxsize); // FIXME: sierra sci returned byte count and didn't react on NUL characters
 		// The returned string must not have an ending LF
 		if (readBytes > 0) {
 			if (dest[readBytes - 1] == 0x0A)

engines/sci/engine/kgraphics.cpp

@@ -824,8 +824,7 @@ void _k_GenericDrawControl(EngineState *s, reg_t controlObject, bool hilite) {
 	int16 celNo;
 	int16 priority;
 	reg_t listSeeker;
-	Common::String *listStrings = NULL;
-	const char **listEntries = NULL;
+	Common::String *listStrings = nullptr;
 	bool isAlias = false;
 
 	rect = kControlCreateRect(x, y,
@@ -922,11 +921,9 @@ void _k_GenericDrawControl(EngineState *s, reg_t controlObject, bool hilite) {
 		if (listCount) {
 			// We create a pointer-list to the different strings, we also find out whats upper and cursor position
 			listSeeker = textReference;
-			listEntries = (const char**)malloc(sizeof(char *) * listCount);
 			listStrings = new Common::String[listCount];
 			for (i = 0; i < listCount; i++) {
 				listStrings[i] = s->_segMan->getString(listSeeker);
-				listEntries[i] = listStrings[i].c_str();
 				if (listSeeker.getOffset() == upperOffset)
 					upperPos = i;
 				if (listSeeker.getOffset() == cursorOffset)
@@ -936,8 +933,7 @@ void _k_GenericDrawControl(EngineState *s, reg_t controlObject, bool hilite) {
 		}
 
 		debugC(kDebugLevelGraphics, "drawing list control %04x:%04x to %d,%d, diff %d", PRINT_REG(controlObject), x, y, SCI_MAX_SAVENAME_LENGTH);
-		g_sci->_gfxControls16->kernelDrawList(rect, controlObject, maxChars, listCount, listEntries, fontId, style, upperPos, cursorPos, isAlias, hilite);
-		free(listEntries);
+		g_sci->_gfxControls16->kernelDrawList(rect, controlObject, maxChars, listCount, listStrings, fontId, style, upperPos, cursorPos, isAlias, hilite);
 		delete[] listStrings;
 		return;
 

engines/sci/engine/kstring.cpp

@@ -305,7 +305,7 @@ reg_t kFormat(EngineState *s, int argc, reg_t *argv) {
 
 				Common::String tempsource = g_sci->getKernel()->lookupText(reg,
 				                                  arguments[paramindex + 1]);
-				int slen = strlen(tempsource.c_str());
+				int slen = tempsource.size();
 				int extralen = strLength - slen;
 				assert((target - targetbuf) + extralen <= maxsize);
 				if (extralen < 0)

engines/sci/engine/message.cpp

@@ -32,6 +32,7 @@ struct MessageRecord {
 	MessageTuple tuple;
 	MessageTuple refTuple;
 	const char *string;
+	uint32 length;
 	byte talker;
 };
 
@@ -77,7 +78,13 @@ class MessageReaderV2 : public MessageReader {
 				record.tuple = tuple;
 				record.refTuple = MessageTuple();
 				record.talker = 0;
-				record.string = (const char *)_data + READ_LE_UINT16(recordPtr + 2);
+				const uint16 stringOffset = READ_LE_UINT16(recordPtr + 2);
+				const uint32 maxSize = _size - stringOffset;
+				record.string = (const char *)_data + stringOffset;
+				record.length = Common::strnlen(record.string, maxSize);
+				if (record.length == maxSize) {
+					warning("Message %s appears truncated at %ld", tuple.toString().c_str(), recordPtr - _data);
+				}
 				return true;
 			}
 			recordPtr += _recordSize;
@@ -100,7 +107,13 @@ class MessageReaderV3 : public MessageReader {
 				record.tuple = tuple;
 				record.refTuple = MessageTuple();
 				record.talker = recordPtr[4];
-				record.string = (const char *)_data + READ_LE_UINT16(recordPtr + 5);
+				const uint16 stringOffset = READ_LE_UINT16(recordPtr + 5);
+				const uint32 maxSize = _size - stringOffset;
+				record.string = (const char *)_data + stringOffset;
+				record.length = Common::strnlen(record.string, maxSize);
+				if (record.length == maxSize) {
+					warning("Message %s appears truncated at %ld", tuple.toString().c_str(), recordPtr - _data);
+				}
 				return true;
 			}
 			recordPtr += _recordSize;
@@ -123,7 +136,13 @@ class MessageReaderV4 : public MessageReader {
 				record.tuple = tuple;
 				record.refTuple = MessageTuple(recordPtr[7], recordPtr[8], recordPtr[9]);
 				record.talker = recordPtr[4];
-				record.string = (const char *)_data + READ_SCI11ENDIAN_UINT16(recordPtr + 5);
+				const uint16 stringOffset = READ_SCI11ENDIAN_UINT16(recordPtr + 5);
+				const uint32 maxSize = _size - stringOffset;
+				record.string = (const char *)_data + stringOffset;
+				record.length = Common::strnlen(record.string, maxSize);
+				if (record.length == maxSize) {
+					warning("Message %s appears truncated at %ld", tuple.toString().c_str(), recordPtr - _data);
+				}
 				return true;
 			}
 			recordPtr += _recordSize;
@@ -149,7 +168,13 @@ class MessageReaderV4_MacSCI32 : public MessageReader {
 				record.tuple = tuple;
 				record.refTuple = MessageTuple(recordPtr[8], recordPtr[9], recordPtr[10]);
 				record.talker = recordPtr[4];
-				record.string = (const char *)_data + READ_BE_UINT16(recordPtr + 6);
+				const uint16 stringOffset = READ_BE_UINT16(recordPtr + 6);
+				const uint32 maxSize = _size - stringOffset;
+				record.string = (const char *)_data + stringOffset;
+				record.length = Common::strnlen(record.string, maxSize);
+				if (record.length == maxSize) {
+					warning("Message %s appears truncated at %ld", tuple.toString().c_str(), recordPtr - _data);
+				}
 				return true;
 			}
 			recordPtr += _recordSize;
@@ -161,7 +186,7 @@ class MessageReaderV4_MacSCI32 : public MessageReader {
 #endif
 
 bool MessageState::getRecord(CursorStack &stack, bool recurse, MessageRecord &record) {
-	Resource *res = g_sci->getResMan()->findResource(ResourceId(kResourceTypeMessage, stack.getModule()), 0);
+	Resource *res = g_sci->getResMan()->findResource(ResourceId(kResourceTypeMessage, stack.getModule()), false);
 
 	if (!res) {
 		warning("Failed to open message resource %d", stack.getModule());
@@ -238,6 +263,7 @@ bool MessageState::getRecord(CursorStack &stack, bool recurse, MessageRecord &re
 			// as the text shown in this screen is very short (one-liners).
 			// Just output an empty string here instead of showing an error.
 			record.string = "";
+			record.length = 0;
 			delete reader;
 			return true;
 		}
@@ -285,7 +311,7 @@ int MessageState::nextMessage(reg_t buf) {
 			return record.talker;
 		} else {
 			MessageTuple &t = _cursorStack.top();
-			outputString(buf, Common::String::format("Msg %d: %d %d %d %d not found", _cursorStack.getModule(), t.noun, t.verb, t.cond, t.seq));
+			outputString(buf, Common::String::format("Msg %d: %s not found", _cursorStack.getModule(), t.toString().c_str()));
 			return 0;
 		}
 	} else {
@@ -304,7 +330,7 @@ int MessageState::messageSize(int module, MessageTuple &t) {
 
 	stack.init(module, t);
 	if (getRecord(stack, true, record))
-		return strlen(record.string) + 1;
+		return record.length + 1;
 	else
 		return 0;
 }

engines/sci/engine/message.h

@@ -40,6 +40,11 @@ struct MessageTuple {
 
 	MessageTuple(byte noun_ = 0, byte verb_ = 0, byte cond_ = 0, byte seq_ = 1)
 		: noun(noun_), verb(verb_), cond(cond_), seq(seq_) { }
+
+	Common::String toString() const {
+		return Common::String::format("noun %d, verb %d, cond %d, seq %d",
+									  noun, verb, cond, seq);
+	}
 };
 
 class CursorStack : public Common::Stack<MessageTuple> {

engines/sci/engine/scriptdebug.cpp

@@ -598,8 +598,12 @@ void Kernel::dissectScript(int scriptNumber, Vocabulary *vocab) {
 		case SCI_OBJ_STRINGS:
 			debugN("Strings\n");
 			while (script->data [seeker]) {
-				debugN("%04x: %s\n", seeker, script->data + seeker);
-				seeker += strlen((char *)script->data + seeker) + 1;
+				debugN("%04x: %s", seeker, script->data + seeker);
+				seeker += Common::strnlen((char *)script->data + seeker, script->size - seeker) + 1;
+				if (seeker > script->size) {
+					debugN("[TRUNCATED]");
+				}
+				debugN("\n");
 			}
 			seeker++; // the ending zero byte
 			break;

engines/sci/engine/seg_manager.cpp

@@ -786,6 +786,9 @@ size_t SegManager::strlen(reg_t str) {
 	}
 
 	if (str_r.isRaw) {
+		// There is no guarantee that raw strings are zero-terminated; for
+		// example, Phant1 reads "\r\n" from a pointer of size 2 during the
+		// chase
 		return Common::strnlen((const char *)str_r.raw, str_r.maxSize);
 	} else {
 		int i = 0;

engines/sci/engine/segment.h

@@ -528,7 +528,7 @@ class SciArray : public Common::Serializable {
 	 */
 	void snug() {
 		assert(_type == kArrayTypeString || _type == kArrayTypeByte);
-		resize(strlen((char *)_data) + 1, true);
+		resize(Common::strnlen((char *)_data, _size) + 1, true);
 	}
 
 	/**
@@ -808,7 +808,7 @@ class SciArray : public Common::Serializable {
 		}
 
 		if (flags & kArrayTrimRight) {
-			source = data + strlen((char *)data) - 1;
+			source = data + Common::strnlen((char *)data, _size) - 1;
 			while (source > data && *source != showChar && *source <= kWhitespaceBoundary) {
 				*source = '\0';
 				--source;
@@ -844,7 +844,7 @@ class SciArray : public Common::Serializable {
 					}
 					++source;
 
-					memmove(target, source, strlen((char *)source) + 1);
+					memmove(target, source, Common::strnlen((char *)source, _size - 1) + 1);
 				}
 			}
 		}

engines/sci/graphics/controls16.cpp

@@ -52,14 +52,12 @@ GfxControls16::~GfxControls16() {
 const char controlListUpArrow[2]	= { 0x18, 0 };
 const char controlListDownArrow[2]	= { 0x19, 0 };
 
-void GfxControls16::drawListControl(Common::Rect rect, reg_t obj, int16 maxChars, int16 count, const char **entries, GuiResourceId fontId, int16 upperPos, int16 cursorPos, bool isAlias) {
+void GfxControls16::drawListControl(Common::Rect rect, reg_t obj, int16 maxChars, int16 count, const Common::String *entries, GuiResourceId fontId, int16 upperPos, int16 cursorPos, bool isAlias) {
 	Common::Rect workerRect = rect;
 	GuiResourceId oldFontId = _text16->GetFontId();
 	int16 oldPenColor = _ports->_curPort->penClr;
 	uint16 fontSize = 0;
 	int16 i;
-	const char *listEntry;
-	int16 listEntryLen;
 	int16 lastYpos;
 
 	// draw basic window
@@ -92,11 +90,10 @@ void GfxControls16::drawListControl(Common::Rect rect, reg_t obj, int16 maxChars
 	// Write actual text
 	for (i = upperPos; i < count; i++) {
 		_paint16->eraseRect(workerRect);
-		listEntry = entries[i];
+		const Common::String &listEntry = entries[i];
 		if (listEntry[0]) {
 			_ports->moveTo(workerRect.left, workerRect.top);
-			listEntryLen = strlen(listEntry);
-			_text16->Draw(listEntry, 0, MIN(maxChars, listEntryLen), oldFontId, oldPenColor);
+			_text16->Draw(listEntry.c_str(), 0, MIN<int16>(maxChars, listEntry.size()), oldFontId, oldPenColor);
 			if ((!isAlias) && (i == cursorPos)) {
 				_paint16->invertRect(workerRect);
 			}
@@ -370,7 +367,7 @@ void GfxControls16::kernelDrawIcon(Common::Rect rect, reg_t obj, GuiResourceId v
 	}
 }
 
-void GfxControls16::kernelDrawList(Common::Rect rect, reg_t obj, int16 maxChars, int16 count, const char **entries, GuiResourceId fontId, int16 style, int16 upperPos, int16 cursorPos, bool isAlias, bool hilite) {
+void GfxControls16::kernelDrawList(Common::Rect rect, reg_t obj, int16 maxChars, int16 count, const Common::String *entries, GuiResourceId fontId, int16 style, int16 upperPos, int16 cursorPos, bool isAlias, bool hilite) {
 	if (!hilite) {
 		drawListControl(rect, obj, maxChars, count, entries, fontId, upperPos, cursorPos, isAlias);
 		rect.grow(1);

engines/sci/graphics/controls16.h

@@ -59,13 +59,13 @@ class GfxControls16 {
 	void kernelDrawText(Common::Rect rect, reg_t obj, const char *text, uint16 languageSplitter, int16 fontId, int16 alignment, int16 style, bool hilite);
 	void kernelDrawTextEdit(Common::Rect rect, reg_t obj, const char *text, uint16 languageSplitter, int16 fontId, int16 mode, int16 style, int16 cursorPos, int16 maxChars, bool hilite);
 	void kernelDrawIcon(Common::Rect rect, reg_t obj, GuiResourceId viewId, int16 loopNo, int16 celNo, int16 priority, int16 style, bool hilite);
-	void kernelDrawList(Common::Rect rect, reg_t obj, int16 maxChars, int16 count, const char **entries, GuiResourceId fontId, int16 style, int16 upperPos, int16 cursorPos, bool isAlias, bool hilite);
+	void kernelDrawList(Common::Rect rect, reg_t obj, int16 maxChars, int16 count, const Common::String *entries, GuiResourceId fontId, int16 style, int16 upperPos, int16 cursorPos, bool isAlias, bool hilite);
 	void kernelTexteditChange(reg_t controlObject, reg_t eventObject);
 
 private:
 	void texteditSetBlinkTime();
 
-	void drawListControl(Common::Rect rect, reg_t obj, int16 maxChars, int16 count, const char **entries, GuiResourceId fontId, int16 upperPos, int16 cursorPos, bool isAlias);
+	void drawListControl(Common::Rect rect, reg_t obj, int16 maxChars, int16 count, const Common::String *entries, GuiResourceId fontId, int16 upperPos, int16 cursorPos, bool isAlias);
 	void texteditCursorDraw(Common::Rect rect, const char *text, uint16 curPos);
 	void texteditCursorErase();
 	int getPicNotValid();

engines/sci/graphics/text16.cpp

@@ -200,7 +200,7 @@ int16 GfxText16::GetLongest(const char *&textPtr, int16 maxWidth, GuiResourceId
 	if (!_font)
 		return 0;
 
-	while (1) {
+	for (;;) {
 		curChar = (*(const byte *)textPtr);
 		if (_font->isDoubleByte(curChar)) {
 			curChar |= (*(const byte *)(textPtr + 1)) << 8;
@@ -300,7 +300,7 @@ int16 GfxText16::GetLongest(const char *&textPtr, int16 maxWidth, GuiResourceId
 				punctuationTable = text16_shiftJIS_punctuation_SCI01;
 			}
 
-			while (1) {
+			for (;;) {
 				// Look up if character shouldn't be the first on a new line
 				nonBreakingPos = 0;
 				while (punctuationTable[nonBreakingPos]) {
@@ -383,15 +383,15 @@ void GfxText16::Width(const char *text, int16 from, int16 len, GuiResourceId org
 	return;
 }
 
-void GfxText16::StringWidth(const char *str, GuiResourceId orgFontId, int16 &textWidth, int16 &textHeight) {
-	Width(str, 0, (int16)strlen(str), orgFontId, textWidth, textHeight, true);
+void GfxText16::StringWidth(const Common::String &str, GuiResourceId orgFontId, int16 &textWidth, int16 &textHeight) {
+	Width(str.c_str(), 0, str.size(), orgFontId, textWidth, textHeight, true);
 }
 
-void GfxText16::ShowString(const char *str, GuiResourceId orgFontId, int16 orgPenColor) {
-	Show(str, 0, (int16)strlen(str), orgFontId, orgPenColor);
+void GfxText16::ShowString(const Common::String &str, GuiResourceId orgFontId, int16 orgPenColor) {
+	Show(str.c_str(), 0, str.size(), orgFontId, orgPenColor);
 }
-void GfxText16::DrawString(const char *str, GuiResourceId orgFontId, int16 orgPenColor) {
-	Draw(str, 0, (int16)strlen(str), orgFontId, orgPenColor);
+void GfxText16::DrawString(const Common::String &str, GuiResourceId orgFontId, int16 orgPenColor) {
+	Draw(str.c_str(), 0, str.size(), orgFontId, orgPenColor);
 }
 
 int16 GfxText16::Size(Common::Rect &rect, const char *text, uint16 languageSplitter, GuiResourceId fontId, int16 maxWidth) {
@@ -580,20 +580,21 @@ void GfxText16::Box(const char *text, uint16 languageSplitter, bool show, const
 	}
 }
 
-void GfxText16::DrawString(const char *text) {
+void GfxText16::DrawString(const Common::String &text) {
 	GuiResourceId previousFontId = GetFontId();
 	int16 previousPenColor = _ports->_curPort->penClr;
 
-	Draw(text, 0, strlen(text), previousFontId, previousPenColor);
+	Draw(text.c_str(), 0, text.size(), previousFontId, previousPenColor);
 	SetFont(previousFontId);
 	_ports->penColor(previousPenColor);
 }
 
 // we need to have a separate status drawing code
 //  In KQ4 the IV char is actually 0xA, which would otherwise get considered as linebreak and not printed
-void GfxText16::DrawStatus(const char *text) {
+void GfxText16::DrawStatus(const Common::String &str) {
 	uint16 curChar, charWidth;
-	uint16 textLen = strlen(text);
+	const byte *text = (const byte *)str.c_str();
+	uint16 textLen = str.size();
 	Common::Rect rect;
 
 	GetFont();
@@ -603,7 +604,7 @@ void GfxText16::DrawStatus(const char *text) {
 	rect.top = _ports->_curPort->curTop;
 	rect.bottom = rect.top + _ports->_curPort->fontHeight;
 	while (textLen--) {
-		curChar = (*(const byte *)text++);
+		curChar = *text++;
 		switch (curChar) {
 		case 0:
 			break;

engines/sci/graphics/text16.h

@@ -53,9 +53,9 @@ class GfxText16 {
 
 	int16 GetLongest(const char *&text, int16 maxWidth, GuiResourceId orgFontId);
 	void Width(const char *text, int16 from, int16 len, GuiResourceId orgFontId, int16 &textWidth, int16 &textHeight, bool restoreFont);
-	void StringWidth(const char *str, GuiResourceId orgFontId, int16 &textWidth, int16 &textHeight);
-	void ShowString(const char *str, GuiResourceId orgFontId, int16 orgPenColor);
-	void DrawString(const char *str, GuiResourceId orgFontId, int16 orgPenColor);
+	void StringWidth(const Common::String &str, GuiResourceId orgFontId, int16 &textWidth, int16 &textHeight);
+	void ShowString(const Common::String &str, GuiResourceId orgFontId, int16 orgPenColor);
+	void DrawString(const Common::String &str, GuiResourceId orgFontId, int16 orgPenColor);
 	int16 Size(Common::Rect &rect, const char *text, uint16 textLanguage, GuiResourceId fontId, int16 maxWidth);
 	void Draw(const char *text, int16 from, int16 len, GuiResourceId orgFontId, int16 orgPenColor);
 	void Show(const char *text, int16 from, int16 len, GuiResourceId orgFontId, int16 orgPenColor);
@@ -65,8 +65,8 @@ class GfxText16 {
 		Box(text, 0, show, rect, alignment, fontId);
 	}
 
-	void DrawString(const char *text);
-	void DrawStatus(const char *text);
+	void DrawString(const Common::String &str);
+	void DrawStatus(const Common::String &str);
 
 	GfxFont *_font;