[Scummvm-tracker] [ScummVM :: Bugs] #13841: ENGINES: Setting a file's _saveType value by where it’s stored can cause a regular save to be overwritten.
ScummVM :: Bugs
trac at scummvm.org
Mon Sep 12 07:13:14 UTC 2022
#13841: ENGINES: Setting a file's _saveType value by where it’s stored can cause a
regular save to be overwritten.
-------------------+---------------------
Reporter: macca8 | Owner: (none)
Type: defect | Status: new
Priority: high | Component: Common
Version: | Resolution:
Keywords: | Game:
-------------------+---------------------
Description changed by macca8:
Old description:
> The golden rule of autosaving is that an autosave should NEVER overwrite
> a user’s regular save.
>
> Recently, the autosave test was switched back to isAutosave() to resolve
> #13703
> ([https://github.com/scummvm/scummvm/commit/9b05c206993d3107f98ac5b437801e33ba3c79b7
> 9b05c206]), after having previously used hasAutosaveName() exclusively.
>
> This latest switch has revealed a nasty bug which sets a save file’s
> _saveType property value to kSaveTypeAutosave, regardless of the file’s
> true type. This allows an autosave to automatically overwrite any regular
> save that may be stored in the autosave slot of affected engines (e.g.
> ZGI).
>
> This appears to originate from an expectation that the saveType test
> wouldn’t be reinstated to the autosave test.
>
> It’s due to an alternative method introduced while the saveType test was
> removed from autosave testing, which sets the _saveType value based on
> the slot in which the file is stored. Apparently, this was to allow
> isAutosave() to replace calls of type "slot == autosave slot", ensuring
> it would always return true if a file was found in the autosave slot
> (refer [https://github.com/scummvm/scummvm/pull/3261 PR3261], and in
> particular
> [https://github.com/scummvm/scummvm/commit/7adad5aaf5831dc5adcee140f38aacc4a5db2518
> 7adad5aa] for the original changes specific to this issue).
>
> Now that the saveType test has been restored to its intended purpose of
> identifying a save file’s type, changes relevant to this alternative
> method should be reverted as soon as possible, to restore the integrity
> of the autosave system. It offers no objective way of differentiating
> between an autosave and a regular save, and is therefore inappropriate
> for determining which type of save is stored in the autosave slot.
>
> As it stands, we can expect this bug to affect most engines whose
> querySaveMetaInfos() method returns a SaveStateDescriptor instance of
> type SSD(slot, description). It’s initiated when creating the SSD, and
> overwrites any previous _saveType value which may have been stored in the
> save file’s metadata. Note that engines which return a SSD without
> parameters appear to be unaffected, and retain their original _saveType
> values (e.g. Myst III, Riven).
>
> Hopefully, most users will have removed any regular saves from their more
> popular games, via the in-game warning dialog, before this latest switch
> was effected.
>
> However, as a matter of urgency, the changes in engines/savestate.cpp &
> engines/metaengine.cpp should be reverted or adjusted as appropriate, to
> remove any further risk to users.
>
> Proposed changes for consideration to complement removal of _saveType
> initialization from initSaveType() function (including renaming) in
> engines/savestate.cpp:
> {{{
>
> // restore default _saveType & rename initSaveType() call in SSD
> definitions
>
> SaveStateDescriptor::SaveStateDescriptor(const MetaEngine *metaEngine,
> int slot, const Common::U32String &d)
> : _slot(slot), _description(d), _isLocked(false),
> _playTimeMSecs(0), _saveType(kSaveTypeUndetermined) {
> initSaveSlot(metaEngine);
> }
>
> SaveStateDescriptor::SaveStateDescriptor(const MetaEngine *metaEngine,
> int slot, const Common::String &d)
> : _slot(slot), _description(Common::U32String(d)),
> _isLocked(false), _playTimeMSecs(0), _saveType(kSaveTypeUndetermined) {
> initSaveSlot(metaEngine);
> }
>
> // remove _saveType initialisation & rename function to reflect change
>
> void SaveStateDescriptor::initSaveSlot(const MetaEngine *metaEngine) {
> // Do not allow auto-save slot to be deleted or overwritten.
> if (!metaEngine && g_engine)
> metaEngine = g_engine->getMetaEngine();
> const bool autosave =
> metaEngine && ConfMan.getInt("autosave_period")
> && _slot == metaEngine->getAutosaveSlot();
> _isWriteProtected = autosave;
> _isDeletable = !autosave;
> }
>
> // rename function declaration (in engines/savestate.h):
>
> void initSaveSlot(const MetaEngine *metaEngine);
>
> }}}
>
> Revert relevant changes to listSaves() & querySaveMetaInfos() functions
> in engines/metaengine.cpp:
> {{{
> // replace isAutosave() call with previous test
>
> SaveStateList MetaEngine::listSaves(const char *target, bool saveMode)
> const {
> SaveStateList saveList = listSaves(target);
> int autosaveSlot = ConfMan.getInt("autosave_period") ?
> getAutosaveSlot() : -1;
> if (!saveMode || autosaveSlot == -1)
> return saveList;
>
> // Check to see if an autosave is present
> for (SaveStateList::iterator it = saveList.begin(); it !=
> saveList.end(); ++it) {
> int slot = it->getSaveSlot();
> if (slot == autosaveSlot) {
> // It has an autosave
> return saveList;
> }
> }
>
> // No autosave yet. We want to add a dummy one in so that it can
> be marked as
> // write protected, and thus be prevented from being saved in
> SaveStateDescriptor desc(this, autosaveSlot, _("Autosave"));
> saveList.push_back(desc);
> Common::sort(saveList.begin(), saveList.end(),
> SaveStateDescriptorSlotComparator());
>
> return saveList;
> }
>
> // restore desc.setAutosave(header.isAutosave) call
>
> SaveStateDescriptor MetaEngine::querySaveMetaInfos(const char *target,
> int slot) const {
> if (!hasFeature(kSavesUseExtendedFormat))
> return SaveStateDescriptor();
>
> Common::ScopedPtr<Common::InSaveFile>
> f(g_system->getSavefileManager()->openForLoading(
> getSavegameFile(slot, target)));
>
> if (f) {
> ExtendedSavegameHeader header;
> if (!readSavegameHeader(f.get(), &header, false)) {
> return SaveStateDescriptor();
> }
>
> // Create the return descriptor
> SaveStateDescriptor desc(this, slot,
> Common::U32String());
> parseSavegameHeader(&header, &desc);
> desc.setThumbnail(header.thumbnail);
> desc.setAutosave(header.isAutosave);
> return desc;
> }
>
> return SaveStateDescriptor();
> }
> }}}
>
> Individual engines targeted in
> [https://github.com/scummvm/scummvm/commit/7adad5aaf5831dc5adcee140f38aacc4a5db2518
> 7adad5aa] will need to be reviewed for potential changes, though most
> only involved the removal of calls setting the writeProtected & Deletable
> flags (which can be ignored).
New description:
The golden rule of autosaving is that an autosave should NEVER overwrite a
user’s regular save.
Recently, the autosave test was switched back to isAutosave() to resolve
#13703
([https://github.com/scummvm/scummvm/commit/9b05c206993d3107f98ac5b437801e33ba3c79b7
9b05c206]), after having previously used hasAutosaveName() exclusively.
This latest switch has revealed a nasty bug which sets a save file’s
_saveType property value to kSaveTypeAutosave, regardless of the file’s
true type. This allows an autosave to automatically overwrite any regular
save that may be stored in the autosave slot of affected engines (e.g.
ZGI).
This appears to originate from an expectation that the saveType test
wouldn’t be reinstated to the autosave test (refer #13842 for why this
test is required).
It’s due to an alternative method introduced while the saveType test was
removed from autosave testing, which sets the _saveType value based on the
slot in which the file is stored. Apparently, this was to allow
isAutosave() to replace calls of type "slot == autosave slot", ensuring it
would always return true if a file was found in the autosave slot (refer
[https://github.com/scummvm/scummvm/pull/3261 PR3261], and in particular
[https://github.com/scummvm/scummvm/commit/7adad5aaf5831dc5adcee140f38aacc4a5db2518
7adad5aa] for the original changes specific to this issue).
Now that the saveType test has been restored to its intended purpose of
identifying a save file’s type, changes relevant to this alternative
method should be reverted as soon as possible, to restore the integrity of
the autosave system. It offers no objective way of differentiating between
an autosave and a regular save, and is therefore inappropriate for
determining which type of save is stored in the autosave slot.
As it stands, we can expect this bug to affect most engines whose
querySaveMetaInfos() method returns a SaveStateDescriptor instance of type
SSD(slot, description). It’s initiated when creating the SSD, and
overwrites any previous _saveType value which may have been stored in the
save file’s metadata. Note that engines which return a SSD without
parameters appear to be unaffected, and retain their original _saveType
values (e.g. Myst III, Riven).
Hopefully, most users will have removed any regular saves from their more
popular games, via the in-game warning dialog, before this latest switch
was effected.
However, as a matter of urgency, the changes in engines/savestate.cpp &
engines/metaengine.cpp should be reverted or adjusted as appropriate, to
remove any further risk to users.
Proposed changes for consideration to complement removal of _saveType
initialization from initSaveType() function (including renaming) in
engines/savestate.cpp:
{{{
// restore default _saveType & rename initSaveType() call in SSD
definitions
SaveStateDescriptor::SaveStateDescriptor(const MetaEngine *metaEngine, int
slot, const Common::U32String &d)
: _slot(slot), _description(d), _isLocked(false),
_playTimeMSecs(0), _saveType(kSaveTypeUndetermined) {
initSaveSlot(metaEngine);
}
SaveStateDescriptor::SaveStateDescriptor(const MetaEngine *metaEngine, int
slot, const Common::String &d)
: _slot(slot), _description(Common::U32String(d)),
_isLocked(false), _playTimeMSecs(0), _saveType(kSaveTypeUndetermined) {
initSaveSlot(metaEngine);
}
// remove _saveType initialisation & rename function to reflect change
void SaveStateDescriptor::initSaveSlot(const MetaEngine *metaEngine) {
// Do not allow auto-save slot to be deleted or overwritten.
if (!metaEngine && g_engine)
metaEngine = g_engine->getMetaEngine();
const bool autosave =
metaEngine && ConfMan.getInt("autosave_period") &&
_slot == metaEngine->getAutosaveSlot();
_isWriteProtected = autosave;
_isDeletable = !autosave;
}
// rename function declaration (in engines/savestate.h):
void initSaveSlot(const MetaEngine *metaEngine);
}}}
Revert relevant changes to listSaves() & querySaveMetaInfos() functions in
engines/metaengine.cpp:
{{{
// replace isAutosave() call with previous test
SaveStateList MetaEngine::listSaves(const char *target, bool saveMode)
const {
SaveStateList saveList = listSaves(target);
int autosaveSlot = ConfMan.getInt("autosave_period") ?
getAutosaveSlot() : -1;
if (!saveMode || autosaveSlot == -1)
return saveList;
// Check to see if an autosave is present
for (SaveStateList::iterator it = saveList.begin(); it !=
saveList.end(); ++it) {
int slot = it->getSaveSlot();
if (slot == autosaveSlot) {
// It has an autosave
return saveList;
}
}
// No autosave yet. We want to add a dummy one in so that it can
be marked as
// write protected, and thus be prevented from being saved in
SaveStateDescriptor desc(this, autosaveSlot, _("Autosave"));
saveList.push_back(desc);
Common::sort(saveList.begin(), saveList.end(),
SaveStateDescriptorSlotComparator());
return saveList;
}
// restore desc.setAutosave(header.isAutosave) call
SaveStateDescriptor MetaEngine::querySaveMetaInfos(const char *target, int
slot) const {
if (!hasFeature(kSavesUseExtendedFormat))
return SaveStateDescriptor();
Common::ScopedPtr<Common::InSaveFile>
f(g_system->getSavefileManager()->openForLoading(
getSavegameFile(slot, target)));
if (f) {
ExtendedSavegameHeader header;
if (!readSavegameHeader(f.get(), &header, false)) {
return SaveStateDescriptor();
}
// Create the return descriptor
SaveStateDescriptor desc(this, slot, Common::U32String());
parseSavegameHeader(&header, &desc);
desc.setThumbnail(header.thumbnail);
desc.setAutosave(header.isAutosave);
return desc;
}
return SaveStateDescriptor();
}
}}}
Individual engines targeted in
[https://github.com/scummvm/scummvm/commit/7adad5aaf5831dc5adcee140f38aacc4a5db2518
7adad5aa] will need to be reviewed for potential changes, though most only
involved the removal of calls setting the writeProtected & Deletable flags
(which can be ignored).
--
--
Ticket URL: <https://bugs.scummvm.org/ticket/13841#comment:1>
ScummVM :: Bugs <https://bugs.scummvm.org>
ScummVM
More information about the Scummvm-tracker
mailing list