From f360ed563e13a6158de3f5a62ca61020d0e77f9d Mon Sep 17 00:00:00 2001
From: A Thousand Ships <96648715+AThousandShips@users.noreply.github.com>
Date: Tue, 27 Feb 2024 13:39:02 +0100
Subject: [PATCH] [GDScript] Prevent some vararg methods accessing member
 variables

Calling some methods from GDScript with member variable arguments causes
the pointer to that argument being passed instead of the value, affects:
* `Signal.emit`
* `Object.emit_signal`
* `SceneTree.call_group/_flags`
---
 core/object/object.cpp                        | 20 +++++++--
 core/variant/variant_call.cpp                 | 21 +++++++++-
 .../features/vararg_member_not_shared.gd      | 23 ++++++++++
 .../features/vararg_member_not_shared.out     |  5 +++
 scene/main/scene_tree.cpp                     | 42 ++++++++++++++++++-
 5 files changed, 105 insertions(+), 6 deletions(-)
 create mode 100644 modules/gdscript/tests/scripts/runtime/features/vararg_member_not_shared.gd
 create mode 100644 modules/gdscript/tests/scripts/runtime/features/vararg_member_not_shared.out

diff --git a/core/object/object.cpp b/core/object/object.cpp
index 638545fc73a7..cbf1da79fde9 100644
--- a/core/object/object.cpp
+++ b/core/object/object.cpp
@@ -1124,14 +1124,28 @@ Error Object::_emit_signal(const Variant **p_args, int p_argcount, Callable::Cal
 
 	StringName signal = *p_args[0];
 
-	const Variant **args = nullptr;
+	// Prevent GDScript member variables being passed by pointer, see:
+	// https://github.com/godotengine/godot/issues/88885
+	Variant *args = nullptr;
+	const Variant **argptrs = nullptr;
 
 	int argc = p_argcount - 1;
 	if (argc) {
-		args = &p_args[1];
+		args = (Variant *)alloca(sizeof(Variant) * argc);
+		argptrs = (const Variant **)alloca(sizeof(Variant *) * argc);
+		for (int i = 0; i < argc; i++) {
+			memnew_placement(&args[i], Variant(*p_args[i + 1]));
+			argptrs[i] = &args[i];
+		}
+	}
+
+	Error ret = emit_signalp(signal, argptrs, argc);
+
+	for (int i = 0; i < argc; i++) {
+		args[i].~Variant();
 	}
 
-	return emit_signalp(signal, args, argc);
+	return ret;
 }
 
 Error Object::emit_signalp(const StringName &p_name, const Variant **p_args, int p_argcount) {
diff --git a/core/variant/variant_call.cpp b/core/variant/variant_call.cpp
index ef26820a5087..af99deade212 100644
--- a/core/variant/variant_call.cpp
+++ b/core/variant/variant_call.cpp
@@ -1058,7 +1058,26 @@ struct _VariantCall {
 
 	static void func_Signal_emit(Variant *v, const Variant **p_args, int p_argcount, Variant &r_ret, Callable::CallError &r_error) {
 		Signal *signal = VariantGetInternalPtr<Signal>::get_ptr(v);
-		signal->emit(p_args, p_argcount);
+
+		// Prevent GDScript member variables being passed by pointer, see:
+		// https://github.com/godotengine/godot/issues/88885
+		Variant *args = nullptr;
+		const Variant **argptrs = nullptr;
+
+		if (p_argcount) {
+			args = (Variant *)alloca(sizeof(Variant) * p_argcount);
+			argptrs = (const Variant **)alloca(sizeof(Variant *) * p_argcount);
+			for (int i = 0; i < p_argcount; i++) {
+				memnew_placement(&args[i], Variant(*p_args[i]));
+				argptrs[i] = &args[i];
+			}
+		}
+
+		signal->emit(argptrs, p_argcount);
+
+		for (int i = 0; i < p_argcount; i++) {
+			args[i].~Variant();
+		}
 	}
 
 	struct ConstantData {
diff --git a/modules/gdscript/tests/scripts/runtime/features/vararg_member_not_shared.gd b/modules/gdscript/tests/scripts/runtime/features/vararg_member_not_shared.gd
new file mode 100644
index 000000000000..218c3b09c51c
--- /dev/null
+++ b/modules/gdscript/tests/scripts/runtime/features/vararg_member_not_shared.gd
@@ -0,0 +1,23 @@
+# https://github.com/godotengine/godot/issues/88885
+
+extends Node
+
+var member: int
+
+signal s(int)
+
+func _first(arg: int) -> void:
+	print("In first: ", arg)
+	member = 1
+
+func _second(arg: int) -> void:
+	print("In second: ", arg)
+
+@warning_ignore("return_value_discarded")
+func test():
+	member = 0
+	s.connect(_first)
+	s.connect(_second)
+	s.emit(member)
+	member = 0
+	emit_signal(&"s", member)
diff --git a/modules/gdscript/tests/scripts/runtime/features/vararg_member_not_shared.out b/modules/gdscript/tests/scripts/runtime/features/vararg_member_not_shared.out
new file mode 100644
index 000000000000..919f4c63a5aa
--- /dev/null
+++ b/modules/gdscript/tests/scripts/runtime/features/vararg_member_not_shared.out
@@ -0,0 +1,5 @@
+GDTEST_OK
+In first: 0
+In second: 0
+In first: 0
+In second: 0
diff --git a/scene/main/scene_tree.cpp b/scene/main/scene_tree.cpp
index 67cb35e23a25..65e39d9d7cf8 100644
--- a/scene/main/scene_tree.cpp
+++ b/scene/main/scene_tree.cpp
@@ -1322,7 +1322,26 @@ void SceneTree::_call_group_flags(const Variant **p_args, int p_argcount, Callab
 	StringName group = *p_args[1];
 	StringName method = *p_args[2];
 
-	call_group_flagsp(flags, group, method, p_args + 3, p_argcount - 3);
+	// Prevent GDScript member variables being passed by pointer, see:
+	// https://github.com/godotengine/godot/issues/88885
+	Variant *args = nullptr;
+	const Variant **argptrs = nullptr;
+
+	int argc = p_argcount - 3;
+	if (argc) {
+		args = (Variant *)alloca(sizeof(Variant) * argc);
+		argptrs = (const Variant **)alloca(sizeof(Variant *) * argc);
+		for (int i = 0; i < argc; i++) {
+			memnew_placement(&args[i], Variant(*p_args[i + 3]));
+			argptrs[i] = &args[i];
+		}
+	}
+
+	call_group_flagsp(flags, group, method, argptrs, argc);
+
+	for (int i = 0; i < argc; i++) {
+		args[i].~Variant();
+	}
 }
 
 void SceneTree::_call_group(const Variant **p_args, int p_argcount, Callable::CallError &r_error) {
@@ -1335,7 +1354,26 @@ void SceneTree::_call_group(const Variant **p_args, int p_argcount, Callable::Ca
 	StringName group = *p_args[0];
 	StringName method = *p_args[1];
 
-	call_group_flagsp(GROUP_CALL_DEFAULT, group, method, p_args + 2, p_argcount - 2);
+	// Prevent GDScript member variables being passed by pointer, see:
+	// https://github.com/godotengine/godot/issues/88885
+	Variant *args = nullptr;
+	const Variant **argptrs = nullptr;
+
+	int argc = p_argcount - 2;
+	if (argc) {
+		args = (Variant *)alloca(sizeof(Variant) * argc);
+		argptrs = (const Variant **)alloca(sizeof(Variant *) * argc);
+		for (int i = 0; i < argc; i++) {
+			memnew_placement(&args[i], Variant(*p_args[i + 2]));
+			argptrs[i] = &args[i];
+		}
+	}
+
+	call_group_flagsp(GROUP_CALL_DEFAULT, group, method, argptrs, argc);
+
+	for (int i = 0; i < argc; i++) {
+		args[i].~Variant();
+	}
 }
 
 int64_t SceneTree::get_frame() const {