Fix four LP64 defects and add Oracle 19 client discovery - #34
Conversation
These three AutoDeltaPackedMap specializations wrote their command count as int32 but wrote the baseline count, and read both counts, as size_t. Under ILP32 size_t was 4 bytes so pack and unpack agreed. Under LP64 size_t binds to the 8-byte uint64 Archive overload, so pack writes 4+8 bytes while unpack reads 8+8 and leaves the stream misaligned by 4 bytes for every subsequent key and value. Switch both counts to int32_t, matching the generic internal_pack/ internal_unpack path and the already-converted PlayerQuestData specialization. This also keeps the serialized form identical to the 32-bit server, so data written by either architecture stays readable by the other.
DebugHelp::getCallStack() handed the caller's uint32 buffer straight to backtrace(), which writes void* entries. Under LP64 that wrote 8 bytes per frame into a 4-byte-per-frame array, overrunning every buffer by 2x: Fatal.cpp uint32[68] = 272 bytes, up to 544 written (stack) CallStack uint32[32] = 128 bytes, 208 written (heap) CallStackCollector uint32[26] = 104 bytes, 208 written (heap) Fatal.cpp's formatMessage() backs every WARNING and FATAL, so any warning that captured a stack corrupted memory. Observed as SIGSEGV inside cfree() with si_addr 0x8 after an unrelated warning fired. Capture into a native void* array and widen into a uint64 buffer, which is correct under both ILP32 and LP64, and widen the storage and printing to match. lookupAddress() already took uint64 on linux; win32 now agrees and no longer truncates StackWalk's DWORD64 address to 32 bits. Also fixes CallStackCollector copying only one element instead of the whole call stack, and CRCing only half the buffer.
The linux shim sampled gettimeofday(), which reports CLOCK_REALTIME and can be slewed or stepped backwards by time synchronisation. Clock::update() subtracts successive samples to derive frame time, so a backwards step produced a negative elapsed time, which AlterScheduler then reported as "no forward time movement" once per affected frame. Use clock_gettime(CLOCK_MONOTONIC) and keep microsecond units so the value still matches QueryPerformanceFrequency(). Falls back to gettimeofday() if the call fails. All getFrameStartTimeMs() consumers compare values relatively, so nothing depends on the previous epoch-based origin.
JavaLibrary::fatalHandler() read the faulting address from a hardcoded 0x44 byte offset into its own frame. That constant describes the 32-bit x86 frame layout; on x86-64 it is misaligned and points at unrelated stack memory, and dereferencing it from inside a SIGSEGV handler risks a recursive fault that loses the crash report entirely. The original code carried a TODO noting the offset was never verified. Restrict that probe to __i386__ and rely on the portable __builtin_return_address() probe already present below it. Zero initialise the lib and file buffers, which the strstr() checks read whenever a lookup fails.
|
just throwing in copilot review to see how it does but otherwise LGTM 2 nits on code style but need to just write a md for it |
There was a problem hiding this comment.
Pull request overview
This PR addresses several LP64 (64-bit) correctness issues in serialization and debugging utilities, and extends CMake-based Oracle client discovery to support Oracle 19 Instant Client layouts. The changes aim to preserve 32-bit behavior and serialized compatibility while fixing 64-bit-only corruption/overflow risks.
Changes:
- Fix packed-map serialization/deserialization count type mismatches by using
int32_tfor command counts. - Correct call stack capture/storage to be pointer-width safe (
uint64), including Linuxbacktrace()handling and Win32 address preservation, and fixCallStackCollectorbuffer copy/CRC sizing. - Make Linux
QueryPerformanceCounteruseCLOCK_MONOTONIC, harden the SIGSEGV handler to avoid invalid frame-offset dereference on x86-64, and add Oracle 19 (nnz19) +${ORACLE_HOME}library discovery paths.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| external/ours/library/unicodeArchive/src/shared/UnicodeAutoDeltaPackedMap.cpp | Uses int32_t for packed-map command counts to keep LP64 streams aligned. |
| engine/shared/library/sharedUtility/src/shared/NetworkIdAutoDeltaPackedMap.cpp | Same packed-map count fix for AutoDeltaPackedMap<int, NetworkId>. |
| engine/shared/library/sharedFoundation/src/shared/AutoDeltaNetworkIdPackedMap.h | Same packed-map count fix for AutoDeltaPackedMap<NetworkId, int>. |
| engine/shared/library/sharedFoundation/src/shared/Fatal.cpp | Widen call stack storage/printing to 64-bit addresses. |
| engine/shared/library/sharedFoundation/src/linux/PlatformGlue.cpp | Make QueryPerformanceCounter monotonic via clock_gettime(CLOCK_MONOTONIC). |
| engine/shared/library/sharedDebug/src/win32/DebugHelp.h | Widen call stack APIs to uint64 on Win32. |
| engine/shared/library/sharedDebug/src/win32/DebugHelp.cpp | Preserve full instruction addresses and widen stack storage/log formatting. |
| engine/shared/library/sharedDebug/src/shared/CallStackCollector.cpp | Fix callstack buffer type, memcpy sizing, and CRC sizing. |
| engine/shared/library/sharedDebug/src/shared/CallStack.h | Store instruction addresses as uint64 to match pointer width. |
| engine/shared/library/sharedDebug/src/shared/CallStack.cpp | Print and resolve widened call stack addresses. |
| engine/shared/library/sharedDebug/src/linux/DebugHelp.h | Widen call stack API to uint64 and document pointer-width requirement. |
| engine/shared/library/sharedDebug/src/linux/DebugHelp.cpp | Fix Linux backtrace() usage by capturing into void*[] then widening. |
| engine/server/library/serverScript/src/shared/JavaLibrary.cpp | Restrict frame-offset crash-address probe to __i386__ and zero-init buffers. |
| cmake/linux/FindOracle.cmake | Add nnz19 and ${ORACLE_HOME} to Oracle client library discovery. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Would love any feedback on the nits so I can improve style for future PRs - Thanks! |
| // 4-byte-per-entry array and overran the buffer by 2x. Capture into a | ||
| // native pointer array and widen instead, which is correct for both | ||
| // ILP32 and LP64. | ||
| enum { cs_maximumFrameCount = 256 }; |
There was a problem hiding this comment.
nit: this is functioning as a magic number, so could instead be at top of file, even though it is scoped and only relevant to the single function so it's 50/50 (I also don't love use of an untyped enum here since the compiler then has to make an inference and god knows what it may do in the future); alternatively, e.g., at top of file:
namespace
{
constexpr int cs_maximumFrameCount = 256;
}| enum { cs_maximumFrameCount = 256 }; | ||
|
|
||
| if (sizeOfCallStack <= 0) | ||
| return; |
There was a problem hiding this comment.
source generally (but admittedly inconsistently) uses allman style bracing and doesn't tend to use indentation block syntax/off-side rule, so code like this would properly be (but again doesn't technically matter and would surely start a not so fun debate)
if (sizeOfCallStack <= 0)
{
return;
}
Four LP64 defects found while bringing up a 64-bit server from
64-bit-types, plus Oracle 19 client discovery. Each is present on64-bit-typestoday. All five compile and none change 32-bit behaviour.1. Packed maps desynchronise their own stream
AutoDeltaPackedMap<NetworkId,int>,<int,NetworkId>and<int,Unicode::String>write the command count asint32but write the baseline count, and read both counts, assize_t.Under LP64 the reader consumes 4 bytes more than the writer produced. Little-endian makes this worse than a clean failure: the first
getreads[count:4][zero:4]and yields the correct count, so the loop starts normally; the secondgetthen swallows the remaining baseline bytes plus the first 4 bytes of command #1, and every subsequent key and value is misaligned. These run throughSwgSnapshot, so the symptom is silent object-persistence corruption rather than a crash.Fixed by using
int32_tfor both counts, matching the genericinternal_pack/internal_unpackpath and the already-convertedPlayerQuestDataspecialization. This also keeps the serialized form byte-identical to a 32-bit server, so data written by either architecture stays readable by the other.2.
getCallStack()overruns every buffer by 2xDebugHelp::getCallStack()passes the caller'suint32buffer straight tobacktrace(), which writesvoid*entries:backtracewritesFatal.cppuint32[68]= 272 BCallStackuint32[32]= 128 BCallStackCollectoruint32[26]= 104 BFatal.cpp'sformatMessage()backs everyWARNINGandFATAL, so on a 64-bit build any warning that captures a stack corrupts memory. We first saw this as aSIGSEGVinsidecfree()withsi_addr 0x8shortly after an unrelated missing-asset warning — the warning was the corruption and the crash was the aftermath, which makes it very hard to attribute.Fixed by capturing into a native
void*array and widening into auint64buffer, which is correct under both ILP32 and LP64, and widening the storage,lookupAddress()and the printing to match. On win32 the same code truncatedStackWalk'sDWORD64address to 32 bits, so symbols were unusable there; that is fixed too.Also corrects
CallStackCollectorcopying a single element instead of the whole call stack (sizeof(*newCallStack)without the element count), and CRCing only half the buffer.3.
QueryPerformanceCounteris not monotonic on linuxThe linux shim samples
gettimeofday(), i.e.CLOCK_REALTIME, which time synchronisation can slew or step backwards.Clock::update()subtracts successive samples to derive frame time, so a backwards step yields a negative elapsed time andAlterSchedulerreports:Under a hypervisor that re-syncs guest time this fires continuously. Combined with defect 2, every one of those warnings was also smashing 272 bytes of stack.
Fixed by using
clock_gettime(CLOCK_MONOTONIC), keeping microsecond units so the value still matchesQueryPerformanceFrequency(), with agettimeofday()fallback if the call fails. EverygetFrameStartTimeMs()consumer compares values relatively, so nothing depends on the previous epoch-based origin. This one is architecture-independent and applies equally to a 32-bit build.4. SIGSEGV handler dereferences a 32-bit frame offset
JavaLibrary::fatalHandler()reads the faulting address from a hardcoded0x44byte offset into its own frame:That constant describes the 32-bit x86 frame layout. On x86-64 it is both misaligned and meaningless, and dereferencing it from inside a SIGSEGV handler risks a recursive fault that loses the crash report entirely. The existing
@todoalready flagged the offset as unverified.Restricted to
__i386__, relying on the portable__builtin_return_address()probe already present immediately below it. Also zero-initialises thelib/filebuffers, which thestrstr()checks read whenever a lookup fails.5. Oracle 19 Instant Client discovery
FindOracle.cmakeknowsnnz18andnnz21but notnnz19, and does not considerORACLE_HOMEwhen searching. Addslibnnz19/nnz19and${ORACLE_HOME}to the search paths. Purely additive.Verification
Built from this branch against
SWG-Source/swg-main:64-bit-typeswith its stock Ant/CMake path andbits = 64:20 binaries produced;
LoginServer,TaskManager,CentralServer,ConnectionServer,SwgDatabaseServerandSwgGameServerall verifiedELF 64-bit LSB shared object, x86-64.Separately, these same fixes were exercised at runtime on a downstream integration branch: Oracle schema creation and template load completed with 0
ORA-errors and 60,077 object templates, the full server process set reached a healthy state, and client login and logout worked. That run carried additional downstream deltas, so it is supporting evidence for these fixes rather than a clean-room test of this branch alone.Not exhaustively swept: format specifiers server-wide,
time_tin persisted structures, alignment and packing assumptions, and third-party ABI.