From 88451023fba8a26bd26f5812a24652508e9c2797 Mon Sep 17 00:00:00 2001
From: Peace-Maker <peace-maker@wcfan.de>
Date: Sat, 3 Aug 2019 18:26:53 +0200
Subject: [PATCH] Fix changing of charptr and vectorptr returns and parameters

The new buffer/vector was freed before the old function was called with the new parameters
or the return value could be used by the called. This caused undefined behavior which seemed
to be fine before, where free didn't change the user-payload. free does change the user data now,
causing the changed values to be garbage.

Wait until the next frame before deleting the newly allocated buffers/vectors, so the original code had a chance
to use the live pointers. AddFrameAction might be a bad choice if our hook happens before the game ticks, but
we can tackle that problem when it happens ;)
---
 natives.cpp | 29 +++++++++++++++++++----------
 vhook.cpp   | 29 -----------------------------
 2 files changed, 19 insertions(+), 39 deletions(-)

diff --git a/natives.cpp b/natives.cpp
index a86c099..373caa1 100644
--- a/natives.cpp
+++ b/natives.cpp
@@ -753,6 +753,12 @@ cell_t Native_GetParamVector(IPluginContext *pContext, const cell_t *params)
 
 	return pContext->ThrowNativeError("Invalid param type to get. Param is not a vector.");
 }
+
+static void FreeChangedVector(void *pData)
+{
+	delete (SDKVector *)pData;
+}
+
 // native DHookSetParamVector(Handle:hParams, num, Float:vec[3])
 cell_t Native_SetParamVector(IPluginContext *pContext, const cell_t *params)
 {
@@ -777,14 +783,13 @@ cell_t Native_SetParamVector(IPluginContext *pContext, const cell_t *params)
 	{
 		case HookParamType_VectorPtr:
 		{
-			if(paramStruct->isChanged[index])
-				delete *(SDKVector **)addr;
-
 			cell_t *buffer;
 			pContext->LocalToPhysAddr(params[3], &buffer);
 
 			*(SDKVector **)addr = new SDKVector(sp_ctof(buffer[0]), sp_ctof(buffer[1]), sp_ctof(buffer[2]));
 			paramStruct->isChanged[index] = true;
+			// Free it later (cheaply) after the function returned.
+			smutils->AddFrameAction(FreeChangedVector, *(SDKVector **)addr);
 			return 1;
 		}
 	}
@@ -850,6 +855,11 @@ cell_t Native_GetReturnString(IPluginContext *pContext, const cell_t *params)
 	}
 }
 
+static void FreeChangedCharPtr(void *pData)
+{
+	delete[](char *)pData;
+}
+
 //native DHookSetReturnString(Handle:hReturn, String:value[])
 cell_t Native_SetReturnString(IPluginContext *pContext, const cell_t *params)
 {
@@ -867,13 +877,11 @@ cell_t Native_SetReturnString(IPluginContext *pContext, const cell_t *params)
 	{
 		case ReturnType_CharPtr:
 		{
-			if(returnStruct->isChanged == true)
-			{
-				delete (char *)returnStruct->newResult;
-			}
 			returnStruct->newResult = new char[strlen(value) + 1];
 			strcpy((char *)returnStruct->newResult, value);
 			returnStruct->isChanged = true;
+			// Free it later (cheaply) after the function returned.
+			smutils->AddFrameAction(FreeChangedCharPtr, returnStruct->newResult);
 			return 1;
 		}
 		default:
@@ -905,12 +913,11 @@ cell_t Native_SetParamString(IPluginContext *pContext, const cell_t *params)
 
 	if(paramStruct->dg->params.at(index).type == HookParamType_CharPtr)
 	{
-		if(paramStruct->isChanged[index])
-			delete *(char **)addr;
-
 		*(char **)addr = new char[strlen(value)+1];
 		strcpy(*(char **)addr, value);
 		paramStruct->isChanged[index] = true;
+		// Free it later (cheaply) after the function returned.
+		smutils->AddFrameAction(FreeChangedCharPtr, *(char **)addr);
 	}
 	return 1;
 }
@@ -1299,6 +1306,8 @@ cell_t Native_SetReturnVector(IPluginContext *pContext, const cell_t *params)
 	{
 		returnStruct->newResult = new SDKVector(sp_ctof(buffer[0]), sp_ctof(buffer[1]), sp_ctof(buffer[2]));
 		returnStruct->isChanged = true;
+		// Free it later (cheaply) after the function returned.
+		smutils->AddFrameAction(FreeChangedVector, returnStruct->newResult);
 		
 		return 1;
 	}
diff --git a/vhook.cpp b/vhook.cpp
index 4d83bbe..dd7b7ef 100644
--- a/vhook.cpp
+++ b/vhook.cpp
@@ -219,18 +219,6 @@ HookReturnStruct::~HookReturnStruct()
 		free(this->newResult);
 		free(this->orgResult);
 	}
-	else if (this->isChanged)
-	{
-		if (this->type == ReturnType_CharPtr)
-		{
-			delete[](char *)this->newResult;
-		}
-		else if (this->type == ReturnType_VectorPtr)
-		{
-			delete (SDKVector *)this->newResult;
-		}
-	}
-
 }
 
 HookParamsStruct::~HookParamsStruct()
@@ -245,23 +233,6 @@ HookParamsStruct::~HookParamsStruct()
 	}
 	if (this->newParams != NULL)
 	{
-		for (int i = dg->params.size() - 1; i >= 0; i--)
-		{
-			size_t offset = GetParamOffset(this, i);
-			void *addr = (void **)((intptr_t)this->newParams + offset);
-
-			if (*(void **)addr == NULL)
-				continue;
-
-			if (dg->params.at(i).type == HookParamType_VectorPtr)
-			{
-				delete *(SDKVector **)addr;
-			}
-			else if (dg->params.at(i).type == HookParamType_CharPtr)
-			{
-				delete *(char **)addr;
-			}
-		}
 		free(this->newParams);
 	}
 }