From 079ddef60f28db53666dadb24650166ed12abece Mon Sep 17 00:00:00 2001 From: seelenkuchen <10817856+seelenkuchen@users.noreply.github.com> Date: Mon, 2 Nov 2026 13:09:50 +0100 Subject: [PATCH 1/4] [sound] Keep channel loops from stepping before the array Three loops decremented the channel pointer before checking the count, so the last step pointed before Sound_apChannels (NULL - 1 with no channels). Same iteration order. Validation (Linux host, MSVC not run): - GCC ASan+UBSan: Canyonlands headless, 1,000 frames, no reports (was 15 locations). End state identical to the Clang native run and to the run before this change. - MinGW i686 Debug and standalone Release build. Unit tests: 13 of 14 pass; std fails with the 2 known stdConffile tests. std.System: 61 tests, 2 known failures, 2 ignored. Regression run: 0.0025, OK. - Not run: MSVC DX6/DX9 builds, host check. --- Libs/sound/Sound.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/Libs/sound/Sound.c b/Libs/sound/Sound.c index 164d78b39..0a9927e00 100644 --- a/Libs/sound/Sound.c +++ b/Libs/sound/Sound.c @@ -533,12 +533,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) @@ -2501,12 +2502,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; @@ -2842,12 +2844,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 ) From faa996c34a177738e17e9761ae9b1be916983cd9 Mon Sep 17 00:00:00 2001 From: seelenkuchen <10817856+seelenkuchen@users.noreply.github.com> Date: Mon, 2 Nov 2026 13:09:50 +0100 Subject: [PATCH 2/4] [sith] Skip zeroing the puppet classes when none are allocated With size 0 the array stays NULL, and memset on NULL is undefined. Validation (Linux host, MSVC not run): - GCC ASan+UBSan: Canyonlands headless, 1,000 frames, no reports (was 15 locations). End state identical to the Clang native run and to the run before this change. - MinGW i686 Debug and standalone Release build. Unit tests: 13 of 14 pass; std fails with the 2 known stdConffile tests. std.System: 61 tests, 2 known failures, 2 ignored. Regression run: 0.0025, OK. - Not run: MSVC DX6/DX9 builds, host check. --- Libs/sith/Engine/sithPuppet.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/Libs/sith/Engine/sithPuppet.c b/Libs/sith/Engine/sithPuppet.c index 999230679..8fad45c71 100644 --- a/Libs/sith/Engine/sithPuppet.c +++ b/Libs/sith/Engine/sithPuppet.c @@ -2832,7 +2832,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; } From f478d81715497d723d256fd644ee5eecb68a5c6c Mon Sep 17 00:00:00 2001 From: seelenkuchen <10817856+seelenkuchen@users.noreply.github.com> Date: Mon, 2 Nov 2026 13:09:50 +0100 Subject: [PATCH 3/4] [std] Store and check heap end markers through memcpy The marker sits right after the user's bytes, so its address is often unaligned. It's now always 4 bytes, which is what Free already checked. Validation (Linux host, MSVC not run): - GCC ASan+UBSan: Canyonlands headless, 1,000 frames, no reports (was 15 locations). End state identical to the Clang native run and to the run before this change. - MinGW i686 Debug and standalone Release build. Unit tests: 13 of 14 pass; std fails with the 2 known stdConffile tests. std.System: 61 tests, 2 known failures, 2 ignored. Regression run: 0.0025, OK. - Not run: MSVC DX6/DX9 builds, host check. --- Libs/std/General/stdMemory.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/Libs/std/General/stdMemory.c b/Libs/std/General/stdMemory.c index 6892f8b3b..81789f1d7 100644 --- a/Libs/std/General/stdMemory.c +++ b/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 ) { From 44b2771a408699ab141262537f7a1dee9f665872 Mon Sep 17 00:00:00 2001 From: seelenkuchen <10817856+seelenkuchen@users.noreply.github.com> Date: Mon, 2 Nov 2026 21:39:50 +0100 Subject: [PATCH 4/4] [sith] Reject cogstatus indexes equal to the cog count sithCog_CogStatus checked index > numCogs, so index == numCogs read one element past aCogs. Adds the first sithCog tests to sithTests: the boundary index, an index past the array, and the last valid index. Validation (MinGW-w64 i686 GCC 13, DX9 Debug, under Wine 10): - Passed: sith 41/41, including the 3 new sithCog tests. With the old bound, TestRejectsIndexEqualToNumCogs fails (expected 0, was 1). - Unit suites otherwise unchanged: 12 of 13 pass; std fails the 2 known stdConffile tests. - Not run: MSVC, DX6, Release, guard and QOL profiles. --- Docs/Testing.md | 2 +- Libs/sith/Cog/Tests/sithCogTest.c | 120 ++++++++++++++++++++++++++++++ Libs/sith/Cog/sithCog.c | 2 +- Libs/sith/Tests/CMakeLists.txt | 11 +++ Libs/sith/Tests/sithTestRunner.c | 2 + 5 files changed, 135 insertions(+), 2 deletions(-) create mode 100644 Libs/sith/Cog/Tests/sithCogTest.c diff --git a/Docs/Testing.md b/Docs/Testing.md index 60f93e1b5..68d52d0ad 100644 --- a/Docs/Testing.md +++ b/Docs/Testing.md @@ -232,7 +232,7 @@ startup and a stable module-level CTest entry. | `stdDisruptiveSystemTests` | `std.DisruptiveSystem` | Opt-in fullscreen/device changes | Available | | `soundTests` | `sound` | Deterministic lip-sync generation and lookup, AudioLib codec and header vectors | Available | | `smushTests` | `smush` | INSANE movie player: core, bitmap, codec, sound, stream areas and original-engine assembly vectors | Available | -| `sithTests` | `sith` | World thing/template I/O and voice lip-sync | Available | +| `sithTests` | `sith` | World thing/template I/O, voice lip-sync and the cogstatus console command bounds | Available | | `sithThingQuetzVecTests` | `sith.ThingQuetz` | Original-engine vectors of the Quetz tail functions (world-state harness, `Libs/sith/Tests/Vec`) | Available | | `sithAIInstinctVecTests` | `sith.AIInstinct` | Original-engine vectors of the instinct registration (world-state harness) | Available | | `sithAIInstinctFnVecTests` | `sith.AIInstinctFn` | Original-engine vectors of the instinct functions (world-state harness) | Available | diff --git a/Libs/sith/Cog/Tests/sithCogTest.c b/Libs/sith/Cog/Tests/sithCogTest.c new file mode 100644 index 000000000..46f500951 --- /dev/null +++ b/Libs/sith/Cog/Tests/sithCogTest.c @@ -0,0 +1,120 @@ +#include + +#include + +#include +#include + +#include +#include + +#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); +} diff --git a/Libs/sith/Cog/sithCog.c b/Libs/sith/Cog/sithCog.c index cbb17c41a..815a0ae72 100644 --- a/Libs/sith/Cog/sithCog.c +++ b/Libs/sith/Cog/sithCog.c @@ -2052,7 +2052,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 ) { diff --git a/Libs/sith/Tests/CMakeLists.txt b/Libs/sith/Tests/CMakeLists.txt index ee41a823f..891701ae5 100644 --- a/Libs/sith/Tests/CMakeLists.txt +++ b/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 diff --git a/Libs/sith/Tests/sithTestRunner.c b/Libs/sith/Tests/sithTestRunner.c index 024295ffc..0d4c535ca 100644 --- a/Libs/sith/Tests/sithTestRunner.c +++ b/Libs/sith/Tests/sithTestRunner.c @@ -5,12 +5,14 @@ void TEST_sithTemplate_GROUP_RUNNER(void); void TEST_sithThingBinary_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_sithTemplate_GROUP_RUNNER(); TEST_sithVoice_GROUP_RUNNER(); + TEST_sithCog_GROUP_RUNNER(); } int main(int argc, const char* argv[])