alliedmodders
Regression in detour after Dhooks safetyhook
Environment
- Operating System version: Ubuntu 20.04.6 LTS
- Game/AppID (with version if applicable): Black Mesa 346680
- Current SourceMod version: 1.12.7164
Description
One of 16 detours we use with dhooks has started crashing after commit https://github.com/alliedmodders/sourcemod/commit/6b6dbc6c06971a551111520208d14bb.
- The issue only concerns Linux, game is 32bit.
- Does not occur when using dhooks from SM 7163.
- Occurs when hooking function
UTIL_GetLocalPlayeras a pre-hook. - Returning
MRES_Ignoredmakes the original function crash any time it is called by game. - Superceding with
DHookSetReturn(hReturn, -1); return MRES_Supercede;instead does not crash.
Problematic Code (or Steps to Reproduce)
lp_test.games.txt
"Games"
{
"bms"
{
"Functions"
{
"UTIL_GetLocalPlayer"
{
"signature" "UTIL_GetLocalPlayer"
"callconv" "cdecl"
"return" "cbaseentity"
}
}
"Signatures"
{
"UTIL_GetLocalPlayer" // CBasePlayer UTIL_GetLocalPlayer(void)
{
"windows" "\xA1\x2A\x2A\x2A\x2A\x8B\x40\x14\x83\xF8\x01\x7E\x2A\xA1\x2A\x2A\x2A\x2A"
"linux" "@_Z19UTIL_GetLocalPlayerv"
}
}
}
}
lp_test.sp
#include <sourcemod>
#include <dhooks>
#pragma newdecls required
#pragma semicolon 1
#define GAMEDATA_NAME "lp_test.games"
DynamicDetour hkUTIL_GetLocalPlayer;
public void OnPluginStart()
{
GameData pGameConfig = LoadGameConfigFile(GAMEDATA_NAME);
if (pGameConfig == null)
SetFailState("Couldn't load game config: \"%s\"", GAMEDATA_NAME);
hkUTIL_GetLocalPlayer = DynamicDetour.FromConf(pGameConfig, "UTIL_GetLocalPlayer");
if (hkUTIL_GetLocalPlayer == null)
SetFailState("Couldn't create hook");
if (!hkUTIL_GetLocalPlayer.Enable(Hook_Pre, Hook_UTIL_GetLocalPlayer))
SetFailState("Couldn't enable pre detour hook");
pGameConfig.Close();
}
public MRESReturn Hook_UTIL_GetLocalPlayer(DHookReturn hReturn)
{
PrintToServer("Hook_UTIL_GetLocalPlayer");
return MRES_Ignored;
}
Logs
https://crash.limetech.org/cvvrva7bbtoj
0 server_srv.so!UTIL_GetLocalPlayer() + 0x18
1 0xdb02f754
2 server_srv.so!CBaseEntity::AcceptInput(char const*, CBaseEntity*, CBaseEntity*, variant_t, int) + 0x5ae
3 server_srv.so!CEventQueue::ServiceEvents() + 0x261
4 server_srv.so!ServiceEventQueue() + 0x37
5 server_srv.so!CServerGameDLL::GameFrame(bool) + 0x172
6 sourcemod.2.bms.so!__SourceHook_FHCls_IServerGameDLLGameFramefalse::Func(bool) [sourcemod.cpp:54 + 0x11]
7 engine_srv.so!CServerPlugin::GameFrame(bool) + 0x77
8 engine_srv.so!SV_Think(bool) + 0xcc
9 engine_srv.so!SV_Frame(bool) + 0xfe
10 engine_srv.so!_Host_RunFrame_Server(bool) + 0x71
11 engine_srv.so!_Host_RunFrame(float) + 0x2d1
12 engine_srv.so!CHostState::State_Run(float) + 0x11c
13 engine_srv.so!CHostState::FrameUpdate(float) + 0x186
14 engine_srv.so!HostState_Frame(float) + 0x2b
15 engine_srv.so!CEngine::Frame() + 0x552
16 engine_srv.so!CDedicatedServerAPI::RunFrame() + 0x33
17 dedicated_srv.so!RunServer() + 0x53
18 dedicated_srv.so!CDedicatedExports::RunServer() + 0x17
19 engine_srv.so!CModAppSystemGroup::Main() + 0xbe
20 engine_srv.so!CAppSystemGroup::Run() + 0x58
21 engine_srv.so!CDedicatedServerAPI::ModInit(ModInfo_t&) + 0x247
22 dedicated_srv.so!CDedicatedAppSystemGroup::Main() + 0xa5
23 dedicated_srv.so!CAppSystemGroup::Run() + 0x58
24 dedicated_srv.so!CSteamApplication::Main() + 0x37
25 dedicated_srv.so!CAppSystemGroup::Run() + 0x58
26 dedicated_srv.so!main + 0x1f8
27 dedicated_srv.so!DedicatedMain + 0x24
28 srcds_linux!main + 0x2b8
29 libc-2.31.so!__libc_start_main + 0xf5
30 srcds_linux + 0xbd5
31 srcds_linux + 0x780
32 srcds_linux + 0xcb0
33 srcds_linux + 0xd20
Could you send the register values from your crash dump during UTIL_GetLocalPlayer() call.
Could you send the register values from your crash dump during
UTIL_GetLocalPlayer()call.
These?
SIGSEGV /SEGV_MAPERR accessing 0xb6ffb445
Thread 0 (crashed):
0: server_srv.so!UTIL_GetLocalPlayer() + 0x18
eip: 0xf1184558 esp: 0xffd99310 ebp: 0xffd99328 ebx: 0xdfa16ab4
esi: 0xffd99340 edi: 0x00000038 eax: 0xb6ffb445 ecx: 0x0aa2bce0
edx: 0x00000000 efl: 0x00210282
f1184547 90 nop
f1184548 90 nop
f1184549 81 c3 ab 1a 40 00 add ebx, 0x401aab
f118454f 83 ec 14 sub esp, 0x14
f1184552 8b 83 50 f6 ff ff mov eax, [ebx-0x9b0]
> f1184558 8b 00 mov eax, [eax]
f118455a 83 78 14 01 cmp dword [eax+0x14], 0x1
f118455e 7f 18 jg 0xf1184578
f1184560 c7 04 24 01 00 00 00 mov dword [esp], 0x1
f1184567 e8 74 fd ff ff call 0xf11842e0
f118456c 83 c4 14 add esp, 0x14
IDA view for reference:
Similar to this, also had a crash from PassServerEntityFilter detour, which requires setting specific regression.
Gamedata:
"PassServerEntityFilter"
{
"library" "server"
"linux" "@_Z22PassServerEntityFilterPK13IHandleEntityS1_.part.0"
"windows" "\x55\x8B\xEC\x56\x8B\x75\x0C\x57\x85\xF6\x74\x2A\x8B\x7D\x08"
}
"PassServerEntityFilter"
{
// bool PassServerEntityFilter( const IHandleEntity *pTouch, const IHandleEntity *pPass )
"signature" "PassServerEntityFilter"
"callconv" "cdecl"
"return" "bool"
"arguments"
{
"touch"
{
"type" "cbaseentity"
"linux"
{
"register" "eax"
}
}
"pass"
{
"type" "cbaseentity"
"linux"
{
"register" "edx"
}
}
}
}
Crash dump from function:
1: server_srv.so!PassServerEntityFilter(IHandleEntity const*, IHandleEntity const*) [clone .part.0] + 0x19
eip: 0xf0ae8f99 esp: 0xfff78870 ebp: 0xfff788a8 ebx: 0xf3b04106
esi: 0x1e765f90 edi: 0xfff79a30
fff78870 a8 41 6d f6 06 41 b0 f3 18 de 86 1e b2 69 31 59 .Am..A.......i1Y
fff78880 00 00 00 00 00 00 dc d9 01 00 00 00 0b 40 00 02 .............@..
fff78890 30 9a f7 ff 70 c6 ff 1f d8 88 f7 ff 70 c6 ff 1f 0...p.......p...
fff788a0 0b 40 00 02 30 9a f7 ff d8 88 f7 ff 04 f9 9b d0 [email protected]...........
SIGSEGV /SEGV_MAPERR accessing 0xb6ffb445 Thread 0 (crashed): 0: server_srv.so!UTIL_GetLocalPlayer() + 0x18 eip: 0xf1184558 esp: 0xffd99310 ebp: 0xffd99328 ebx: 0xdfa16ab4 esi: 0xffd99340 edi: 0x00000038 eax: 0xb6ffb445 ecx: 0x0aa2bce0 edx: 0x00000000 efl: 0x00210282 f1184547 90 nop f1184548 90 nop f1184549 81 c3 ab 1a 40 00 add ebx, 0x401aab f118454f 83 ec 14 sub esp, 0x14 f1184552 8b 83 50 f6 ff ff mov eax, [ebx-0x9b0] > f1184558 8b 00 mov eax, [eax] f118455a 83 78 14 01 cmp dword [eax+0x14], 0x1 f118455e 7f 18 jg 0xf1184578 f1184560 c7 04 24 01 00 00 00 mov dword [esp], 0x1 f1184567 e8 74 fd ff ff call 0xf11842e0 f118456c 83 c4 14 add esp, 0x14IDA view for reference:
Seeing the call to __x86_get_pc_thunk_bx is rather concerning as that definitively got moved into the trampoline, as demonstrated by the presence of nop opcodes where it should have been in your crash dump. And I'm not sure if safetyhook handles that correctly. At least ebx value doesn't seem anywhere close to the value it should have 0xF1585FF4. This is fixeable but it will be hard to determine the amount of work required.
This is still an issue as of build 7187. The only detour I can see this happening on is PassServerEntityFilter, which is a clone function and requires specifying registers. Gamedata has been provided in a post above. Accelerator crash log: https://crash.limetech.org/hy3yz2rjprme
This is a bit problematic since 1.12 is considered stable but upgrading to 7164 and above is not possible for some people.
This is still an issue as of build 7187. The only detour I can see this happening on is
PassServerEntityFilter, which is a clone function and requires specifying registers. Gamedata has been provided in a post above. Accelerator crash log: https://crash.limetech.org/hy3yz2rjprmeThis is a bit problematic since 1.12 is considered stable but upgrading to 7164 and above is not possible for some people.
I would prefer if this was opened as a separate issue. The root cause of UTIL_GetLocalPlayer detour crash and PassServerEntityFilter[clone] isn't the same, can't note the presence of __x86_get_pc_thunk_bx
And while UTIL_GetLocalPlayer detour crash, might not be fixeable in the foreseeable future, simply because safetyhook doesn't handle those instructions.
The PassServerEntityFilter[clone] crash might be fixeable. But we can't know that because not enough information has been provided, and instead another potentially unrelated issue was piggybacked.
Or putting this another way, this needs more information. If it really is the same crash and root issue, then please disregard the comment.
@psychonic Perhaps a fair compromise would be to reintroduce libudis86 for CDetour in 32bits build only ?
https://github.com/alliedmodders/sourcemod/blob/cb0f8f0eaa8f1edb46888d7f1d797c0493f9f51f/extensions/dhooks/DynamicHooks/hook.cpp#L624-L625
This change has confused me since the day of the safetyhook commit, and I bet it's responsible for the PassServerEntityFilter[clone] crash mentioned above.
Full context of the changes :
-masm.jmp(ExternalAddress(m_pTrampoline));
+masm.movl(eax, Operand(ExternalAddress(&m_pTrampoline)));
+masm.jmp(eax);
This change has confused me since the day of the safetyhook commit
When dhooks used libudis86, the trampoline value was known and could be written directly into the assembly.
jmp 0xADDROFTRAMPOLINE
As of safetyhook, the bridge needs to be built before the trampoline is constructed (limitation of safetyhook). So instead it's necessary to write the assembly instructions like so
mov eax, [0xPTR_TO_ADDROFTRANPOLINE]
jmp eax
We could also just rewrite that section of the memory. But it's unnecessary extra logic for dhooks that would make the process even more confusing than it is now.
and I bet it's responsible for the
PassServerEntityFilter[clone]crash mentioned above.
Again, most likely a different crash than OP's issue. If the jump was "wrong" we would also crash on regular function detour but we're not. Of course I'm open to the idea any changes that I made to dhooks could be wrong (instead of the issue being in safetyhook), but this needs analysis rather than conjectures.
If the jump was "wrong" we would also crash on regular function detour but we're not.
PassServerEntityFilter[clone] is not a regular function, because as written in the gamedata the first argument is passed on eax, while the mentioned operations happen after registers are restored.
Hmm I don't recall that register being used, but either way you're absolutely right, this is a problem, and I failed to account for that. Then this should be fixeable, dhooks code confusion be dammed. Thanks! I'll have a fix ready soon. Edit: Thinking this again, this is definitively the culprit.
Still out of luck for the other crash __x86_get_pc_thunk_bx.