Fix UB in virtual method calls that take objects

Fix incorrect incrementing of refcount when calling in to godot
Fix refcount not being incremented when we receive a refcounted object in virtual methods

Author: lilizoey

godot-core/src/builtin/quaternion.rs

@@ -181,7 +181,7 @@ impl Quaternion {
     }
 
     // pub fn spherical_cubic_interpolate(self, b: Self, pre_a: Self, post_b: Self, weight: real) -> Self {}
-    // TODO: Implement godot's function in rust
+    // TODO: Implement godot's function in Rust
     /*
         pub fn spherical_cubic_interpolate_in_time(
             self,

godot-core/src/builtin/string/godot_string.rs

@@ -157,7 +157,7 @@ impl fmt::Debug for GodotString {
 }
 
 // ----------------------------------------------------------------------------------------------------------------------------------------------
-// Conversion from/into rust string-types
+// Conversion from/into Rust string-types
 
 impl<S> From<S> for GodotString
 where

godot-core/src/obj/as_arg.rs

@@ -22,8 +22,11 @@ pub trait AsArg: Sealed {
 impl<T: GodotClass> Sealed for Gd<T> {}
 impl<T: GodotClass> AsArg for Gd<T> {
     fn as_arg_ptr(&self) -> sys::GDExtensionConstTypePtr {
-        // Pass argument to engine: increment refcount
-        <T::Mem as crate::obj::mem::Memory>::maybe_inc_ref(self);
+        // We're passing a reference to the object to the callee. If the reference count needs to be
+        // incremented then the callee will do so. We do not need to prematurely do so.
+        //
+        // In Rust terms, if `T` is refcounted then we are effectively passing a `&Arc<T>`, and the callee
+        // would need to call `.clone()` if desired.
         self.sys_const()
     }
 }

godot-core/src/obj/gd.rs

@@ -552,13 +552,20 @@ where
     // https://github.com/godotengine/godot-cpp/issues/954
 
     unsafe fn from_arg_ptr(ptr: sys::GDExtensionTypePtr, call_type: PtrcallType) -> Self {
-        if T::Mem::pass_as_ref(call_type) {
-            let obj_ptr = interface_fn!(ref_get_object)(ptr as sys::GDExtensionRefPtr);
-            // ref_get_object increments the ref_count for us
-            Self::from_obj_sys_weak(obj_ptr)
+        let obj_ptr = if T::Mem::pass_as_ref(call_type) {
+            // ptr is `Ref<T>*`
+            // See the docs for `PtrcallType::Virtual` for more info on `Ref<T>`.
+            interface_fn!(ref_get_object)(ptr as sys::GDExtensionRefPtr)
+        } else if matches!(call_type, PtrcallType::Virtual) {
+            // ptr is `T**`
+            *(ptr as *mut sys::GDExtensionObjectPtr)
         } else {
-            Self::from_obj_sys(ptr as sys::GDExtensionObjectPtr)
-        }
+            // ptr is `T*`
+            ptr as sys::GDExtensionObjectPtr
+        };
+
+        // obj_ptr is `T*`
+        Self::from_obj_sys(obj_ptr)
     }
 
     unsafe fn move_return_ptr(self, ptr: sys::GDExtensionTypePtr, call_type: PtrcallType) {

godot-ffi/src/godot_ffi.rs

@@ -57,7 +57,7 @@ pub unsafe trait GodotFfi {
     /// Return Godot opaque pointer, for an immutable operation.
     ///
     /// Note that this is a `*mut` pointer despite taking `&self` by shared-ref.
-    /// This is because most of Godot's rust API is not const-correct. This can still
+    /// This is because most of Godot's Rust API is not const-correct. This can still
     /// enhance user code (calling `sys_mut` ensures no aliasing at the time of the call).
     fn sys(&self) -> sys::GDExtensionTypePtr;
 
@@ -155,11 +155,15 @@ pub enum PtrcallType {
 
     /// Virtual pointer call.
     ///
-    /// A virtual call behaves like [`PtrcallType::Standard`], except for `RefCounted` objects.
-    /// `RefCounted` objects are instead passed in and returned as `Ref<T>` objects in Godot.
+    /// A virtual call behaves like [`PtrcallType::Standard`], except for Objects.
     ///
-    /// To properly get a value from an argument in a pointer call, you must use `ref_get_object`. And to
-    /// return a value you must use `ref_set_object`.
+    /// Objects that do not inherit from `RefCounted` are passed in as `Object**`
+    /// (`*mut GDExtensionObjectPtr` in GDExtension terms), and objects that inherit from
+    /// `RefCounted` are passed in as `Ref<T>*` (`GDExtensionRefPtr` in GDExtension
+    /// terms) and returned as `Ref<T>` objects in Godot.
+    ///
+    /// To get a `GDExtensionObjectPtr` from a `GDExtensionRefPtr`, you must use `ref_get_object`, and to
+    /// set a `GDExtensionRefPtr` to some object, you must use `ref_set_object`.
     ///
     /// See also https://github.com/godotengine/godot-cpp/issues/954.
     Virtual,

itest/godot/.godot/global_script_class_cache.cfg

@@ -1,4 +1,10 @@
 list=Array[Dictionary]([{
+"base": &"Node",
+"class": &"GDScriptTestRunner",
+"icon": "",
+"language": &"GDScript",
+"path": "res://TestRunner.gd"
+}, {
 "base": &"RefCounted",
 "class": &"TestStats",
 "icon": "",
@@ -10,4 +16,10 @@ list=Array[Dictionary]([{
 "icon": "",
 "language": &"GDScript",
 "path": "res://TestSuite.gd"
+}, {
+"base": &"TestSuite",
+"class": &"TestSuiteSpecial",
+"icon": "",
+"language": &"GDScript",
+"path": "res://TestSuiteSpecial.gd"
 }])

itest/godot/SpecialTests.gd

@@ -0,0 +1,49 @@
+# This Source Code Form is subject to the terms of the Mozilla Public
+# License, v. 2.0. If a copy of the MPL was not distributed with this
+# file, You can obtain one at https://mozilla.org/MPL/2.0/.
+
+extends TestSuiteSpecial
+
+# Tests in here require specific setup/configuration that is not easily achievable through the standard 
+# integration testing API.
+# 
+# Using the standard API if possible is highly preferred.
+
+# Test that we can call `_input_event` on a class defined in rust, as a virtual method. 
+#
+# This tests #267, which was caused by us incorrectly handing Objects when passed as arguments to virtual 
+# methods. `_input_event` is the easiest such method to test. However it can only be triggered by letting a 
+# full physics frame pass after calling `push_unhandled_input`. Thus we cannot use the standard API for 
+# testing this at the moment, since we dont have any way to let frames pass in between the start and end of 
+# an integration test. 
+func test_collision_object_2d_input_event():
+	var root: Node = Engine.get_main_loop().root
+
+	var window := Window.new()
+	window.physics_object_picking = true
+	root.add_child(window)
+
+	var collision_object := CollisionObject2DTest.new()
+	collision_object.input_pickable = true
+
+	var collision_shape := CollisionShape2D.new()
+	collision_shape.shape = RectangleShape2D.new()
+	collision_object.add_child(collision_shape)
+
+	window.add_child(collision_object)
+
+	assert_that(not collision_object.input_event_called())
+	assert_eq(collision_object.get_viewport(), null)
+
+	var event := InputEventMouseMotion.new()
+	event.global_position = Vector2.ZERO
+	window.push_unhandled_input(event)
+
+	# Ensure we run a full physics frame
+	await root.get_tree().physics_frame
+
+	assert_that(collision_object.input_event_called())
+	assert_eq(collision_object.get_viewport(), window)
+
+	window.queue_free()
+

itest/godot/TestRunner.gd

@@ -3,8 +3,12 @@
 # file, You can obtain one at https://mozilla.org/MPL/2.0/.
 
 extends Node
+class_name GDScriptTestRunner
 
 func _ready():
+	# Ensure physics is initialized, for tests that require it.
+	await get_tree().physics_frame
+
 	var allow_focus := true
 	var unrecognized_args: Array = []
 	for arg in OS.get_cmdline_user_args():
@@ -32,7 +36,17 @@ func _ready():
 		for method in suite.get_method_list():
 			var method_name: String = method.name
 			if method_name.begins_with("test_"):
-				gdscript_tests.push_back(GDScriptTestCase.new(suite, method_name))
+				gdscript_tests.push_back(GDScriptExecutableTestCase.new(suite, method_name))
+
+	var special_case_test_suites: Array = [
+		preload("res://SpecialTests.gd").new(),
+	]
+
+	for suite in special_case_test_suites:
+		for method in suite.get_method_list():
+			var method_name: String = method.name
+			if method_name.begins_with("test_"):
+				gdscript_tests.push_back(await suite.run_test(suite, method_name))
 
 	var success: bool = rust_runner.run_all_tests(
 		gdscript_tests,
@@ -55,13 +69,32 @@ class GDScriptTestCase:
 		self.method_name = method_name
 		self.suite_name = _suite_name(suite)
 
+	func run():
+		push_error("run unimplemented")
+		return false
+	
+	static func _suite_name(suite: Object) -> String:
+		var script: GDScript = suite.get_script()
+		return str(script.resource_path.get_file().get_basename(), ".gd")
+
+# Standard test case used for whenever something can be tested by just running a gdscript function.
+class GDScriptExecutableTestCase extends GDScriptTestCase:
 	func run():
 		# This is a no-op if the suite doesn't have this property.
 		suite.set("_assertion_failed", false)
 		var result = suite.call(method_name)
 		var ok: bool = (result == true || result == null) && !suite.get("_assertion_failed")
 		return ok
-	
-	static func _suite_name(suite: Object) -> String:
-		var script: GDScript = suite.get_script()
-		return str(script.resource_path.get_file().get_basename(), ".gd")
+
+# Hardcoded test case used for special cases where the standard testing API is not sufficient.
+#
+# Stores the errors generated during the execution, so they can be printed when it is appropriate to do so.
+# As we may not run this test case at the time we say we do in the terminal.
+class GDScriptHardcodedTestCase extends GDScriptTestCase:
+	# Errors generated during execution of the test.
+	var errors: Array[String] = []
+	var execution_time_seconds: float = 0
+	var result: bool = false
+
+	func run():
+		return result

itest/godot/TestSuite.gd

@@ -7,6 +7,12 @@ extends RefCounted
 
 var _assertion_failed: bool = false
 
+func print_newline():
+	printerr()
+
+func print_error(s: String):
+	push_error(s)
+
 ## Asserts that `what` is `true`, but does not abort the test. Returns `what` so you can return
 ## early from the test function if the assertion failed.
 func assert_that(what: bool, message: String = "") -> bool:
@@ -15,11 +21,11 @@ func assert_that(what: bool, message: String = "") -> bool:
 
 	_assertion_failed = true
 
-	printerr() # previous line not yet broken
+	print_newline() # previous line not yet broken
 	if message:
-		push_error("GDScript assertion failed:  %s" % message)
+		print_error("GDScript assertion failed:  %s" % message)
 	else:
-		push_error("GDScript assertion failed.")
+		print_error("GDScript assertion failed.")
 	return false
 
 func assert_eq(left, right, message: String = "") -> bool:
@@ -28,9 +34,9 @@ func assert_eq(left, right, message: String = "") -> bool:
 
 	_assertion_failed = true
 
-	printerr() # previous line not yet broken
+	print_newline() # previous line not yet broken
 	if message:
-		push_error("GDScript assertion failed:  %s\n  left: %s\n right: %s" % [message, left, right])
+		print_error("GDScript assertion failed:  %s\n  left: %s\n right: %s" % [message, left, right])
 	else:
-		push_error("GDScript assertion failed:  `(left == right)`\n  left: %s\n right: %s" % [left, right])
+		print_error("GDScript assertion failed:  `(left == right)`\n  left: %s\n right: %s" % [left, right])
 	return false

itest/godot/TestSuiteSpecial.gd

@@ -0,0 +1,34 @@
+# This Source Code Form is subject to the terms of the Mozilla Public
+# License, v. 2.0. If a copy of the MPL was not distributed with this
+# file, You can obtain one at https://mozilla.org/MPL/2.0/.
+
+class_name TestSuiteSpecial
+extends TestSuite
+
+var errors: Array[String] = []
+
+func print_newline():
+	errors.push_back("")
+
+func print_error(s: String):
+	errors.push_back(s)
+
+# Run a special test case, generating a hardcoded test-case based on the outcome of the test.
+func run_test(suite: Object, method_name: String) -> GDScriptTestRunner.GDScriptHardcodedTestCase:
+	var callable: Callable = Callable(suite, method_name)
+	
+	_assertion_failed = false
+	var start_time = Time.get_ticks_usec()
+	var result = await callable.call()
+	var end_time = Time.get_ticks_usec()
+
+	var test_case := GDScriptTestRunner.GDScriptHardcodedTestCase.new(suite, method_name)
+	test_case.execution_time_seconds = float(end_time - start_time) / 1000000.0
+	test_case.result = (result or result == null) and not _assertion_failed
+	test_case.errors = clear_errors()
+	return test_case
+
+func clear_errors() -> Array[String]:
+	var old_errors := errors
+	errors = []
+	return old_errors

itest/rust/src/runner.rs

@@ -7,8 +7,9 @@
 use std::time::{Duration, Instant};
 
 use godot::bind::{godot_api, GodotClass};
-use godot::builtin::{ToVariant, Variant, VariantArray};
+use godot::builtin::{Array, GodotString, ToVariant, Variant, VariantArray};
 use godot::engine::Node;
+use godot::log::godot_error;
 use godot::obj::Gd;
 
 use crate::{RustTestCase, TestContext};
@@ -57,8 +58,8 @@ impl IntegrationTests {
         self.run_rust_tests(rust_tests, scene_tree);
         let rust_time = clock.elapsed();
         let gdscript_time = if !focus_run {
-            self.run_gdscript_tests(gdscript_tests);
-            Some(clock.elapsed() - rust_time)
+            let extra_duration = self.run_gdscript_tests(gdscript_tests);
+            Some((clock.elapsed() - rust_time) + extra_duration)
         } else {
             None
         };
@@ -79,22 +80,31 @@ impl IntegrationTests {
         }
     }
 
-    fn run_gdscript_tests(&mut self, tests: VariantArray) {
+    fn run_gdscript_tests(&mut self, tests: VariantArray) -> Duration {
         let mut last_file = None;
+        let mut extra_duration = Duration::new(0, 0);
+
         for test in tests.iter_shared() {
             let test_file = get_property(&test, "suite_name");
             let test_case = get_property(&test, "method_name");
 
             print_test_pre(&test_case, test_file, &mut last_file, true);
             let result = test.call("run", &[]);
+            if let Some(duration) = get_execution_time(&test) {
+                extra_duration += duration;
+            }
             let success = result.try_to::<bool>().unwrap_or_else(|_| {
                 panic!("GDScript test case {test} returned non-bool: {result}")
             });
+            for error in get_errors(&test).iter_shared() {
+                godot_error!("{error}");
+            }
             let outcome = TestOutcome::from_bool(success);
 
             self.update_stats(&outcome);
             print_test_post(&test_case, outcome);
         }
+        extra_duration
     }
 
     fn conclude(
@@ -224,6 +234,20 @@ fn get_property(test: &Variant, property: &str) -> String {
     test.call("get", &[property.to_variant()]).to::<String>()
 }
 
+fn get_execution_time(test: &Variant) -> Option<Duration> {
+    let seconds = test
+        .call("get", &["execution_time_seconds".to_variant()])
+        .try_to::<f64>()
+        .ok()?;
+    Some(Duration::from_secs_f64(seconds))
+}
+
+fn get_errors(test: &Variant) -> Array<GodotString> {
+    test.call("get", &["errors".to_variant()])
+        .try_to::<Array<GodotString>>()
+        .unwrap_or(Array::new())
+}
+
 #[must_use]
 enum TestOutcome {
     Passed,