alliedmodders / sourcemod

SourceMod - Source Engine Scripting and Administration
http://www.sourcemod.net/
973 stars 422 forks source link

[DHooks] Superceding pre-detour causes unsaved return values #1959

Closed jensewe closed 1 year ago

jensewe commented 1 year ago

Help us help you

Environment

Description

DHooks doesn't save the modified return values by pre-detours from plugins if they end up with MRES_Suprecede, while it fixes itself if returning MRES_Override. This may not be problematic except detouring functions returning float, in which case NaNs will be loaded to ST0 from empty registers.

I've tried fixing and recompiling the DHooks ext under SM 1.12.0.6986, which worked out and is available here.

Steps to Reproduce

#include <sourcemod>
#include <sdktools>
#include <dhooks>

Handle sdkcall;

public void OnPluginStart()
{
    GameData gd = new GameData("test-dhooks-float-return");

    DynamicDetour dtr = DynamicDetour.FromConf(gd, "func-with-float-return");
    dtr.Enable(Hook_Pre, DTR_Pre);
    dtr.Enable(Hook_Post, DTR_Post);

    sdkcall = gimme_a_sdkcall(gd, "func-with-float-return");

    delete dtr;
    delete gd;

    RegServerCmd("sm_test_dhooks", sm_test_dhooks);
}

Action sm_test_dhooks(int args)
{
    PrintToServer("sdkcall res: %f", SDKCall(sdkcall));
    return Plugin_Handled;
}

MRESReturn DTR_Pre(DHookReturn hReturn)
{
    PrintToServer("PRE> %f", hReturn.Value);

    hReturn.Value = 1234.5;

    // BUG: NaN will be written to the ST0
    return MRES_Supercede;

    // It fixes itself if return MRES_Override instead.
    // return MRES_Override;
}

MRESReturn DTR_Post(DHookReturn hReturn)
{
    PrintToServer("POST> %f", hReturn.Value);
    return MRES_Ignored;
}

Problematic Code

https://github.com/alliedmodders/sourcemod/blob/5539484f9294e539cb7d570872a23f617a7b8b46/extensions/dhooks/DynamicHooks/hook.cpp#L163 https://github.com/alliedmodders/sourcemod/blob/5539484f9294e539cb7d570872a23f617a7b8b46/extensions/dhooks/DynamicHooks/hook.cpp#L194

KyleSanderson commented 1 year ago

Do you think this would be better as a pull request?