Don't lie to the player if a file failed to save

InfoTeddy commented 4 years ago

2.3 has already fixed the fact that the game doesn't produce an error message if a custom level fails to be saved (#266), but it turns out the game will basically never produce an error message if touching a teleporter fails to save your game, or if quicksaving (in custom level or not) fails to save your game, and so on. This is most noticeable if you playtest a level in Ved and quicksave, where the game will attempt to write the file saves/special/stdin.vvvvvv.vvv, and fails because the special/ folder doesn't exist. The fact that you're able to quicksave (among other things) during command-line playtesting is a separate bug, but the game will still say "Game saved ok!" even though it didn't.

Furthermore, 2.2's unlock.vvv saving is iffy at best, leading to things like Super Gravitron high scores being lost because the player didn't specifically close the game by using the "quit game" option in the main menu.

While 2.3's saving is more robust, it would be best if there was a notification saying unlock.vvv got saved properly whenever it gets written to, and if the game attempted to save the file but somehow failed, then it should say that, too.

Maybe the notification should say "Your score has been saved" for Time Trials and the Super Gravitron, "Your unlock has been saved" whenever something new got unlocked, and "Your settings have been saved" whenever your settings have been saved, but the point is, the player should actually be confident that their progress got saved, the game shouldn't succeed or fail silently, and if it fails the game shouldn't be lying to the player.

flibitijibibo commented 3 years ago

Failure messages are now in the game as of the latest commit, thanks @Dav999-v!

Daaaav commented 3 years ago

If #553 is merged, the following general cases - both referred to in this issue and related to it - will remain unhandled as of yet:

[ ] Super Gravitron high scores and possibly other things not automatically saving
[ ] Any notifications that settings, unlocks or scores were saved successfully, or when telesave() is called as part of a script
[ ] The player uses "clear data" or "new game" and deleting existing saves failed, but not because they already didn't exist
[ ] Not listed in this issue, but anything that can go wrong with file loading - like unparseable XML, expected XML elements not being there (especially <Data> tags that enclose everything else), maybe illogical or "corrupted" values?

So, from #267:

377 is the highest priority of the unassigned issues. This is something that usually isn't a big deal on PC but would fail cert just about anywhere else. Since we're now making lots of changes to the engine we'll need to be a lot safer than we were before.

Since this issue is not "atomic" but is labeled as required for 2.3 in its entirity - are each of these items important and required for 2.3? In particular, to what extent is it necessary to display notifications every time a setting is toggled or the player gets a new high score or unlocks a new play mode?

InfoTeddy commented 3 years ago

XML loading is currently a big memory safety issue... there's a bunch of potential NULLs that the game never checks and could blindly dereference. At the very least, those checks should be put in place.

flibitijibibo commented 3 years ago

Following up on this as I've been pulling in a lot of patches that have nothing to do with this. To answer @Dav999-v's list:

We should try to save high scores as soon as they happen - I'm okay with saves happening after these events.
This one's not so critical, as the issue is usually that you need to tell users when it's in the process of saving, not necessarily that it's saved. VVV is a special case since saves are basically instant, so the likelihood of this being necessary is really slim (I have actually shipped console titles without this for that very reason).
This one's not critical as long as the game clears the state in memory. So even if the file is still there, the game still thinks it's cleared, and the game is running as if it's fresh. Presumably saving will either succeed/fail later (which we handle), or the user will exit while it still exists and all they have to do is try again. That latter case is extremely narrow so I wouldn't worry about it.
If a static analyzer points it out, fix it, otherwise it's effectively going through the impossible task of hardening XML parsing. We're not exactly security-sensitive so I'm not too worried about this. If someone wants to fuzz the game, go for it, but I won't make this a 2.3 blocker.

So really we only care about bullets 1 and 3. Once both are good I'll close this and we can open XML hardening as a separate security issue for 2.4.

InfoTeddy commented 3 years ago

