diff options
author | Colin Snover | 2017-01-05 21:05:22 -0600 |
---|---|---|
committer | Colin Snover | 2017-02-05 12:38:21 -0600 |
commit | 10d97ce37947362e6fa7161cd85221f59ca49490 (patch) | |
tree | 096af451474abe5ab3f65e61016a0ff2d413608c /engines/sci/engine | |
parent | b1c3332fddbb16838f1a654d6fe35ddbe09bd051 (diff) | |
download | scummvm-rg350-10d97ce37947362e6fa7161cd85221f59ca49490.tar.gz scummvm-rg350-10d97ce37947362e6fa7161cd85221f59ca49490.tar.bz2 scummvm-rg350-10d97ce37947362e6fa7161cd85221f59ca49490.zip |
SCI: Fix more unsafe C-string usage
Diffstat (limited to 'engines/sci/engine')
-rw-r--r-- | engines/sci/engine/file.cpp | 2 | ||||
-rw-r--r-- | engines/sci/engine/kgraphics.cpp | 8 | ||||
-rw-r--r-- | engines/sci/engine/kstring.cpp | 2 | ||||
-rw-r--r-- | engines/sci/engine/message.cpp | 40 | ||||
-rw-r--r-- | engines/sci/engine/message.h | 5 | ||||
-rw-r--r-- | engines/sci/engine/scriptdebug.cpp | 8 | ||||
-rw-r--r-- | engines/sci/engine/seg_manager.cpp | 3 | ||||
-rw-r--r-- | engines/sci/engine/segment.h | 6 |
8 files changed, 54 insertions, 20 deletions
diff --git a/engines/sci/engine/file.cpp b/engines/sci/engine/file.cpp index f4bd437d3e..91cf189d0a 100644 --- a/engines/sci/engine/file.cpp +++ b/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) diff --git a/engines/sci/engine/kgraphics.cpp b/engines/sci/engine/kgraphics.cpp index 08e3115e48..07a1c47547 100644 --- a/engines/sci/engine/kgraphics.cpp +++ b/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; diff --git a/engines/sci/engine/kstring.cpp b/engines/sci/engine/kstring.cpp index ae91ef088a..4c5f1228de 100644 --- a/engines/sci/engine/kstring.cpp +++ b/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) diff --git a/engines/sci/engine/message.cpp b/engines/sci/engine/message.cpp index 26ab9b47a5..5e07ead5d7 100644 --- a/engines/sci/engine/message.cpp +++ b/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 @@ public: 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 @@ public: 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 @@ public: 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 @@ public: 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 @@ public: #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; } diff --git a/engines/sci/engine/message.h b/engines/sci/engine/message.h index 5847e4767e..a4de19bc74 100644 --- a/engines/sci/engine/message.h +++ b/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> { diff --git a/engines/sci/engine/scriptdebug.cpp b/engines/sci/engine/scriptdebug.cpp index 54b925a1b6..6002cbd8e4 100644 --- a/engines/sci/engine/scriptdebug.cpp +++ b/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; diff --git a/engines/sci/engine/seg_manager.cpp b/engines/sci/engine/seg_manager.cpp index f8138ef649..9ccd1098d3 100644 --- a/engines/sci/engine/seg_manager.cpp +++ b/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; diff --git a/engines/sci/engine/segment.h b/engines/sci/engine/segment.h index 1ec13176e4..281837d921 100644 --- a/engines/sci/engine/segment.h +++ b/engines/sci/engine/segment.h @@ -528,7 +528,7 @@ public: */ 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 @@ public: } 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 @@ public: } ++source; - memmove(target, source, strlen((char *)source) + 1); + memmove(target, source, Common::strnlen((char *)source, _size - 1) + 1); } } } |