This impulse cheat crashes the game with bunnymodxt

[Spicytin]

The impulse 76 crashes the game when used and if the player attempts to restart the game by using "restart" command or equipping Hazard suit.

Half-Life classic (https://gamebanana.com/mods/36011) seems to crash when entering a game probably because of hud color conflicts with your mod.

[YaLTeR]

If two bugs are unrelated, please create 2 separate issues.

[chinese-soup]

Ad impulse & restart: I think this is caused by this commit: https://github.com/YaLTeR/BunnymodXT/commit/d3343d219279b1f173824b18e15b84031dc7a3db

Specifically this hooked function: https://github.com/YaLTeR/BunnymodXT/blob/cad2bd4ef1e34cf2b80af69e0f29de9369188558/BunnymodXT/modules/ServerDLL.cpp#L1923-L1935

which I assume sometimes never sets pEngfuncs->pfnCVarGetFloat back to ORIG_CVarGetFloat, this happens because "impulse 0" is actually sent on every frame (CBasePlayer::ItemPostFrame), unless the player types an "impulse" command and sets its pev->impulse to a different value:

void CBasePlayer::ImpulseCommands( )
{
    TraceResult tr;// UNDONE: kill me! This is temporary for PreAlpha CDs

    // Handle use events
    PlayerUse();

    int iImpulse = (int)pev->impulse;
    switch (iImpulse)
    {
    case 99:
        {
 /* omitted for brevity */
        break;
        }
    case 100:
    /* omitted for brevity */
        break;

    case    201:// paint decal
         /* omitted for brevity */
        break;

    default:
        // check all of the cheat impulse commands now
        CheatImpulseCommands( iImpulse );
        break;
    }

    pev->impulse = 0;
}

Therefore CheatImpulseCommands is called on every frame and a race condition can happen upon restart / map change / save & load, I could randomly reproduce this using impulse 76 and restarting (restart) / saving / loading / and using impulse 76 (impulse doesn't seem to be that related to it, since "impulse 0" i sent every frame anyway?) several times.

Why I think it's a race condition and the pfnCvarGetFloat is never set back to the original function can be seen here in the backtrace, it just gets stuck in a loop since it's calling itself again and again:

static float (*ORIG_CVarGetFloat) (const char* szVarName);
float fast_cvar_get_float(const char* name)
{
    if (!strcmp(name, "sv_cheats"))
        return CVars::sv_cheats.GetFloat();

    return ORIG_CVarGetFloat(name); // <-- ORIG_CVarGetFloat points to fast_cvar_get_float, here.
}

Traceback after like 4th-5th random load from save:

#174480 0xe91daf7a in fast_cvar_get_float (name=0x782a9d23 "skill") at /home/unko/repos/BunnymodXT/BunnymodXT/modules/ServerDLL.cpp:1761
#174481 0xe91daf7a in fast_cvar_get_float (name=0x782a9d23 "skill") at /home/unko/repos/BunnymodXT/BunnymodXT/modules/ServerDLL.cpp:1761
#174482 0xe91daf7a in fast_cvar_get_float (name=0x782a9d23 "skill") at /home/unko/repos/BunnymodXT/BunnymodXT/modules/ServerDLL.cpp:1761
#174483 0xe91daf7a in fast_cvar_get_float (name=0x782a9d23 "skill") at /home/unko/repos/BunnymodXT/BunnymodXT/modules/ServerDLL.cpp:1761
#174484 0xe91daf7a in fast_cvar_get_float (name=0x782a9d23 "skill") at /home/unko/repos/BunnymodXT/BunnymodXT/modules/ServerDLL.cpp:1761
#174485 0xe91daf7a in fast_cvar_get_float (name=0x782a9d23 "skill") at /home/unko/repos/BunnymodXT/BunnymodXT/modules/ServerDLL.cpp:1761
#174486 0xe91daf7a in fast_cvar_get_float (name=0x782a9d23 "skill") at /home/unko/repos/BunnymodXT/BunnymodXT/modules/ServerDLL.cpp:1761
#174487 0xe91daf7a in fast_cvar_get_float (name=0x782a9d23 "skill") at /home/unko/repos/BunnymodXT/BunnymodXT/modules/ServerDLL.cpp:1761
#174488 0xe91daf7a in fast_cvar_get_float (name=0x782a9d23 "skill") at /home/unko/repos/BunnymodXT/BunnymodXT/modules/ServerDLL.cpp:1761
#174489 0xe91daf7a in fast_cvar_get_float (name=0x782a9d23 "skill") at /home/unko/repos/BunnymodXT/BunnymodXT/modules/ServerDLL.cpp:1761
#174490 0xe91daf7a in fast_cvar_get_float (name=0x782a9d23 "skill") at /home/unko/repos/BunnymodXT/BunnymodXT/modules/ServerDLL.cpp:1761
#174491 0x78209c70 in CGameRules::RefreshSkillData (this=0xac02140) at ../dlls/gamerules.cpp:117
#174492 0x7820a4f7 in InstallGameRules () at ../dlls/gamerules.cpp:320
#174493 0x7828f56b in CWorld::Precache (this=0xa5456b0) at ../dlls/world.cpp:497
#174494 0x781e5c31 in DispatchSpawn (pent=0xba010448) at ../dlls/cbase.cpp:139
#174495 0xe72942c9 in ED_LoadFromFile (data=0xba14ffde "\n{\n\"model\" \"*1\"\n\"unlocked_sentence\" \"0\"\n\"locked_sentence\" \"0\"\n\"unlocked_sound\" \"0\"\n\"locked_sound\" \"10\"\n\"targetname\" \"sl08_door\"\n\"healthvalue\" \"0\"\n\"health\" \"0\"\n\"dmg\" \"1000\"\n\"lip\" \"24\"\n\"wait\" \"-1\"\n\"stop"...) at ../engine/pr_edict.c:475
#174496 0xe72b5816 in SV_LoadEntities () at ../engine/sv_main.c:7731
#174497 0xe7282e35 in Host_Changelevel2_f () at ../engine/host_cmd.c:3467
#174498 Host_Changelevel2_f () at ../engine/host_cmd.c:3379
#174499 0xe9180571 in HwDLL::HOOKED_Host_Changelevel2_f_Func (this=0xe9643de0 <HwDLL::GetInstance()::instance>) at /home/unko/repos/BunnymodXT/BunnymodXT/modules/HwDLL.cpp:4219
#174500 0xe9180355 in HwDLL::HOOKED_Host_Changelevel2_f () at /home/unko/repos/BunnymodXT/BunnymodXT/modules/HwDLL.cpp:4194
#174501 0xe9170bdc in Host_Changelevel2_f () at /home/unko/repos/BunnymodXT/BunnymodXT/modules/HwDLL.cpp:42
#174502 0xe72628de in Cmd_ExecuteStringWithPrivilegeCheck (text=0xffa31000 "changelevel2 c1a3 c1a3toc1a4", bIsPrivileged=<optimized out>, src=<optimized out>) at ../engine/cmd.c:1257
#174503 0xe7262b38 in Cmd_ExecuteStringWithPrivilegeCheck (bIsPrivileged=true, src=src_command, text=0xffa31000 "changelevel2 c1a3 c1a3toc1a4") at ../engine/cmd.c:1216
#174504 Cbuf_ExecuteCommandsFromBuffer (buf=0xe7acbb50 <cmd_text>, bIsPrivileged=true, nCmdsToExecute=-1) at ../engine/cmd.c:245
#174505 0xe7262b71 in Cbuf_ExecuteFromBuffer (bIsPrivileged=true, buf=0xe7acbb50 <cmd_text>) at ../engine/cmd.c:259
#174506 Cbuf_Execute () at ../engine/cmd.c:269
#174507 0xe917ddbc in HwDLL::HOOKED_Cbuf_Execute_Func (this=0xe9643de0 <HwDLL::GetInstance()::instance>) at /home/unko/repos/BunnymodXT/BunnymodXT/modules/HwDLL.cpp:3730
#174508 0xe917d9ad in HwDLL::HOOKED_Cbuf_Execute () at /home/unko/repos/BunnymodXT/BunnymodXT/modules/HwDLL.cpp:3684
#174509 0xe9170b98 in Cbuf_Execute () at /home/unko/repos/BunnymodXT/BunnymodXT/modules/HwDLL.cpp:32
#174510 0xe727a7a3 in _Host_Frame (time=3.61999992e-06) at ../engine/host.c:1410
#174511 0xe727ac52 in Host_Frame (time=3.61999992e-06, iState=1, stateInfo=0xffa3184c) at ../engine/host.c:1548
#174512 0xe72a7b04 in CEngine::Frame (this=0xe74c1aa0 <g_Engine>) at ../engine/sys_engine.cpp:245
#174513 0xe72a558b in RunListenServer (instance=0x0, basedir=0x804b220 <szBaseDir> ".", cmdline=0x9fdae80 "./hl_linux -steam -game valve", postRestartCmdLineArgs=0x804d360 <main::szNewCommandParams> "", launcherFactory=0x8049350 <CreateInterfaceLocal(char const*, int*)>, filesystemFactory=0xe84e2d40 <CreateInterface(char const*, int*)>) at ../engine/sys_dll2.cpp:955
#174514 0x08048d67 in main (argc=4, argv=0xffa31a94) at ../launcher/launcher.cpp:439

Commenting out the pEngfuncs->pfnCVarGetFloat replacements in the HOOKED function seems to fix this issue, but since I don't understand the rationale behind that commit, I don't know how to fix this without getting rid of the functionality that the commit presented.

[YaLTeR]

A race condition would imply that something calls the hook from 2 threads at once, which as far as I understand shouldn't happen as GoldSrc doesn't use threads (apart from audio).

This could be caused by re-entrancy, e.g. if the real CheatImpulseCommands calls something that ends up calling CheatImpulseCommands again. When writing the hook I did not take this into account, so if it's actually possible there needs to be an additional check (if cvar get float is already replaced then return CheatImpulseCommands(...) without doing anything extra).

The rationale is that something checks sv_cheats every frame via CVarGetFloat which walks all cvars until it finds the one called sv_cheats, which is a significant slow-down with _bxt_norefresh 1, so by short-circuiting the sv_cheats check I make it very fast.

[Spicytin]

Same issue when loading an another map