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

Fixed various errors spotted with Application Verifier #8215

Merged
merged 3 commits into from Jun 29, 2019

Conversation

4 participants
@CookiePLMonster
Copy link
Contributor

commented Jun 23, 2019

First of all, I haven't found any notes about squashing commits in the Contribution Guidelines, so if I need to do so please let me know and I'll update this PR accordingly.

This PR consists of three separate bugfixes to crashes/defects I found while running Dolphin with Application Verifier on Windows. It also contains a code sanity change I consider optional, so in case of any doubts I'll just recall the last commit.

  1. DolphinAnalytics has been made a true singleton now, unifying its Instance() method with the other singletons in the codebase. Previously, it used a mutex-protected std::shared_ptr allegedly to ensure proper behaviour in case of concurrent access. However, since C++11 static local variables are guaranteed to be initialized in a thread safe manner, as outlined here:
    https://en.cppreference.com/w/cpp/language/storage_duration#Static_local_variables

    Additionally, not using static inline class fields here actually works around a known VS2017 compiler bug, where destructor for static inline class fields is invoked more than once - in the case of Dolphin, an already destroyed std::shared_ptr was destroyed again, which led to a possible crash on exit.

    Reference to a VS2017 compiler bug, allegedly fixed in VS2019:
    https://developercommunity.visualstudio.com/content/problem/300686/static-inline-class-variables-have-their-destructo.html

  2. Fixed a possible infoleak/crash in HttpRequest::Response HttpRequest::Impl::Fetch if request fails. ERROR_LOG attempted to use the request body as if it was a null terminated string, but it's not - leading to a out-of-bounds read and possibly output such to a log file. I fixed it by specifying length as a format specifier, so there is no need for a temporary std::string.

  3. Fixed a crash in DSPTool if -h or -o arguments were included without a following argument. As argv[argc] is guaranteed to be null, the code would try to assign such to an std::string - which is illegal.

  4. Replaced RtlGetVersion with GetVersionEx again. Comments above this code hint this function has been removed after Win8, which is not true. I assume this change was made in the past before Dolphin was properly manifested for Windows 8.1 and Windows 10 - in which case GetVersionEx indeed "lied" to it. Since Dolphin is now properly manifested, there is no need for such workarounds anymore.

    That said, if anybody thinks this change is not needed I can easily roll back this commit from the PR.

    EDIT: Following a discussion down below, this has been recalled from this PR, due to be further improved separately in the future (using a documented, non-deprecated way instead).

@BhaaLseN

This comment has been minimized.

Copy link
Member

commented Jun 23, 2019

Keeping commits separate is fine, as long as the individual commits make sense. You can also amend older commits if changes should be in there.

With that out of the way, it seems you missed a few places where Analytics are being called, particularly the NoGUI build. Our CI should help you find those places if you haven't already found them.

@CookiePLMonster

This comment has been minimized.

Copy link
Contributor Author

commented Jun 23, 2019

With that out of the way, it seems you missed a few places where Analytics are being called, particularly the NoGUI build. Our CI should help you find those places if you haven't already found them.

Yep I noticed! On my way to fix those now. Seems to be non-Windows code.

CookiePLMonster added some commits Jun 23, 2019

Make DolphinAnalytics a true singleton - static local variables are i…
…nitialized in a thread safe manner since C++11

Also works around a Visual Studio 2017 bug where static inline class fields are destructed multiple times

@CookiePLMonster CookiePLMonster force-pushed the CookiePLMonster:appverifier-sanitize branch from c93b7d8 to f220fea Jun 23, 2019

@CookiePLMonster

This comment has been minimized.

Copy link
Contributor Author

commented Jun 23, 2019

Should be fixed now!

@BhaaLseN

This comment has been minimized.

Copy link
Member

commented Jun 23, 2019

Thanks!

Could you add some additional curlies to DSPTool? We prefer balanced braces on all statements if one of them needs them; and you just added two curly blocks in there.

Also, that ERROR_LOG will likely become a std::string_view in the future (once it switches over to fmt), so it might be fine using that format specifier like that - but I'll let others comment on this one.

@CookiePLMonster CookiePLMonster force-pushed the CookiePLMonster:appverifier-sanitize branch from f220fea to fd56003 Jun 23, 2019

@CookiePLMonster

This comment has been minimized.

Copy link
Contributor Author

commented Jun 23, 2019

Could you add some additional curlies to DSPTool? We prefer balanced braces on all statements if one of them needs them; and you just added two curly blocks in there.

Done! Do note they were unbalanced previously too, as -pm and -psm had braces while the others did not - however, they were at the very end. I now added braces everywhere to make it uniform with no doubts.

Also, that ERROR_LOG will likely become a std::string_view in the future (once it switches over to fmt), so it might be fine using that format specifier like that - but I'll let others comment on this one.

Sure, I added the typecast to conform to the warning (as per contribution guidelines), I guess it can be reverted once ERROR_LOG is modified.

Although on an unrelated note, why do responses even use vectors? This whole issue would've been void if responses were passed around as strings - after all, they are texts.

@CookiePLMonster CookiePLMonster force-pushed the CookiePLMonster:appverifier-sanitize branch from fd56003 to 5f0b4d8 Jun 29, 2019

@stenzek stenzek merged commit e388f01 into dolphin-emu:master Jun 29, 2019

9 checks passed

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
pr-win-x64 Build succeeded on builder pr-win-x64
Details

@CookiePLMonster CookiePLMonster deleted the CookiePLMonster:appverifier-sanitize branch Jun 29, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.