Fix undefined behavior in sound, sith and std

Libs/sith/Cog/Tests/sithCogTest.c

@@ -0,0 +1,120 @@
+#include <unity_fixture.h>
+
+#include <stddef.h>
+
+#include <sith/Cog/sithCog.h>
+#include <sith/World/sithWorld.h>
+
+#include <std/General/std.h>
+#include <std/General/stdUtil.h>
+
+#define SITHCOG_TEST_MAXPRINTS    8u
+#define SITHCOG_TEST_PRINTSIZE  256u
+#define SITHCOG_TEST_NUMCOGS      2u
+
+TEST_GROUP(sithCog);
+
+#if defined(J3D_LOCAL_RTI)
+// The tested copy of sithCog.c reaches std_g_genBuffer through the local RTI
+// externs; std.c's fixed-address definition is not linked into sithTests, so
+// this fixture owns the buffer the SITHCONSOLE_PRINTF path formats into.
+char std_g_genBuffer[2048] = { 0 };
+#endif
+
+// Replacement for sithConsole_PrintString in the tested copy of sithCog.c,
+// selected by the per-source COMPILE_DEFINITIONS in Tests/CMakeLists.txt.
+void J3DAPI sithCogTest_PrintString(const char* pString);
+
+static char sithCogTest_aPrints[SITHCOG_TEST_MAXPRINTS][SITHCOG_TEST_PRINTSIZE];
+static size_t sithCogTest_numPrints;
+
+static SithWorld sithCogTest_world;
+static SithCog sithCogTest_aCogs[3];
+static SithCogScript sithCogTest_aScripts[3];
+static SithCogSymbolTable sithCogTest_aSymbolTables[3];
+static const char sithCogTest_aCogNames[3][16]    = { "firstCog", "midCog", "ghostCog" };
+static const char sithCogTest_aScriptNames[3][16] = { "first.cog", "mid.cog", "ghost.cog" };
+
+void J3DAPI sithCogTest_PrintString(const char* pString)
+{
+    // Capture each full console string so cases can compare exact output.
+    if ( pString && sithCogTest_numPrints < SITHCOG_TEST_MAXPRINTS )
+    {
+        STD_STRCPY(sithCogTest_aPrints[sithCogTest_numPrints], pString);
+    }
+
+    ++sithCogTest_numPrints;
+}
+
+TEST_SETUP(sithCog)
+{
+    // Build a two-cog world whose aCogs array retains one more fully populated
+    // entry past numCogs; the original 'index > numCogs' bound let cogstatus
+    // read that element at index == numCogs and print its status instead of
+    // rejecting it.
+    sithCogTest_numPrints = 0;
+    STD_ZEROMEM(sithCogTest_aPrints, sizeof(sithCogTest_aPrints));
+    STD_ZEROMEM(&sithCogTest_world, sizeof(sithCogTest_world));
+    STD_ZEROMEM(sithCogTest_aCogs, sizeof(sithCogTest_aCogs));
+    STD_ZEROMEM(sithCogTest_aScripts, sizeof(sithCogTest_aScripts));
+    STD_ZEROMEM(sithCogTest_aSymbolTables, sizeof(sithCogTest_aSymbolTables));
+
+    sithCogTest_world.numCogs  = SITHCOG_TEST_NUMCOGS;
+    sithCogTest_world.sizeCogs = STD_ARRAYLEN(sithCogTest_aCogs);
+    sithCogTest_world.aCogs    = sithCogTest_aCogs;
+
+    for ( size_t i = 0; i < STD_ARRAYLEN(sithCogTest_aCogs); i++ )
+    {
+        SithCog* pCog = &sithCogTest_aCogs[i];
+        pCog->idx          = (int)i;
+        pCog->pScript      = &sithCogTest_aScripts[i];
+        pCog->pSymbolTable = &sithCogTest_aSymbolTables[i];
+        STD_STRCPY(pCog->aName, sithCogTest_aCogNames[i]);
+        STD_STRCPY(pCog->pScript->aName, sithCogTest_aScriptNames[i]);
+    }
+
+    sithWorld_g_pCurrentWorld = &sithCogTest_world;
+}
+
+TEST_TEAR_DOWN(sithCog)
+{
+    // Restore the shared world global so every group initializes its own state.
+    sithWorld_g_pCurrentWorld = NULL;
+    sithCogTest_numPrints     = 0;
+}
+
+TEST(sithCog, TestRejectsIndexEqualToNumCogs)
+{
+    // Regression: the original bound tested index > numCogs, so index == numCogs
+    // slipped through, read aCogs[2] one element past the world's two logical
+    // cogs, and printed that fully populated element's status.
+    TEST_ASSERT_EQUAL_INT(0, sithCog_CogStatus(NULL, "2"));
+    TEST_ASSERT_EQUAL_size_t(1u, sithCogTest_numPrints);
+    TEST_ASSERT_EQUAL_STRING("Error, bad parameters.\n", sithCogTest_aPrints[0]);
+}
+
+TEST(sithCog, TestRejectsIndexPastCogArray)
+{
+    // Index 3 exceeds both numCogs and sizeCogs; the command must report the
+    // same parameter error without reading the array.
+    TEST_ASSERT_EQUAL_INT(0, sithCog_CogStatus(NULL, "3"));
+    TEST_ASSERT_EQUAL_size_t(1u, sithCogTest_numPrints);
+    TEST_ASSERT_EQUAL_STRING("Error, bad parameters.\n", sithCogTest_aPrints[0]);
+}
+
+TEST(sithCog, TestPrintsCogStatusForValidIndex)
+{
+    // The last valid index (numCogs - 1) must keep printing its status line and
+    // return 1; a boundary fix that over-rejects would fail here. The zero-symbol
+    // table keeps the expected console output to exactly this line.
+    TEST_ASSERT_EQUAL_INT(1, sithCog_CogStatus(NULL, "1"));
+    TEST_ASSERT_EQUAL_size_t(1u, sithCogTest_numPrints);
+    TEST_ASSERT_EQUAL_STRING("Cog #1: Name:midCog  Script mid.cog\n", sithCogTest_aPrints[0]);
+}
+
+TEST_GROUP_RUNNER(sithCog)
+{
+    RUN_TEST_CASE(sithCog, TestRejectsIndexEqualToNumCogs);
+    RUN_TEST_CASE(sithCog, TestRejectsIndexPastCogArray);
+    RUN_TEST_CASE(sithCog, TestPrintsCogStatusForValidIndex);
+}

