Skip to content

Commit

Permalink
SCI: Add a hack for a bug in the script handling code
Browse files Browse the repository at this point in the history
When resetting the segment manager, sometimes the locals block for a
script is placed in a segment smaller than the script itself. This
shouldn't be happening, but it isn't fatal, however it should be resolved
in a proper manner
  • Loading branch information
bluegr committed Jul 2, 2012
1 parent 4493511 commit f6e4312
Show file tree
Hide file tree
Showing 2 changed files with 22 additions and 27 deletions.
41 changes: 22 additions & 19 deletions engines/sci/engine/seg_manager.cpp
Expand Up @@ -143,16 +143,34 @@ Script *SegManager::allocateScript(int script_nr, SegmentId *segid) {
}

void SegManager::deallocate(SegmentId seg) {
if (!check(seg))
error("SegManager::deallocate(): invalid segment ID");
if (seg < 1 || (uint)seg >= _heap.size())
error("Attempt to deallocate an invalid segment ID");

SegmentObj *mobj = _heap[seg];
if (!mobj)
error("Attempt to deallocate an already freed segment");

if (mobj->getType() == SEG_TYPE_SCRIPT) {
Script *scr = (Script *)mobj;
_scriptSegMap.erase(scr->getScriptNumber());
if (scr->getLocalsSegment())
deallocate(scr->getLocalsSegment());
if (scr->getLocalsSegment()) {
// HACK: Check if the locals segment has already been deallocated.
// This happens sometimes in SQ4CD when resetting the segment
// manager: the locals for script 808 are somehow stored in a
// smaller segment than the script itself, so by the time the script
// is about to be freed, the locals block has already been freed.

This comment has been minimized.

Copy link
@wjp

wjp Jul 2, 2012

Contributor

By "smaller", do you really mean "with a smaller ID"?

This comment has been minimized.

Copy link
@bluegr

bluegr Jul 2, 2012

Author Member

Yes, a segment with a smaller ID. For me, script 808 was in segment 48 and its locals in segment 42, so its locals were deleted before the script itself when the segment manager was reset

This comment has been minimized.

Copy link
@wjp

wjp Jul 2, 2012

Contributor

I don't see why you say this shouldn't be happening, though. Scripts that are uninstantiated and then reinstantiated keep their segment id, but get a new localsSegment. And this new localsSegment may very well have a lower ID than the old one, because new segments get the lowest unused ID.

This comment has been minimized.

Copy link
@bluegr

bluegr Jul 2, 2012

Author Member

Hmm... yes, in fact you're absolutely right. So this is normal behavior. I'll remove the warning.

// This isn't fatal, but it shouldn't be happening at all.
// FIXME: Check why this happens. Perhaps there's a bug in the
// script handling code?
if (!_heap[scr->getLocalsSegment()]) {
warning("SegManager::deallocate(): The locals block of script "
"%d has already been deallocated. Script segment: %d, "
"locals segment: %d", scr->getScriptNumber(), seg,
scr->getLocalsSegment());
} else {
deallocate(scr->getLocalsSegment());
}
}
}

delete mobj;
Expand Down Expand Up @@ -307,21 +325,6 @@ reg_t SegManager::findObjectByName(const Common::String &name, int index) {
return result[index];
}

// validate the seg
// return:
// false - invalid seg
// true - valid seg
bool SegManager::check(SegmentId seg) {
if (seg < 1 || (uint)seg >= _heap.size()) {
return false;
}
if (!_heap[seg]) {
warning("SegManager: seg %x is removed from memory, but not removed from hash_map", seg);
return false;
}
return true;
}

// return the seg if script_id is valid and in the map, else 0
SegmentId SegManager::getScriptSegment(int script_id) const {
return _scriptSegMap.getVal(script_id, 0);
Expand Down
8 changes: 0 additions & 8 deletions engines/sci/engine/seg_manager.h
Expand Up @@ -471,14 +471,6 @@ class SegManager : public Common::Serializable {
void createClassTable();

SegmentId findFreeSegment() const;

/**
* Check segment validity
* @param[in] seg The segment to validate
* @return false if 'seg' is an invalid segment, true if
* 'seg' is a valid segment
*/
bool check(SegmentId seg);
};

} // End of namespace Sci
Expand Down

0 comments on commit f6e4312

Please sign in to comment.