IPluginFunction implementation is re-entrant across native calls, as heap allocations are delayed until execution

removed ICallable::GetAddressOfPushedParam
removed phys_addr from ICallable::PushArray
fixed a bug where sp_context_t::n_idx was overwritten upon re-entrant calls

--HG--
extra : convert_revision : svn%3A39bc706e-5318-0410-9160-8a85361fbb7c/trunk%40595

core/CMsgListenerWrapper.h

@@ -105,7 +105,7 @@ inline void MsgListenerWrapper::OnUserMessage(int msg_id, bf_write *bf, IRecipie
 
 	m_Hook->PushCell(msg_id);
 	m_Hook->PushCell(0); //:TODO: push handle!
-	m_Hook->PushArray(g_MsgPlayers, size, NULL);
+	m_Hook->PushArray(g_MsgPlayers, size);
 	m_Hook->PushCell(size);
 	m_Hook->PushCell(pFilter->IsReliable());
 	m_Hook->PushCell(pFilter->IsInitMessage());
@@ -119,7 +119,7 @@ inline ResultType MsgListenerWrapper::InterceptUserMessage(int msg_id, bf_write
 
 	m_Intercept->PushCell(msg_id);
 	m_Intercept->PushCell(0); //:TODO: push handle!
-	m_Intercept->PushArray(g_MsgPlayers, size, NULL);
+	m_Intercept->PushArray(g_MsgPlayers, size);
 	m_Intercept->PushCell(size);
 	m_Intercept->PushCell(pFilter->IsReliable());
 	m_Intercept->PushCell(pFilter->IsInitMessage());

core/systems/ForwardSys.cpp

@@ -299,7 +299,7 @@ int CForward::Execute(cell_t *result, IForwardFilter *filter)
 				} else if (type == Param_Float || type == Param_Cell) {
 					func->PushCellByRef(&param->val); 
 				} else {
-					func->PushArray(param->byref.orig_addr, param->byref.cells, NULL, param->byref.flags);
+					func->PushArray(param->byref.orig_addr, param->byref.cells, param->byref.flags);
 					assert(type == Param_Array || type == Param_FloatByRef || type == Param_CellByRef);
 				}
 			} else {
@@ -489,7 +489,7 @@ void CForward::_Int_PushArray(cell_t *inarray, unsigned int cells, int flags)
 	m_params[m_curparam].byref.orig_addr = inarray;
 }
 
-int CForward::PushArray(cell_t *inarray, unsigned int cells, cell_t **phys_addr, int flags)
+int CForward::PushArray(cell_t *inarray, unsigned int cells, int flags)
 {
 	/* We don't allow this here */
 	if (!inarray)
@@ -513,11 +513,6 @@ int CForward::PushArray(cell_t *inarray, unsigned int cells, cell_t **phys_addr,
 		m_params[m_curparam].pushedas = Param_Array;
 	}
 
-	if (phys_addr)
-	{
-		*phys_addr = NULL;
-	}
-
 	_Int_PushArray(inarray, cells, flags);
 
 	m_curparam++;

core/systems/ForwardSys.h

@@ -50,7 +50,7 @@ public: //ICallable
 	virtual int PushCellByRef(cell_t *cell, int flags);
 	virtual int PushFloat(float number);
 	virtual int PushFloatByRef(float *number, int flags);
-	virtual int PushArray(cell_t *inarray, unsigned int cells, cell_t **phys_addr, int flags);
+	virtual int PushArray(cell_t *inarray, unsigned int cells, int flags);
 	virtual int PushString(const char *string);
 	virtual int PushStringEx(char *buffer, size_t length, int sz_flags, int cp_flags);
 	virtual void Cancel();

core/vm/sp_vm_basecontext.cpp

@@ -167,6 +167,7 @@ int BaseContext::Execute(uint32_t code_addr, cell_t *result)
 
 	cell_t save_sp = ctx->sp;
 	cell_t save_hp = ctx->hp;
+	uint32_t n_idx = ctx->n_idx;
 
 	bool wasExec = m_InExec;
 
@@ -202,6 +203,8 @@ int BaseContext::Execute(uint32_t code_addr, cell_t *result)
 		ctx->hp = save_hp;
 	}
 
+	ctx->n_idx = n_idx;
+
 	return err;
 }
 

core/vm/sp_vm_function.cpp

@@ -86,7 +86,7 @@ int CFunction::PushCellByRef(cell_t *cell, int flags)
 		return SetError(SP_ERROR_PARAMS_MAX);
 	}
 
-	return PushArray(cell, 1, NULL, flags);
+	return PushArray(cell, 1, flags);
 }
 
 int CFunction::PushFloat(float number)