Libs/sith/Cog/sithCog.c

@@ -2041,7 +2041,7 @@ int J3DAPI sithCog_CogStatus(const SithConsoleCommand* pFunc, const char* pArg)
     if ( !pWorld
         || !pArg
         || sscanf_s(pArg, "%d", &index) != 1
-        || index > pWorld->numCogs
+        || index >= pWorld->numCogs // Fixed: index == numCogs read one past aCogs
         || (pCog = &pWorld->aCogs[index], !pCog->pScript)
         || !pCog->pSymbolTable )
     {

Libs/sith/Engine/sithPuppet.c

@@ -2834,7 +2834,10 @@ int J3DAPI sithPuppet_AllocWorldPuppets(SithWorld* pWorld, size_t size)
 
     pWorld->sizePuppetClasses = size;
     pWorld->numPuppetClasses  = 0;
-    STD_ZEROMEM(pWorld->aPuppetClasses, sizeof(SithPuppetClass) * pWorld->sizePuppetClasses);
+    if ( pWorld->aPuppetClasses ) // Fixed: memset with a NULL pointer is UB when size is 0 and the allocation was skipped
+    {
+        STD_ZEROMEM(pWorld->aPuppetClasses, sizeof(SithPuppetClass) * pWorld->sizePuppetClasses);
+    }
     return 0;
 }
 

Libs/sith/Tests/CMakeLists.txt

@@ -1,17 +1,28 @@
 set(SITH_TEST_SOURCES
     sithTestRunner.c
+    ../Cog/Tests/sithCogTest.c
     ../World/Tests/sithTemplateTest.c
     ../World/Tests/sithThingTest.c
     ../World/Tests/sithVoiceTest.c
 )
 
 set(SITH_TESTED_SOURCES
+    "${CMAKE_CURRENT_SOURCE_DIR}/../Cog/sithCog.c"
     "${CMAKE_CURRENT_SOURCE_DIR}/../World/sithTemplate.c"
     "${CMAKE_CURRENT_SOURCE_DIR}/../World/sithThing.c"
     "${CMAKE_CURRENT_SOURCE_DIR}/../World/sithVoice.c"
     "${CMAKE_CURRENT_SOURCE_DIR}/../../std/General/stdConffile.c"
 )
 
