Fixed a crash caused by NPC variable references in callfunc

- Fixes bugreport:8074, thanks to ahmadshidqi
  http://hercules.ws/board/tracker/issue-8074-help-crash/
- Also fixed some DBMap allocation mistakes introduced in 4f3156b.
- Added testcases to the self-test script.
- Made possible thanks to Ind.

Signed-off-by: Haru <haru@dotalux.com>

3 files changed, 196 insertions, 45 deletions

diff --git a/npc/custom/test.txt b/npc/custom/test.txt
index d13570135..00f9c376e 100644
--- a/npc/custom/test.txt
+++ b/npc/custom/test.txt
@@ -8,9 +8,115 @@
 //= Script to test operators and possibly other elements of
 //= the script engine, useful for regression testing.
 
+function	script	F_TestReturnValue	{
+	return getarg(0);
+}
+
+function	script	F_TestScopeVars	{
+	.@x = 2;
+	return .@x+1;
+}
+
+function	script	F_TestNPCVars	{
+	.x = 2;
+	return .x+1;
+}
+
+function	script	F_TestDeepNestedScope	{
+	if (getarg(0) <= 0)
+		return getarg(1); // Stop recursion
+	if (getarg(1))
+		return callfunc("F_TestDeepNestedScope", getarg(0)-1, getarg(1)); // Recursion step
+	.@x = 1;
+	return callfunc("F_TestDeepNestedScope", getarg(0)-1, .@x); // First step
+}
+
+function	script	F_TestDeepNestedScopeNPC2	{
+	if (getarg(0) <= 0)
+		return getarg(1); // Stop recursion
+	if (getarg(1))
+		return callfunc("F_TestDeepNestedScopeNPC", getarg(0)-1, getarg(1)); // Recursion step
+	.x = 1;
+	return callfunc("F_TestDeepNestedScopeNPC", getarg(0)-1, .x); // First step
+}
+
+function	script	F_TestDeepNestedScopeNPC	{
+	if (getarg(0) <= 0)
+		return getarg(1); // Stop recursion
+	if (getarg(1))
+		return callfunc("F_TestDeepNestedScopeNPC2", getarg(0)-1, getarg(1)); // Recursion step
+	.x = 1;
+	return callfunc("F_TestDeepNestedScopeNPC2", getarg(0)-1, .x); // First step
+}
+
+function	script	F_TestNestedScope	{
+	.@x = 1;
+	.@y = callfunc("F_TestReturnValue", .@x);
+	return .@y;
+}
+
+function	script	F_TestNestedScopeNPC	{
+	.x = 1;
+	.y = callfunc("F_TestReturnValue", .x);
+	return .y;
+}
+
+function	script	F_TestArrayRefs	{
+	return getelementofarray(getarg(0), getarraysize(getarg(0)) - 1);
+}
+
+function	script	F_TestReturnArrayRef	{
+	setarray getarg(0), 5, 6, 7, 8;
+	return getarraysize(getarg(0));
+}
+
+function	script	F_TestScopeArrays	{
+	setarray .@x, 1, 2, 3, 4;
+	copyarray .@y, getarg(0), getarraysize(getarg(0));
+	return getarraysize(.@y);
+}
+
+function	script	F_TestNPCArrays	{
+	setarray .x, 1, 2, 3, 4;
+	copyarray .y, getarg(0), getarraysize(getarg(0));
+	return getarraysize(.y);
+}
+
 -	script	HerculesSelfTest	-1,{
 	end;
 
+OnTestReturnValue:
+	return getarg(0);
+
+OnTestScopeVars:
+	.@x = 2;
+	return .@x+1;
+
+OnTestDeepNestedScope:
+	if (getarg(0) <= 0)
+		return getarg(1); // Stop recursion
+	if (getarg(1))
+		return callsub(OnTestDeepNestedScope, getarg(0)-1, getarg(1)); // Recursion step
+	.@x = 1;
+	return callsub(OnTestDeepNestedScope, getarg(0)-1, .@x); // First step
+
+OnTestNestedScope:
+	.@x = 1;
+	.@y = callsub(OnTestReturnValue, .@x);
+	return .@y;
+
+OnTestArrayRefs:
+	return getelementofarray(getarg(0), getarraysize(getarg(0)) - 1);
+
+OnTestReturnArrayRef:
+	setarray getarg(0), 5, 6, 7, 8;
+	return getarraysize(getarg(0));
+
+OnTestScopeArrays:
+	setarray .@x, 1, 2, 3, 4;
+	copyarray .@y, getarg(0), getarraysize(getarg(0));
+	return getarraysize(.@y);
+
 OnReportError:
 	.@msg$ = getarg(0,"Unknown Error");
 	.@val$ = getarg(1,"");
@@ -546,7 +652,53 @@ OnInit:
 	setarray .@y, 5, 6, 7, 8, 9;
 	callsub(OnCheck, "Callsub (copyarray from reference with the same name)", getarraysize(.@y), callsub(OnTestScopeArrays, .@y));
 	callsub(OnCheck, "Callsub (parent array vars isolation)", getarraysize(.@x), .@z);
+	deletearray .@x;
+	deletearray .@y;
 
+	// Callfunc
+	callsub(OnCheck, "Callfunc return value", callfunc("F_TestReturnValue", 1));
+	.@x = 1;
+	callsub(OnCheck, "Callfunc return with scope variables", callfunc("F_TestScopeVars"), 3);
+	callsub(OnCheck, "Callfunc (parent scope vars isolation)", .@x, 1);
+	callsub(OnCheck, "Callfunc (nested scopes)", callfunc("F_TestNestedScope"), 1);
+	callsub(OnCheck, "Callfunc (deeply nested scopes)", callfunc("F_TestDeepNestedScope", 30, 0), 1);
+	deletearray .@x;
+	setarray .@x, 1, 2, 3, 4;
+	callsub(OnCheck, "Callfunc (array references)", callfunc("F_TestArrayRefs", .@x), 4);
+	deletearray .@x;
+	.@y = callfunc("F_TestReturnArrayRef", .@x);
+	callsub(OnCheck, "Callfunc return array references (size check)", getarraysize(.@x), .@y);
+	callsub(OnCheck, "Callfunc return array references", getelementofarray(.@x, 3), 8);
+	deletearray .@x;
+	deletearray .@y;
+	setarray .@x, 1, 2;
+	.@z = getarraysize(.@x);
+	setarray .@y, 5, 6, 7, 8, 9;
+	callsub(OnCheck, "Callfunc (copyarray from reference with the same name)", getarraysize(.@y), callfunc("F_TestScopeArrays", .@y));
+	callsub(OnCheck, "Callfunc (parent array vars isolation)", getarraysize(.@x), .@z);
+	deletearray .@x;
+	deletearray .@y;
+	.x = 1;
+	callsub(OnCheck, "Callfunc return with NPC variables", callfunc("F_TestNPCVars"), 3); // FIXME: segfault
+	callsub(OnCheck, "Callfunc (parent NPC vars isolation)", .x, 1);
+	callsub(OnCheck, "Callfunc (nested scopes and NPC variables)", callfunc("F_TestNestedScopeNPC"), 1); // FIXME: segfault
+	callsub(OnCheck, "Callfunc (deeply nested scopes and NPC variables)", callfunc("F_TestDeepNestedScopeNPC", 30, 0), 1); // FIXME: segfault
+	deletearray .x;
+	setarray .x, 1, 2, 3, 4;
+	callsub(OnCheck, "Callfunc (array references and NPC variables)", callfunc("F_TestArrayRefs", .x), 4); // FIXME: segfault
+	deletearray .x;
+	.y = callfunc("F_TestReturnArrayRef", .x);
+	callsub(OnCheck, "Callfunc return array references with NPC variables (size check)", getarraysize(.x), .y);
+	callsub(OnCheck, "Callfunc return array references wuth NPC variables", getelementofarray(.x, 3), 8);
+	deletearray .x;
+	deletearray .y;
+	setarray .x, 1, 2;
+	.@z = getarraysize(.@x);
+	setarray .y, 5, 6, 7, 8, 9;
+	callsub(OnCheck, "Callfunc (copyarray from NPC variable reference with the same name)", getarraysize(.@y), callfunc("F_TestNPCArrays", .@y)); // FIXME: segfault
+	callsub(OnCheck, "Callfunc (parent array NPC vars isolation)", getarraysize(.@x), .@z);
+	deletearray .x;
+	deletearray .y;
 
 	if (.errors) {
 		debugmes "Script engine self-test   [ \033[0;31mFAILED\033[0m ]";
@@ -555,37 +707,5 @@ OnInit:
 		debugmes "Script engine self-test   [ \033[0;32mPASSED\033[0m ]";
 	}
 	end;
-
-OnTestReturnValue:
-	return getarg(0);
-
-OnTestScopeVars:
-	.@x = 2;
-	return .@x+1;
-
-OnTestDeepNestedScope:
-	if (getarg(0) <= 0)
-		return getarg(1); // Stop recursion
-	if (getarg(1))
-		return callsub(OnTestDeepNestedScope, getarg(0)-1, getarg(1)); // Recursion step
-	.@x = 1;
-	return callsub(OnTestDeepNestedScope, getarg(0)-1, .@x); // First step
-
-OnTestNestedScope:
-	.@x = 1;
-	.@y = callsub(OnTestReturnValue, .@x);
-	return .@y;
-
-OnTestArrayRefs:
-	return getelementofarray(getarg(0), getarraysize(getarg(0)) - 1);
-
-OnTestReturnArrayRef:
-	setarray getarg(0), 5, 6, 7, 8;
-	return getarraysize(getarg(0));
-
-OnTestScopeArrays:
-	setarray .@x, 1, 2, 3, 4;
-	copyarray .@y, getarg(0), getarraysize(getarg(0));
-	return getarraysize(.@y);
 }
 
