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 Gecko codes loading #8542

Merged
merged 1 commit into from Feb 15, 2020
Merged

Fix Gecko codes loading #8542

merged 1 commit into from Feb 15, 2020

Conversation

@Itrimel
Copy link

Itrimel commented Dec 31, 2019

Fixes https://bugs.dolphin-emu.org/issues/11809

I found on my system (Archlinux) that using a locale with UTF-8 was breaking loading of Gecko codes. Hopefully forcing the C locale on this string stream should be enough without breaking it on other systems.

@Itrimel Itrimel force-pushed the Itrimel:fix-gecko-loading branch from bc1b5cb to 61b61cc Dec 31, 2019
@JosJuice

This comment has been minimized.

Copy link
Member

JosJuice commented Dec 31, 2019

Do we know why this fixes it? It doesn't intuitively make sense to me.

@jordan-woyak

This comment has been minimized.

Copy link
Member

jordan-woyak commented Dec 31, 2019

FYI, The DownloadCodes function in this file also needs to be fixed.
Actually, downloading seems to work but the codes are displayed incorrectly..

I can reproduce the problem using LANG=fr_FR.UTF-8 (en_US.UTF-8 works fine).

@Itrimel

This comment has been minimized.

Copy link
Author

Itrimel commented Jan 1, 2020

While searching where this behavior could come from, I stumbled upon this :
while using this commit (Itrimel@b9844e5) on master and the locale fr_FR.UTF-8, the second part of all codes (which was before 00000000) will turn into a number which can be changed, among other things, by adding or removing comments from the source file (I typically get something around 00007f50).

So ... I am very lost about the true origin of this bug ...

@jordan-woyak

This comment has been minimized.

Copy link
Member

jordan-woyak commented Jan 1, 2020

Okay, apparently certain locales allow spaces within a single formatted number.
e.g. "1 2 3" is read as "123" even when reading decimal numbers.

Both GeckoCode values are read as a single 64 bit hex value which overflows causing the issue.

This is horrible. We probably assume formatted number input does not cross white space throughout dolphin.

@Itrimel

This comment has been minimized.

Copy link
Author

Itrimel commented Jan 2, 2020

Well that's bad ... I can also fix DownloadCodes (so that the display just after download works), but if there are more places to fix I think it would be best for you to take over.

Some locales (e.g. fr_FR.UTF-8 on ArchLinux) don't split the string stream on a space. As such, when extracted formatted data from te stream, it will return the two numbers as one for the first call, effectively overflowing the u32 variable, then will do an out-of-bounds read for the second call. Forcing the use of the C locale on the streams where it would cause a problem allows to workaround this behavior.
@Itrimel Itrimel force-pushed the Itrimel:fix-gecko-loading branch from 61b61cc to fe900e0 Feb 14, 2020
@Itrimel

This comment has been minimized.

Copy link
Author

Itrimel commented Feb 14, 2020

Updated to better reflect jordan-woyak's findings in the comments and the commit message, and to add the fix to DownloadCodes function

Copy link
Member

jordan-woyak left a comment

In the future we should probably stop parsing things with stringstreams but I think this is a good quick fix for now to fix Gecko Codes.

@JosJuice JosJuice merged commit 62046d9 into dolphin-emu:master Feb 15, 2020
8 of 10 checks passed
8 of 10 checks passed
pr-win-dbg-x64 Build failed on builder pr-win-dbg-x64
Details
pr-win-x64 Build failed on builder pr-win-x64
Details
default Very basic checks passed, handed off to Buildbot.
Details
lint Build succeeded on builder lint
Details
pr-android Build succeeded on builder pr-android
Details
pr-deb-dbg-x64 Build succeeded on builder pr-deb-dbg-x64
Details
pr-deb-x64 Build succeeded on builder pr-deb-x64
Details
pr-freebsd-x64 Build succeeded on builder pr-freebsd-x64
Details
pr-osx-x64 Build succeeded on builder pr-osx-x64
Details
pr-ubu-x64 Build succeeded on builder pr-ubu-x64
Details
@Itrimel Itrimel deleted the Itrimel:fix-gecko-loading branch Feb 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.