Skip to content

Commit 0d4670f

Browse files
BromeonBromeon
authored
Merge pull request #417 from godot-rust/feature/object-validity
Ensure object validity in class method calls
2 parents e5a3087 + 563b574 commit 0d4670f

File tree

8 files changed

+196
-102
lines changed

8 files changed

+196
-102
lines changed

‎.github/composite/godot-itest/action.yml

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -179,10 +179,12 @@ runs:
179179
# * grep -q: no output, use exit code 0 if found -> thus also &&
180180
# * pkill: stop Godot execution (since it hangs in headless mode); simple 'head -1' did not work as expected
181181
# * exit: the terminated process would return 143, but this is more explicit and future-proof
182+
#
183+
# --disallow-focus: fail if #[itest(focus)] is encountered, to prevent running only a few tests for full CI
182184
run: |
183185
cd itest/godot
184186
echo "OUTCOME=itest" >> $GITHUB_ENV
185-
$GODOT4_BIN --headless ${{ inputs.godot-args }} 2>&1 \
187+
$GODOT4_BIN --headless -- --disallow-focus ${{ inputs.godot-args }} 2>&1 \
186188
| tee "${{ runner.temp }}/log.txt" \
187189
| tee >(grep -E "SCRIPT ERROR:|Can't open dynamic library" -q && {
188190
printf "\n::error::godot-itest: unrecoverable Godot error, abort...\n";

‎.github/workflows/full-ci.yml

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -153,8 +153,6 @@ jobs:
153153
# Naming: {os}[-{runtimeVersion}]-{apiVersion}
154154
# runtimeVersion = version of Godot binary; apiVersion = version of GDExtension API against which gdext is compiled.
155155

156-
# --disallow-focus: fail if #[itest(focus)] is encountered, to prevent running only a few tests for full CI
157-
158156
# Order this way because macOS typically has the longest duration, followed by Windows, so it benefits total workflow execution time.
159157
# Additionally, the 'linux (msrv *)' special case will then be listed next to the other 'linux' jobs.
160158
# Note: Windows uses '--target x86_64-pc-windows-msvc' by default as Cargo argument.
@@ -253,7 +251,6 @@ jobs:
253251
os: ubuntu-20.04
254252
artifact-name: linux-memcheck-clang-nightly
255253
godot-binary: godot.linuxbsd.editor.dev.x86_64.llvm.san
256-
godot-args: -- --disallow-focus
257254
rust-toolchain: nightly
258255
rust-env-rustflags: -Zrandomize-layout -Zsanitizer=address
259256
rust-extra-args: --features godot/custom-godot
@@ -264,7 +261,6 @@ jobs:
264261
os: ubuntu-20.04
265262
artifact-name: linux-memcheck-clang-4.0.4
266263
godot-binary: godot.linuxbsd.editor.dev.x86_64.llvm.san
267-
godot-args: -- --disallow-focus
268264
godot-prebuilt-patch: '4.0.4'
269265
rust-toolchain: nightly
270266
rust-env-rustflags: -Zrandomize-layout -Zsanitizer=address
@@ -279,7 +275,7 @@ jobs:
279275
with:
280276
artifact-name: godot-${{ matrix.artifact-name }}
281277
godot-binary: ${{ matrix.godot-binary }}
282-
godot-args: ${{ matrix.godot-args }}
278+
godot-args: ${{ matrix.godot-args }} # currently unused
283279
godot-prebuilt-patch: ${{ matrix.godot-prebuilt-patch }}
284280
rust-extra-args: ${{ matrix.rust-extra-args }} --features godot/codegen-full
285281
rust-toolchain: ${{ matrix.rust-toolchain || 'stable' }}

‎.github/workflows/minimal-ci.yml

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -165,7 +165,6 @@ jobs:
165165
os: ubuntu-20.04
166166
artifact-name: linux-memcheck-clang-nightly
167167
godot-binary: godot.linuxbsd.editor.dev.x86_64.llvm.san
168-
godot-args: -- --disallow-focus
169168
rust-toolchain: nightly
170169
rust-env-rustflags: -Zrandomize-layout -Zsanitizer=address
171170
rust-extra-args: --features godot/custom-godot
@@ -176,7 +175,6 @@ jobs:
176175
os: ubuntu-20.04
177176
artifact-name: linux-memcheck-clang-4.0.4
178177
godot-binary: godot.linuxbsd.editor.dev.x86_64.llvm.san
179-
godot-args: -- --disallow-focus
180178
godot-prebuilt-patch: '4.0.4'
181179
rust-toolchain: nightly
182180
rust-env-rustflags: -Zrandomize-layout -Zsanitizer=address
@@ -191,7 +189,7 @@ jobs:
191189
with:
192190
artifact-name: godot-${{ matrix.artifact-name }}
193191
godot-binary: ${{ matrix.godot-binary }}
194-
godot-args: ${{ matrix.godot-args }}
192+
godot-args: ${{ matrix.godot-args }} # currently unused
195193
godot-prebuilt-patch: ${{ matrix.godot-prebuilt-patch }}
196194
rust-extra-args: ${{ matrix.rust-extra-args }}
197195
rust-toolchain: ${{ matrix.rust-toolchain || 'stable' }}

‎godot-codegen/src/class_generator.rs

Lines changed: 27 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -468,10 +468,9 @@ fn make_constructor(class: &Class, ctx: &Context) -> TokenStream {
468468
quote! {
469469
pub fn new() -> Gd<Self> {
470470
unsafe {
471-
let __class_name = #godot_class_stringname;
472-
let __object_ptr = sys::interface_fn!(classdb_construct_object)(__class_name.string_sys());
473-
//let instance = Self { object_ptr };
474-
Gd::from_obj_sys(__object_ptr)
471+
let class_name = #godot_class_stringname;
472+
let object_ptr = sys::interface_fn!(classdb_construct_object)(class_name.string_sys());
473+
Gd::from_obj_sys(object_ptr)
475474
}
476475
}
477476
}
@@ -481,9 +480,9 @@ fn make_constructor(class: &Class, ctx: &Context) -> TokenStream {
481480
#[must_use]
482481
pub fn new_alloc() -> Gd<Self> {
483482
unsafe {
484-
let __class_name = #godot_class_stringname;
485-
let __object_ptr = sys::interface_fn!(classdb_construct_object)(__class_name.string_sys());
486-
Gd::from_obj_sys(__object_ptr)
483+
let class_name = #godot_class_stringname;
484+
let object_ptr = sys::interface_fn!(classdb_construct_object)(class_name.string_sys());
485+
Gd::from_obj_sys(object_ptr)
487486
}
488487
}
489488
}
@@ -602,9 +601,10 @@ fn make_class(class: &Class, class_name: &TyName, ctx: &mut Context) -> Generate
602601

603602
#[doc = #class_doc]
604603
#[derive(Debug)]
605-
#[repr(transparent)]
604+
#[repr(C)]
606605
pub struct #class_name {
607606
object_ptr: sys::GDExtensionObjectPtr,
607+
instance_id: crate::obj::InstanceId,
608608
}
609609
#virtual_trait
610610
#notification_enum
@@ -1107,26 +1107,33 @@ fn make_method_definition(
11071107
method_name: method.name.clone(),
11081108
});
11091109

1110-
let receiver_ffi_arg = &receiver.ffi_arg;
1110+
let maybe_instance_id = if method.is_static {
1111+
quote! { None }
1112+
} else {
1113+
quote! { Some(self.instance_id) }
1114+
};
1115+
1116+
let object_ptr = &receiver.ffi_arg;
11111117
let ptrcall_invocation = quote! {
11121118
let method_bind = sys::#get_method_table().fptr_by_index(#table_index);
1113-
let object_ptr = #receiver_ffi_arg;
11141119

11151120
<CallSig as PtrcallSignatureTuple>::out_class_ptrcall::<RetMarshal>(
11161121
method_bind,
1117-
object_ptr,
1118-
args
1122+
#method_name_str,
1123+
#object_ptr,
1124+
#maybe_instance_id,
1125+
args,
11191126
)
11201127
};
11211128

11221129
let varcall_invocation = quote! {
11231130
let method_bind = sys::#get_method_table().fptr_by_index(#table_index);
1124-
let type_ptr = #receiver_ffi_arg;
11251131

11261132
<CallSig as VarcallSignatureTuple>::out_class_varcall(
11271133
method_bind,
11281134
#method_name_str,
1129-
type_ptr,
1135+
#object_ptr,
1136+
#maybe_instance_id,
11301137
args,
11311138
varargs
11321139
)
@@ -1174,27 +1181,27 @@ fn make_builtin_method_definition(
11741181
});
11751182

11761183
let receiver = make_receiver(method.is_static, method.is_const, quote! { self.sys_ptr });
1177-
let receiver_ffi_arg = &receiver.ffi_arg;
1184+
let object_ptr = &receiver.ffi_arg;
11781185

11791186
let ptrcall_invocation = quote! {
11801187
let method_bind = sys::builtin_method_table().fptr_by_index(#table_index);
1181-
let object_ptr = #receiver_ffi_arg;
11821188

11831189
<CallSig as PtrcallSignatureTuple>::out_builtin_ptrcall::<RetMarshal>(
11841190
method_bind,
1185-
object_ptr,
1191+
#object_ptr,
11861192
args
11871193
)
11881194
};
11891195

1196+
// TODO(#382): wait for https://github.com/godot-rust/gdext/issues/382
11901197
let varcall_invocation = quote! {
1191-
<CallSig as VarcallSignatureTuple>::out_class_varcall(
1198+
/*<CallSig as VarcallSignatureTuple>::out_class_varcall(
11921199
method_bind,
11931200
#method_name_str,
1194-
object_ptr,
1201+
#object_ptr,
11951202
args,
11961203
varargs
1197-
)
1204+
)*/
11981205
};
11991206

12001207
make_function_definition(

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

Lines changed: 24 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -4,11 +4,16 @@
44
* file, You can obtain one at https://mozilla.org/MPL/2.0/.
55
*/
66

7-
use godot_ffi as sys;
87
use std::fmt::Debug;
98

9+
use godot_ffi as sys;
1010
use sys::{BuiltinMethodBind, ClassMethodBind, UtilityFunctionBind};
1111

12+
use crate::builtin::meta::*;
13+
//use crate::builtin::meta::MethodParamOrReturnInfo;
14+
use crate::builtin::{FromVariant, ToVariant, Variant};
15+
use crate::obj::InstanceId;
16+
1217
#[doc(hidden)]
1318
pub trait VarcallSignatureTuple: PtrcallSignatureTuple {
1419
const PARAM_COUNT: usize;
@@ -33,6 +38,7 @@ pub trait VarcallSignatureTuple: PtrcallSignatureTuple {
3338
method_bind: sys::GDExtensionMethodBindPtr,
3439
method_name: &'static str,
3540
object_ptr: sys::GDExtensionObjectPtr,
41+
maybe_instance_id: Option<InstanceId>, // if not static
3642
args: Self::Params,
3743
varargs: &[Variant],
3844
) -> Self::Ret;
@@ -58,13 +64,15 @@ pub trait PtrcallSignatureTuple {
5864
args_ptr: *const sys::GDExtensionConstTypePtr,
5965
ret: sys::GDExtensionTypePtr,
6066
func: fn(sys::GDExtensionClassInstancePtr, Self::Params) -> Self::Ret,
61-
method_name: &str,
67+
method_name: &'static str,
6268
call_type: sys::PtrcallType,
6369
);
6470

6571
unsafe fn out_class_ptrcall<Rr: PtrcallReturn<Ret = Self::Ret>>(
6672
method_bind: sys::GDExtensionMethodBindPtr,
73+
method_name: &'static str,
6774
object_ptr: sys::GDExtensionObjectPtr,
75+
maybe_instance_id: Option<InstanceId>, // if not static
6876
args: Self::Params,
6977
) -> Self::Ret;
7078

@@ -94,10 +102,6 @@ pub trait PtrcallSignatureTuple {
94102
// }
95103
// }
96104
//
97-
use crate::builtin::meta::*;
98-
use crate::builtin::{FromVariant, ToVariant, Variant};
99-
100-
use super::registration::method::MethodParamOrReturnInfo;
101105

102106
macro_rules! impl_varcall_signature_for_tuple {
103107
(
@@ -162,9 +166,16 @@ macro_rules! impl_varcall_signature_for_tuple {
162166
method_bind: ClassMethodBind,
163167
method_name: &'static str,
164168
object_ptr: sys::GDExtensionObjectPtr,
169+
maybe_instance_id: Option<InstanceId>, // if not static
165170
args: Self::Params,
166171
varargs: &[Variant],
167172
) -> Self::Ret {
173+
eprintln!("varcall: {method_name}");
174+
// Note: varcalls are not safe from failing, if the happen through an object pointer -> validity check necessary.
175+
if let Some(instance_id) = maybe_instance_id {
176+
crate::engine::ensure_object_alive(instance_id, object_ptr, method_name);
177+
}
178+
168179
let class_fn = sys::interface_fn!(object_method_bind_call);
169180

170181
let explicit_args = [
@@ -200,7 +211,6 @@ macro_rules! impl_varcall_signature_for_tuple {
200211
args: Self::Params,
201212
varargs: &[Variant],
202213
) -> Self::Ret {
203-
204214
let explicit_args: [Variant; $PARAM_COUNT] = [
205215
$(
206216
<$Pn as ToVariant>::to_variant(&args.$n),
@@ -249,7 +259,7 @@ macro_rules! impl_ptrcall_signature_for_tuple {
249259
args_ptr: *const sys::GDExtensionConstTypePtr,
250260
ret: sys::GDExtensionTypePtr,
251261
func: fn(sys::GDExtensionClassInstancePtr, Self::Params) -> Self::Ret,
252-
method_name: &str,
262+
method_name: &'static str,
253263
call_type: sys::PtrcallType,
254264
) {
255265
// $crate::out!("ptrcall: {}", method_name);
@@ -267,9 +277,15 @@ macro_rules! impl_ptrcall_signature_for_tuple {
267277
#[inline]
268278
unsafe fn out_class_ptrcall<Rr: PtrcallReturn<Ret = Self::Ret>>(
269279
method_bind: ClassMethodBind,
280+
method_name: &'static str,
270281
object_ptr: sys::GDExtensionObjectPtr,
282+
maybe_instance_id: Option<InstanceId>, // if not static
271283
args: Self::Params,
272284
) -> Self::Ret {
285+
if let Some(instance_id) = maybe_instance_id {
286+
crate::engine::ensure_object_alive(instance_id, object_ptr, method_name);
287+
}
288+
273289
let class_fn = sys::interface_fn!(object_method_bind_ptrcall);
274290

275291
#[allow(clippy::let_unit_value)]

‎godot-core/src/engine.rs

Lines changed: 28 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,12 +9,14 @@
99
// Re-exports of generated symbols
1010
use crate::builtin::{GodotString, NodePath};
1111
use crate::obj::dom::EngineDomain;
12-
use crate::obj::{Gd, GodotClass, Inherits};
12+
use crate::obj::{Gd, GodotClass, Inherits, InstanceId};
1313

1414
pub use crate::gen::central::global;
1515
pub use crate::gen::classes::*;
1616
pub use crate::gen::utilities;
1717

18+
use crate::sys;
19+
1820
/// Support for Godot _native structures_.
1921
///
2022
/// Native structures are a niche API in Godot. These are low-level data types that are passed as pointers to/from the engine.
@@ -206,6 +208,31 @@ pub(crate) fn display_string<T: GodotClass>(
206208
<GodotString as std::fmt::Display>::fmt(&string, f)
207209
}
208210

211+
pub(crate) fn object_ptr_from_id(instance_id: InstanceId) -> sys::GDExtensionObjectPtr {
212+
// SAFETY: Godot looks up ID in ObjectDB and returns null if not found.
213+
unsafe { sys::interface_fn!(object_get_instance_from_id)(instance_id.to_u64()) }
214+
}
215+
216+
pub(crate) fn ensure_object_alive(
217+
instance_id: InstanceId,
218+
old_object_ptr: sys::GDExtensionObjectPtr,
219+
method_name: &'static str,
220+
) {
221+
let new_object_ptr = object_ptr_from_id(instance_id);
222+
223+
assert!(
224+
!new_object_ptr.is_null(),
225+
"{method_name}: access to instance with ID {instance_id} after it has been freed"
226+
);
227+
228+
// This should not happen, as reuse of instance IDs was fixed according to https://github.com/godotengine/godot/issues/32383,
229+
// namely in PR https://github.com/godotengine/godot/pull/36189. Double-check to make sure.
230+
assert_eq!(
231+
new_object_ptr, old_object_ptr,
232+
"{method_name}: instance ID {instance_id} points to a stale, reused object. Please report this to gdext maintainers."
233+
);
234+
}
235+
209236
// ----------------------------------------------------------------------------------------------------------------------------------------------
210237
// Implementation of this file
211238

0 commit comments

Comments
 (0)