Regression in detour after Dhooks safetyhook #2212

Alienmario · 2024-10-26T16:28:00Z

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 6b6dbc6.

The issue only concerns Linux, game is 32bit.
Does not occur when using dhooks from SM 7163.
Occurs when hooking function UTIL_GetLocalPlayer as a pre-hook.
Returning MRES_Ignored makes 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

The text was updated successfully, but these errors were encountered:

Kenzzer · 2024-10-26T22:38:37Z

Could you send the register values from your crash dump during UTIL_GetLocalPlayer() call.

Alienmario · 2024-10-26T22:41:58Z

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:

FortyTwoFortyTwo · 2024-10-26T22:54:57Z

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]...........

Kenzzer · 2024-10-26T23:30:45Z

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:

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.

Mikusch · 2024-12-31T08:12:04Z

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.

Kenzzer · 2025-01-09T10:36:12Z

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.

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.

Kenzzer · 2025-01-09T10:46:26Z

@psychonic Perhaps a fair compromise would be to reintroduce libudis86 for CDetour in 32bits build only ?

jensewe · 2025-01-14T09:54:06Z

sourcemod/extensions/dhooks/DynamicHooks/hook.cpp

Lines 624 to 625 in cb0f8f0

    
           masm.movl(eax, Operand(ExternalAddress(&m_pTrampoline))); 
        
           masm.jmp(eax);

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.

Kenzzer · 2025-01-14T12:36:38Z

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.

jensewe · 2025-01-14T18:52:10Z

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.

Kenzzer · 2025-01-15T11:26:28Z

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.

Kenzzer self-assigned this Oct 26, 2024

peace-maker mentioned this issue Jan 9, 2025

Extension Crashes On 'SourceTV_OnStartRecording' Forward. peace-maker/sourcetvmanager#42

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Regression in detour after Dhooks safetyhook #2212

Regression in detour after Dhooks safetyhook #2212

Alienmario commented Oct 26, 2024 •

edited

Loading

Kenzzer commented Oct 26, 2024

Alienmario commented Oct 26, 2024 •

edited

Loading

FortyTwoFortyTwo commented Oct 26, 2024

Kenzzer commented Oct 26, 2024 •

edited

Loading

Mikusch commented Dec 31, 2024 •

edited

Loading

Kenzzer commented Jan 9, 2025 •

edited

Loading

Kenzzer commented Jan 9, 2025

jensewe commented Jan 14, 2025 •

edited

Loading

Kenzzer commented Jan 14, 2025 •

edited

Loading

jensewe commented Jan 14, 2025

Kenzzer commented Jan 15, 2025 •

edited

Loading

Regression in detour after Dhooks safetyhook #2212

Regression in detour after Dhooks safetyhook #2212

Comments

Alienmario commented Oct 26, 2024 • edited Loading

Environment

Description

Problematic Code (or Steps to Reproduce)

Logs

Kenzzer commented Oct 26, 2024

Alienmario commented Oct 26, 2024 • edited Loading

FortyTwoFortyTwo commented Oct 26, 2024

Kenzzer commented Oct 26, 2024 • edited Loading

Mikusch commented Dec 31, 2024 • edited Loading

Kenzzer commented Jan 9, 2025 • edited Loading

Kenzzer commented Jan 9, 2025

jensewe commented Jan 14, 2025 • edited Loading

Kenzzer commented Jan 14, 2025 • edited Loading

jensewe commented Jan 14, 2025

Kenzzer commented Jan 15, 2025 • edited Loading

Alienmario commented Oct 26, 2024 •

edited

Loading

Alienmario commented Oct 26, 2024 •

edited

Loading

Kenzzer commented Oct 26, 2024 •

edited

Loading

Mikusch commented Dec 31, 2024 •

edited

Loading

Kenzzer commented Jan 9, 2025 •

edited

Loading

jensewe commented Jan 14, 2025 •

edited

Loading

Kenzzer commented Jan 14, 2025 •

edited

Loading

Kenzzer commented Jan 15, 2025 •

edited

Loading