From 3ad384a0da2cd30b04fd25de1584226d40aeea56 Mon Sep 17 00:00:00 2001 From: Daniel Gibson Date: Thu, 31 Oct 2024 03:01:27 +0100 Subject: [PATCH] Fix script interpreter on 64bit Big Endian platforms idInterpreter::Push() is used only for int and (reinterpreted) float values, not pointers (as far as I can tell), so 32bit values on all relevant platforms. It stored its value as intptr_t at `&localstack[ localstackUsed ]` - on 64bit platforms intptr_t is 64bit. Unfortunately, all code reading from the stack just get got a pointer to `&localstack[ localstackUsed ]` in the type they want to read (like `int*` or `float*`) and read that. On Little Endian that happens to work, on 64bit Big Endian it reads the wrong 4 bytes of the intptr_t, so it doesn't work. fixes #625, #472 --- neo/d3xp/script/Script_Interpreter.h | 21 ++++++++++++++++++++- neo/game/script/Script_Interpreter.h | 21 ++++++++++++++++++++- 2 files changed, 40 insertions(+), 2 deletions(-) diff --git a/neo/d3xp/script/Script_Interpreter.h b/neo/d3xp/script/Script_Interpreter.h index e7ec0f64..7da58457 100644 --- a/neo/d3xp/script/Script_Interpreter.h +++ b/neo/d3xp/script/Script_Interpreter.h @@ -146,7 +146,26 @@ ID_INLINE void idInterpreter::Push( intptr_t value ) { if ( localstackUsed + sizeof( intptr_t ) > LOCALSTACK_SIZE ) { Error( "Push: locals stack overflow\n" ); } - *( intptr_t * )&localstack[ localstackUsed ] = value; + // DG: fix for 64bit Big Endian machines + //*( intptr_t * )&localstack[ localstackUsed ] = value; + int val = value; + assert(value == val && "I really assumed that value would always fit into 32bit"); + // dhewm3 used to store these values on localstack[] as intptr_t, even though + // they always seem to be int32. Unfortunately, all the code reading localstack[] + // gets the pointer to &localstack[ localstackUsed ] and then interprets that + // as int* or float* or whatever. + // So with 64bit machines it's stored as the wrong size (intptr_t is int64, int is int32), + // and on Big Endian the values are just 0 (or UINT_MAX if value < 0) - on Little Endian + // the first 4 bytes are the ones with actual data so the bug was hidden). + // To fix this, now a regular itn is stored at `&localstack[ localstackUsed ]`, + // as expected by the code using localstack[]. + // TODO: once this has been tested more (and the assertion above has never triggered), + // change idInterpreter::Push() to take a regular int argument instead of intptr_t. + int *stackVar = ( int * )&localstack[ localstackUsed ]; + *stackVar = val; + + // even though a 32bit int is put onto the stack, increase it by sizeof(intptr_t) + // so it remains aligned to multiples of the native pointer size localstackUsed += sizeof( intptr_t ); } diff --git a/neo/game/script/Script_Interpreter.h b/neo/game/script/Script_Interpreter.h index e7ec0f64..7da58457 100644 --- a/neo/game/script/Script_Interpreter.h +++ b/neo/game/script/Script_Interpreter.h @@ -146,7 +146,26 @@ ID_INLINE void idInterpreter::Push( intptr_t value ) { if ( localstackUsed + sizeof( intptr_t ) > LOCALSTACK_SIZE ) { Error( "Push: locals stack overflow\n" ); } - *( intptr_t * )&localstack[ localstackUsed ] = value; + // DG: fix for 64bit Big Endian machines + //*( intptr_t * )&localstack[ localstackUsed ] = value; + int val = value; + assert(value == val && "I really assumed that value would always fit into 32bit"); + // dhewm3 used to store these values on localstack[] as intptr_t, even though + // they always seem to be int32. Unfortunately, all the code reading localstack[] + // gets the pointer to &localstack[ localstackUsed ] and then interprets that + // as int* or float* or whatever. + // So with 64bit machines it's stored as the wrong size (intptr_t is int64, int is int32), + // and on Big Endian the values are just 0 (or UINT_MAX if value < 0) - on Little Endian + // the first 4 bytes are the ones with actual data so the bug was hidden). + // To fix this, now a regular itn is stored at `&localstack[ localstackUsed ]`, + // as expected by the code using localstack[]. + // TODO: once this has been tested more (and the assertion above has never triggered), + // change idInterpreter::Push() to take a regular int argument instead of intptr_t. + int *stackVar = ( int * )&localstack[ localstackUsed ]; + *stackVar = val; + + // even though a 32bit int is put onto the stack, increase it by sizeof(intptr_t) + // so it remains aligned to multiples of the native pointer size localstackUsed += sizeof( intptr_t ); }