Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[EXPERIMENTAL] GCMemcardDirectory: Set card formatting timestamp to known value. #8476

Conversation

AdmiralCurtiss
Copy link
Contributor

@AdmiralCurtiss AdmiralCurtiss commented Nov 17, 2019

TODO list for me being comfortable actually merging this:

  • Assuming we don't want hardcoded IDs, store enough data to reproduce the same card serial number on savestate load.
  • This data should not only be used for savestates, but also when switching from GCI folder to a different device (none/rawcard/mic/...) and back while the game is running.
  • Send card eject/insert signals to the game when we provably have a different memory card now than we did on savestate creation.

This is a fix/workaround for issues like this one: https://bugs.dolphin-emu.org/issues/11849
Code will be made nicer if we decide to actually go with it.

To summarize, some games refuse to save to a memory card if they can detect that the card is not the same card the save was loaded from. They, or at least some of them, do this by comparing the m_serial field of the currently inserted memory card's header to the one remembered from the card they loaded the save from.

This field is pseudorandomly generated when the card is formatted based on the current timestamp of the GameCube's internal clock. This is also how we do it, with the wrinkle that our virtual GCI folder memory card is formatted every time a Dolphin game session starts. This means that the following sequence of tasks will cause a game to detect a changed memory card:

  • Boot a game.
  • Load a save.
  • Make a savestate.
  • Close the game.
  • Boot the game again.
  • Load the savestate.
  • Try to save.

Assuming no one modified the contents of the GCI folder in the meantime, the save we're attempting to overwrite is still the exact same save we loaded from, yet the game detects it as a different one because the virtual memory card was reinitialized with a different timestamp on the second boot.

I've tested this for the game in the linked issue (Paper Mario TTYD) and it does indeed allow you to save now when you do this even though it didn't before. However, I'm pretty afraid of potential false positives. If a game thinks that the currently inserted card has not changed from the one it has loaded the data from, and doesn't bother to re-read the BAT of the card, it could corrupt data in the card if it has changed, for example when you added or removed a GCI file from the GCI folder between making the savestate and booting the game the second time. This is technically not impossible in real life, though somewhat convoluted (while game is running remove card, put it in second GC, copy around files, put it back in first GC), so you'd hope games would be able to deal with it, but you never know.

@JMC47 Since you requested this in the linked bug report, please test this with as many combinations of cases you can think of!

@AdmiralCurtiss
Copy link
Contributor Author

@AdmiralCurtiss
Copy link
Contributor Author

As a sidenote, am I misreading this or do we have a weird circular dependency related to g_SRAM.settings_ex.flash_id? The formatting code reads from this value to derive the card serial, but at the same time it's set based on the currently inserted memory card's header (called from here). This seems somewhat fishy to me.

@mbc07
Copy link
Contributor

mbc07 commented Nov 17, 2019

Wouldn't saving m_serial field to the save state then reading it back when loading the save state be enough?

@AdmiralCurtiss
Copy link
Contributor Author

Potentially, but I think that would actually complicate the code a lot more, as you'd have to special case GCI folders compared to raw card files, keep track of whether a GCI folder has been already used in the session to remember its serial and so on, all of which we can just ignore by just assuming a GCI folder is formatted at a known timestamp.

@JMC47
Copy link
Contributor

JMC47 commented Nov 19, 2019

Wow, no wonder I could never 100% reproduce it, I was missing a step that had to do with the initial save! Great work, and I think we need to do this ASAP.

@JMC47
Copy link
Contributor

JMC47 commented Jan 13, 2020

Is there anything we're planning to do with this? This is a fairly common thing to run into with people using savestates.

@AdmiralCurtiss
Copy link
Contributor Author

AdmiralCurtiss commented Jan 13, 2020

If you want to go ahead then do so, but I'd rather someone do some experiments with potential corruption cases with this before actually merging it. Games should handle this correctly, because I can't imagine they'd cache the inserted memory card's BAT/Dir structure if they receive eject/insert signals and the card ID happens to match the previously inserted*, but like I said in the opening statement, who knows if they actually do, and I'd hate if someone actually corrupted their GCI files due to this.

