[Scummvm-cvs-logs] scummvm master -> 741ac22e176934cdb7bca38c9880cb41f85de763
csnover
csnover at users.noreply.github.com
Sat Aug 13 22:42:26 CEST 2016
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:
741ac22e17 SCI: Fix pointer invalidation caused by array storage moves
Commit: 741ac22e176934cdb7bca38c9880cb41f85de763
https://github.com/scummvm/scummvm/commit/741ac22e176934cdb7bca38c9880cb41f85de763
Author: Colin Snover (github.com at zetafleet.com)
Date: 2016-08-13T15:41:31-05:00
Commit Message:
SCI: Fix pointer invalidation caused by array storage moves
When objects are added to a SegmentObjTable, it may cause the
internal storage for the table to expand and move to a new
region of memory. When this happens, all pointers to objects held
by a SegmentObjTable of the same type would be invalidated, due
to an implementation detail that should not be exposed. To prevent
this, objects are now allocated separately on the heap, so even if
the table's storage moves due to insertions, the objects owned by
the table will not, so references remain valid for the lifetime of
the object.
Changed paths:
engines/sci/engine/savegame.cpp
engines/sci/engine/savegame.h
engines/sci/engine/seg_manager.cpp
engines/sci/engine/segment.h
diff --git a/engines/sci/engine/savegame.cpp b/engines/sci/engine/savegame.cpp
index c4d53a2..c5d2834 100644
--- a/engines/sci/engine/savegame.cpp
+++ b/engines/sci/engine/savegame.cpp
@@ -159,23 +159,6 @@ void syncWithSerializer(Common::Serializer &s, SciString &obj) {
}
}
-void syncWithSerializer(Common::Serializer &s, SciBitmap *&obj) {
- bool hasEntry;
- if (s.isSaving()) {
- hasEntry = obj != nullptr;
- }
- s.syncAsByte(hasEntry);
-
- if (hasEntry) {
- if (s.isLoading()) {
- obj = new SciBitmap;
- }
-
- obj->saveLoadWithSerializer(s);
- } else {
- obj = nullptr;
- }
-}
#endif
#pragma mark -
@@ -183,7 +166,7 @@ void syncWithSerializer(Common::Serializer &s, SciBitmap *&obj) {
// By default, sync using syncWithSerializer, which in turn can easily be overloaded.
template<typename T>
struct DefaultSyncer : Common::BinaryFunction<Common::Serializer, T, void> {
- void operator()(Common::Serializer &s, T &obj) const {
+ void operator()(Common::Serializer &s, T &obj, int) const {
syncWithSerializer(s, obj);
}
};
@@ -191,10 +174,31 @@ struct DefaultSyncer : Common::BinaryFunction<Common::Serializer, T, void> {
// Syncer for entries in a segment obj table
template<typename T>
struct SegmentObjTableEntrySyncer : Common::BinaryFunction<Common::Serializer, typename T::Entry &, void> {
- void operator()(Common::Serializer &s, typename T::Entry &entry) const {
+ void operator()(Common::Serializer &s, typename T::Entry &entry, int index) const {
s.syncAsSint32LE(entry.next_free);
- syncWithSerializer(s, entry.data);
+ bool hasData;
+ if (s.getVersion() >= 37) {
+ if (s.isSaving()) {
+ hasData = entry.data != nullptr;
+ }
+ s.syncAsByte(hasData);
+ } else {
+ hasData = (entry.next_free == index);
+ }
+
+ if (hasData) {
+ if (s.isLoading()) {
+ entry.data = new typename T::value_type;
+ }
+ syncWithSerializer(s, *entry.data);
+ } else if (s.isLoading()) {
+ if (s.getVersion() < 37) {
+ typename T::value_type dummy;
+ syncWithSerializer(s, dummy);
+ }
+ entry.data = nullptr;
+ }
}
};
@@ -222,9 +226,8 @@ struct ArraySyncer : Common::BinaryFunction<Common::Serializer, T, void> {
if (s.isLoading())
arr.resize(len);
- typename Common::Array<T>::iterator i;
- for (i = arr.begin(); i != arr.end(); ++i) {
- sync(s, *i);
+ for (int i = 0; i < len; ++i) {
+ sync(s, arr[i], i);
}
}
};
diff --git a/engines/sci/engine/savegame.h b/engines/sci/engine/savegame.h
index c5c2bce..1bf6686 100644
--- a/engines/sci/engine/savegame.h
+++ b/engines/sci/engine/savegame.h
@@ -37,6 +37,7 @@ struct EngineState;
*
* Version - new/changed feature
* =============================
+ * 37 - Segment entry data changed to pointers
* 36 - SCI32 bitmap segment
* 35 - SCI32 remap
* 34 - SCI32 palettes, and store play time in ticks
@@ -61,7 +62,7 @@ struct EngineState;
*/
enum {
- CURRENT_SAVEGAME_VERSION = 36,
+ CURRENT_SAVEGAME_VERSION = 37,
MINIMUM_SAVEGAME_VERSION = 14
};
diff --git a/engines/sci/engine/seg_manager.cpp b/engines/sci/engine/seg_manager.cpp
index 6084529..5cf8d61 100644
--- a/engines/sci/engine/seg_manager.cpp
+++ b/engines/sci/engine/seg_manager.cpp
@@ -959,15 +959,11 @@ SciBitmap *SegManager::allocateBitmap(reg_t *addr, const int16 width, const int1
offset = table->allocEntry();
*addr = make_reg(_bitmapSegId, offset);
- SciBitmap *bitmap = table->at(offset);
+ SciBitmap &bitmap = table->at(offset);
- if (bitmap == nullptr) {
- *addr = NULL_REG;
- }
-
- bitmap->create(width, height, skipColor, displaceX, displaceY, scaledWidth, scaledHeight, paletteSize, remap, gc);
+ bitmap.create(width, height, skipColor, displaceX, displaceY, scaledWidth, scaledHeight, paletteSize, remap, gc);
- return bitmap;
+ return &bitmap;
}
SciBitmap *SegManager::lookupBitmap(const reg_t addr) {
@@ -979,7 +975,7 @@ SciBitmap *SegManager::lookupBitmap(const reg_t addr) {
if (!bitmapTable.isValidEntry(addr.getOffset()))
error("Attempt to use invalid entry %04x:%04x as bitmap", PRINT_REG(addr));
- return (bitmapTable.at(addr.getOffset()));
+ return &(bitmapTable.at(addr.getOffset()));
}
void SegManager::freeBitmap(const reg_t addr) {
diff --git a/engines/sci/engine/segment.h b/engines/sci/engine/segment.h
index 97a6cb5..add5f4c 100644
--- a/engines/sci/engine/segment.h
+++ b/engines/sci/engine/segment.h
@@ -227,7 +227,7 @@ template<typename T>
struct SegmentObjTable : public SegmentObj {
typedef T value_type;
struct Entry {
- T data;
+ T *data;
int next_free; /* Only used for free entries */
};
enum { HEAPENTRY_INVALID = -1 };
@@ -243,6 +243,14 @@ public:
initTable();
}
+ ~SegmentObjTable() {
+ for (uint i = 0; i < _table.size(); i++) {
+ if (isValidEntry(i)) {
+ freeEntry(i);
+ }
+ }
+ }
+
void initTable() {
entries_used = 0;
first_free = HEAPENTRY_INVALID;
@@ -256,10 +264,13 @@ public:
first_free = _table[oldff].next_free;
_table[oldff].next_free = oldff;
+ assert(_table[oldff].data == nullptr);
+ _table[oldff].data = new T;
return oldff;
} else {
uint newIdx = _table.size();
_table.push_back(Entry());
+ _table.back().data = new T;
_table[newIdx].next_free = newIdx; // Tag as 'valid'
return newIdx;
}
@@ -278,6 +289,8 @@ public:
::error("Table::freeEntry: Attempt to release invalid table index %d", idx);
_table[idx].next_free = first_free;
+ delete _table[idx].data;
+ _table[idx].data = nullptr;
first_free = idx;
entries_used--;
}
@@ -292,8 +305,8 @@ public:
uint size() const { return _table.size(); }
- T &at(uint index) { return _table[index].data; }
- const T &at(uint index) const { return _table[index].data; }
+ T &at(uint index) { return *_table[index].data; }
+ const T &at(uint index) const { return *_table[index].data; }
T &operator[](uint index) { return at(index); }
const T &operator[](uint index) const { return at(index); }
@@ -353,8 +366,8 @@ struct HunkTable : public SegmentObjTable<Hunk> {
}
virtual void freeEntry(int idx) {
- SegmentObjTable<Hunk>::freeEntry(idx);
freeEntryContents(idx);
+ SegmentObjTable<Hunk>::freeEntry(idx);
}
virtual void freeAtAddress(SegManager *segMan, reg_t sub_addr) {
@@ -792,42 +805,14 @@ public:
virtual void saveLoadWithSerializer(Common::Serializer &ser);
};
-struct BitmapTable : public SegmentObjTable<SciBitmap *> {
- BitmapTable() : SegmentObjTable<SciBitmap *>(SEG_TYPE_BITMAP) {}
-
- virtual ~BitmapTable() {
- for (uint i = 0; i < _table.size(); i++) {
- if (isValidEntry(i)) {
- freeEntryContents(i);
- }
- }
- }
-
- int allocEntry() {
- int offset = SegmentObjTable<SciBitmap *>::allocEntry();
- at(offset) = new SciBitmap;
- return offset;
- }
-
- void freeEntryContents(const int offset) {
- delete at(offset);
- at(offset) = nullptr;
- }
-
- virtual void freeEntry(const int offset) override {
- SegmentObjTable<SciBitmap *>::freeEntry(offset);
- freeEntryContents(offset);
- }
-
- virtual void freeAtAddress(SegManager *segMan, reg_t sub_addr) override {
- freeEntry(sub_addr.getOffset());
- }
+struct BitmapTable : public SegmentObjTable<SciBitmap> {
+ BitmapTable() : SegmentObjTable<SciBitmap>(SEG_TYPE_BITMAP) {}
SegmentRef dereference(reg_t pointer) {
SegmentRef ret;
ret.isRaw = true;
- ret.maxSize = at(pointer.getOffset())->getRawSize();
- ret.raw = at(pointer.getOffset())->getRawData();
+ ret.maxSize = at(pointer.getOffset()).getRawSize();
+ ret.raw = at(pointer.getOffset()).getRawData();
return ret;
}
More information about the Scummvm-git-logs
mailing list