+set_property(SOURCE "${CMAKE_CURRENT_SOURCE_DIR}/../Cog/sithCog.c" PROPERTY COMPILE_DEFINITIONS
+    J3D_LOCAL_RTI
+    sithConsole_PrintString=sithCogTest_PrintString
+)
+
+set_property(SOURCE "${CMAKE_CURRENT_SOURCE_DIR}/../Cog/Tests/sithCogTest.c" PROPERTY COMPILE_DEFINITIONS
+    J3D_LOCAL_RTI
+)
+
 set_property(SOURCE "${CMAKE_CURRENT_SOURCE_DIR}/../World/sithTemplate.c" PROPERTY COMPILE_DEFINITIONS
     J3D_LOCAL_RTI
     J3D_LOCAL_RDROID_RTI

Libs/sith/Tests/sithTestRunner.c

@@ -6,13 +6,15 @@ void TEST_sithTemplate_GROUP_RUNNER(void);
 void TEST_sithThingBinary_GROUP_RUNNER(void);
 void TEST_sithThingParseArg_GROUP_RUNNER(void);
 void TEST_sithVoice_GROUP_RUNNER(void);
+void TEST_sithCog_GROUP_RUNNER(void);
 
 static void SithTest_RunAllTests(void)
 {
     TEST_sithThingBinary_GROUP_RUNNER();
     TEST_sithThingParseArg_GROUP_RUNNER();
     TEST_sithTemplate_GROUP_RUNNER();
     TEST_sithVoice_GROUP_RUNNER();
+    TEST_sithCog_GROUP_RUNNER();
 }
 
 int main(int argc, const char* argv[])

Libs/sound/Sound.c

@@ -538,12 +538,13 @@ int J3DAPI Sound_Save(tFileHandle fh)
 
     while ( 1 )
     {
-        --pCurChannel;
         if ( !channelNum-- )
         {
             break;
         }
 
+        --pCurChannel; // Fixed: decrement after the count check so the pointer is never moved before the start of Sound_apChannels
+
         // TODO: Save also 2D sounds
         if ( pCurChannel->handle != SOUND_INVALIDHANDLE
             && ((pCurChannel->flags & (SOUND_CHANNEL_LOOP | SOUND_CHANNEL_PLAYING)) == (SOUND_CHANNEL_LOOP | SOUND_CHANNEL_PLAYING)
@@ -2516,12 +2517,13 @@ void J3DAPI Sound_Update(const rdVector3* pPos, const rdVector3* pVelocity, cons
 
     while ( 1 )
     {
-        --pCurChannel;
         if ( !count-- )
         {
             break;
         }
 
+        --pCurChannel; // Fixed: decrement after the count check so the pointer is never moved before the start of Sound_apChannels
+
         if ( pCurChannel->handle != SOUND_INVALIDHANDLE && (pCurChannel->flags & SOUND_CHANNEL_3DSOUND) != 0 )
         {
             SoundThingInfo thngSndInfo;
@@ -2857,12 +2859,13 @@ size_t J3DAPI Sound_GetAllInstanceInfo(SoundInstanceInfo* pCurInstance, size_t s
     tSoundChannel* pCurChannel = &Sound_apChannels[Sound_numChannels];
     while ( 1 )
     {
-        --pCurChannel;
         if ( !numChannels-- )
         {
             break;
         }
 
+        --pCurChannel; // Fixed: decrement after the count check so the pointer is never moved before the start of Sound_apChannels
+
         if ( pCurChannel->handle != SOUND_INVALIDHANDLE )
         {
             if ( numSounds >= sizeInstances )

Libs/std/General/stdMemory.c

@@ -199,7 +199,8 @@ void* J3DAPI stdMemory_Malloc(size_t size, const char* pFilename, size_t line)
     memset(&pHeap->pMemory, STDMEMORY_UNINIT_PATTERN, size);
 
     pHeap->header.magic = STDMEMORY_HEADERMAGIC;
-    *(void**)((char*)&pHeap->pMemory + size) = (void*)STDMEMORY_HEADERMAGIC; // Set end marker
+    uint32_t endMarker = STDMEMORY_HEADERMAGIC; // Fixed: store the end marker through memcpy; its address can be unaligned, so a void* dereference is UB (same bytes on 32-bit)
+    memcpy((char*)&pHeap->pMemory + size, &endMarker, sizeof(endMarker)); // Set end marker
 
     stdMemory_g_curState.totalBytes += size;
 
@@ -223,7 +224,9 @@ void J3DAPI stdMemory_Free(void* pBytes)
 
     STD_ASSERTREL((pHeader != NULL) && ((uint32_t)pHeader == pHeader->id));
     STD_ASSERTREL(pHeader->magic == STDMEMORY_HEADERMAGIC);
-    STD_ASSERTREL(*(uint32_t*)((uint8_t*)pBytes + pHeader->size) == STDMEMORY_HEADERMAGIC);
+    uint32_t endMarker; // Fixed: read the end marker through memcpy; its address can be unaligned, so a uint32_t dereference is UB
+    memcpy(&endMarker, (uint8_t*)pBytes + pHeader->size, sizeof(endMarker));
+    STD_ASSERTREL(endMarker == STDMEMORY_HEADERMAGIC);
 
     // Set freed memory to a free pattern
     memset(pBytes, STDMEMORY_FREE_PATTERN, pHeader->size);
@@ -293,7 +296,8 @@ void* J3DAPI stdMemory_Realloc(void* pBytes, size_t size, const char* pFilename,
     // TODO: If new size is larger set the unused memory to uninitialized pattern
 
     pHeap->header.magic = STDMEMORY_HEADERMAGIC;
-    *(void**)((char*)&pHeap->pMemory + size) = (void*)STDMEMORY_HEADERMAGIC; // Set end marker
+    uint32_t endMarker = STDMEMORY_HEADERMAGIC; // Fixed: store the end marker through memcpy; its address can be unaligned, so a void* dereference is UB (same bytes on 32-bit)
+    memcpy((char*)&pHeap->pMemory + size, &endMarker, sizeof(endMarker)); // Set end marker
 
     if ( size >= curSize )
     {