ENGINES: AUTOSAVE: Setting a file's _saveType value by where it’s stored can cause a regular save to be overwritten.

[macca8]

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 (​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 ​PR3261, and in particular ​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 (or perhaps, overrides?) 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.

The following suggestions simply revert the saveType test to its previous values & behaviour before PR3261, and include any subsequent changes in the same blocks of code since PR3261.

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 ​7adad5aa will need to be reviewed for potential changes (in particular Cine (#13432) & SCI (and see also ​Disable ScummVM autosaves)), though most only involved the removal of calls setting the writeProtected & Deletable flags (which can be ignored).

[macca8]

Since there's been no response, I'm changing the component to something more generic, in the hope of stimulating some interest, and avoiding this issue finding its way into the 2.6.1 release (the same applies to #13842).

Not sure which is the correct component here. It's really a global issue affecting most engines, but there's no general "Engines" component listed (perhaps that should be added to the list, similar to Ports?), so I've settled on Other, rather than Common (which was used with ​9b05c206, which originally exposed this issue).