*Note: This assumes we always send these signals correctly on savestate load, maybe double-check that as well.

Actually that makes me think, is there any game that (intentionally) writes to saves with game IDs not their own? Should there be a protection against that? (Or is there already?)

@JMC47
Copy link
Contributor

JMC47 commented Jan 13, 2020

as far as I know, there are only games that read other games savefiles.

@JosJuice
Copy link
Member

Actually that makes me think, is there any game that (intentionally) writes to saves with game IDs not their own? Should there be a protection against that? (Or is there already?)

I know of one case: Homeland writes to a save with the ID 0x00000000, which is used for network settings IIRC.

@AdmiralCurtiss AdmiralCurtiss marked this pull request as ready for review January 25, 2020 12:20
@delroth delroth marked this pull request as draft May 4, 2020 00:03
@Stovehead
Copy link

I tried using this, but I can't actually load states on this build.

@AdmiralCurtiss
Copy link
Contributor Author

What do you mean, savestates made in a different dolphin version? Or savestates you made with this dolphin version? Because if the former, yeah, that's not unexpected.

@Stovehead
Copy link

Yeah, I just checked it out. It works if I do it from the same version.

@Stovehead
Copy link

Out of curiosity, would there be a way to load states from the newest version?

@JMC47
Copy link
Contributor

JMC47 commented Jun 12, 2020

Considering how many issues I see with savestates and GCI folders, I do think we should go with a solution like this. The sooner something is decided the better, as there are a lot of people running into these issues all the time and it's really frustrating to see them struggle.

@Stovehead
Copy link

I agree, I found this PR while trying to find a solution to my problem. I'm surprised this hasn't been merged yet, although I suppose there's still some testing to do. Still, the sooner the better.

@AdmiralCurtiss
Copy link
Contributor Author

@JMC47 Okay, I've added a little log message to help with testing this. Turn on EXI logging with Info to get it, it will print for every GCI file it loads in which blocks of the memory card it puts it.

Now, the basic idea of what I want you to test is the following: Load a game, make a note of which blocks the loaded saves occupy, then make a savestate and close the game. Once the game has been shut down, mess with your GCI folder a bit so that the virtual memory card looks different on reload (most importantly: make sure the save you're intending to write to ends up in different blocks!). Then once you have done that, load the savestate then make a regular save and close the game again. Then confirm that the game has been properly saved and none of the other loaded GCI files have been modified.

The easiest and probably most common scenario here is to load a game where you have no saves, then make a save, then make a savestate. Your not-this-game GCI files will necessarily take up the early blocks of the memory card and the new save will be after them. Then when reopening the game, our GCI folder logic will prioritize this new save so it ends up being loaded before all the other ones, which affects the block layout quite a bit.

You can probably come up with other scenarios too, but if that one works, I think we can safely assume that other similar scenarios work as well.

@AdmiralCurtiss
Copy link
Contributor Author

And as a general comment for this PR... are we fine with just hardcoding the timestamp, or should I instead generate a timestamp on boot as we do now and then store that into the save file, and reload the memory card on savestate load when the timestamp mismatches? Does it matter?

@AdmiralCurtiss
Copy link
Contributor Author

Oh, and I'd still like to resolve that fishy circular dependency of g_SRAM.settings_ex.flash_id because this potentially alters the generated memory card serial depending on the memory card that was previously inserted, if I'm understanding this correctly...

@Stovehead
Copy link

@AdmiralCurtiss We could just make it a setting. I'd say hardcode by default.

@Stovehead
Copy link

Also, is there a list of games with save features? We should probably try and test it on most if not all of them.

@JMC47
Copy link
Contributor

JMC47 commented Jun 13, 2020

I think hardcoding it is worse, but, I don't know how hard saving it to the savefile/savestate would be.

@Stovehead
Copy link

Is there any way to test these changes on the latest version?

@AdmiralCurtiss
Copy link
Contributor Author

Obsoleted by #8879.

@AdmiralCurtiss AdmiralCurtiss deleted the gci-folder-hardcoded-id branch October 20, 2020 16:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
6 participants