Add "validate_property" virtual func #1030

Yarwin · 2025-01-31T22:30:42Z

Adds "validate property" virtual func, modeled after godot-cpp implementation: godotengine/godot#81515 with proper test (can be moved to property_test.rs? 🤔).

The procedure looks as follows:

Receive shared *mut sys::GDExtensionPropertyInfo from Godot
Create PropertyInfo out of it and expose it to user from modification
Move constructed&modified PropertyInfo back to *mut sys::GDExtensionPropertyInfo.

Bromeon

Thank you! 🙂

Do we actually need the "move into sys::GDExtensionPropertyInfo" part? This mainly brings benefits for uninitialized storage, or performance-critical paths. But here, we could probably just use the existing PropertyInfo::property_sys() and assign it?

    pub(crate) unsafe fn move_into_property_info_ptr(
        self,
        property_info_ptr: *mut sys::GDExtensionPropertyInfo,
    ) {
        *property_info_ptr = self.property_sys();
        ...
    }

(Maybe I'm also missing something)

Bromeon · 2025-02-01T18:55:05Z

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

@@ -143,6 +143,17 @@ impl StringName {
        *boxed
    }

+    /// Moves this string into a string name sys pointer. This is the same as using [`GodotFfi::move_return_ptr`].


Suggested change

/// Moves this string into a string name sys pointer. This is the same as using [`GodotFfi::move_return_ptr`].

/// Moves this string into a `StringName` sys pointer. This is the same as using [`GodotFfi::move_return_ptr()`].

Bromeon · 2025-02-01T18:55:20Z

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

+    ///
+    /// # Safety
+    ///
+    /// `dst` must be a valid string name pointer.


Suggested change

/// `dst` must be a valid string name pointer.

/// `dst` must be a valid `StringName` pointer.

Bromeon · 2025-02-01T18:57:51Z

godot-core/src/registry/class.rs

+            // Not present in GdExtension
+            #[cfg(since_api = "4.2")]
+            {
+                c.godot_params.validate_property_func = validate_property_fn;
+            }


Suggested change

// Not present in GdExtension

#[cfg(since_api = "4.2")]

{

c.godot_params.validate_property_func = validate_property_fn;

}

// Not present in GDExtension.

#[cfg(since_api = "4.2")]

c.godot_params.validate_property_func = validate_property_fn;

Sadly attributes on expressions are experimental: rust-lang/rust#15701

Bromeon · 2025-02-01T19:01:29Z

godot-core/src/meta/property_info.rs

+        let variant_type = VariantType::from_sys((*property_info_ptr).type_);
+        let property_name = StringName::new_from_string_sys((*property_info_ptr).name);
+        let hint_string = GString::new_from_string_sys((*property_info_ptr).hint_string);
+        let hint = PropertyHint::from_ord((*property_info_ptr).hint.to_owned() as i32);
+        let usage = PropertyUsageFlags::from_ord((*property_info_ptr).usage as u64);


This should definitely obtain a reference once, and then use that 😉

Bromeon · 2025-02-01T19:07:37Z

godot-core/src/registry/callbacks.rs

+    let mut property_info = unsafe { PropertyInfo::new_from_sys(property_info_ptr) };
+    T::__godot_validate_property(&*instance, &mut property_info);
+    // SAFETY: property_info_ptr is still valid
+    unsafe { property_info.move_into_property_info_ptr(property_info_ptr) };


Empty line and period (.)

Suggested change

let mut property_info = unsafe { PropertyInfo::new_from_sys(property_info_ptr) };

T::__godot_validate_property(&*instance, &mut property_info);

// SAFETY: property_info_ptr is still valid

unsafe { property_info.move_into_property_info_ptr(property_info_ptr) };

let mut property_info = unsafe { PropertyInfo::new_from_sys(property_info_ptr) };

T::__godot_validate_property(&*instance, &mut property_info);

// SAFETY: property_info_ptr is still valid.

unsafe { property_info.move_into_property_info_ptr(property_info_ptr) };

Bromeon · 2025-02-01T19:08:24Z

itest/rust/src/object_tests/mod.rs

@@ -23,6 +23,9 @@ mod property_template_test;
 mod property_test;
 mod reentrant_test;
 mod singleton_test;
+// `validate_property` is only supported in Godot 4.2+


Suggested change

// `validate_property` is only supported in Godot 4.2+

// `validate_property` is only supported in Godot 4.2+.

Bromeon · 2025-02-01T19:09:20Z

itest/rust/src/object_tests/validate_property_test.rs

+        if property
+            .get("name")
+            .map(|v| v.to_string() == "SuperNewTestPropertyName")
+            .unwrap_or(false)


The .map().unwrap_or(false) pattern can often be replaced with is_some_and().

And maybe this could also use an early exit (continue) to un-indent the following code 1 level?

Bromeon · 2025-02-01T19:12:34Z

itest/rust/src/object_tests/validate_property_test.rs

+            .map(|v| v.to_string() == "SuperNewTestPropertyName")
+            .unwrap_or(false)
+        {
+            let Some(usage) = property.get("usage").map(|v| v.to::<PropertyUsageFlags>()) else {


I just wonder, can this be passed a generic function "reference"? 🧐

Suggested change

let Some(usage) = property.get("usage").map(|v| v.to::<PropertyUsageFlags>()) else {

let Some(usage) = property.get("usage").map(Variant::to::<PropertyUsageFlags>) else {

Rust is unfortunately very restrictive here, if exact deref count doesn't match, this doesn't work... Anyway just a curiosity 🙂

Yarwin · 2025-02-06T12:22:55Z

Do we actually need the "move into sys::GDExtensionPropertyInfo" part? This mainly brings benefits for uninitialized storage, or performance-critical paths. But here, we could probably just use the existing PropertyInfo::property_sys() and assign it?
    pub(crate) unsafe fn move_into_property_info_ptr(
        self,
        property_info_ptr: *mut sys::GDExtensionPropertyInfo,
    ) {
        *property_info_ptr = self.property_sys();
        ...
    }
(Maybe I'm also missing something)

I think I'm missing something too!

Long story short, this approach doesn't work and I have no idea why! I spent some time debugging what is going on with lldb and I'm still ultra clueless.

Here are my notes from debugging with lldb:

Firstly, let's check the "overwriting" approach (previous commit; godot-cpp implementation).

I put the breakpoint just before we are entering gdext library function: https://github.com/godotengine/godot/blob/master/core/object/object.cpp#L572 and checked the state of variables:

(PropertyInfo) p_property = {
  type = INT
  name = {
    _cowdata = (_ptr = U"my_var")
  }
  class_name = {
    _data = nullptr
  }
  hint = PROPERTY_HINT_NONE
  hint_string = {
    _cowdata = (_ptr = U"initial")
  }
  usage = 6
}
(GDExtensionPropertyInfo) gdext_prop = (type = GDEXTENSION_VARIANT_TYPE_INT, name = 0x00007fffffff6780, class_name = 0x00007fffffff6860, hint = 0, hint_string = 0x00007fffffff6870, usage = 6)

hint_string points to some solid object on the Godot's side. name is created from prop_name.name. - its data param points directly to our prop_name.name (thus can be overwritten without any risks or thoughts).

(lldb) expr *prop_name._data
(StringName::_Data) $60 = {
  refcount = {
    count = {
      value = {
        std::__atomic_base<unsigned int> = (_M_i = 3)
      }
    }
  }
  static_count = {
    value = {
      std::__atomic_base<unsigned int> = (_M_i = 0)
    }
  }
  cname = 0x0000000000000000
  name = {
    _cowdata = (_ptr = U"my_var")
  }
  debug_references = 0
  idx = 307
  hash = 249889075
  prev = nullptr
  next = 0x00005555627fcce0
}

One can check refcount for all the refcounted types using their _cowdata._ptr. The information about memory alignment for this data structure is layed down here: https://github.com/godotengine/godot/blob/f0f5319b0b5b56db9f8023ac4132088df9e86b9e/core/templates/cowdata.h#L101, and can be invoked via lldb using something along the lines of:

(lldb) expr  p_property.hint_string._cowdata._get_refcount()
(SafeNumeric<unsigned long> *) $31 = 0x000055556286cb80
(lldb) expr *(int *) p_property.hint_string._cowdata._get_refcount()
(int) $29 = 3

(lldb) expr p_property.hint_string._cowdata._ptr
(char32_t *) $28 = 0x000055556286d640 U"initial"
(lldb) expr (uint8_t *) p_property.hint_string._cowdata._ptr - 16
(uint8_t *) $33 = 0x000055556286d630 "\U00000003" 
(lldb) expr *(int *)((uint8_t *) p_property.hint_string._cowdata._ptr - 16)
(int) $34 = 3

After writing our values into given addresses state of our property looks like that:

expr p_property 
(PropertyInfo) = {
  type = INT
  name = {
    _cowdata = (_ptr = U"my_var")
  }
  class_name = {
    _data = nullptr
  }
  hint = PROPERTY_HINT_NONE
  hint_string = {
    _cowdata = (_ptr = U"SomePropertyHint")
  }
  usage = 6
}

(lldb) expr *(int *)((uint8_t *) p_property.hint_string._cowdata._ptr - 16)
(int) $36 = 1 // refcount has been reduced from 3 to 1!

(lldb) expr *prop_name._data
(StringName::_Data) $62 = {
  refcount = {
    count = {
      value = {
        std::__atomic_base<unsigned int> = (_M_i = 1)
      }
    }
  }
  static_count = {
    value = {
      std::__atomic_base<unsigned int> = (_M_i = 0)
    }
  }
  cname = 0x0000000000000000
  name = {
    _cowdata = (_ptr = U"SuperNewTestPropertyName")
  }
  debug_references = 0
  idx = 17924
  hash = 295323140
  prev = nullptr
  next = nullptr
}

It seems that overwriting given values, by writing directly into its pointers, messes with refcount. Initial refcount is 2 both for hint_string and name – which means that they are being referenced in one other place while entering scope of this function.

   566  void Object::validate_property(PropertyInfo &p_property) const {
-> 567          _validate_propertyv(p_property);
   568  
   569          if (_extension && _extension->validate_property) {
   570                  // GDExtension uses a StringName rather than a String for property name.
(lldb) expr *(int *) p_property.name._cowdata._get_refcount()
(int) $67 = 2
(lldb) expr *(int *) p_property.hint_string._cowdata._get_refcount()
(int) $68 = 2
(lldb)

Now let's move to our crashing implementation. It looks like that:

   (…)
    let mut property_info = unsafe { PropertyInfo::new_from_sys(property_info_ptr) };
    T::__godot_validate_property(&*instance, &mut property_info);
    // SAFETY: property_info_ptr is still valid
    unsafe { 
    (*property_info_ptr) = property_info.property_sys(); 
std::mem::forget(property_info)
} ;

    sys::conv::SYS_TRUE

The crash in question:

   validate_property_test.rs:

================================================================
handle_crash: Program crashed with signal 11
Engine version: Godot Engine v4.4.beta.custom_build (eee39f004b42a4948af90cdebedf65c34a4a4970)

Dumping the backtrace. Please include this when reporting the bug to the project developer.
[1] /lib/x86_64-linux-gnu/libc.so.6(+0x42520) [0x7f8863c42520] (??:0)
[2] CowData<char32_t>::_ref(CowData<char32_t> const&) (/usr/include/c++/11/bits/atomic_base.h:488)
[3] String::operator=(String const&) (/core/string/ustring.h:605)
[4] Object::validate_property(PropertyInfo&) const (core/object/object.cpp:586)
[5] ClassDB::get_property_list(StringName const&, List<PropertyInfo, DefaultAllocator>*, bool, Object const*) (/core/object/class_db.cpp:1547)
[6] Object::get_property_list(List<PropertyInfo, DefaultAllocator>*, bool) const (core/object/object.cpp:512)

(GDExtensionPropertyInfo) gdext_prop = (type = GDEXTENSION_VARIANT_TYPE_INT, name = 0x00007fffffff6780, class_name = 0x00007fffffff6860, hint = 0, hint_string = 0x00007fffffff6870, usage = 6)

Our created, initial property info:

(godot_core::meta::property_info::PropertyInfo) property_info = {
  variant_type = (ord = 2)
  class_name = (global_index = 0)
  property_name = {
    opaque = (storage = "0چbUU\0", marker = core::marker::PhantomData<unsigned char *> @ 0x00007fffffff6700)
  }
  hint_info = {
    hint = (ord = 0)
    hint_string = {
      _opaque = (storage = "\0ֆbUU\0", marker = core::marker::PhantomData<unsigned char *> @ 0x00007fffffff66f0)
    }
  }
  usage = (ord = 6)
}

Opaque holds our String's _ptr:

(lldb) expr *(char32_t **) property_info.hint_info.hint_string._opaque.storage
(char32_t *) $6 = 0x000055556286d600 U"initial"

(lldb) expr *(int *)((uint8_t *) 0x000055556286d600 - 16)
(int) $13 = 4 // refcount haven't been increased – that's excepted behavior while creating this type out of the ptr!

After user's validate property, our property_info looks like that:

(godot_core::meta::property_info::PropertyInfo) $14 = {
  variant_type = (ord = 2)
  class_name = (global_index = 36)
  property_name = {
    opaque = (storage = " \xb4\xe7cUU\0", marker = core::marker::PhantomData<unsigned char *> @ 0x000000002213a048)
  }
  hint_info = {
    hint = (ord = 0)
    hint_string = {
      _opaque = (storage = "\xf0\xed\x87cUU\0", marker = core::marker::PhantomData<unsigned char *> @ 0x000000002213a038)
    }
  }
  usage = (ord = 2)
}


(lldb) expr *(char32_t **) property_info.hint_info.hint_string._opaque.storage
(char32_t *) $15 = 0x000055556387edf0 U"SomePropertyHint"
(lldb) expr *(int *) ((uint8_t *)0x000055556387edf0 - 16)
(int) $17 = 1 // refcount == 1

We write new property_sys into our GDExtensionPropertyInfo:

(godot_ffi::gen::gdextension_interface::GDExtensionPropertyInfo) $22 = {
  type_ = 2
  name = 0x00007fffffff6720
  class_name = 0x0000555562433070
  hint = 0
  hint_string = 0x00007fffffff6710
  usage = 2
}

Let's get out!

Refcount of our hint_string is still 1 and it even has good value too! I am 100% certain, that it points to valid String!

-> 580                  if (_extension->validate_property(_extension_instance, &gdext_prop)) {
   581                          p_property.type = (Variant::Type)gdext_prop.type;
   582                          p_property.name = *reinterpret_cast<StringName *>(gdext_prop.name);
   583                          p_property.class_name = *reinterpret_cast<StringName *>(gdext_prop.class_name);
(lldb) expr *(int *) ((uint8_t *)0x000055556387edf0 - 16)
(int) $23 = 1
(lldb) expr *(char32_t **) gdext_prop.hint_string
(char32_t *) $27 = 0x000055556387edf0 U"SomePropertyHint"

lmao, no

* thread #1, name = 'godot.linuxbsd.', stop reason = signal SIGSEGV: invalid address (fault address: 0xafffffff2)
    frame #0: 0x000055555889d557 godot.linuxbsd.editor.dev.x86_64`CowData<char32_t>::_ref(CowData<char32_t> const&) [inlined] std::__atomic_base<unsigned long>::load(__m=memory_order_acquire, this=0x0000000afffffff2) const at atomic_base.h:488:24
   485          __glibcxx_assert(__b != memory_order_release);
   486          __glibcxx_assert(__b != memory_order_acq_rel);
   487  
-> 488          return __atomic_load_n(&_M_i, int(__m));
   489        }
   490  
   491        _GLIBCXX_ALWAYS_INLINE __int_type

I don't know what extacly causes this crash, and navigation there is ultra hard 😬. Albeit there is one valid workaround! After changing our implementation to:

   (…)
    let mut property_info = unsafe { PropertyInfo::new_from_sys(property_info_ptr) };
    T::__godot_validate_property(&*instance, &mut property_info);
    // SAFETY: property_info_ptr is still valid
    unsafe { 
    (*property_info_ptr) = property_info.property_sys(); 
std::mem::forget(property_info);
        godot_print!("{}", GString::new_from_string_sys((*property_info_ptr).hint_string));

} ;

    sys::conv::SYS_TRUE

everything works fine! No memory leaks, no crashes, no UBs.

I'm ultra puzzled. I'll continue debugging in the near future.

Yarwin · 2025-02-07T06:23:38Z

Update – I found out what is going on. For some reason compare_exchange_weak executed here corrupts my memory

debugging log

* thread #1, name = 'godot.linuxbsd.', stop reason = step over
    frame #0: 0x000055555e21ae4c godot.linuxbsd.editor.dev.x86_64`Object::validate_property(this=0x00005555638c21a0, p_property=0x00007fffffff6850) const at object.cpp:582:65
   579                  };
   580                  if (_extension->validate_property(_extension_instance, &gdext_prop)) {
   581                          p_property.type = (Variant::Type)gdext_prop.type;
-> 582                          p_property.name = *reinterpret_cast<StringName *>(gdext_prop.name);
   583                          p_property.class_name = *reinterpret_cast<StringName *>(gdext_prop.class_name);
   584                          p_property.hint = (PropertyHint)gdext_prop.hint;
   585                          p_property.hint_string = *reinterpret_cast<String *>(gdext_prop.hint_string);
(lldb) expr *(char32_t **) gdext_prop.hint_string
(char32_t *) $18 = 0x000055556386f7b0 U"SomePropertyHint"
(lldb) expr gdext_prop 
(GDExtensionPropertyInfo) $19 = (type = GDEXTENSION_VARIANT_TYPE_INT, name = 0x00007fffffff6720, class_name = 0x0000555562432fb0, hint = 0, hint_string = 0x00007fffffff6710, usage = 2)
(…)
>step
at ustring.h:602:45
-> 602          _FORCE_INLINE_ String(const String &p_str) { _cowdata._ref(p_str._cowdata); }
(…)
>step
    frame #0: 0x000055555889d492 godot.linuxbsd.editor.dev.x86_64`CowData<char32_t>::CowData(this=0x0000555500000000) at cowdata.h:256:17
   253          Size rfind(const T &p_val, Size p_from = -1) const;
   254          Size count(const T &p_val) const;
   255  
-> 256          _FORCE_INLINE_ CowData() {}
   257          _FORCE_INLINE_ ~CowData() { _unref(); }
   258          _FORCE_INLINE_ CowData(std::initializer_list<T> p_init);
   259          _FORCE_INLINE_ CowData(const CowData<T> &p_from) { _ref(p_from); }
(…)
(…)
* thread #1, name = 'godot.linuxbsd.', stop reason = step in
    frame #0: 0x000055555889d522 godot.linuxbsd.editor.dev.x86_64`CowData<char32_t>::_ref(this=0x00007fffffff6788, p_from=0x0000555564211200) at cowdata.h:502:51
   499                  return; //nothing to do
   500          }
   501  
-> 502          if (p_from._get_refcount()->conditional_increment() > 0) { // could reference
   503                  _ptr = p_from._ptr;
   504          }
   505  }


(lldb) s
Process 811987 stopped
* thread #1, name = 'godot.linuxbsd.', stop reason = step in
    frame #0: 0x000055555889d532 godot.linuxbsd.editor.dev.x86_64`CowData<char32_t>::_ref(CowData<char32_t> const&) at safe_refcount.h:139:20
   136  
   137          _ALWAYS_INLINE_ T conditional_increment() {
   138                  while (true) {
-> 139                          T c = value.load(std::memory_order_acquire);
   140                          if (c == 0) {
   141                                  return 0;
   142                          }

Process 811987 stopped
* thread #1, name = 'godot.linuxbsd.', stop reason = step in
    frame #0: 0x000055555889d572 godot.linuxbsd.editor.dev.x86_64`CowData<char32_t>::_ref(CowData<char32_t> const&) at safe_refcount.h:143:35
   140                          if (c == 0) {
   141                                  return 0;
   142                          }
-> 143                          if (value.compare_exchange_weak(c, c + 1, std::memory_order_acq_rel)) {
   144                                  return c + 1;
   145                          }
   146                  }
(lldb) expr *(char32_t **) 0x00007fffffff6710
(char32_t *) $48 = 0x000055556386f7b0 U"SomePropertyHint"
(lldb) s
Process 811987 stopped
* thread #1, name = 'godot.linuxbsd.', stop reason = step in
    frame #0: 0x000055555889d5ef godot.linuxbsd.editor.dev.x86_64`CowData<char32_t>::_ref(CowData<char32_t> const&) at safe_refcount.h:144:14
   141                                  return 0;
   142                          }
   143                          if (value.compare_exchange_weak(c, c + 1, std::memory_order_acq_rel)) {
-> 144                                  return c + 1;
   145                          }
   146                  }
   147          }
(lldb) expr *(char32_t **) 0x00007fffffff6710
(char32_t *) $49 = 0x0000000000000002 unable to read data

Over a weekend I'll think what to do with it – I'll try to recreate it via other means as well! I think that shrugging it off for now and writing new StringName into exposed property might be the best choice.

I can't also figure out why godot_print! is a valid and consistent workaround 🤔…

Bromeon · 2025-02-07T08:10:44Z

For some reason compare_exchange_weak executed here corrupts my memory

I can't also figure out why godot_print! is a valid and consistent workaround 🤔…

These are both very typical indicators for undefined behavior: very non-local code (in this case Godot atomics) seemingly "causes" the corruption, and entirely unrelated changes like printing "fix" them. I would not jump to conclusions here, the UB is likely happening in Rust code already, but may only manifest in follow-up code (compare_exchange or print).

Unfortunately, miri is completely useless as soon as FFI is involved, but sometimes, the memcheck CI helps to detect UB instances. In this case, it seems however to only notice it at the segfault, which is too late.

Bromeon · 2025-02-07T08:56:07Z

godot-core/src/meta/property_info.rs

+        } else {
+            sys::SysPtr::force_mut(self.class_name.string_sys())
+        };
+        property_info_ptr.write(self.property_sys());


Hm, now I'm not sure if I was correct with my previous suggestion to use this approach instead of deep-copying fields back. I think we need to be clear about the ownership of the values behind property_info_ptr, and whether we can swap pointers.

If you do nothing in move_into_property_info_ptr(), does the UB go away?

Yeah. The easiest - and only, as far as I'm aware 🤔 - way to cause the UB is overwriting (*property_info_ptr).name and hint_string fields. Something along the lines of:

T::__godot_validate_property(&*instance, &mut property_info); unsafe { (*property_info_ptr).name = sys::SysPtr::force_mut(property_info.property_name.string_sys()); (*property_info_ptr).hint_string = sys::SysPtr::force_mut(property_info.property_name.string_sys()); };

Leaving pointer alone or writing directly into it doesn't cause UB. Writing into exposed property_name should be safe: https://github.com/godotengine/godot/blob/master/core/object/object.cpp#L570C1-L570C75 - Godot creates new StringName prop_name isolated from property name. I don't like writing into anything else (class name and hint_string) though and I'm curious what extacly causes the UB - thus the digging

Yeah. The easiest - and only, as far as I'm aware 🤔 - way to cause the UB is overwriting (*property_info_ptr).name and hint_string fields.

This makes the Godot pointers refer to fields in Rust-side property_info object. As soon as that object goes out of scope, those pointers become dangling -> UAF (use-after-free).

Leaving pointer alone or writing directly into it doesn't cause UB. Writing into exposed property_name should be safe: https://github.com/godotengine/godot/blob/master/core/object/object.cpp#L570C1-L570C75 - Godot creates new StringName prop_name isolated from property name.

I'm not sure why you deduce that writing to StringName is safe, but to String isn't?

Based on the code you showed, in both cases we need to consider:

We can in fact not just change the pointers (at least not without ensuring they point to something valid after the Rust validate_property callback returns -- which would mean we'd need to keep storage allocated just for that).

Instead, we need to modify existing StringName/String objects, via pointers to them

Simply overwriting them may leak memory, if their destructors aren't invoked

At the time of writing back to Godot, we should be able to get a &mut reference to the original String/StringName objects. We can then try something like:

let rust_name: StringName = ...; // set/modified by the user function // SAFETY: StringName constructed at location, and we're the only one accessing it. let godot_name: &mut StringName = unsafe { &mut (*property_info_ptr).name }; // Copy back: invokes first Drop of the old StringName, then moves value into. *godot_name = rust_name; // Or with Drop + Clone (shouldn't be necessary): *godot_name = rust_name.clone();

Also, there is StringName::borrow_string_sys() but no mut overload:

gdext/godot-core/src/builtin/string/string_name.rs

Lines 146 to 156 in 71bbfb8

/// Convert a `StringName` sys pointer to a reference with unbounded lifetime.

///

/// # Safety

///

/// `ptr` must point to a live `StringName` for the duration of `'a`.

pub(crate) unsafe fn borrow_string_sys<'a>(

ptr: sys::GDExtensionConstStringNamePtr,

) -> &'a StringName {

sys::static_assert_eq_size_align!(StringName, sys::types::OpaqueStringName);

&*(ptr.cast::<StringName>())

}

But it would be a more elaborate variant of the above unsafe { &mut string_name_ptr };

Bromeon · 2025-02-07T09:07:40Z

godot-core/src/meta/property_info.rs

+        // Decreasing the refcount on refcounted types (such as StringName or GString)
+        // is a responsibility of the `property_info_ptr`'s owner.
+        std::mem::forget(self);


With the current implementation, Godot cannot know when to decrease the refcounts though -- you simply overwrite the pointers. So I think this would leak memory.

I'm not sure about that – new_from_string_sys doesn't increase the refcount, we also want values created in user space to persist and I haven't noticed the memory leak with previous implementation.

I'll validate it with valgrind and Godot compiled with sanitizers support: https://docs.godotengine.org/en/stable/contributing/development/debugging/using_sanitizers.html (same we use for memchecks afaik).

I think with the above approach you can move the dynamically-allocated objects back to Godot without leaking memory.

I'm then not sure whether forget is necessary -- rustc might complain if some fields have been moved out, and it can no longer invoke Drop::drop(&mut self).

I'm not sure about that – new_from_string_sys doesn't increase the refcount, we also want values created in user space to persist and I haven't noticed the memory leak with previous implementation.

new_from_string_sys absolutely does increase the ref count. that's the point of it, if you dont want to increase the refcount you'd use borrow_string_sys.

Bromeon

Very cool to see that the CI is green, thanks for tinkering!

Added some more comments, eventually you could also squash the commits.

Bromeon · 2025-02-09T20:08:37Z

godot-codegen/src/generator/virtual_traits.rs

+        #[cfg(since_api = "4.2")]
+        fn validate_property(&self, property: &mut crate::meta::PropertyInfo) {
+            unimplemented!()
+        }


Would be good to add docs, see other methods 🙂

Bromeon · 2025-02-09T20:10:49Z

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

+    /// # Safety
+    ///
+    /// `ptr` must point to a live `GString` for the duration of `'a`.
+    pub(crate) unsafe fn borrow_string_sys_mut<'a>(ptr: sys::GDExtensionStringPtr) -> &'a mut Self {


There's another safety precondition: no other Rust reference to that location must exist, otherwise &mut is not exclusive -> immediate UB.

(also, you can start directly after # Safety on the next line, without empty line, we're not very consistent with it yet)

Bromeon · 2025-02-09T20:16:58Z

godot-core/src/meta/property_info.rs

+        (*property_info_ptr).usage = u32::try_from(self.usage.ord()).expect("usage.ord()");
+        (*property_info_ptr).hint = u32::try_from(self.hint_info.hint.ord()).expect("hint.ord()");
+        (*property_info_ptr).type_ = self.variant_type.sys();


Please dereference the raw pointer once and use a reference afterwards.

Bromeon · 2025-02-09T20:19:25Z

godot-core/src/meta/property_info.rs

+            variant_type: VariantType::from_sys((*property_info_ptr).type_),
+            class_name: ClassName::none(),
+            property_name: StringName::new_from_string_sys((*property_info_ptr).name),
+            hint_info: PropertyHintInfo {
+                hint: PropertyHint::from_ord((*property_info_ptr).hint.to_owned() as i32),
+                hint_string: GString::new_from_string_sys((*property_info_ptr).hint_string),
+            },
+            usage: PropertyUsageFlags::from_ord((*property_info_ptr).usage as u64),


Also here, avoid repeated dereferencings.

Bromeon · 2025-02-09T20:20:53Z

godot-core/src/registry/callbacks.rs

+/// # Safety
+///
+/// - Must only be called by Godot as a callback for `validate_property` for a rust-defined class of type `T`.
+/// - `sys_property_info` must be valid for the whole duration of this function call (ie - can't be freed nor consumed).


Suggested change

/// - `sys_property_info` must be valid for the whole duration of this function call (ie - can't be freed nor consumed).

/// - `property_info_ptr` must be valid for the whole duration of this function call (i.e. can't be freed nor consumed).

Bromeon · 2025-02-09T20:21:25Z

godot-core/src/registry/callbacks.rs

+    let storage = unsafe { as_storage::<T>(instance) };
+    let instance = storage.get();
+
+    let mut property_info = unsafe { PropertyInfo::new_from_sys(property_info_ptr) };


Safety statement? 🙂

Bromeon · 2025-02-09T20:21:38Z

godot-core/src/registry/class.rs

@@ -477,6 +479,11 @@ fn fill_class_info(item: PluginItem, c: &mut ClassRegistrationInfo) {
            c.godot_params.property_can_revert_func = user_property_can_revert_fn;
            c.godot_params.property_get_revert_func = user_property_get_revert_fn;
            c.user_virtual_fn = get_virtual_fn;
+            // Not present in GdExtension.


Suggested change

// Not present in GdExtension.

// Not present in GDExtension.

Also, what do you mean by that? validate_property_func is clearly available...

I meant that it wasn't present in GDExtension before 4.2 😅 – I decided to remove this comment altogether, since it is just a noise (#[cfg(since_api = "4.2")] tells all one needs to know)

Bromeon · 2025-02-09T20:24:09Z

itest/rust/src/object_tests/validate_property_test.rs

+    #[var(
+        hint = NONE,
+        hint_string = "initial"
+    )]


Suggested change

#[var(

hint = NONE,

hint_string = "initial"

)]

#[var(hint = NONE, hint_string = "initial")]

Bromeon · 2025-02-09T20:29:35Z

itest/rust/src/object_tests/validate_property_test.rs

+    for property in properties.iter_shared() {
+        if !property
+            .get("name")
+            .is_some_and(|v| v.to_string() == "SuperNewTestPropertyName")
+        {
+            continue;
+        }
+
+        let Some(hint_string) = property.get("hint_string").map(|v| v.to::<GString>()) else {
+            continue;
+        };
+        assert_eq!(hint_string, GString::from("SomePropertyHint"));
+
+        let Some(class) = property.get("class_name").map(|v| v.to::<StringName>()) else {
+            continue;
+        };
+        assert_eq!(class, StringName::from("ValidatePropertyTest"));
+
+        let Some(usage) = property.get("usage").map(|v| v.to::<PropertyUsageFlags>()) else {
+            continue;
+        };
+        assert_eq!(usage, PropertyUsageFlags::NO_EDITOR);
+
+        obj.free();
+        return;
+    }


The individual continue statements define an order, i.e. those tested later are never reached if earlier ones fall into the continue branch. Is this intended?

Also, you currently skip each test when a key like "usage" is absent -- shouldn't you verify that it's present and has a given value? Dictionary::at() may help here as well.

Changed to explicit panic in case if given value can't be retrieved (should be easier to spot during run).

- Add validate_property func virtual. - Create internal API for easier handling of ``*mut GDExtensionPropertyInfo` - functions that allow to easily handle conversions from/to PropertyInfo and `borrow_string_sys_mut` for GString and StringName. Signed-off-by: Jan Haller <[email protected]>

Bromeon · 2025-02-16T13:56:12Z

Rebased and slightly updated the logic in validate_property_test:

no map(), because we can just change the order of expect() to be before to().
use find() for simpler control flow than loop + continue + return.

Thanks a lot!

Yarwin force-pushed the add_validate_property_fn branch 3 times, most recently from 9959df5 to f929b27 Compare February 1, 2025 10:20

Bromeon reviewed Feb 1, 2025

View reviewed changes

Bromeon reviewed Feb 7, 2025

View reviewed changes

Bromeon added feature Adds functionality to the library c: ffi Low-level components and interaction with GDExtension API labels Feb 8, 2025

Yarwin force-pushed the add_validate_property_fn branch 2 times, most recently from b88ece7 to 311b133 Compare February 9, 2025 15:57

Bromeon reviewed Feb 9, 2025

View reviewed changes

Yarwin force-pushed the add_validate_property_fn branch from c5ea54d to 32c740f Compare February 10, 2025 11:55

Yarwin marked this pull request as ready for review February 10, 2025 16:15

Yarwin force-pushed the add_validate_property_fn branch 2 times, most recently from 5905290 to a291637 Compare February 10, 2025 16:17

Bromeon force-pushed the add_validate_property_fn branch from a291637 to ed99757 Compare February 16, 2025 13:53

Bromeon enabled auto-merge February 16, 2025 13:56

Bromeon added this pull request to the merge queue Feb 16, 2025

Merged via the queue into godot-rust:master with commit f6a309a Feb 16, 2025
16 checks passed

	/// Moves this string into a string name sys pointer. This is the same as using [`GodotFfi::move_return_ptr`].
	/// Moves this string into a `StringName` sys pointer. This is the same as using [`GodotFfi::move_return_ptr()`].

	/// `dst` must be a valid string name pointer.
	/// `dst` must be a valid `StringName` pointer.

	// `validate_property` is only supported in Godot 4.2+
	// `validate_property` is only supported in Godot 4.2+.

	let Some(usage) = property.get("usage").map(\|v\| v.to::<PropertyUsageFlags>()) else {
	let Some(usage) = property.get("usage").map(Variant::to::<PropertyUsageFlags>) else {

	/// Convert a `StringName` sys pointer to a reference with unbounded lifetime.
	///
	/// # Safety
	///
	/// `ptr` must point to a live `StringName` for the duration of `'a`.
	pub(crate) unsafe fn borrow_string_sys<'a>(
	ptr: sys::GDExtensionConstStringNamePtr,
	) -> &'a StringName {
	sys::static_assert_eq_size_align!(StringName, sys::types::OpaqueStringName);
	&*(ptr.cast::<StringName>())
	}

	/// - `sys_property_info` must be valid for the whole duration of this function call (ie - can't be freed nor consumed).
	/// - `property_info_ptr` must be valid for the whole duration of this function call (i.e. can't be freed nor consumed).

	// Not present in GdExtension.
	// Not present in GDExtension.

Add "validate_property" virtual func #1030

Add "validate_property" virtual func #1030

Conversation

Yarwin commented Jan 31, 2025 • edited Loading

Bromeon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yarwin commented Feb 6, 2025 • edited Loading

Yarwin commented Feb 7, 2025 • edited Loading

Bromeon commented Feb 7, 2025

Choose a reason for hiding this comment

Yarwin Feb 7, 2025 • edited Loading

Choose a reason for hiding this comment

Bromeon Feb 8, 2025 • edited Loading

Choose a reason for hiding this comment

Bromeon Feb 8, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yarwin Feb 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bromeon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bromeon commented Feb 16, 2025

Yarwin commented Jan 31, 2025 •

edited

Loading

Yarwin commented Feb 6, 2025 •

edited

Loading

Yarwin commented Feb 7, 2025 •

edited

Loading

Yarwin Feb 7, 2025 •

edited

Loading

Bromeon Feb 8, 2025 •

edited

Loading

Bromeon Feb 8, 2025 •

edited

Loading

Yarwin Feb 7, 2025 •

edited

Loading