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:https://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@";
 };