@@ -101,7 +101,7 @@ int CFunction::PushFloatByRef(float *number, int flags)
 	return PushCellByRef((cell_t *)number, flags);
 }
 
-int CFunction::PushArray(cell_t *inarray, unsigned int cells, cell_t **phys_addr, int copyback)
+int CFunction::PushArray(cell_t *inarray, unsigned int cells, int copyback)
 {
 	if (m_curparam >= SP_MAX_EXEC_PARAMS)
 	{
@@ -109,32 +109,15 @@ int CFunction::PushArray(cell_t *inarray, unsigned int cells, cell_t **phys_addr
 	}
 
 	ParamInfo *info = &m_info[m_curparam];
-	int err;
-
-	if ((err=m_pContext->HeapAlloc(cells, &info->local_addr, &info->phys_addr)) != SP_ERROR_NONE)
-	{
-		return SetError(err);
-	}
 
 	info->flags = inarray ? copyback : 0;
 	info->marked = true;
-	info->size = cells * sizeof(cell_t);
+	info->size = cells;
-	m_params[m_curparam] = info->local_addr;
+	info->str.is_sz = false;
+	info->orig_addr = inarray;
+
 	m_curparam++;
 
-	if (inarray)
-	{
-		memcpy(info->phys_addr, inarray, sizeof(cell_t) * cells);
-		info->orig_addr = inarray;
-	} else {
-		info->orig_addr = info->phys_addr;
-	}
-
-	if (phys_addr)
-	{
-		*phys_addr = info->phys_addr;
-	}
-
 	return SP_ERROR_NONE;
 }
 
@@ -157,39 +140,15 @@ int CFunction::_PushString(const char *string, int sz_flags, int cp_flags, size_
 
 	ParamInfo *info = &m_info[m_curparam];
 	size_t cells = (len + sizeof(cell_t) - 1) / sizeof(cell_t);
-	int err;
-
-	if ((err=m_pContext->HeapAlloc(cells, &info->local_addr, &info->phys_addr)) != SP_ERROR_NONE)
-	{
-		return SetError(err);
-	}
 
 	info->marked = true;
-	m_params[m_curparam] = info->local_addr;
-	m_curparam++;	/* Prevent a leak */
-
-	if (!(sz_flags & SM_PARAM_STRING_COPY))
-	{
-		goto skip_localtostr;
-	}
-
-	if (sz_flags & SM_PARAM_STRING_UTF8)
-	{
-		if ((err=m_pContext->StringToLocalUTF8(info->local_addr, len, string, NULL)) != SP_ERROR_NONE)
-		{
-			return SetError(err);
-		}
-	} else {
-		if ((err=m_pContext->StringToLocal(info->local_addr, len, string)) != SP_ERROR_NONE)
-		{
-			return SetError(err);
-		}
-	}
-
-skip_localtostr:
-	info->flags = cp_flags;
 	info->orig_addr = (cell_t *)string;
+	info->flags = cp_flags;
 	info->size = len;
+	info->str.sz_flags = sz_flags;
+	info->str.is_sz = true;
+
+	m_curparam++;
 
 	return SP_ERROR_NONE;
 }
@@ -201,21 +160,13 @@ void CFunction::Cancel()
 		return;
 	}
 
-	while (m_curparam--)
-	{
-		if (m_info[m_curparam].marked)
-		{
-			m_pContext->HeapRelease(m_info[m_curparam].local_addr);
-			m_info[m_curparam].marked = false;
-		}
-	}
-
 	m_errorstate = SP_ERROR_NONE;
