Merge pull request #1585 from dpogue/san-fixes

Sanitizer fixes and plProduct static init fix

.github/workflows/ci.yml

@@ -104,6 +104,8 @@ jobs:
       - name: Test
         run: |
           cmake --build build --target check --config "${{ matrix.cfg.type }}" -j 4
+        env:
+          CTEST_OUTPUT_ON_FAILURE: 1
 
   max:
     # Can only run if we have a token for our super seekrit Max SDK repo. Sad.
@@ -247,6 +249,8 @@ jobs:
       - name: Test
         run: |
           cmake --build build --target check -j 4
+        env:
+          CTEST_OUTPUT_ON_FAILURE: 1
 
       - name: Install
         run: |
@@ -326,9 +330,10 @@ jobs:
           cmake --build build --config "${{ matrix.cfg.type }}" -j 2
 
       - name: Test
-        if: matrix.platform.arch == 'x86_64'
         run: |
           cmake --build build --target check --config "${{ matrix.cfg.type }}" -j 2
+        env:
+          CTEST_OUTPUT_ON_FAILURE: 1
 
       - name: Install
         run: |

Sources/Plasma/CoreLib/hsStream.cpp

@@ -629,6 +629,8 @@ uint32_t hsRAMStream::Read(uint32_t byteCount, void * buffer)
 
 uint32_t hsRAMStream::Write(uint32_t byteCount, const void* buffer)
 {
+    hsAssert(fVector.data(), "Trying to write to a null RAM buffer");
+
     size_t spaceUntilEof = fVector.size() - fPosition;
     if (byteCount <= spaceUntilEof) {
         memcpy(fVector.data() + fPosition, buffer, byteCount);
@@ -957,26 +959,6 @@ bool hsQueueStream::AtEnd()
 // hsBufferedStream
 ///////////////////////////////////////////////////////////////////////////////
 
-inline void FastByteCopy(void* dest, const void* src, uint32_t bytes)
-{
-    // Don't use memcpy if the read is 4 bytes or less, it's faster to just do a
-    // direct copy
-    switch (bytes)
-    {
-    case 4:
-        *((uint32_t*)dest) = *((const uint32_t*)src);
-        break;
-    case 2:
-        *((uint16_t*)dest) = *((const uint16_t*)src);
-        break;
-    case 1:
-        *((uint8_t*)dest) = *((const uint8_t*)src);
-        break;
-    default:
-        memcpy(dest, src, bytes);
-    }
-}
-
 //#define LOG_BUFFERED
 
 hsBufferedStream::hsBufferedStream()
@@ -1081,7 +1063,7 @@ uint32_t hsBufferedStream::Read(uint32_t bytes, void* buffer)
             uint32_t bytesInBuffer = fBufferLen - bufferPos;
             uint32_t cachedReadSize = bytesInBuffer < bytes ? bytesInBuffer : bytes;
 
-            FastByteCopy(buffer, &fBuffer[bufferPos], cachedReadSize);
+            memcpy(buffer, &fBuffer[bufferPos], cachedReadSize);
 
             fPosition += cachedReadSize;
             numReadBytes += cachedReadSize;

Sources/Plasma/NucleusLib/pnUUID/pnUUID_Unix.cpp

@@ -50,41 +50,28 @@ static_assert(sizeof(plUUID) == sizeof(uuid_t), "plUUID and uuid_t types differ
 
 struct plUUIDHelper
 {
-    typedef struct _GUID {
-      uint32_t Data1;
-      uint16_t Data2;
-      uint16_t Data3;
-      uint8_t  Data4[8];
-    } GUID;
-
-    static inline void CopyToPlasma( plUUID * dst, const uuid_t & src )
+    static inline void CopyToPlasma(plUUID* dst, const uuid_t& src)
     {
-        hsAssert( sizeof(uuid_t)==sizeof(dst->fData), "sizeof(uuid_t)!=sizeof(plUUID)" );
-        memcpy( (void*)dst->fData, (const void *)src, sizeof( plUUID ) );
-
-        /*
-         GUIDs are always internally encoded in little endian,
-         while the uuid_t structure is big endian.
-         */
-        GUID* guid = reinterpret_cast<GUID*>(dst->fData);
-        guid->Data1 = hsSwapEndian32(guid->Data1);
-        guid->Data2 = hsSwapEndian16(guid->Data2);
-        guid->Data3 = hsSwapEndian16(guid->Data3);
+        memcpy(dst->fData, src, sizeof(plUUID));
+
+        // plUUIDs need to always be encoded in little endian, but the uuid_t struct is big endian
+        // This isn't guaranteed to be 32-bit-aligned, so casting to uint16_t/uint32_t pointers is UB
+        std::swap(dst->fData[0], dst->fData[3]);
+        std::swap(dst->fData[1], dst->fData[2]);
+        std::swap(dst->fData[4], dst->fData[5]);
+        std::swap(dst->fData[6], dst->fData[7]);
     }
 
-    static inline void CopyToNative( uuid_t & dst, const plUUID * src )
+    static inline void CopyToNative(uuid_t& dst, const plUUID* src)
     {
-        hsAssert( sizeof(uuid_t)==sizeof(src->fData), "sizeof(uuid_t)!=sizeof(plUUID)" );
-        memcpy( (void*)dst, (const void *)src->fData, sizeof( plUUID ) );
-
-        /*
-         GUIDs are always internally encoded in little endian,
-         while the uuid_t structure is big endian.
-         */
-        GUID* guid = reinterpret_cast<GUID*>(dst);
-        guid->Data1 = hsSwapEndian32(guid->Data1);
-        guid->Data2 = hsSwapEndian16(guid->Data2);
-        guid->Data3 = hsSwapEndian16(guid->Data3);
+        memcpy(dst, src->fData, sizeof(plUUID));
+
+        // uuid_t struct is big endian, but plUUIDs are always encoded in little endian
+        // This isn't guaranteed to be 32-bit-aligned, so casting to uint16_t/uint32_t pointers is UB
+        std::swap(dst[0], dst[3]);
+        std::swap(dst[1], dst[2]);
+        std::swap(dst[4], dst[5]);
+        std::swap(dst[6], dst[7]);
     }
 };
 
@@ -133,12 +120,12 @@ bool plUUID::FromString( const char * str )
     return true;
 }
 
-bool plUUID::ToString( ST::string & out ) const
+bool plUUID::ToString(ST::string& out) const
 {
     uuid_t g;
-    plUUIDHelper::CopyToNative( g, this );
+    plUUIDHelper::CopyToNative(g, this);
     char buf[40];
-    uuid_unparse( g, buf );
+    uuid_unparse_lower(g, buf);
     out = buf;
     return true;
 }

Sources/Plasma/PubUtilLib/plAudioCore/plBufferedFileReader.cpp

@@ -113,7 +113,7 @@ void    plBufferedFileReader::Close()
 {
     //plProfile_DelMem( SndBufferedMem, fBufferSize );
 
-    delete fBuffer;
+    delete[] fBuffer;
     fBuffer = nullptr;
     fBufferSize = 0;
     fCursor = 0;

Sources/Plasma/PubUtilLib/plPipeline/plTransitionMgr.cpp

@@ -79,10 +79,10 @@ You can contact Cyan Worlds, Inc. by email [email protected]
 //// Constructor/Destructor //////////////////////////////////////////////////
 
 plTransitionMgr::plTransitionMgr()
+    : fEffectPlate(), fCurrentEffect(kIdle), fRegisteredForTime(),
+    fHoldAtEnd(), fPlaying(), fNoSoundFade(), fEffectLength(), fCurrOpacity(),
+    fOpacDelta(), fLastTime()
 {
-    fEffectPlate = nullptr;
-    fCurrentEffect = kIdle;
-    fPlaying = false;
 }
 
 void    plTransitionMgr::Init()

Sources/Tests/NucleusTests/CMakeLists.txt

@@ -2,3 +2,4 @@ include_directories("${PLASMA_SOURCE_ROOT}/CoreLib")
 include_directories("${PLASMA_SOURCE_ROOT}/NucleusLib")
 
 add_subdirectory(pnEncryptionTest)
+add_subdirectory(pnUUIDTest)

Sources/Tests/NucleusTests/pnUUIDTest/CMakeLists.txt

@@ -0,0 +1,13 @@
+set(pnUUIDTest_SOURCES
+    test_plUUID.cpp
+)
+
+plasma_test(test_pnUUID SOURCES ${pnUUIDTest_SOURCES})
+target_link_libraries(
+    test_pnUUID
+    PRIVATE
+        CoreLib
+        pnUUID
+        gtest_main
+)
+

Sources/Tests/NucleusTests/pnUUIDTest/test_plUUID.cpp

@@ -0,0 +1,135 @@
+/*==LICENSE==*
+
+CyanWorlds.com Engine - MMOG client, server and tools
+Copyright (C) 2011  Cyan Worlds, Inc.
+
+This program is free software: you can redistribute it and/or modify
+it under the terms of the GNU General Public License as published by
+the Free Software Foundation, either version 3 of the License, or
+(at your option) any later version.
+
+This program is distributed in the hope that it will be useful,
+but WITHOUT ANY WARRANTY; without even the implied warranty of
+MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+GNU General Public License for more details.
+
+You should have received a copy of the GNU General Public License
+along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+Additional permissions under GNU GPL version 3 section 7
+
+If you modify this Program, or any covered work, by linking or
+combining it with any of RAD Game Tools Bink SDK, Autodesk 3ds Max SDK,
+NVIDIA PhysX SDK, Microsoft DirectX SDK, OpenSSL library, Independent
+JPEG Group JPEG library, Microsoft Windows Media SDK, or Apple QuickTime SDK
+(or a modified version of those libraries),
+containing parts covered by the terms of the Bink SDK EULA, 3ds Max EULA,
+PhysX SDK EULA, DirectX SDK EULA, OpenSSL and SSLeay licenses, IJG
+JPEG Library README, Windows Media SDK EULA, or QuickTime SDK EULA, the
+licensors of this Program grant you additional
+permission to convey the resulting work. Corresponding Source for a
+non-source form of such a combination shall include the source code for
+the parts of OpenSSL and IJG JPEG Library used as well as that of the covered
+work.
+
+You can contact Cyan Worlds, Inc. by email [email protected]
+ or by snail mail at:
+      Cyan Worlds, Inc.
+      14617 N Newport Hwy
+      Mead, WA   99021
+
+*==LICENSE==*/
+
+#include <gtest/gtest.h>
+#include "pnUUID/pnUUID.h"
+
+const char* TEST_UUID = "307c1e1c-c0a2-456b-91f0-fd6faef4920a";
+
+TEST(plUUID, NullUUID)
+{
+    plUUID u = kNilUuid;
+    EXPECT_TRUE(u.IsNull());
+    EXPECT_FALSE(u.IsSet());
+
+    plUUID u2;
+    EXPECT_TRUE(u2.IsNull());
+    EXPECT_FALSE(u2.IsSet());
+    EXPECT_TRUE(u2.IsEqualTo(&u));
+}
+
+TEST(plUUID, GeneratedUUID)
+{
+    plUUID u = plUUID::Generate();
+    EXPECT_FALSE(u.IsNull());
+    EXPECT_TRUE(u.IsSet());
+    EXPECT_FALSE(u.IsEqualTo(&kNilUuid));
+
+    plUUID u2 = plUUID::Generate();
+    EXPECT_FALSE(u.IsEqualTo(&u2)); // Should be unique
+}
+
+TEST(plUUID, Clear)
+{
+    plUUID u = plUUID::Generate();
+    EXPECT_FALSE(u.IsEqualTo(&kNilUuid));
+
+    u.Clear();
+    EXPECT_TRUE(u.IsEqualTo(&kNilUuid));
+}
+
+TEST(plUUID, CopyFrom)
+{
+    plUUID u1 = plUUID::Generate();
+    plUUID u2 = plUUID::Generate();
+    plUUID u3(u2);
+
+    EXPECT_FALSE(u1.IsEqualTo(&u3));
+    EXPECT_TRUE(u2.IsEqualTo(&u3));
+
+    u1.CopyFrom(u2);
+    EXPECT_TRUE(u1.IsEqualTo(&u3));
+}
+
+TEST(plUUID, FromString)
+{
+    plUUID u1(TEST_UUID);
+    plUUID u2;
+
+    EXPECT_FALSE(u1.IsEqualTo(&u2));
+
+    u2.FromString(TEST_UUID);
+
+    EXPECT_TRUE(u1.IsEqualTo(&u2));
+
+    EXPECT_STREQ(TEST_UUID, u1.AsString().c_str());
+}
+
+TEST(plUUID, endianness)
+{
+    plUUID u(TEST_UUID);
+
+    // First 32 bits in little endian
+    EXPECT_EQ(0x1c, u.fData[0]);
+    EXPECT_EQ(0x1e, u.fData[1]);
+    EXPECT_EQ(0x7c, u.fData[2]);
+    EXPECT_EQ(0x30, u.fData[3]);
+
+    // Next 16 bits in little endian
+    EXPECT_EQ(0xa2, u.fData[4]);
+    EXPECT_EQ(0xc0, u.fData[5]);
+
+    // Next 16 bits in little endian
+    EXPECT_EQ(0x6b, u.fData[6]);
+    EXPECT_EQ(0x45, u.fData[7]);
+
+    // Rest in big endian
+    EXPECT_EQ(0x91, u.fData[8]);
+    EXPECT_EQ(0xf0, u.fData[9]);
+
+    EXPECT_EQ(0xfd, u.fData[10]);
+    EXPECT_EQ(0x6f, u.fData[11]);
+    EXPECT_EQ(0xae, u.fData[12]);
+    EXPECT_EQ(0xf4, u.fData[13]);
+    EXPECT_EQ(0x92, u.fData[14]);
+    EXPECT_EQ(0x0a, u.fData[15]);
+}

cmake/hsBuildInfo.inl.cmake

@@ -7,14 +7,14 @@ namespace plProduct
     constexpr uint32_t PRODUCT_BUILD_ID = @PRODUCT_BUILD_ID@;
     constexpr uint32_t PRODUCT_BUILD_TYPE = @PRODUCT_BUILD_TYPE@;
 
-    static const ST::string PRODUCT_CORE_NAME = ST_LITERAL("@PRODUCT_CORE_NAME@");
-    static const ST::string PRODUCT_SHORT_NAME = ST_LITERAL("@PRODUCT_SHORT_NAME@");
-    static const ST::string PRODUCT_LONG_NAME = ST_LITERAL("@PRODUCT_LONG_NAME@");
+    constexpr std::string_view PRODUCT_CORE_NAME = "@PRODUCT_CORE_NAME@";
+    constexpr std::string_view PRODUCT_SHORT_NAME = "@PRODUCT_SHORT_NAME@";
+    constexpr std::string_view PRODUCT_LONG_NAME = "@PRODUCT_LONG_NAME@";
     constexpr std::string_view PRODUCT_UUID = "@PRODUCT_UUID@";
 
-    static const ST::string GIT_REV = ST_LITERAL("@GIT_REV@");
-    static const ST::string GIT_TAG = ST_LITERAL("@GIT_TAG@");
+    constexpr std::string_view GIT_REV = "@GIT_REV@";
+    constexpr std::string_view GIT_TAG = "@GIT_TAG@";
 
-    static const ST::string VOLATILE_BUILD_DATE = ST_LITERAL("@BUILD_DATE@");
-    static const ST::string VOLATILE_BUILD_TIME = ST_LITERAL("@BUILD_TIME@");
+    constexpr std::string_view VOLATILE_BUILD_DATE = "@BUILD_DATE@";
+    constexpr std::string_view VOLATILE_BUILD_TIME = "@BUILD_TIME@";
 };