[Scummvm-git-logs] scummvm master -> 0c40d60d24e2b7419999c8c05bf00458dae3489e
csnover
csnover at users.noreply.github.com
Thu Jan 5 23:04:39 CET 2017
This automated email contains information about 2 new commits which have been
pushed to the 'scummvm' repo located at https://github.com/scummvm/scummvm .
Summary:
70d1edf615 SCI32: Add validity checks to kList iteration methods
0c40d60d24 SCI32: Fix off-by-one error in array resizing
Commit: 70d1edf615bf997261066b1601b7af823af9ca1e
https://github.com/scummvm/scummvm/commit/70d1edf615bf997261066b1601b7af823af9ca1e
Author: Colin Snover (github.com at zetafleet.com)
Date: 2017-01-05T16:00:59-06:00
Commit Message:
SCI32: Add validity checks to kList iteration methods
In GK2, restoring a save game causes the segment manager to reset
in the middle of a kListFirstTrue call, which invalidates all
pointers and reg_ts to stored data. This means that when
kListFirstTrue tries to decrement the list recursion counter at
the end of iteration, it is writing to freed memory, potentially
resulting in heap corruption.
SCI3 added checks to prevent this from happening, but these checks
seem like they should have also been applied to some SCI2.1 games
as well (like GK2).
Since there should be no negative side-effect to this check, it
is applied universally to all SCI32 games.
Changed paths:
engines/sci/engine/kgraphics32.cpp
engines/sci/engine/klists.cpp
engines/sci/engine/seg_manager.h
diff --git a/engines/sci/engine/kgraphics32.cpp b/engines/sci/engine/kgraphics32.cpp
index 880ea0f..a3f7e4f 100644
--- a/engines/sci/engine/kgraphics32.cpp
+++ b/engines/sci/engine/kgraphics32.cpp
@@ -656,16 +656,9 @@ reg_t kBitmapCreate(EngineState *s, int argc, reg_t *argv) {
}
reg_t kBitmapDestroy(EngineState *s, int argc, reg_t *argv) {
- const reg_t &addr = argv[0];
- const SegmentObj *const segment = s->_segMan->getSegmentObj(addr.getSegment());
-
- if (segment != nullptr &&
- segment->getType() == SEG_TYPE_BITMAP &&
- segment->isValidOffset(addr.getOffset())) {
-
- s->_segMan->freeBitmap(addr);
+ if (s->_segMan->isValidAddr(argv[0], SEG_TYPE_BITMAP)) {
+ s->_segMan->freeBitmap(argv[0]);
}
-
return s->r_acc;
}
diff --git a/engines/sci/engine/klists.cpp b/engines/sci/engine/klists.cpp
index de5d7f1..305d459 100644
--- a/engines/sci/engine/klists.cpp
+++ b/engines/sci/engine/klists.cpp
@@ -586,7 +586,8 @@ reg_t kListIndexOf(EngineState *s, int argc, reg_t *argv) {
}
reg_t kListEachElementDo(EngineState *s, int argc, reg_t *argv) {
- List *list = s->_segMan->lookupList(argv[0]);
+ const reg_t listReg = argv[0];
+ List *list = s->_segMan->lookupList(listReg);
Node *curNode = s->_segMan->lookupNode(list->first);
Selector slc = argv[1].toUint16();
@@ -627,13 +628,16 @@ reg_t kListEachElementDo(EngineState *s, int argc, reg_t *argv) {
curNode = s->_segMan->lookupNode(list->nextNodes[list->numRecursions]);
}
- --list->numRecursions;
+ if (s->_segMan->isValidAddr(listReg, SEG_TYPE_LISTS)) {
+ --list->numRecursions;
+ }
return s->r_acc;
}
reg_t kListFirstTrue(EngineState *s, int argc, reg_t *argv) {
- List *list = s->_segMan->lookupList(argv[0]);
+ const reg_t listReg = argv[0];
+ List *list = s->_segMan->lookupList(listReg);
Node *curNode = s->_segMan->lookupNode(list->first);
Selector slc = argv[1].toUint16();
@@ -678,13 +682,16 @@ reg_t kListFirstTrue(EngineState *s, int argc, reg_t *argv) {
curNode = s->_segMan->lookupNode(list->nextNodes[list->numRecursions]);
}
- --list->numRecursions;
+ if (s->_segMan->isValidAddr(listReg, SEG_TYPE_LISTS)) {
+ --list->numRecursions;
+ }
return s->r_acc;
}
reg_t kListAllTrue(EngineState *s, int argc, reg_t *argv) {
- List *list = s->_segMan->lookupList(argv[0]);
+ const reg_t listReg = argv[0];
+ List *list = s->_segMan->lookupList(listReg);
Node *curNode = s->_segMan->lookupNode(list->first);
reg_t curObject;
@@ -723,7 +730,9 @@ reg_t kListAllTrue(EngineState *s, int argc, reg_t *argv) {
curNode = s->_segMan->lookupNode(list->nextNodes[list->numRecursions]);
}
- --list->numRecursions;
+ if (s->_segMan->isValidAddr(listReg, SEG_TYPE_LISTS)) {
+ --list->numRecursions;
+ }
return s->r_acc;
}
diff --git a/engines/sci/engine/seg_manager.h b/engines/sci/engine/seg_manager.h
index e3ccb9a..916b813 100644
--- a/engines/sci/engine/seg_manager.h
+++ b/engines/sci/engine/seg_manager.h
@@ -432,6 +432,13 @@ public:
reg_t getParserPtr() const { return _parserPtr; }
#ifdef ENABLE_SCI32
+ bool isValidAddr(reg_t reg, SegmentType expected) const {
+ SegmentObj *mobj = getSegmentObj(reg.getSegment());
+ return (mobj &&
+ mobj->getType() == expected &&
+ mobj->isValidOffset(reg.getOffset()));
+ }
+
SciArray *allocateArray(SciArrayType type, uint16 size, reg_t *addr);
SciArray *lookupArray(reg_t addr);
void freeArray(reg_t addr);
Commit: 0c40d60d24e2b7419999c8c05bf00458dae3489e
https://github.com/scummvm/scummvm/commit/0c40d60d24e2b7419999c8c05bf00458dae3489e
Author: Colin Snover (github.com at zetafleet.com)
Date: 2017-01-05T16:01:05-06:00
Commit Message:
SCI32: Fix off-by-one error in array resizing
This bug existed in SSCI and was pulled in carelessly during
initial implementation of SciArray. Closer examination of SCI3
reveals that this only happened to work in SSCI because it would
always allocate on the first resize, and would always allocate 25
extra elements per allocation.
Changed paths:
engines/sci/engine/segment.h
diff --git a/engines/sci/engine/segment.h b/engines/sci/engine/segment.h
index c5295c5..538f311 100644
--- a/engines/sci/engine/segment.h
+++ b/engines/sci/engine/segment.h
@@ -542,7 +542,14 @@ public:
*/
reg_t getAsID(const uint16 index) {
if (getSciVersion() >= SCI_VERSION_3) {
- resize(index);
+ // SCI3 resizes arrays automatically when out-of-bounds indices are
+ // passed, but it has an off-by-one error, always passing the index
+ // instead of `index + 1` on a read. This happens to work in SSCI
+ // only because the resize method there actually allocates memory
+ // for `index + 25` elements when growing the array, and it always
+ // grows the array on its first resize because it decides whether to
+ // grow based on byte size including an extra array header.
+ resize(index + 1);
} else {
assert(index < _size);
}
@@ -573,7 +580,8 @@ public:
*/
void setFromID(const uint16 index, const reg_t value) {
if (getSciVersion() >= SCI_VERSION_3) {
- resize(index);
+ // This code is different from SSCI; see getAsID for an explanation
+ resize(index + 1);
} else {
assert(index < _size);
}
@@ -599,7 +607,8 @@ public:
assert(_type == kArrayTypeInt16);
if (getSciVersion() >= SCI_VERSION_3) {
- resize(index);
+ // This code is different from SSCI; see getAsID for an explanation
+ resize(index + 1);
} else {
assert(index < _size);
}
@@ -616,6 +625,7 @@ public:
assert(_type == kArrayTypeInt16);
if (getSciVersion() >= SCI_VERSION_3) {
+ // This code is different from SSCI; see getAsID for an explanation
resize(index + 1);
} else {
assert(index < _size);
@@ -632,7 +642,8 @@ public:
assert(_type == kArrayTypeString || _type == kArrayTypeByte);
if (getSciVersion() >= SCI_VERSION_3) {
- resize(index);
+ // This code is different from SSCI; see getAsID for an explanation
+ resize(index + 1);
} else {
assert(index < _size);
}
@@ -648,7 +659,8 @@ public:
assert(_type == kArrayTypeString || _type == kArrayTypeByte);
if (getSciVersion() >= SCI_VERSION_3) {
- resize(index);
+ // This code is different from SSCI; see getAsID for an explanation
+ resize(index + 1);
} else {
assert(index < _size);
}
@@ -664,7 +676,8 @@ public:
assert(_type == kArrayTypeID || _type == kArrayTypeInt16);
if (getSciVersion() >= SCI_VERSION_3) {
- resize(index);
+ // This code is different from SSCI; see getAsID for an explanation
+ resize(index + 1);
} else {
assert(index < _size);
}
More information about the Scummvm-git-logs
mailing list