diff --git a/src/map/script.c b/src/map/script.c
index bef5f37e0..fef25b927 100644
--- a/src/map/script.c
+++ b/src/map/script.c
@@ -3130,8 +3130,7 @@ struct script_data* push_copy(struct script_stack* stack, int pos) {
 
 /// Removes the values in indexes [start,end[ from the stack.
 /// Adjusts all stack pointers.
-void pop_stack(struct script_state* st, int start, int end)
-{
+void pop_stack(struct script_state* st, int start, int end) {
 	struct script_stack* stack = st->stack;
 	struct script_data* data;
 	int i;
@@ -3153,6 +3152,10 @@ void pop_stack(struct script_state* st, int start, int end)
 		{
 			struct script_retinfo* ri = data->u.ri;
 			if( ri->scope.vars ) {
+				// Note: This is necessary evern if we're also doing it in run_func
+				// (in the RETFUNC block) because not all functions return.  If a
+				// function (or a sub) has an 'end' or a 'close', it'll reach this
+				// block with its scope vars still to be freed.
 				script->free_vars(ri->scope.vars);
 				ri->scope.vars = NULL;
 			}
@@ -3224,6 +3227,8 @@ struct script_state* script_alloc_state(struct script_code* rootscript, int pos,
 
 	st = ers_alloc(script->st_ers, struct script_state);
 	st->stack = ers_alloc(script->stack_ers, struct script_stack);
+	st->pending_refs = NULL;
+	st->pending_ref_count = 0;
 	st->stack->sp = 0;
 	st->stack->sp_max = 64;
 	CREATE(st->stack->stack_data, struct script_data, st->stack->sp_max);
@@ -3280,6 +3285,12 @@ void script_free_state(struct script_state* st) {
 			}
 		}
 		st->pos = -1;
+		if (st->pending_ref_count > 0) {
+			while (st->pending_ref_count > 0)
+				aFree(st->pending_refs[--st->pending_ref_count]);
+			aFree(st->pending_refs);
+			st->pending_refs = NULL;
+		}
 		idb_remove(script->st_db, st->id);
 		ers_free(script->st_ers, st);
 		if( --script->active_scripts == 0 ) {
@@ -3288,6 +3299,19 @@ void script_free_state(struct script_state* st) {
 	}
 }
 
+/**
+ * Adds a pending reference entry to the current script.
+ *
+ * @see struct script_state::pending_refs
+ *
+ * @param st[in]  Script state.
+ * @param ref[in] Reference to be added.
+ */
+void script_add_pending_ref(struct script_state *st, struct reg_db *ref) {
+	RECREATE(st->pending_refs, struct reg_db*, ++st->pending_ref_count);
+	st->pending_refs[st->pending_ref_count-1] = ref;
+}
+
 //
 // Main execution unit
 //
@@ -4883,7 +4907,10 @@ BUILDIN(callfunc)
 	st->stack->defsp = st->stack->sp;
 	st->state = GOTO;
 	st->stack->scope.vars = i64db_alloc(DB_OPT_RELEASE_DATA);
-	st->stack->scope.arrays = i64db_alloc(DB_OPT_BASE);
+	st->stack->scope.arrays = idb_alloc(DB_OPT_BASE);
+
+	if( !st->script->local.vars )
+		st->script->local.vars = i64db_alloc(DB_OPT_RELEASE_DATA);
 
 	return true;
 }
@@ -4934,7 +4961,7 @@ BUILDIN(callsub)
 	st->stack->defsp = st->stack->sp;
 	st->state = GOTO;
 	st->stack->scope.vars = i64db_alloc(DB_OPT_RELEASE_DATA);
-	st->stack->scope.arrays = i64db_alloc(DB_OPT_BASE);
+	st->stack->scope.arrays = idb_alloc(DB_OPT_BASE);
 
 	return true;
 }
@@ -4977,30 +5004,30 @@ BUILDIN(getarg)
 ///
 /// return;
 /// return <value>;
-BUILDIN(return)
-{
+BUILDIN(return) {
 	if( script_hasdata(st,2) )
 	{// return value
 		struct script_data* data;
 		script_pushcopy(st, 2);
 		data = script_getdatatop(st, -1);
-		if( data_isreference(data) )
-		{
+		if( data_isreference(data) ) {
 			const char* name = reference_getname(data);
-			if( name[0] == '.' && name[1] == '@' )
-			{// scope variable
+			if( name[0] == '.' && name[1] == '@' ) {
+				// scope variable
 				if( !data->ref || data->ref->vars == st->stack->scope.vars )
 					script->get_val(st, data);// current scope, convert to value
 				if( data->ref && data->ref->vars == st->stack->stack_data[st->stack->defsp-1].u.ri->scope.vars )
 					data->ref = NULL; // Reference to the parent scope, remove reference pointer
-			}
-			else if( name[0] == '.' && !data->ref )
-			{// script variable, link to current script
+			} else if( name[0] == '.' && !data->ref ) {
+				// script variable without a reference set, link to current script
 				data->ref = (struct reg_db *)aCalloc(sizeof(struct reg_db), 1);
+				script->add_pending_ref(st, data->ref);
 				data->ref->vars = st->script->local.vars;
 				if( !st->script->local.arrays )
 					st->script->local.arrays = idb_alloc(DB_OPT_BASE);
 				data->ref->arrays = st->script->local.arrays;
+			} else if ( name[0] == '.' /* && data->ref != NULL */ ) {
+				data->ref = NULL; // Reference to the parent scope's script, remove reference pointer.
 			}
 		}
 	}
@@ -19231,6 +19258,7 @@ void script_defaults(void) {
 	script->free_vars = script_free_vars;
 	script->alloc_state = script_alloc_state;
 	script->free_state = script_free_state;
+	script->add_pending_ref = script_add_pending_ref;
 	script->run_autobonus = script_run_autobonus;
 	script->cleararray_pc = script_cleararray_pc;
 	script->setarray_pc = script_setarray_pc;
diff --git a/src/map/script.h b/src/map/script.h
index ff947bf79..73ba7303e 100644
--- a/src/map/script.h
+++ b/src/map/script.h
@@ -401,6 +401,8 @@ struct hQueueIterator {
 
 struct script_state {
 	struct script_stack* stack;
+	struct reg_db **pending_refs; ///< References to .vars returned by sub-functions, pending deletion.
+	int pending_ref_count;        ///< Amount of pending_refs currently stored.
 	int start,end;
 	int pos;
 	enum e_script_state state;
@@ -589,6 +591,7 @@ struct script_interface {
 	void (*free_vars) (struct DBMap *var_storage);
 	struct script_state* (*alloc_state) (struct script_code* rootscript, int pos, int rid, int oid);
 	void (*free_state) (struct script_state* st);
+	void (*add_pending_ref) (struct script_state *st, struct reg_db *ref);
 	void (*run_autobonus) (const char *autobonus,int id, int pos);
 	void (*cleararray_pc) (struct map_session_data* sd, const char* varname, void* value);
 	void (*setarray_pc) (struct map_session_data* sd, const char* varname, uint32 idx, void* value, int* refcache);