+	m_curparam = 0;
 }
 
 int CFunction::Execute(cell_t *result)
 {
-	int err;
+	int err = SP_ERROR_NONE;
 	if (!IsRunnable())
 	{
 		m_errorstate = SP_ERROR_NOT_RUNNABLE;
@@ -231,56 +182,122 @@ int CFunction::Execute(cell_t *result)
 	cell_t temp_params[SP_MAX_EXEC_PARAMS];
 	ParamInfo temp_info[SP_MAX_EXEC_PARAMS];
 	unsigned int numparams = m_curparam;
+	unsigned int i;
 	bool docopies = true;
 
 	if (numparams)
 	{
 		//Save the info locally, then reset it for re-entrant calls.
-		memcpy(temp_params, m_params, numparams * sizeof(cell_t));
 		memcpy(temp_info, m_info, numparams * sizeof(ParamInfo));
 	}
 	m_curparam = 0;
 
-	if ((err = CallFunction(temp_params, numparams, result)) != SP_ERROR_NONE)
+	/* Browse the parameters and build arrays */
+	for (i=0; i<numparams; i++)
 	{
+		/* Is this marked as an array? */
+		if (temp_info[i].marked)
+		{
+			if (!temp_info[i].str.is_sz)
+			{
+				/* Allocate a normal/generic array */
+				if ((err=m_pContext->HeapAlloc(temp_info[i].size, 
+											   &(temp_info[i].local_addr),
+											   &(temp_info[i].phys_addr)))
+					!= SP_ERROR_NONE)
+				{
+					break;
+				}
+				if (temp_info[i].orig_addr)
+				{
+					memcpy(temp_info[i].phys_addr, temp_info[i].orig_addr, sizeof(cell_t) * temp_info[i].size);
+				}
+			} else {
+				/* Calculate cells required for the string */
+				size_t cells = (temp_info[i].size + sizeof(cell_t) - 1) / sizeof(cell_t);
+
+				/* Allocate the buffer */
+				if ((err=m_pContext->HeapAlloc(cells,
+												&(temp_info[i].local_addr),
+												&(temp_info[i].phys_addr)))
+					!= SP_ERROR_NONE)
+				{
+					break;
+				}
+				/* Copy original string if necessary */
+				if ((temp_info[i].str.sz_flags & SM_PARAM_STRING_COPY) && (temp_info[i].orig_addr != NULL))
+				{
+					if (temp_info[i].str.sz_flags & SM_PARAM_STRING_UTF8)
+					{
+						if ((err=m_pContext->StringToLocalUTF8(temp_info[i].local_addr, 
+																temp_info[i].size, 
+																(const char *)temp_info[i].orig_addr,
+																NULL))
+							!= SP_ERROR_NONE)
+						{
+							break;
+						}
+					} else {
+						if ((err=m_pContext->StringToLocal(temp_info[i].local_addr,
+															temp_info[i].size,
+															(const char *)temp_info[i].orig_addr))
+							!= SP_ERROR_NONE)
+						{
+							break;
+						}
+					}
+				}
+			} /* End array/string calculation */
+			/* Update the pushed parameter with the byref local address */
+			temp_params[i] = temp_info[i].local_addr;
+		} else {
+			/* Just copy the value normally */
+			temp_params[i] = m_params[i];
+		}
+	}
+
+	/* Make the call if we can */
+	if (err == SP_ERROR_NONE)
+	{
+		if ((err = CallFunction(temp_params, numparams, result)) != SP_ERROR_NONE)
+		{
+			docopies = false;
+		}
+	} else {
 		docopies = false;
 	}
 
-	while (numparams--)
+	/* i should be equal to the last valid parameter + 1 */
+	while (i--)
 	{
-		if (!temp_info[numparams].marked)
+		if (!temp_info[i].marked)
 		{
 			continue;
 		}
-		if (docopies && temp_info[numparams].flags)
+		if (docopies && (temp_info[i].flags & SM_PARAM_COPYBACK))
 		{
-			if (temp_info[numparams].orig_addr)
+			if (temp_info[i].orig_addr)
 			{
-				if (temp_info[numparams].size == sizeof(cell_t))
+				if (temp_info[i].str.is_sz)
 				{
-					*temp_info[numparams].orig_addr = *temp_info[numparams].phys_addr;
+					memcpy(temp_info[i].orig_addr, temp_info[i].phys_addr, temp_info[i].size);
 				} else {
-					memcpy(temp_info[numparams].orig_addr, 
+					if (temp_info[i].size == 1)
-							temp_info[numparams].phys_addr, 
+					{
-							temp_info[numparams].size);
+						*temp_info[i].orig_addr = *(temp_info[i].phys_addr);
+					} else {
+						memcpy(temp_info[i].orig_addr, 
+								temp_info[i].phys_addr, 
+								temp_info[i].size * sizeof(cell_t));
+					}
 				}
 			}
 		}
-		m_pContext->HeapPop(temp_info[numparams].local_addr);
+		if ((err=m_pContext->HeapPop(temp_info[i].local_addr)) != SP_ERROR_NONE)
-		temp_info[numparams].marked = false;
+		{
+			return err;
+		}
 	}
 
 	return err;
 }
-
-cell_t *CFunction::GetAddressOfPushedParam(unsigned int param)
-{
-	if (m_errorstate != SP_ERROR_NONE
-		|| param >= m_curparam
-		|| !m_info[param].marked)
-	{
-		return NULL;
-	}
-
-	return m_info[param].phys_addr;
-}

core/vm/sp_vm_function.h

@@ -24,6 +24,11 @@ struct ParamInfo
 	cell_t *phys_addr;	/* Physical address of our copy */
 	cell_t *orig_addr;	/* Original address to copy back to */
 	ucell_t size;		/* Size of array in bytes */
+	struct
+	{
+		bool is_sz;		/* is a string */
+		int sz_flags;	/* has sz flags */
+	} str;
 };
 
 class CPlugin;
