From 8d9ff8b9af7d3abeb1e21d6dbbc2be50a67cab26 Mon Sep 17 00:00:00 2001 From: Jan Haller Date: Mon, 14 Nov 2022 23:36:13 +0100 Subject: [PATCH 1/2] Fix UB in Gd::from_sys_init_opt() The dereferenced type-pointer (yielding the object-ptr) was not initialized to null before usage, based on the wrong assumption that Godot would do that. Also changes the implementation to use pointers instead of opaques. --- godot-core/src/obj/gd.rs | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/godot-core/src/obj/gd.rs b/godot-core/src/obj/gd.rs index 490172260..86b076fba 100644 --- a/godot-core/src/obj/gd.rs +++ b/godot-core/src/obj/gd.rs @@ -468,23 +468,27 @@ impl GodotFfi for Gd { impl Gd { pub unsafe fn from_sys_init_opt(init_fn: impl FnOnce(sys::GDNativeTypePtr)) -> Option { // Note: see _call_native_mb_ret_obj() in godot-cpp, which does things quite different (e.g. querying the instance binding). + // This method could be simplified if it would modify the object directly and not wrap from_sys_init(). // Much elegant let mut is_null = false; - let outer_fn = |ptr| { - init_fn(ptr); + let outer_fn = |return_ptr| { + // ptr has type GDNativeTypePtr = GDNativeObjectPtr* = OpaqueObject* = Object** + // (in other words, the opaque struct encodes an Object*; or, the type-ptr contains the _address_ of an object-ptr) + let object_ptr_ptr = return_ptr as *mut sys::GDNativeObjectPtr; + *object_ptr_ptr = ptr::null_mut(); + + init_fn(return_ptr); - // ptr has type TypePtr = OpaqueObject* = Object** (in other words, the opaque encodes an Object*) // we don't need to know if Object** is null, but if Object* (modified through Object**) is null. - let opaque_ptr = ptr as *const OpaqueObject; - if is_zeroed(*opaque_ptr) { + if (*object_ptr_ptr).is_null() { is_null = true; } }; - // TODO forget, ready etc let gd = Self::from_sys_init(outer_fn); if is_null { + std::mem::forget(gd); // null Gd is not a valid state which destructor should handle None } else { Some(gd) @@ -492,10 +496,6 @@ impl Gd { } } -fn is_zeroed(opaque: OpaqueObject) -> bool { - unsafe { std::mem::transmute::(opaque) == 0 } -} - /// Destructor with semantics depending on memory strategy. /// /// * If this `Gd` smart pointer holds a reference-counted type, this will decrement the reference counter. From 4924b4408312de1c48d84fa22245f573105a5ab0 Mon Sep 17 00:00:00 2001 From: Jan Haller Date: Tue, 15 Nov 2022 00:56:58 +0100 Subject: [PATCH 2/2] Simplify Gd::from_sys_init_opt() -- from_obj_sys() instead of from_sys_init() --- godot-core/src/obj/gd.rs | 28 +++++++++------------------- 1 file changed, 9 insertions(+), 19 deletions(-) diff --git a/godot-core/src/obj/gd.rs b/godot-core/src/obj/gd.rs index 86b076fba..b44603528 100644 --- a/godot-core/src/obj/gd.rs +++ b/godot-core/src/obj/gd.rs @@ -468,30 +468,20 @@ impl GodotFfi for Gd { impl Gd { pub unsafe fn from_sys_init_opt(init_fn: impl FnOnce(sys::GDNativeTypePtr)) -> Option { // Note: see _call_native_mb_ret_obj() in godot-cpp, which does things quite different (e.g. querying the instance binding). - // This method could be simplified if it would modify the object directly and not wrap from_sys_init(). - // Much elegant - let mut is_null = false; - let outer_fn = |return_ptr| { - // ptr has type GDNativeTypePtr = GDNativeObjectPtr* = OpaqueObject* = Object** - // (in other words, the opaque struct encodes an Object*; or, the type-ptr contains the _address_ of an object-ptr) - let object_ptr_ptr = return_ptr as *mut sys::GDNativeObjectPtr; - *object_ptr_ptr = ptr::null_mut(); + // return_ptr has type GDNativeTypePtr = GDNativeObjectPtr* = OpaqueObject* = Object** + // (in other words, the type-ptr contains the _address_ of an object-ptr). + let mut object_ptr: sys::GDNativeObjectPtr = ptr::null_mut(); + let return_ptr: *mut sys::GDNativeObjectPtr = ptr::addr_of_mut!(object_ptr); - init_fn(return_ptr); + init_fn(return_ptr as sys::GDNativeTypePtr); - // we don't need to know if Object** is null, but if Object* (modified through Object**) is null. - if (*object_ptr_ptr).is_null() { - is_null = true; - } - }; - - let gd = Self::from_sys_init(outer_fn); - if is_null { - std::mem::forget(gd); // null Gd is not a valid state which destructor should handle + // We don't need to know if Object** is null, but if Object* is null; return_ptr has the address of a local (never null). + if object_ptr.is_null() { None } else { - Some(gd) + let obj = Gd::from_obj_sys(object_ptr); // equivalent to Gd::from_sys(return_ptr) + Some(obj) } } }