Skip to content

Commit 7482941

Browse files
BromeonBromeon
authored
Merge pull request #660 from lilizoey/fix/object-return-ub
Fix objects not getting initialized properly when returned from ffi
2 parents e7cc362 + d1051ea commit 7482941

File tree

16 files changed

+86
-140
lines changed

16 files changed

+86
-140
lines changed

‎godot-codegen/src/generator/builtins.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -215,7 +215,7 @@ fn make_builtin_method_definition(
215215
let ptrcall_invocation = quote! {
216216
let method_bind = sys::builtin_method_table().#fptr_access;
217217

218-
<CallSig as PtrcallSignatureTuple>::out_builtin_ptrcall::<RetMarshal>(
218+
<CallSig as PtrcallSignatureTuple>::out_builtin_ptrcall(
219219
method_bind,
220220
#builtin_name_str,
221221
#method_name_str,

‎godot-codegen/src/generator/classes.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -439,7 +439,7 @@ fn make_class_method_definition(
439439
let ptrcall_invocation = quote! {
440440
let method_bind = sys::#get_method_table().#fptr_access;
441441

442-
<CallSig as PtrcallSignatureTuple>::out_class_ptrcall::<RetMarshal>(
442+
<CallSig as PtrcallSignatureTuple>::out_class_ptrcall(
443443
method_bind,
444444
#rust_class_name,
445445
#rust_method_name,

‎godot-codegen/src/generator/functions_common.rs

-10
Original file line numberDiff line numberDiff line change
@@ -220,23 +220,13 @@ pub fn make_function_definition(
220220
// Always ptrcall, no varargs
221221

222222
let ptrcall_invocation = &code.ptrcall_invocation;
223-
let maybe_return_ty = &sig.return_value().type_;
224-
225-
// This differentiation is needed because we need to differentiate between Option<Gd<T>>, T and () as return types.
226-
// Rust traits don't provide specialization and thus would encounter overlapping blanket impls, so we cannot use the type system here.
227-
let ret_marshal = match maybe_return_ty {
228-
Some(RustTy::EngineClass { tokens, .. }) => quote! { PtrcallReturnOptionGdT<#tokens> },
229-
Some(return_ty) => quote! { PtrcallReturnT<#return_ty> },
230-
None => quote! { PtrcallReturnUnit },
231-
};
232223

233224
quote! {
234225
#maybe_safety_doc
235226
#vis #maybe_unsafe fn #primary_fn_name(
236227
#receiver_param
237228
#( #params, )*
238229
) #return_decl {
239-
type RetMarshal = #ret_marshal;
240230
type CallSig = #call_sig;
241231

242232
let args = (#( #arg_names, )*);

‎godot-codegen/src/util.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ pub fn make_imports() -> TokenStream {
2222
quote! {
2323
use godot_ffi as sys;
2424
use crate::builtin::*;
25-
use crate::builtin::meta::{ClassName, PtrcallReturnUnit, PtrcallReturnT, PtrcallReturnOptionGdT, PtrcallSignatureTuple, VarcallSignatureTuple};
25+
use crate::builtin::meta::{ClassName, PtrcallSignatureTuple, VarcallSignatureTuple};
2626
use crate::engine::native::*;
2727
use crate::engine::Object;
2828
use crate::obj::Gd;

‎godot-core/src/builtin/callable.rs

+2-2
Original file line numberDiff line numberDiff line change
@@ -303,9 +303,9 @@ unsafe impl GodotFfi for Callable {
303303
fn move_return_ptr;
304304
}
305305

306-
fn new_with_init(init_fn: impl FnOnce(&mut Self)) -> Self {
306+
unsafe fn new_with_init(init_fn: impl FnOnce(sys::GDExtensionTypePtr)) -> Self {
307307
let mut result = Self::invalid();
308-
init_fn(&mut result);
308+
init_fn(result.sys_mut());
309309
result
310310
}
311311
}

‎godot-core/src/builtin/meta/mod.rs

-3
Original file line numberDiff line numberDiff line change
@@ -10,15 +10,12 @@ pub mod registration;
1010
mod call_error;
1111
mod class_name;
1212
mod godot_convert;
13-
mod return_marshal;
1413
mod signature;
1514

1615
pub use call_error::*;
1716
pub use class_name::*;
1817
pub use godot_convert::*;
1918
#[doc(hidden)]
20-
pub use return_marshal::*;
21-
#[doc(hidden)]
2219
pub use signature::*;
2320

2421
pub(crate) use godot_convert::convert_error::*;

‎godot-core/src/builtin/meta/return_marshal.rs

-78
This file was deleted.

‎godot-core/src/builtin/meta/signature.rs

+18-8
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,7 @@ pub trait PtrcallSignatureTuple {
8888
call_type: sys::PtrcallType,
8989
);
9090

91-
unsafe fn out_class_ptrcall<Rr: PtrcallReturn<Ret = Self::Ret>>(
91+
unsafe fn out_class_ptrcall(
9292
method_bind: ClassMethodBind,
9393
// Separate parameters to reduce tokens in generated class API.
9494
class_name: &'static str,
@@ -98,7 +98,7 @@ pub trait PtrcallSignatureTuple {
9898
args: Self::Params,
9999
) -> Self::Ret;
100100

101-
unsafe fn out_builtin_ptrcall<Rr: PtrcallReturn<Ret = Self::Ret>>(
101+
unsafe fn out_builtin_ptrcall(
102102
builtin_fn: BuiltinMethodBind,
103103
// Separate parameters to reduce tokens in generated class API.
104104
class_name: &'static str,
@@ -306,7 +306,7 @@ macro_rules! impl_varcall_signature_for_tuple {
306306
type_ptrs.extend(varargs.iter().map(sys::GodotFfi::sys));
307307

308308
// Important: this calls from_sys_init_default().
309-
let result = PtrcallReturnT::<$R>::call(|return_ptr| {
309+
let result = new_from_ptrcall::<Self::Ret>(|return_ptr| {
310310
utility_fn(return_ptr, type_ptrs.as_ptr(), type_ptrs.len() as i32);
311311
});
312312
result.unwrap_or_else(|err| return_error::<Self::Ret>(&call_ctx, err))
@@ -360,7 +360,7 @@ macro_rules! impl_ptrcall_signature_for_tuple {
360360
}
361361

362362
#[inline]
363-
unsafe fn out_class_ptrcall<Rr: PtrcallReturn<Ret = Self::Ret>>(
363+
unsafe fn out_class_ptrcall(
364364
method_bind: ClassMethodBind,
365365
// Separate parameters to reduce tokens in generated class API.
366366
class_name: &'static str,
@@ -391,14 +391,14 @@ macro_rules! impl_ptrcall_signature_for_tuple {
391391
)*
392392
];
393393

394-
let result = Rr::call(|return_ptr| {
394+
let result = new_from_ptrcall::<Self::Ret>(|return_ptr| {
395395
class_fn(method_bind.0, object_ptr, type_ptrs.as_ptr(), return_ptr);
396396
});
397397
result.unwrap_or_else(|err| return_error::<Self::Ret>(&call_ctx, err))
398398
}
399399

400400
#[inline]
401-
unsafe fn out_builtin_ptrcall<Rr: PtrcallReturn<Ret = Self::Ret>>(
401+
unsafe fn out_builtin_ptrcall(
402402
builtin_fn: BuiltinMethodBind,
403403
// Separate parameters to reduce tokens in generated class API.
404404
class_name: &'static str,
@@ -422,7 +422,7 @@ macro_rules! impl_ptrcall_signature_for_tuple {
422422
)*
423423
];
424424

425-
let result = Rr::call(|return_ptr| {
425+
let result = new_from_ptrcall::<Self::Ret>(|return_ptr| {
426426
builtin_fn(type_ptr, type_ptrs.as_ptr(), return_ptr, type_ptrs.len() as i32);
427427
});
428428
result.unwrap_or_else(|err| return_error::<Self::Ret>(&call_ctx, err))
@@ -450,7 +450,7 @@ macro_rules! impl_ptrcall_signature_for_tuple {
450450
)*
451451
];
452452

453-
let result = PtrcallReturnT::<$R>::call(|return_ptr| {
453+
let result = new_from_ptrcall::<Self::Ret>(|return_ptr| {
454454
utility_fn(return_ptr, arg_ptrs.as_ptr(), arg_ptrs.len() as i32);
455455
});
456456
result.unwrap_or_else(|err| return_error::<Self::Ret>(&call_ctx, err))
@@ -562,6 +562,16 @@ impl<T: FromGodot> FromVariantIndirect for T {
562562
}
563563
}
564564

565+
unsafe fn new_from_ptrcall<T: FromGodot>(
566+
process_return_ptr: impl FnOnce(sys::GDExtensionTypePtr),
567+
) -> Result<T, ConvertError> {
568+
let ffi = <<T::Via as GodotType>::Ffi as sys::GodotFfi>::new_with_init(|return_ptr| {
569+
process_return_ptr(return_ptr)
570+
});
571+
572+
T::Via::try_from_ffi(ffi).and_then(T::try_from_godot)
573+
}
574+
565575
// ----------------------------------------------------------------------------------------------------------------------------------------------
566576
// Poor man's variadic templates.
567577
// For example, RenderingServer::environment_set_volumetric_fog() has 14 parameters. We may need to extend this if the API adds more such methods.

‎godot-core/src/builtin/plane.rs

+2-2
Original file line numberDiff line numberDiff line change
@@ -276,9 +276,9 @@ unsafe impl GodotFfi for Plane {
276276
fn move_return_ptr;
277277
}
278278

279-
fn new_with_init(init: impl FnOnce(&mut Self)) -> Self {
279+
unsafe fn new_with_init(init: impl FnOnce(sys::GDExtensionTypePtr)) -> Self {
280280
let mut plane = Plane::new(Vector3::UP, 0.0);
281-
init(&mut plane);
281+
init(plane.sys_mut());
282282
plane
283283
}
284284
}

‎godot-core/src/builtin/rid.rs

+2-2
Original file line numberDiff line numberDiff line change
@@ -132,9 +132,9 @@ unsafe impl GodotFfi for Rid {
132132
fn move_return_ptr;
133133
}
134134

135-
fn new_with_init(init: impl FnOnce(&mut Self)) -> Self {
135+
unsafe fn new_with_init(init: impl FnOnce(sys::GDExtensionTypePtr)) -> Self {
136136
let mut rid = Self::Invalid;
137-
init(&mut rid);
137+
init(rid.sys_mut());
138138
rid
139139
}
140140
}

‎godot-core/src/builtin/signal.rs

+2-2
Original file line numberDiff line numberDiff line change
@@ -170,9 +170,9 @@ unsafe impl GodotFfi for Signal {
170170
fn move_return_ptr;
171171
}
172172

173-
fn new_with_init(init_fn: impl FnOnce(&mut Self)) -> Self {
173+
unsafe fn new_with_init(init_fn: impl FnOnce(sys::GDExtensionTypePtr)) -> Self {
174174
let mut result = Self::invalid();
175-
init_fn(&mut result);
175+
init_fn(result.sys_mut());
176176
result
177177
}
178178
}

‎godot-core/src/builtin/variant/mod.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -246,7 +246,7 @@ impl Variant {
246246
init_fn: impl FnOnce(sys::GDExtensionVariantPtr),
247247
) -> Self {
248248
// SAFETY: We're in Godot 4.0, and so the caller must ensure this is safe.
249-
Self::new_with_var_init(|value| init_fn(value.var_sys_mut()))
249+
unsafe { Self::new_with_var_init(init_fn) }
250250
}
251251

252252
/// # Safety

‎godot-core/src/log.rs

+4-4
Original file line numberDiff line numberDiff line change
@@ -126,8 +126,8 @@ pub fn print(varargs: &[Variant]) {
126126
args.extend(varargs.iter().map(Variant::sys));
127127

128128
let args_ptr = args.as_ptr();
129-
let _variant = Variant::new_with_init(|return_var| {
130-
call_fn(return_var.sys_mut(), args_ptr, args.len() as i32);
129+
let _variant = Variant::new_with_init(|return_ptr| {
130+
call_fn(return_ptr, args_ptr, args.len() as i32);
131131
});
132132
}
133133

@@ -150,8 +150,8 @@ pub fn print_rich(varargs: &[Variant]) {
150150
args.extend(varargs.iter().map(Variant::sys));
151151

152152
let args_ptr = args.as_ptr();
153-
let _variant = Variant::new_with_init(|return_var| {
154-
call_fn(return_var.sys_mut(), args_ptr, args.len() as i32);
153+
let _variant = Variant::new_with_init(|return_ptr| {
154+
call_fn(return_ptr, args_ptr, args.len() as i32);
155155
});
156156
}
157157

‎godot-core/src/obj/raw.rs

+5-11
Original file line numberDiff line numberDiff line change
@@ -52,8 +52,7 @@ impl<T: GodotClass> RawGd<T> {
5252
let rtti = if obj.is_null() {
5353
None
5454
} else {
55-
let raw_id =
56-
unsafe { interface_fn!(object_get_instance_id)(obj as sys::GDExtensionObjectPtr) };
55+
let raw_id = unsafe { interface_fn!(object_get_instance_id)(obj) };
5756

5857
let instance_id = InstanceId::try_from_u64(raw_id)
5958
.expect("constructed RawGd weak pointer with instance ID 0");
@@ -64,7 +63,7 @@ impl<T: GodotClass> RawGd<T> {
6463
};
6564

6665
Self {
67-
obj: obj as *mut T,
66+
obj: obj.cast::<T>(),
6867
cached_rtti: rtti,
6968
}
7069
}
@@ -460,14 +459,9 @@ where
460459
Self::from_obj_sys_weak(obj)
461460
}
462461

463-
fn new_with_init(init: impl FnOnce(&mut Self)) -> Self {
464-
let mut obj = Self {
465-
obj: std::ptr::null_mut(),
466-
cached_rtti: None,
467-
};
468-
469-
init(&mut obj);
470-
obj
462+
unsafe fn new_with_init(init: impl FnOnce(sys::GDExtensionTypePtr)) -> Self {
463+
// `new_with_uninit` creates an initialized pointer.
464+
Self::new_with_uninit(|return_ptr| init(sys::SysPtr::force_init(return_ptr)))
471465
}
472466

473467
fn sys(&self) -> sys::GDExtensionConstTypePtr {

0 commit comments

Comments
 (0)