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

prevent the overwrite of existing mpq files #7674

Merged
merged 1 commit into from
Jan 26, 2025

Conversation

StephenCWills
Copy link
Member

@StephenCWills StephenCWills commented Jan 25, 2025

Lifted from pionere's fork: pionere@7866861

He simply adds the x flag to indicate that wb should not overwrite existing files. This should indeed prevent the issue of save files being wiped out. It might still be a good idea to handle error messages from PathFileExistsW() more explicitly, but this change seems like an obvious improvement to me.

This resolves #7672

Copy link
Member

@AJenbo AJenbo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you still save with this...?

@AJenbo AJenbo merged commit 9639211 into diasurgical:master Jan 26, 2025
23 checks passed
@StephenCWills StephenCWills deleted the mpq-no-overwrite branch January 26, 2025 14:12
@StephenCWills
Copy link
Member Author

What should happen as a result of this change is...

https://github.com/diasurgical/devilutionX/blob/96392116e03ae2634613818c20f7eaa9b77c4dd5/Source/mpq/mpq_writer.cpp#L161-L162

The game will still crash, but it won't completely overwrite the save file. Still not ideal, since the player will lose progress since their last save, but it's a whole lot better than losing the whole character.

@AJenbo
Copy link
Member

AJenbo commented Jan 26, 2025

So the game deletes the file before writing the new save? And sometimes the file sync tries to restore the file at the same time?

Should we instead overwriting the existing file or write to a temporary file and renamed it at the end?

@StephenCWills
Copy link
Member Author

So the game deletes the file before writing the new save? And sometimes the file sync tries to restore the file at the same time?

Unfortunately, I can't really answer that question for you. I don't understand how it's even possible for OneDrive to interfere with the PathFileExistsW() function, and I don't know what error code would be returned when it does. If I thought it were possible for me to reproduce it consistently, I'd find that out for you.

My best guess is that maybe OneDrive is somehow taking an exclusive lock on the folder containing the file so that it can't be used to check for the existence of files within. At the very least, it makes some sense to me for a tool that does file synchronization to maybe want to ensure that the folder can't be changed while it's in the middle of scanning for changes.

Should we installed over writing the existing file or write to a temporary file and renamed it at the end?

As you can see by the fact that the save file in #7672 only contains templ13, the MPQ writer doesn't actually do a full dump of the entire MPQ archive. It only modifies the existing archive by adding the templ13 file to it. If you want to write to a temporary file and rename it at the end, you'll need to do one of two things.

  1. Copy the file you intend to overwrite, modify that file, then rename it back to the original file.
  2. Modify the code to recreate the entire save file in a separate temporary file from scratch every time you make a change, then rename that at the end.

In either case, it requires more file I/O operations than the current mechanism for updating the existing MPQ file, which means you're probably actually more likely to encounter an unrecoverable conflict between the game client and OneDrive.

Instead, I would recommend referencing the .NET Framework implementation of System.IO.File.Exists(), which I included in my analysis of #7672. It seems like we should be able to handle specific error codes and fall back on FindFirstFile() as a more reliable alternative to PathFileExistsW() if an unexpected error code pops out.

Another alternative I would recommend would be to skip PathFileExistsW() entirely, by using r+b first and then trying again with wbx if that fails.

That said, regardless of what we do, if we are completely unable to get write access to the file, there's probably no other reasonable alternative besides app_fatal().

@AJenbo
Copy link
Member

AJenbo commented Jan 26, 2025

If possible we should probably request an exclusive lock on saves while the game is running.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Issue Report]: Single Player save files can get wiped when warping between dungeon levels
3 participants