godotengine / godot-cpp

C++ bindings for the Godot script API
MIT License
1.74k stars 575 forks source link

Unexpose `UtilityFunctions::is_instance_valid()` #1513

Closed dsnopek closed 4 months ago

dsnopek commented 4 months ago

Fixes https://github.com/godotengine/godot-cpp/issues/1390

The is_instance_valid() function doesn't work as developers expect, and unless used very carefully, will cause crashes. Instead, developers should use ObjectDB::get_instance() with object ids to ensure that an instance is still valid.

This PR unexposes is_instance_valid() so that folks aren't tempted to use it.

I'm unsure if we should cherry-pick this or not. Technically it breaks source compatibility, but if developers are using this function, they probably have a bug that's waiting to be exposed. :-)

Note: Bindings aside from godot-cpp may be able to expose is_instance_valid() in a non-problematic way. However, since we're trying to match the Godot API and allow raw pointers to objects, we are stuck with this issue. If were able to disallow raw object pointers and require developers to use some kind of "object smart pointer object", that would be a different story.

zhwei820 commented 2 months ago

So many change in downstream extensions..

TokisanGames commented 2 months ago

@zhwei820 It's broken.

See the bottom here: https://github.com/godotengine/godot-cpp/issues/1390#issuecomment-2198316403

ashtonmeuser commented 2 months ago

@dsnopek It seems that ObjectDB::get_instance() as suggested here and here isn't a perfect replacement. Starting with a Variant that represents a freed Object, we're unable to get an object ID in order to use ObjectDB::get_instance(). After ensuring a Variant is of type Object, an operator Object*() call causes a crash.

For example, the following dummy GDExtension library function and GDScript will produce a crash when supplying a freed Object:

// Must validate supplied object
godot_error MyLib::dummy(const Variant v) {
  if (v.get_type() != Variant::OBJECT) return ERR_CANT_CREATE;
  auto o = v.operator Object*(); // Crashes here if object was previously freed
  if (o == NULL) return ERR_CANT_CREATE;
  auto id = o->get_instance_id();
  auto x = ObjectDB::get_instance(id);
  if (x == NULL) return ERR_CANT_CREATE;
  return OK;
}
var o = Object.new()
var lib = MyLib.new()
assert(lib.dummy(o) == OK) # Expect object to be valid
o.free()
assert(lib.dummy(o) != OK) # Expect object to be invalid

Both assertion cases were previously correctly handled by UtilityFunctions::is_instance_valid() without error.

In a GDExtension context without UtilityFunctions::is_instance_valid(), I'm unable to find a way to properly validate a freed Object passed in as a Variant (ignoring super-hacky solutions e.g. v.stringify() == <Freed Object>).