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

Fix settings.txt parsing in case of weird line endings #8670

Merged
merged 1 commit into from Mar 15, 2020
Merged

Fix settings.txt parsing in case of weird line endings #8670

merged 1 commit into from Mar 15, 2020

Conversation

Leseratte10
Copy link
Contributor

Related to bug 11930: https://bugs.dolphin-emu.org/issues/11930

The settings.txt file straight from a Wii NAND does sometimes contain one weird line ending - CR-LF-LF instead of CR-LF (0d0a0a). The current parsing code only searches for 0d0a, and doesn't find the line immediately after the broken line ending.

This causes the bug described in issue 11930 - the serial number in the file is ignored and regenerated, until the file is re-written by Dolphin, with correct line endings. This causes problems, because it makes Dolphin regenerate the serial number when importing a NAND.

My change makes a temporary copy of the decoded string, then removes all CR bytes from the string, leaving only LF line endings in the copy. This also replaces the broken CR-LF-LF line feed with LF-LF, making it look like a normal empty line. This way, the current code to search for a settings element correctly finds that entry.

With that change applied, re-importing a NAND from the same console over and over again doesn't cause any identifier change anymore, this means, re-importing a NAND no longer creates a new console identity on Wiimmfi anymore, meaning, the 7-day wait isn't always restarted.

Hoping this change doesn't break anything else, but it doesn't look like it.

@JosJuice
Copy link
Member

JosJuice commented Mar 14, 2020

Thanks for finding the cause!

The way this currently is implemented, the CRLF to LF conversion will be done on every call to GetValue. It would be better to put the conversion at the end of Decrypt so that it only has to run once.

Also, please add a comment (preferably close to where the CR removal is done) about the CRLFLF sequence. Otherwise someone in the future might think that only CRLF needs to be handled, not CRLFLF.

@Leseratte10
Copy link
Contributor Author

If I do that conversion after the call to Decrypt, wouldn't we then need to convert back to CR-LF when exporting from the text format to the encrypted format? As far as I can tell, Dolphin is still storing the settings.txt in the encrypted format - but where's the code that does this encryption? I did only find the code that decrypts.

@Leseratte10
Copy link
Contributor Author

Okay, looking at the code again it seems as if there is no method to encrypt a decrypted file again, only to recreate a new encrypted file from scratch, which results in CR-LF line endings. That means changing the line endings after decryption should be fine.

@JosJuice
Copy link
Member

The code that does the encryption is done in WriteByte. Note that this function only writes to m_buffer, so we're free to do whatever we want with decoded.

It's curious that calling AddSetting doesn't update decoded at all, though... But if we would want to fix that, that'd be an easy one-liner addition to AddSetting. There's no need to do that in this PR, though.

@Leseratte10
Copy link
Contributor Author

Removal of the CR is now done after decryption, and I added a comment explaining why that is needed.

@Leseratte10
Copy link
Contributor Author

oops, another push, I forgot to run clang-format.

@BhaaLseN
Copy link
Member

Did you ever encounter old mac CR-only line breaks (that do not have a LF immediately following)? Depending on your answer to this, it might be necessary to make this behavior a tiny bit smarter and only remove CR that have an LF immediately following (or rather: turn the CR into an LF)

@Leseratte10
Copy link
Contributor Author

I didn't. These files aren't really text files written by the user, more like config files written in the factory when a Wii is produced. I've only ever seen CR-LF and CR-LF-LF line endings.

And if there was a file that had CR-only line endings, that wouldn't have worked with the old code either (which searched for CR-LF), thus, this change doesn't break anything that wasn't already broken.

Copy link
Member

@BhaaLseN BhaaLseN left a comment

Choose a reason for hiding this comment

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

Fair point. LGTM then.

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