@@ -38,10 +43,9 @@ public:
 	virtual int PushCellByRef(cell_t *cell, int flags);
 	virtual int PushFloat(float number);
 	virtual int PushFloatByRef(float *number, int flags);
-	virtual int PushArray(cell_t *inarray, unsigned int cells, cell_t **phys_addr, int copyback);
+	virtual int PushArray(cell_t *inarray, unsigned int cells, int copyback);
 	virtual int PushString(const char *string);
 	virtual int PushStringEx(char *buffer, size_t length, int sz_flags, int cp_flags);
-	virtual cell_t *GetAddressOfPushedParam(unsigned int param);
 	virtual int Execute(cell_t *result);
 	virtual void Cancel();
 	virtual int CallFunction(const cell_t *params, unsigned int num_params, cell_t *result);

public/IForwardSys.h

@@ -37,7 +37,7 @@
 using namespace SourcePawn;
 
 #define SMINTERFACE_FORWARDMANAGER_NAME		"IForwardManager"
-#define SMINTERFACE_FORWARDMANAGER_VERSION	1
+#define SMINTERFACE_FORWARDMANAGER_VERSION	2
 
 /*
  * There is some very important documentation at the bottom of this file.
@@ -177,14 +177,10 @@ namespace SourceMod
 		 *
 		 * @param inarray	Array to copy.  Cannot be NULL, unlike ICallable's version.
 		 * @param cells		Number of cells to allocate and optionally read from the input array.
-		 * @param phys_addr	Unused.  If a value is passed, it will be filled with NULL.
 		 * @param flags		Whether or not changes should be copied back to the input array.
 		 * @return			Error code, if any.
 		 */
-		virtual int PushArray(cell_t *inarray, 
+		virtual int PushArray(cell_t *inarray, unsigned int cells, int flags=0) =0;
-								unsigned int cells, 
-								cell_t **phys_addr, 
-								int flags=0) =0;
 	};
 
 	/**
@@ -270,6 +266,14 @@ namespace SourceMod
 		{
 			return SMINTERFACE_FORWARDMANAGER_VERSION;
 		}
+		virtual bool IsVersionCompatible(unsigned int version)
+		{
+			if (version < 2 || version > GetInterfaceVersion())
+			{
+				return false;
+			}
+			return true;
+		}
 	public:
 		/**
 		 * @brief Creates a managed forward.  This forward exists globally.

public/sourcepawn/sp_vm_api.h

@@ -95,22 +95,20 @@ namespace SourcePawn
 
 		/**
 		 * @brief Pushes an array of cells onto the current call. 
-		 * NOTE: On Execute, the pointer passed will be modified if non-NULL and copy-back
+		 *
+		 * On Execute, the pointer passed will be modified if non-NULL and copy-back
 		 * is enabled.  
-		 * NOTE: By reference parameters are cached and thus are not read until execution.
+		 *
-		 *		 This means you cannot push a pointer, change it, and push it again and expect
+		 * By reference parameters are cached and thus are not read until execution.
-		 *       two different values to come out.
+		 * This means you cannot push a pointer, change it, and push it again and expect
+		 * two different values to come out.
 		 *
 		 * @param inarray	Array to copy, NULL if no initial array should be copied.
 		 * @param cells		Number of cells to allocate and optionally read from the input array.
-		 * @param phys_addr	Optional return address for physical array, if one was made.
 		 * @param flags		Whether or not changes should be copied back to the input array.
 		 * @return			Error code, if any.
 		 */
-		virtual int PushArray(cell_t *inarray, 
+		virtual int PushArray(cell_t *inarray, unsigned int cells, int flags=0) =0;
-								unsigned int cells, 
-								cell_t **phys_addr, 
-								int flags=0) =0;
 
 		/**
 		 * @brief Pushes a string onto the current call.
@@ -177,14 +175,6 @@ namespace SourcePawn
 		 */
 		virtual IPluginContext *GetParentContext() =0;
 
-		/**
-		 * @brief Returns the physical address of a by-reference parameter.
-		 *
-		 * @param param			Parameter index to read (beginning at 0).
-		 * @return				Address, or NULL if invalid parameter specified.
-		 */
-		virtual cell_t *GetAddressOfPushedParam(unsigned int param) =0;
-
 		/**
 		 * @brief Returns whether the parent plugin is paused.
 		 *