This one's not critical as long as the game clears the state in memory. So even if the file is still there, the game still thinks it's cleared, and the game is running as if it's fresh.

If I'm reading you correctly, this is already the current behavior of the game. If deleting saves fails, the game will still clear its internal state, and proceed as if save deletion was successful and all settings have been reset to default.

flibitijibibo commented 3 years ago

Yeah, I believe no matter what it clears progress in memory, so we should be okay. Might be worth testing just in case but I believe it's working as-is.

InfoTeddy commented 3 years ago

We should try to save high scores as soon as they happen - I'm okay with saves happening after these events.

At least for the Super Gravitron, this is already implemented in 2.3 (although it is not indicated to the user that their high score has been automatically saved). This is better than 2.2 behavior, which required you to specifically close the game through the "quit game" menu option on the title screen in order to save your score (it didn't even automatically save the score when you closed your window, which 2.3 also implements).

flibitijibibo commented 3 years ago

Ah, so that's settled as well... wish I could remember all of this stuff. If there's no other place where immediate saving is critical I guess this was technically always done? (Minus the static analysis pass for XML I guess, but I'm guessing it won't get very much compared to fuzzing)

InfoTeddy commented 3 years ago

wish I could remember all of this stuff

Well, that's why you got me! :stuck_out_tongue:

(Minus the static analysis pass for XML I guess, but I'm guessing it won't get very much compared to fuzzing)

I did run cppcheck on the codebase earlier, it didn't really throw up anything regarding the XML.

Although, I think I remember an article by a proprietary static analyzer company (not going to link it here, it's just a walking advertisement - YouTubers shamelessly advertising for Popular Mobile App Game:tm: and aggressively advertising for VPN Company:tm: have more respect from me than this article) where they ran their proprietary thing on 2.2 code, and it threw up warnings about the game checking that a pointer wasn't NULL, but then proceeding to use it anyway regardless of it being NULL or not.

Indeed, the same code still more-or-less exists in the codebase today (other than the fact that it uses TinyXML-2):

pElem=hDoc.FirstChildElement().ToElement();
// should always have a valid root but handle gracefully if it does
if (!pElem)
{
    printf("Save Not Found\n");
}

// save this for later
hRoot=tinyxml2::XMLHandle(pElem);

And the main reason this is probably an issue is because the game just kind of keeps dereferencing things that could potentially be NULL, and this would lead to a segfault. For example, in just this one line of code:

for( pElem = hRoot.FirstChildElement( "Data" ).FirstChild().ToElement(); pElem; pElem=pElem->NextSiblingElement())

hRoot could be NULL. hRoot.FirstChildElement("Data") could return NULL. hRoot.FirstChildElement("Data").ToElement() could also be NULL.

flibitijibibo commented 3 years ago

I'll namedrop since I do have positive prior experience with them: The team behind PVS-Studio did run an analysis very early on, but it's probably way out of sync at this point:

https://www.viva64.com/en/b/0707/

For the really nitpicky errors, I traditionally just shove in a ton of SDL_assert lines since it provides really good debug info while also not interfering with release build performance for something that should be a non-issue, but might be in some wild scenario. If the hardening ends up being mostly asserts and not real handling I wouldn't be too heartbroken about it, since those cases will mostly likely be hit by a fuzzer and not much else.

InfoTeddy commented 3 years ago

The team behind PVS-Studio did run an analysis very early on, but it's probably way out of sync at this point

Well, I mean it is, but the main thing I don't like about that article is that it includes a warning from PhysFS code, which is a third-party dependency of ours. Like, we can't do anything about that, that's on someone else to fix. And on a side note, I'd like to see news outlets cover just how improved 2.3's code is. :wink:

Anyways, I don't think performance here should matter all too much. We're already creating a big XML document object on the stack, so worrying over a bunch of simple checks when something as expensive as that is already happening is just misplaced priorities, and loading a document doesn't happen all that often in the first place. We've probably gotten a lot of performance gains by simply upgrading to TinyXML-2 in the first place, too.

flibitijibibo commented 3 years ago

the main thing I don't like about that article is that it includes a warning from PhysFS code, which is a third-party dependency of ours.

I'm probably biased since it's a project I intimately know, but I imagine even Ryan was thrilled to have a free analysis warning for one of his projects!

We've probably gotten a lot of performance gains by simply upgrading to TinyXML-2 in the first place, too.

Wouldn't surprise me. A lot of what we shipped was absurdly old even when 2.2 went out. Like I said, the asserts are good for pure sanity checking but I'd be surprised if they really tripped in reality, and using SDL_assert avoids the needless perf drop for public builds (not that VVV is resource-intensive, mind). But, you never know.

And on a side note, I'd like to see news outlets cover just how improved 2.3's code is.

Considering we're a rare case where we released the game source with an attitude other than "whatever, leave me alone throws highball" it would be cool, but we'll see if anyone really notices... >_>

InfoTeddy commented 3 years ago

By the way, @Dav999-v, didn't you run into a Windows bug with Ved where a BSoD happening within a few seconds of writing a file would overwrite the entire file with null bytes? Could that also happen here? As far as I know, the solution was to simply call FlushFileBuffers() on the file handle after writing to it.

Daaaav commented 3 years ago

By the way, @Dav999-v, didn't you run into a Windows bug with Ved where a BSoD happening within a few seconds of writing a file would overwrite the entire file with null bytes? Could that also happen here? As far as I know, the solution was to simply call FlushFileBuffers() on the file handle after writing to it.

I don't know if that could also happen here, VVVVVV uses PhysFS to load and save XML files, and from a quick look at the documentation it seems like buffering is disabled by default, otherwise there is a PHYSFS_flush(PHYSFS_File * handle). But that may be completely separate from buffering done by Windows.

A couple of days ago I tested Ved on the Windows I had previously been testing FlushFileBuffers() with (by cutting power after saving) and the first thing it told me was that the settings file was corrupted (it was nulled, probably leftover from testing). I think I've seen Ved's automatic backup on writing a level file also get nulled in those tests previously. Both get written using love.filesystem.write, and as far as I know LÖVE also uses PhysFS, so that would imply it's probably susceptible to this problem, and I don't think FlushFileBuffers() works on anything other than Windows file handles.

InfoTeddy commented 3 years ago

I was browsing PhysFS commits and noticed this one. It is explained in the commit message that:

[PHYSFS_flush()] is meant to send PhysicsFS-buffered data to the PHYSFS_Io's
implementation, [PHYSFS_Io::flush()] is meant to tell the OS to definitely make sure the
data is safely written to disk (or at least, that's what it does in practice).

[...]

Now we still PHYSFS_Io::flush() on PHYSFS_close(), like this has always
worked.

So, if I understand correctly, simply calling PHYSFS_close() will flush the file (and call FlushFileBuffers())? (And previously, PhysFS would always flush to disk with PHYSFS_flush().) Although, this doesn't explain your LÖVE observation...

flibitijibibo commented 3 years ago

Checking up on this... it sounds like the default physfs behavior already does what we want, so I think the only thing left on the list is XML hardening, which a whole separate issue on its own. Have I missed anything?

InfoTeddy commented 3 years ago

That's more-or-less correct - although, my reading of PhysFS source is suggestive of it doing the correct thing; I haven't confirmed or gotten confirmation that it actually behaves as I described (I don't use Windows, after all). But yes, XML hardening is a separate issue.

flibitijibibo commented 3 years ago

Closing since we appear to have gotten this taken care of aside from XML - that can be filed whenever and I'll tack the 2.4 label on it.

TerryCavanagh / VVVVVV

Don't lie to the player if a file failed to save #377

377 is the highest priority of the unassigned issues. This is something that usually isn't a big deal on PC but would fail cert just about anywhere else. Since we're now making lots of changes to the engine we'll need to be a lot safer than we were before.