[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]

#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:          |   Keywords:
    Game:          |
-------------------+--------------------
 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).
-- 
Ticket URL: <https://bugs.scummvm.org/ticket/13841>
ScummVM :: Bugs <https://bugs.scummvm.org>
ScummVM