[Scummvm-git-logs] scummvm master -> f5c2be5983250c14fc984f91d315c7e40b435b59

sluicebox noreply at scummvm.org
Wed Nov 22 06:04:22 UTC 2023


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

Summary:
f2d0b0b33a SCI: Prevent potential stack overflow in errorString
2bbf060740 SCI32: Update "Inside the Chest" description
8dac2204cd SCI: Clean up SegManager
62a5b6dc95 SCI: Refactor away SciEngine::exitGame
f5c2be5983 SWORD1: Improve struct copying. PVS-Studio V512


Commit: f2d0b0b33aae4a6e82b1e2d63f6a69e450906909
    https://github.com/scummvm/scummvm/commit/f2d0b0b33aae4a6e82b1e2d63f6a69e450906909
Author: sluicebox (22204938+sluicebox at users.noreply.github.com)
Date: 2023-11-21T22:02:53-08:00

Commit Message:
SCI: Prevent potential stack overflow in errorString

Changed paths:
    engines/sci/sci.cpp
    engines/sci/sci.h


diff --git a/engines/sci/sci.cpp b/engines/sci/sci.cpp
index e048d582cd9..836d630d61e 100644
--- a/engines/sci/sci.cpp
+++ b/engines/sci/sci.cpp
@@ -135,7 +135,8 @@ SciEngine::SciEngine(OSystem *syst, const ADGameDescription *desc, SciGameId gam
 	_console(nullptr),
 	_tts(nullptr),
 	_rng("sci"),
-	_forceHiresGraphics(false) {
+	_forceHiresGraphics(false),
+	_inErrorString(false) {
 
 	assert(g_sci == nullptr);
 	g_sci = this;
@@ -712,10 +713,19 @@ void SciEngine::runGame() {
 // When `error` is called, this function adds additional SCI engine context to the message
 // to help with bug reporting. It is critical that this function not crash, or else the
 // original error message will be lost and the debugger will be unavailable. This function
-// must not cause a second `error` call, or else it will infinitely recurse and crash with
-// stack overflow. This function must be cautious about the state it inspects, because it
-// can be called at any time during the engine lifecycle.
+// must not cause a second `error` call, or the original error message will also be unavailable,
+// although we detect this to prevent infinite recursion and crashing with a stack overflow.
+// This function must be cautious about the state it inspects, because it can be called at
+// any time during the engine lifecycle.
 void SciEngine::errorString(const char *buf_input, char *buf_output, int buf_output_size) {
+	// safeguard to prevent infinite recursion in case there's a code path that calls `error`.
+	if (_inErrorString) {
+		warning("error called during errorString");
+		Common::strlcpy(buf_output, buf_input, buf_output_size);
+		return;
+	}
+	_inErrorString = true;
+
 	// Detailed context can only be included if VM execution has begun.
 	EngineState *s = _gamestate;
 	if (s != nullptr && !s->_executionStack.empty() && _kernel != nullptr) {
@@ -791,6 +801,7 @@ void SciEngine::errorString(const char *buf_input, char *buf_output, int buf_out
 		// VM not initialized yet, so just copy over the target name and error message.
 		snprintf(buf_output, buf_output_size, "[%s]: %s", _targetName.c_str(), buf_input);
 	}
+	_inErrorString = false;
 }
 
 void SciEngine::exitGame() {
diff --git a/engines/sci/sci.h b/engines/sci/sci.h
index 0339abb51a3..6b36b01ca46 100644
--- a/engines/sci/sci.h
+++ b/engines/sci/sci.h
@@ -433,6 +433,7 @@ private:
 	Common::RandomSource _rng;
 	Common::MacResManager _macExecutable;
 	bool _forceHiresGraphics; // user-option for GK1, KQ6, PQ4
+	bool _inErrorString; /**< Set while `errorString` is executing */
 };
 
 /**


Commit: 2bbf060740f699e8722dcb258dcb22cd9ad4bcad
    https://github.com/scummvm/scummvm/commit/2bbf060740f699e8722dcb258dcb22cd9ad4bcad
Author: sluicebox (22204938+sluicebox at users.noreply.github.com)
Date: 2023-11-21T22:02:53-08:00

Commit Message:
SCI32: Update "Inside the Chest" description

Changed paths:
    engines/sci/detection_tables.h


diff --git a/engines/sci/detection_tables.h b/engines/sci/detection_tables.h
index 584b3ecffb9..6b0279e2724 100644
--- a/engines/sci/detection_tables.h
+++ b/engines/sci/detection_tables.h
@@ -206,6 +206,9 @@ static const struct ADGameDescription SciGameDescriptions[] = {
 
 	// Inside the Chest / Behind the Developer's Shield
 	// SCI interpreter version 2.000.000
+	// This demo would display a different title and logo depending on which batch
+	// file was run: CHEST.BAT or SHIELD.BAT. The second would use SHIELD.CFG
+	// and include the SHIELD patch directory and change the title.
 	{"chest", "", {
 		{"resource.map", 0, "9dd015e79cac4f91e7de805448f39775", 1912},
 		{"resource.000", 0, "e4efcd042f86679dd4e1834bb3a38edb", 3770943},


Commit: 8dac2204cda544ed5d2094800f9d68532d3c44be
    https://github.com/scummvm/scummvm/commit/8dac2204cda544ed5d2094800f9d68532d3c44be
Author: sluicebox (22204938+sluicebox at users.noreply.github.com)
Date: 2023-11-21T22:02:54-08:00

Commit Message:
SCI: Clean up SegManager

Fewer casts, pointers, and parameters

Changed paths:
    engines/sci/engine/script.cpp
    engines/sci/engine/seg_manager.cpp
    engines/sci/engine/seg_manager.h
    engines/sci/sci.cpp


diff --git a/engines/sci/engine/script.cpp b/engines/sci/engine/script.cpp
index 9e59577a9f9..230c1593f9b 100644
--- a/engines/sci/engine/script.cpp
+++ b/engines/sci/engine/script.cpp
@@ -979,12 +979,14 @@ LocalVariables *Script::allocLocalsSegment(SegManager *segMan) {
 	} else {
 		LocalVariables *locals;
 
-		if (_localsSegment) {
+		if (!_localsSegment) {
+			locals = new LocalVariables();
+			_localsSegment = segMan->allocSegment(locals);
+		} else {
 			locals = (LocalVariables *)segMan->getSegment(_localsSegment, SEG_TYPE_LOCALS);
 			if (!locals || locals->getType() != SEG_TYPE_LOCALS || locals->script_id != getScriptNumber())
 				error("Invalid script %d locals segment while allocating locals", _nr);
-		} else
-			locals = (LocalVariables *)segMan->allocSegment(new LocalVariables(), &_localsSegment);
+		}
 
 		_localsBlock = locals;
 		locals->script_id = getScriptNumber();
diff --git a/engines/sci/engine/seg_manager.cpp b/engines/sci/engine/seg_manager.cpp
index 30463d71164..d3d138e220f 100644
--- a/engines/sci/engine/seg_manager.cpp
+++ b/engines/sci/engine/seg_manager.cpp
@@ -109,40 +109,39 @@ SegmentId SegManager::findFreeSegment() const {
 	return seg;
 }
 
-SegmentObj *SegManager::allocSegment(SegmentObj *mem, SegmentId *segid) {
+SegmentId SegManager::allocSegment(SegmentObj *mobj) {
+	if (!mobj)
+		error("SegManager: invalid mobj");
+
 	// Find a free segment
 	SegmentId id = findFreeSegment();
-	if (segid)
-		*segid = id;
-
-	if (!mem)
-		error("SegManager: invalid mobj");
 
 	// ... and put it into the (formerly) free segment.
-	if (id >= (int)_heap.size()) {
-		assert(id == (int)_heap.size());
+	if (id >= _heap.size()) {
+		assert(id == _heap.size());
 		_heap.push_back(0);
 	}
-	_heap[id] = mem;
+	_heap[id] = mobj;
 
-	return mem;
+	return id;
 }
 
-Script *SegManager::allocateScript(int script_nr, SegmentId *segid) {
+Script *SegManager::allocateScript(int script_nr, SegmentId &segid) {
 	// Check if the script already has an allocated segment. If it
 	// does, return that segment.
-	*segid = _scriptSegMap.getValOrDefault(script_nr, 0);
-	if (*segid > 0) {
-		return (Script *)_heap[*segid];
+	segid = _scriptSegMap.getValOrDefault(script_nr, 0);
+	if (segid > 0) {
+		return (Script *)_heap[segid];
 	}
 
 	// allocate the SegmentObj
-	SegmentObj *mem = allocSegment(new Script(), segid);
+	Script *script = new Script();
+	segid = allocSegment(script);
 
 	// Add the script to the "script id -> segment id" hashmap
-	_scriptSegMap[script_nr] = *segid;
+	_scriptSegMap[script_nr] = segid;
 
-	return (Script *)mem;
+	return script;
 }
 
 SegmentId SegManager::getActualSegment(SegmentId seg) const {
@@ -156,7 +155,7 @@ SegmentId SegManager::getActualSegment(SegmentId seg) const {
 
 void SegManager::deallocate(SegmentId seg) {
 	SegmentId actualSegment = getActualSegment(seg);
-	if (actualSegment < 1 || (uint)actualSegment >= _heap.size())
+	if (actualSegment < 1 || actualSegment >= _heap.size())
 		error("Attempt to deallocate an invalid segment ID");
 
 	SegmentObj *mobj = _heap[actualSegment];
@@ -180,7 +179,7 @@ void SegManager::deallocate(SegmentId seg) {
 	}
 
 	delete mobj;
-	_heap[actualSegment] = NULL;
+	_heap[actualSegment] = nullptr;
 }
 
 bool SegManager::isHeapObject(reg_t pos) const {
@@ -197,23 +196,29 @@ void SegManager::deallocateScript(int script_nr) {
 
 Script *SegManager::getScript(const SegmentId seg) {
 	SegmentId actualSegment = getActualSegment(seg);
-	if (actualSegment < 1 || (uint)actualSegment >= _heap.size()) {
+	if (actualSegment < 1 || actualSegment >= _heap.size()) {
 		error("SegManager::getScript(): seg id %x out of bounds", actualSegment);
 	}
-	if (!_heap[actualSegment]) {
+	SegmentObj *mobj = _heap[actualSegment];
+	if (mobj == nullptr) {
 		error("SegManager::getScript(): seg id %x is not in memory", actualSegment);
 	}
-	if (_heap[actualSegment]->getType() != SEG_TYPE_SCRIPT) {
-		error("SegManager::getScript(): seg id %x refers to type %d != SEG_TYPE_SCRIPT", actualSegment, _heap[actualSegment]->getType());
+	if (mobj->getType() != SEG_TYPE_SCRIPT) {
+		error("SegManager::getScript(): seg id %x refers to type %d != SEG_TYPE_SCRIPT", actualSegment, mobj->getType());
 	}
-	return (Script *)_heap[actualSegment];
+	return (Script *)mobj;
 }
 
 Script *SegManager::getScriptIfLoaded(const SegmentId seg) const {
 	SegmentId actualSegment = getActualSegment(seg);
-	if (actualSegment < 1 || (uint)actualSegment >= _heap.size() || !_heap[actualSegment] || _heap[actualSegment]->getType() != SEG_TYPE_SCRIPT)
+	if (actualSegment < 1 || actualSegment >= _heap.size()) {
 		return nullptr;
-	return (Script *)_heap[actualSegment];
+	}
+	SegmentObj *mobj = _heap[actualSegment];
+	if (mobj == nullptr || mobj->getType() != SEG_TYPE_SCRIPT) {
+		return nullptr;
+	}
+	return (Script *)mobj;
 }
 
 SegmentId SegManager::findSegmentByType(int type) const {
@@ -225,21 +230,22 @@ SegmentId SegManager::findSegmentByType(int type) const {
 
 SegmentObj *SegManager::getSegmentObj(SegmentId seg) const {
 	SegmentId actualSegment = getActualSegment(seg);
-	if (actualSegment < 1 || (uint)actualSegment >= _heap.size() || !_heap[actualSegment])
+	if (actualSegment < 1 || actualSegment >= _heap.size() || !_heap[actualSegment])
 		return nullptr;
 	return _heap[actualSegment];
 }
 
 SegmentType SegManager::getSegmentType(SegmentId seg) const {
 	SegmentId actualSegment = getActualSegment(seg);
-	if (actualSegment < 1 || (uint)actualSegment >= _heap.size() || !_heap[actualSegment])
+	if (actualSegment < 1 || actualSegment >= _heap.size() || !_heap[actualSegment])
 		return SEG_TYPE_INVALID;
 	return _heap[actualSegment]->getType();
 }
 
 SegmentObj *SegManager::getSegment(SegmentId seg, SegmentType type) const {
 	SegmentId actualSegment = getActualSegment(seg);
-	return getSegmentType(actualSegment) == type ? _heap[actualSegment] : NULL;
+	SegmentType actualSegmentType = getSegmentType(actualSegment);
+	return (actualSegmentType == type) ? _heap[actualSegment] : nullptr;
 }
 
 Object *SegManager::getObject(reg_t pos) const {
@@ -398,12 +404,10 @@ SegmentId SegManager::getScriptSegment(int script_id) const {
 }
 
 SegmentId SegManager::getScriptSegment(int script_nr, ScriptLoadType load, bool applyScriptPatches) {
-	SegmentId segment;
-
 	if ((load & SCRIPT_GET_LOAD) == SCRIPT_GET_LOAD)
 		instantiateScript(script_nr, applyScriptPatches);
 
-	segment = getScriptSegment(script_nr);
+	SegmentId segment = getScriptSegment(script_nr);
 
 	if (segment > 0) {
 		if ((load & SCRIPT_GET_LOCK) == SCRIPT_GET_LOCK)
@@ -412,20 +416,20 @@ SegmentId SegManager::getScriptSegment(int script_nr, ScriptLoadType load, bool
 	return segment;
 }
 
-DataStack *SegManager::allocateStack(int size, SegmentId *segid) {
-	SegmentObj *mobj = allocSegment(new DataStack(), segid);
-	DataStack *retval = (DataStack *)mobj;
+DataStack *SegManager::allocateStack(int size) {
+	DataStack *stack = new DataStack();
+	allocSegment(stack);
 
-	retval->_entries = (reg_t *)calloc(size, sizeof(reg_t));
-	retval->_capacity = size;
+	stack->_entries = (reg_t *)calloc(size, sizeof(reg_t));
+	stack->_capacity = size;
 
 	// SSCI initializes the stack with "S" characters (uppercase S in SCI0-SCI1,
 	// lowercase s in SCI0 and SCI11) - probably stands for "stack"
 	byte filler = (getSciVersion() >= SCI_VERSION_01 && getSciVersion() <= SCI_VERSION_1_LATE) ? 'S' : 's';
 	for (int i = 0; i < size; i++)
-		retval->_entries[i] = make_reg(0, filler);
+		stack->_entries[i].setOffset(filler);
 
-	return retval;
+	return stack;
 }
 
 void SegManager::freeHunkEntry(reg_t addr) {
@@ -446,13 +450,15 @@ void SegManager::freeHunkEntry(reg_t addr) {
 
 reg_t SegManager::allocateHunkEntry(const char *hunk_type, int size) {
 	HunkTable *table;
-	int offset;
 
-	if (!_hunksSegId)
-		allocSegment(new HunkTable(), &(_hunksSegId));
-	table = (HunkTable *)_heap[_hunksSegId];
+	if (!_hunksSegId) {
+		table = new HunkTable();
+		_hunksSegId = allocSegment(table);
+	} else {
+		table = (HunkTable *)_heap[_hunksSegId];
+	}
 
-	offset = table->allocEntry();
+	int offset = table->allocEntry();
 
 	reg_t addr = make_reg(_hunksSegId, offset);
 	Hunk &h = table->at(offset);
@@ -465,26 +471,27 @@ reg_t SegManager::allocateHunkEntry(const char *hunk_type, int size) {
 }
 
 byte *SegManager::getHunkPointer(reg_t addr) {
-	HunkTable *ht = (HunkTable *)getSegment(addr.getSegment(), SEG_TYPE_HUNK);
+	HunkTable *table = (HunkTable *)getSegment(addr.getSegment(), SEG_TYPE_HUNK);
 
-	if (!ht || !ht->isValidEntry(addr.getOffset())) {
+	if (table == nullptr || !table->isValidEntry(addr.getOffset())) {
 		// Valid SCI behavior, e.g. when loading/quitting
 		return nullptr;
 	}
 
-	return (byte *)ht->at(addr.getOffset()).mem;
+	return (byte *)table->at(addr.getOffset()).mem;
 }
 
 Clone *SegManager::allocateClone(reg_t *addr) {
 	CloneTable *table;
-	int offset;
 
-	if (!_clonesSegId)
-		table = (CloneTable *)allocSegment(new CloneTable(), &(_clonesSegId));
-	else
+	if (!_clonesSegId) {
+		table = new CloneTable();
+		_clonesSegId = allocSegment(table);
+	} else {
 		table = (CloneTable *)_heap[_clonesSegId];
+	}
 
-	offset = table->allocEntry();
+	int offset = table->allocEntry();
 
 	*addr = make_reg(_clonesSegId, offset);
 	return &table->at(offset);
@@ -492,13 +499,15 @@ Clone *SegManager::allocateClone(reg_t *addr) {
 
 List *SegManager::allocateList(reg_t *addr) {
 	ListTable *table;
-	int offset;
 
-	if (!_listsSegId)
-		allocSegment(new ListTable(), &(_listsSegId));
-	table = (ListTable *)_heap[_listsSegId];
+	if (!_listsSegId) {
+		table = new ListTable();
+		_listsSegId = allocSegment(table);
+	} else {
+		table = (ListTable *)_heap[_listsSegId];
+	}
 
-	offset = table->allocEntry();
+	int offset = table->allocEntry();
 
 	*addr = make_reg(_listsSegId, offset);
 	return &table->at(offset);
@@ -506,13 +515,15 @@ List *SegManager::allocateList(reg_t *addr) {
 
 Node *SegManager::allocateNode(reg_t *addr) {
 	NodeTable *table;
-	int offset;
 
-	if (!_nodesSegId)
-		allocSegment(new NodeTable(), &(_nodesSegId));
-	table = (NodeTable *)_heap[_nodesSegId];
+	if (!_nodesSegId) {
+		table = new NodeTable();
+		_nodesSegId = allocSegment(table);
+	} else {
+		table = (NodeTable *)_heap[_nodesSegId];
+	}
 
-	offset = table->allocEntry();
+	int offset = table->allocEntry();
 
 	*addr = make_reg(_nodesSegId, offset);
 	return &table->at(offset);
@@ -895,25 +906,23 @@ Common::String SegManager::getString(reg_t pointer) {
 }
 
 byte *SegManager::allocDynmem(int size, const char *descr, reg_t *addr) {
-	SegmentId seg;
-	SegmentObj *mobj = allocSegment(new DynMem(), &seg);
-	*addr = make_reg(seg, 0);
+	DynMem *dynmem = new DynMem();
+	SegmentId segid = allocSegment(dynmem);
+	*addr = make_reg(segid, 0);
 
-	DynMem &d = *(DynMem *)mobj;
-
-	d._size = size;
+	dynmem->_size = size;
 
 	// Original SCI only zeroed out heap memory on initialize
 	// They didn't do it again for every allocation
 	if (size) {
-		d._buf = (byte *)calloc(size, 1);
+		dynmem->_buf = (byte *)calloc(size, 1);
 	} else {
-		d._buf = nullptr;
+		dynmem->_buf = nullptr;
 	}
 
-	d._description = descr;
+	dynmem->_description = descr;
 
-	return (byte *)(d._buf);
+	return dynmem->_buf;
 }
 
 bool SegManager::freeDynmem(reg_t addr) {
@@ -932,14 +941,15 @@ bool SegManager::freeDynmem(reg_t addr) {
 
 SciArray *SegManager::allocateArray(SciArrayType type, uint16 size, reg_t *addr) {
 	ArrayTable *table;
-	int offset;
 
 	if (!_arraysSegId) {
-		table = (ArrayTable *)allocSegment(new ArrayTable(), &(_arraysSegId));
-	} else
+		table = new ArrayTable();
+		_arraysSegId = allocSegment(table);
+	} else {
 		table = (ArrayTable *)_heap[_arraysSegId];
+	}
 
-	offset = table->allocEntry();
+	int offset = table->allocEntry();
 
 	*addr = make_reg(_arraysSegId, offset);
 
@@ -987,15 +997,15 @@ bool SegManager::isArray(reg_t addr) const {
 
 SciBitmap *SegManager::allocateBitmap(reg_t *addr, const int16 width, const int16 height, const uint8 skipColor, const int16 originX, const int16 originY, const int16 xResolution, const int16 yResolution, const uint32 paletteSize, const bool remap, const bool gc) {
 	BitmapTable *table;
-	int offset;
 
 	if (!_bitmapSegId) {
-		table = (BitmapTable *)allocSegment(new BitmapTable(), &(_bitmapSegId));
+		table = new BitmapTable();
+		_bitmapSegId = allocSegment(table);
 	} else {
 		table = (BitmapTable *)_heap[_bitmapSegId];
 	}
 
-	offset = table->allocEntry();
+	int offset = table->allocEntry();
 
 	*addr = make_reg(_bitmapSegId, offset);
 	SciBitmap &bitmap = table->at(offset);
@@ -1086,7 +1096,7 @@ int SegManager::instantiateScript(int scriptNum, bool applyScriptPatches) {
 			scr->freeScript(true);
 		}
 	} else {
-		scr = allocateScript(scriptNum, &segmentId);
+		scr = allocateScript(scriptNum, segmentId);
 	}
 
 	scr->load(scriptNum, _resMan, _scriptPatcher, applyScriptPatches);
diff --git a/engines/sci/engine/seg_manager.h b/engines/sci/engine/seg_manager.h
index 7fa958d2a2a..556d11179a0 100644
--- a/engines/sci/engine/seg_manager.h
+++ b/engines/sci/engine/seg_manager.h
@@ -67,11 +67,10 @@ public:
 	/**
 	 * Allocate a script into the segment manager.
 	 * @param script_nr		The number of the script to load
-	 * @param seg_id		The segment ID of the newly allocated segment,
-	 * 						on success
-	 * @return				0 on failure, 1 on success
+	 * @param seg_id		The segment ID of the newly allocated segment.
+	 * @return				The script
 	 */
-	Script *allocateScript(int script_nr, SegmentId *seg_id);
+	Script *allocateScript(int script_nr, SegmentId &seg_id);
 
 	// The script must then be initialized; see section (1b.), below.
 
@@ -169,10 +168,9 @@ public:
 	/**
 	 * Allocates a data stack
 	 * @param size	Number of stack entries to reserve
-	 * @param segid	Segment ID of the stack
-	 * @return		The physical stack
+	 * @return		The data stack
 	 */
-	DataStack *allocateStack(int size, SegmentId *segid);
+	DataStack *allocateStack(int size);
 
 
 	// 5. System Strings
@@ -486,7 +484,7 @@ private:
 #endif
 
 public:
-	SegmentObj *allocSegment(SegmentObj *mem, SegmentId *segid);
+	SegmentId allocSegment(SegmentObj *mobj);
 
 private:
 	void deallocate(SegmentId seg);
diff --git a/engines/sci/sci.cpp b/engines/sci/sci.cpp
index 836d630d61e..620373eef7d 100644
--- a/engines/sci/sci.cpp
+++ b/engines/sci/sci.cpp
@@ -540,7 +540,7 @@ void SciEngine::suggestDownloadGK2SubTitlesPatch() {
 bool SciEngine::initGame() {
 	// Script 0 needs to be allocated here before anything else!
 	int script0Segment = _gamestate->_segMan->getScriptSegment(0, SCRIPT_GET_LOCK);
-	DataStack *stack = _gamestate->_segMan->allocateStack(VM_STACK_SIZE, nullptr);
+	DataStack *stack = _gamestate->_segMan->allocateStack(VM_STACK_SIZE);
 
 	_gamestate->_msgState = new MessageState(_gamestate->_segMan);
 	_gamestate->gcCountDown = GC_INTERVAL - 1;


Commit: 62a5b6dc954f83fb86af848ea394ed963b5cbb6f
    https://github.com/scummvm/scummvm/commit/62a5b6dc954f83fb86af848ea394ed963b5cbb6f
Author: sluicebox (22204938+sluicebox at users.noreply.github.com)
Date: 2023-11-21T22:02:54-08:00

Commit Message:
SCI: Refactor away SciEngine::exitGame

Changed paths:
    engines/sci/sci.cpp
    engines/sci/sci.h


diff --git a/engines/sci/sci.cpp b/engines/sci/sci.cpp
index 620373eef7d..88dc5ba27e4 100644
--- a/engines/sci/sci.cpp
+++ b/engines/sci/sci.cpp
@@ -677,11 +677,32 @@ void SciEngine::runGame() {
 	do {
 		_gamestate->_executionStackPosChanged = false;
 		run_vm(_gamestate);
-		exitGame();
+
+		// Stop audio and sound components, unless loading a game.
+		// EngineState::saveLoadWithSerializer has already handled that.
+		if (_gamestate->abortScriptProcessing != kAbortLoadGame) {
+			if (_audio) { // SCI16
+				_audio->stopAllAudio();
+			}
+			_sync->stop();
+			_soundCmd->clearPlayList();
+		}
+
+		// Clear execution stack
+		_gamestate->_executionStack.clear();
+		_gamestate->xs = nullptr;
+
+		// Close all opened file handles
+		_gamestate->_fileHandles.clear();
+		_gamestate->_fileHandles.resize(5);
 
 		_guestAdditions->sciEngineRunGameHook();
 
 		if (_gamestate->abortScriptProcessing == kAbortRestartGame) {
+			// SCI16 game has been restarted with kRestartGame16.
+			// Reset engine state and prepare the VM to call the play method
+			// on the next iteration, but set the gameIsRestarting flag so
+			// that scripts can detect the restart with kGameIsRestarting.
 			_gamestate->_segMan->resetSegMan();
 			initGame();
 			initStackBaseWithSelector(SELECTOR(play));
@@ -694,8 +715,10 @@ void SciEngine::runGame() {
 			_gamestate->abortScriptProcessing = kAbortNone;
 			_guestAdditions->reset();
 		} else if (_gamestate->abortScriptProcessing == kAbortLoadGame) {
+			// Game has been restored from within the game or the launcher.
+			// Prepare the VM to call the replay method of the game object
+			// on the next iteration.
 			_gamestate->abortScriptProcessing = kAbortNone;
-			_gamestate->_executionStack.clear();
 			initStackBaseWithSelector(SELECTOR(replay));
 			_guestAdditions->patchGameSaveRestore();
 			setLauncherLanguage();
@@ -804,25 +827,6 @@ void SciEngine::errorString(const char *buf_input, char *buf_output, int buf_out
 	_inErrorString = false;
 }
 
-void SciEngine::exitGame() {
-	if (_gamestate->abortScriptProcessing != kAbortLoadGame) {
-		_gamestate->_executionStack.clear();
-		if (_audio) {
-			_audio->stopAllAudio();
-		}
-		_sync->stop();
-		_soundCmd->clearPlayList();
-	}
-
-	// TODO Free parser segment here
-
-	// TODO Free scripts here
-
-	// Close all opened file handles
-	_gamestate->_fileHandles.clear();
-	_gamestate->_fileHandles.resize(5);
-}
-
 // Invoked by debugger when a severe error occurs
 void SciEngine::severeError() {
 	if (_gamestate) {
diff --git a/engines/sci/sci.h b/engines/sci/sci.h
index 6b36b01ca46..2dadd0aebe6 100644
--- a/engines/sci/sci.h
+++ b/engines/sci/sci.h
@@ -365,17 +365,6 @@ private:
 	 * or restoring a game re-initializes certain states and continues the loop.
 	 */
 	void runGame();
-
-	/**
-	 * "Uninitializes an initialized SCI game" was the original description.
-	 * This is only called by runGame immediately after calling run_vm.
-	 * It uninitalizes some engine state depending on the abort flag, but it also
-	 * has old TODO comments and doesn't uninitialize the heap. runGame does
-	 * just as much uninitialization after calling this, so maybe exitGame's
-	 * code should just be moved into runGame instead of splitting up the
-	 * re-initialization steps.
-	 */
-	void exitGame();
 	
 	/**
 	 * Initializes the stack to call a method in the game object once the VM starts.


Commit: f5c2be5983250c14fc984f91d315c7e40b435b59
    https://github.com/scummvm/scummvm/commit/f5c2be5983250c14fc984f91d315c7e40b435b59
Author: sluicebox (22204938+sluicebox at users.noreply.github.com)
Date: 2023-11-21T22:02:55-08:00

Commit Message:
SWORD1: Improve struct copying. PVS-Studio V512

This code copied structs by taking the address of the struct's
first field instead of the struct itself

Changed paths:
    engines/sword1/logic.cpp


diff --git a/engines/sword1/logic.cpp b/engines/sword1/logic.cpp
index e60ed62105f..ab77ac23b6b 100644
--- a/engines/sword1/logic.cpp
+++ b/engines/sword1/logic.cpp
@@ -243,7 +243,7 @@ void Logic::processLogic(Object *compact, uint32 id) {
 			logicRet = 1;
 			break;
 		case LOGIC_bookmark:
-			memcpy(&(compact->o_tree.o_script_level), &(compact->o_bookmark.o_script_level), sizeof(ScriptTree));
+			memcpy(&(compact->o_tree), &(compact->o_bookmark), sizeof(ScriptTree));
 			if (id == GMASTER_79) {
 				// workaround for ending script.
 				// GMASTER_79 is not prepared for mega_interact receiving INS_quit
@@ -1101,7 +1101,7 @@ int Logic::fnRestartScript(Object *cpt, int32 id, int32 c, int32 d, int32 e, int
 }
 
 int Logic::fnSetBookmark(Object *cpt, int32 id, int32 c, int32 d, int32 e, int32 f, int32 z, int32 x) {
-	memcpy(&cpt->o_bookmark.o_script_level, &cpt->o_tree.o_script_level, sizeof(ScriptTree));
+	memcpy(&cpt->o_bookmark, &cpt->o_tree, sizeof(ScriptTree));
 	return SCRIPT_CONT;
 }
 




More information about the Scummvm-git-logs mailing list