ValveSoftware / halflife

Half-Life 1 engine based games
Other
3.59k stars 598 forks source link

PM_TraceModel may return uninitialized trace #3283

Open a1batross opened 2 years ago

a1batross commented 2 years ago

Related issue: https://github.com/FWGS/xash3d-fwgs/issues/885

In short, on some maps compiled with ZHLT it's possible that TraceModel may return uninitialized trace values, which then used in ladder code: https://github.com/ValveSoftware/halflife/blob/master/pm_shared/pm_shared.c#L2092

Following the execution, these uninitialized values may get interpreted as NaN, poisoning other float variables down to player's velocity: https://github.com/ValveSoftware/halflife/blob/master/pm_shared/pm_shared.c#L2173

Further execution gets to another trace function, this time PM_PlayerTrace: https://github.com/ValveSoftware/halflife/blob/master/pm_shared/pm_shared.c#L830 which then causes an infinite loop in engine's PM_HullPointContents.

The fix is simple: PM_TraceModel should initialize trace_t early, like this:

static inline void PM_InitTrace( trace_t *trace, const vec3_t end )
{
    memset( trace, 0, sizeof( *trace ));
    VectorCopy( end, trace->endpos );
    trace->allsolid = true;
    trace->fraction = 1.0f;
}

static float GAME_EXPORT pfnTraceModel( physent_t *pe, float *start, float *end, trace_t *trace )
{
    // variable declarations...
    PM_InitTrace( trace, end );
    ...
}

This way result data will be interpreted as "didn't hit anything, stuck in solid", thus fraction = 1.0, allsolid = true and endpos as end point. On game code side, this can be fixed similarly, just initializing trace structure before any call to TraceModel.

Relevant fix in xash3d-fwgs source: https://github.com/FWGS/xash3d-fwgs/commit/c076f4ff8e7bc3bdf72a9a5ec290024b14578417 and https://github.com/FWGS/xash3d-fwgs/commit/85895c5311764667300557b53c66407208f5e10a

Thanks to @FreeSlave for finding a way to reproduce this bug and Unkle Mike for correct trace struct initialization.

tschumann commented 2 years ago

Is there a way to fix this from game code?

On Monday, 27 June 2022, a1batross @.***> wrote:

Related issue: FWGS/xash3d-fwgs#885 https://github.com/FWGS/xash3d-fwgs/issues/885

In short, on some maps compiled with ZHLT it's possible that TraceModel may return uninitialized trace values, which then used in ladder code: https://github.com/ValveSoftware/halflife/blob/ master/pm_shared/pm_shared.c#L2092

Following the execution, these uninitialized values may get interpreted as NaN, poisoning other float variables down to player's velocity: https://github.com/ValveSoftware/halflife/blob/ master/pm_shared/pm_shared.c#L2173

Further execution gets to another trace function, this time PM_PlayerTrace: https://github.com/ValveSoftware/halflife/blob/ master/pm_shared/pm_shared.c#L830 which then causes an infinite loop in engine's PM_HullPointContents.

The fix is simple: PM_TraceModel should initialize trace_t early, like this:

static inline void PM_InitTrace( trace_t trace, const vec3_t end ) { memset( trace, 0, sizeof( trace )); VectorCopy( end, trace->endpos ); trace->allsolid = true; trace->fraction = 1.0f; }

static float GAME_EXPORT pfnTraceModel( physent_t pe, float start, float end, trace_t trace ) { // variable declarations... PM_InitTrace( trace, end ); ... }

This way result data will be interpreted as "didn't hit anything, stuck in solid", thus fraction = 1.0, allsolid = true and endpos as end point. On game code side, this can be fixed similarly, just initializing trace structure before any call to TraceModel.

Relevant fix in xash3d-fwgs source: @. https://github.com/FWGS/xash3d-fwgs/commit/c076f4ff8e7bc3bdf72a9a5ec290024b14578417 and @. https://github.com/FWGS/xash3d-fwgs/commit/85895c5311764667300557b53c66407208f5e10a

Thanks to @FreeSlave https://github.com/FreeSlave for finding a way to reproduce this bug and Unkle Mike for correct trace struct initialization.

— Reply to this email directly, view it on GitHub https://github.com/ValveSoftware/halflife/issues/3283, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA34IYTZGLDYDILWJS7TUFLVRCLFVANCNFSM5Z4JKS4A . You are receiving this because you are subscribed to this thread.Message ID: @.***>

a1batross commented 2 years ago

@tschumann yes!

You may copy the PM_InitTrace function from the example above and call it before PM_TraceModel, assuming that it may not return an initialized trace.

tschumann commented 1 year ago

Okay thanks - is it only a problem for maps compiled with ZHLT?

On Wed, 29 Jun 2022 at 02:11, a1batross @.***> wrote:

@tschumann https://github.com/tschumann yes!

You may copy the PM_InitTrace function from the example above and call it before PM_TraceModel, assuming that it may not return an initialized trace.

— Reply to this email directly, view it on GitHub https://github.com/ValveSoftware/halflife/issues/3283#issuecomment-1168922155, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA34IYROL5SHQDTKELK47HTVRMP2FANCNFSM5Z4JKS4A . You are receiving this because you were mentioned.Message ID: @.***>

a1batross commented 1 year ago

@tschumann I don't remember now but at least this bug shouldn't happen with original compilers.