Trinitycore: 3.3.5a authserver undefined behavior compiling with C++17

[jameyboor]

Description:

When compiling 3.3.5a TC x64 Release MSVC 2017 C++17, the authserver will not work.

Compiling the project authserver and the requirements for authserver (common, database & shared) as C++17 introduces a bug in Release:

In Debug builds the authserver will work correctly.
In RelWithDebInfo or Release builds the authserver will not work correctly. It will reject the player's credentials even if they're correct.

A short video of the problem occuring: https://www.youtube.com/watch?v=pVQoAlbXnkc (sorry for the quality)

What I've tried:

Switching from MySQL to MariaDB, no luck.
Switching OpenSSL versions (1.0.2 - 1.1.0i), no luck.
Switching Boost versions ( 1.64 - 1.68), no luck.
Building my own Boost binaries with /std:c++17 set, no luck.

MSVC Info:

Microsoft Visual Studio Enterprise 2017
Version 15.8.1
Installed Version: Enterprise
Visual C++ 2017
Microsoft Visual C++ 2017

---

This sounds like UB that has surfaced with a compiler & optimizer upgrade (since it doesn't happen in debug), but I have yet to find where.

I'd like to invite anyone that wants to, to build auth and its requirements in GCC or Clang (no MinGW) and post back the results. This way a compiler bug is ruled out.

Branch(es):

3.3.5

TC rev. hash/commit:
(pick your poison, all recent commits thus far), currently on b4a1887c4d8df5a7729ed5c4126f6a7655a02127.

Operating system: Win 7 x64 Ultimate

[Killyana]

I'm using:
Microsoft Visual Studio Community 2017
Version 15.5.2
Installed Version: Community
Visual C++ 2017
Microsoft Visual C++ 2017
Boost 1.67

And I compile x64 on release and debug without any issue related to authserver

[jameyboor]

That's because your toolset isn't set to C++17.

You should add

set (CMAKE_CXX_STANDARD 17)

To your root CMakeLists.txt

And then open cmake/macros/FindPCHSupport.cmake, and edit:

set_property(TARGET ${TARGET_NAME} PROPERTY CXX_STANDARD 14)

To

set_property(TARGET ${TARGET_NAME} PROPERTY CXX_STANDARD 17)

Then re-configure and generate CMake. Then all projects will be using C++17, retry after doing that.

[Warpten]

I faced a similar issue when writing my own grunt client, this is likely to be a cache fail or an optimizer bug. I fixed it by being aggressive with the optimizer. See https://github.com/Warpten/wowgm/issues/1.

[jameyboor]

Thanks @Warpten, I'll try that.

[Warpten]

The idea was to prevent the optimizer from being too brutal. This is a band-aid, but a simple repro test case doesn't bug out and thus can't be submitted as a ticket to MS. (the relevant part is the two new memcpy calls)

I havent tried either Clang or GCC

[jameyboor]

Really hard to provide some repro case to submit as issue ticket then.

[Warpten]

Why was this closed?

[jameyboor]

My mistake.

I tried your rewrite and that seems to have worked, thanks @Warpten. Now onto reproducing a small case for bug reporting.

[jackpoz]

Version 15.8.1

could you try with a more recent VS 2017 version or even VS 2019 ? See for example these /std:c++17 related issues in VS 2017 15.8.1:

• https://developercommunity.visualstudio.com/content/problem/313340/c-vs-2017-update-8-difference-in-member-function-p.html
• https://developercommunity.visualstudio.com/content/problem/322651/code-generated-by-vc-1581-compiler-lead-to-crash.html
• https://developercommunity.visualstudio.com/content/problem/317464/vs2017-1581-bad-code-generated-with-stdc17.html

[jameyboor]

@jackpoz Will test when I get the chance.

@Warpten Your issue linked 404'd, do you still remember what method needed edits? It was something in HandleLogonProof right?

[illfated]

Only 1 searchable repository named "wowgm" on all of Github: https://github.com/bmjoy/wowgm

[jameyboor]

https://github.com/bmjoy/wowgm/blob/master/src/wowgm/Protocol/Authentification/AuthSocket.cpp#L185 found it, thanks @illfated .

[Warpten]

Oops, I moved the repo to private. Won't move it back for now.

Actual patch (not for TC) working around the compiler - plus some logging

diff --git a/src/wowgm/Protocol/Authentification/AuthSocket.cpp b/src/wowgm/Protocol/Authentification/AuthSocket.cpp
index f147984..7553ca0 100644
--- a/src/wowgm/Protocol/Authentification/AuthSocket.cpp
+++ b/src/wowgm/Protocol/Authentification/AuthSocket.cpp
@@ -124,8 +124,6 @@ namespace wowgm::protocol::authentification
         context.Finalize();
         x.SetBinary(context);

-        // LOG_DEBUG << "x = " << x.AsHexStr();
-
         do {
             a.SetRand(19 * 8);
             A = g.ModExp(a, N);
@@ -170,18 +168,30 @@ namespace wowgm::protocol::authentification
         K.SetBinary(keyData, 40);
         sClientServices->SetSessionKey(K);

-        std::uint8_t gNHash[20];
+        std::uint8_t gNHash[20] = { };
+        std::uint8_t nHash[20] = { };
+        std::uint8_t gHash[20] = { };
         context.Initialize();
         context.UpdateBigNumbers(N);
         context.Finalize();
-        memcpy(gNHash, context.GetDigest(), context.GetLength());
+        memcpy(nHash, context.GetDigest(), context.GetLength());
+
+        auto to_cout = [](const char* label, std::uint8_t* data, size_t l)
+        {
+            std::cout << label << " : ";
+            for (std::uint32_t i = 0; i < l; ++i)
+                std::cout << "0x" << std::hex << std::setw(2) << std::setfill('0') << std::uint32_t(data[i]) << ", ";
+            std::cout << std::endl;
+        };
+

         context.Initialize();
         context.UpdateBigNumbers(g);
         context.Finalize();
+        memcpy(gHash, context.GetDigest(), context.GetLength());

-        for (int i = 0; i < 20; ++i)
-            gNHash[i] ^= context.GetDigest()[i];
+        for (std::uint32_t hashItr = 0; hashItr < SHA_DIGEST_LENGTH; ++hashItr)
+            gNHash[hashItr] = nHash[hashItr] ^ gHash[hashItr];

         BigNumber t3;
         t3.SetBinary(gNHash, 20);
@@ -208,16 +218,19 @@ namespace wowgm::protocol::authentification
         M2.SetBinary(context);

         LOG_INFO << "[C->S] AUTH_LOGON_PROOF.";

         AuthPacket<LogonProof> logonChallenge(this->shared_from_this(), AUTH_LOGON_PROOF);
         memcpy(logonChallenge.GetData()->A, A.AsByteArray(32).get(), 32);

[Aokromes]

anyone can test this (without apply any patches) on latest VS 2017 (15.9.16) and latest VS 2019? we are looking to use C++17 features ASAP.

[jackpoz]

I made a PR to enable C++17 https://github.com/TrinityCore/TrinityCore/pull/23868 , much easier than having to manually edit files (why don't people push changes to branches instead of writing manual steps ?)

I still need to raise VS version to a C++17-working one

[Warpten]

(why don't people push changes to branches instead of writing manual steps ?)

The TLDR is that I don't have a clone of TC.

[jackpoz]

You patch is not needed and it doesn't enable c++17 in cmake, so I didn't mean you :)

[Aokromes]

anyone? https://github.com/TrinityCore/TrinityCore/issues/22343#issuecomment-534041584 @jameyboor

[jackpoz]

https://github.com/TrinityCore/TrinityCore/pull/23868 works fine and raises VS version

[ryancheung]

I tested https://github.com/TrinityTBC/core/commit/45cd1850790e54f752ffd9d0cd508417604c6241 and every thing just works fine in Release build .

VS version: Visual studio 2017 Community
MSVC version: 19.16.27034.0

It's time to step into c++17.