[Bug]: Windows x64 DHooks crash on DynamicDetour.Enable()

[rtldg]

Prerequisites

• I have checked that my issue doesn't exist yet in the issue tracker

Operating System and Version

Windows 10

Game / AppID and Version

CS:S (240)

SourceMod Version

1.12.0.1287

Metamod:Source Version

master branch

Version Verification

• I have updated SourceMod to the latest version and the issue persists
• I have updated SourceMod to the latest snapshot and the issue persists
• I have updated Metamod:Source to the latest snapshot and the issue persists

Updated SourceMod Version

No response

Updated Metamod:Source Version

No response

Description

If you construct a thiscall DynamicDetour and don't add any parameters then ConstructCallingConvention() will prepend the this arg/register to vecArgTypes and UpdateRegisterArgumentSizes() will try to index through (an empty) setup->params for the size of vecArgTypes.

sourcemod/extensions/dhooks/dynhooks_sourcepawn.cpp

Lines 236 to 267 in b71d3c0

	#if defined( DHOOKS_DYNAMIC_DETOUR )
	ICallingConvention *ConstructCallingConvention(HookSetup *setup)
	{
	// Convert function parameter types into DynamicHooks structures.
	std::vector<DataTypeSized_t> vecArgTypes;
	for (size_t i = 0; i < setup->params.size(); i++)
	{
	ParamInfo &info = setup->params[i];
	DataTypeSized_t type;
	type.type = DynamicHooks_ConvertParamTypeFrom(info.type);
	type.size = info.size;
	type.custom_register = info.custom_register;
	vecArgTypes.push_back(type);
	}
	DataTypeSized_t returnType;
	returnType.type = DynamicHooks_ConvertReturnTypeFrom(setup->returnType);
	returnType.size = 0;
	// TODO: Add support for a custom return register.
	returnType.custom_register = None;
	#ifdef DYNAMICHOOKS_x86_64
	#ifdef WIN32
	if (setup->callConv == CallConv_THISCALL) {
	DataTypeSized_t type;
	type.type = DATA_TYPE_POINTER;
	type.size = GetDataTypeSize(type, sizeof(void*));
	type.custom_register = RCX;
	vecArgTypes.insert(vecArgTypes.begin(), type);
	}
	#endif
	#endif

sourcemod/extensions/dhooks/dynhooks_sourcepawn.cpp

Lines 303 to 328 in b71d3c0

	bool UpdateRegisterArgumentSizes(CHook* pDetour, HookSetup *setup)
	{
	// The registers the arguments are passed in might not be the same size as the actual parameter type.
	// Update the type info to the size of the register that's now holding that argument,
	// so we can copy the whole value.
	ICallingConvention* callingConvention = pDetour->m_pCallingConvention;
	std::vector<DataTypeSized_t> &argTypes = callingConvention->m_vecArgTypes;
	int numArgs = argTypes.size();
	for (int i = 0; i < numArgs; i++)
	{
	// Ignore regular arguments on the stack.
	if (argTypes[i].custom_register == None)
	continue;
	CRegister *reg = pDetour->m_pRegisters->GetRegister(argTypes[i].custom_register);
	// That register can't be handled yet.
	if (!reg)
	return false;
	argTypes[i].size = reg->m_iSize;
	setup->params[i].size = reg->m_iSize;
	}
	return true;
	}

Steps to Reproduce

#include <dhooks>

public void OnPluginStart()
{
	GameData gamedata = new GameData("maintainbotquota");
	DynamicDetour hMaintainBotQuota = DHookCreateDetour(Address_Null, CallConv_THISCALL, ReturnType_Void, ThisPointer_Ignore);
	DHookSetFromConf(hMaintainBotQuota, gamedata, SDKConf_Signature, "BotManager::MaintainBotQuota");
	hMaintainBotQuota.Enable(Hook_Pre, Detour_MaintainBotQuota);
}

public MRESReturn Detour_MaintainBotQuota()
{
	return MRES_Supercede;
}

"Games"
{
  "cstrike"
  {
    "Signatures"
    {
      "BotManager::MaintainBotQuota"
      {
        "windows"   "\x55\x8B\xEC\x83\xEC\x14\xFF\x15"
        "windows64" "\x48\x83\xEC\x78\xFF\x15"
      }
    }
  }
}

[rtldg]

I don't really like this but it seems to work:

diff --git a/extensions/dhooks/dynhooks_sourcepawn.cpp b/extensions/dhooks/dynhooks_sourcepawn.cpp
index f6367869..cdb001af 100644
--- a/extensions/dhooks/dynhooks_sourcepawn.cpp
+++ b/extensions/dhooks/dynhooks_sourcepawn.cpp
@@ -308,19 +306,27 @@ bool UpdateRegisterArgumentSizes(CHook* pDetour, HookSetup *setup)
 	ICallingConvention* callingConvention = pDetour->m_pCallingConvention;
 	std::vector<DataTypeSized_t> &argTypes = callingConvention->m_vecArgTypes;
 	int numArgs = argTypes.size();
+	int argTypesOffset = 0;
+
+#ifdef DYNAMICHOOKS_x86_64
+	if (setup->callConv == CallConv_THISCALL) {
+		argTypesOffset = 1;
+		--numArgs;
+	}
+#endif
 
 	for (int i = 0; i < numArgs; i++)
 	{
 		// Ignore regular arguments on the stack.
-		if (argTypes[i].custom_register == None)
+		if (argTypes[argTypesOffset+i].custom_register == None)
 			continue;
 
-		CRegister *reg = pDetour->m_pRegisters->GetRegister(argTypes[i].custom_register);
+		CRegister *reg = pDetour->m_pRegisters->GetRegister(argTypes[argTypesOffset+i].custom_register);
 		// That register can't be handled yet.
 		if (!reg)
 			return false;
 
-		argTypes[i].size = reg->m_iSize;
+		argTypes[argTypesOffset+i].size = reg->m_iSize;
 		setup->params[i].size = reg->m_iSize;
 	}

@Kenzzer does this seem like a good idea 👀

[Kenzzer]

There's a similar hack for other functions in dhooks, so yes this would be fine. I don't like it either, but reworking dhooks would take more effort than this silly fix, so this is great.