Unable to validate freed Object

[ashtonmeuser]

Godot version

4.3

godot-cpp version

4.3 (e298f430b55bdbd9cf3eae9e041354c26d2e9c79)

System information

macOS Sonoma 14.6.1

Issue description

With the removal of UtilityFunctions::is_instance_valid() (https://github.com/godotengine/godot-cpp/pull/1513), it is impossible to validate an Object passed to a GDExtension method as a Variant.

Spawned from this comment. Using 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.

Previously, UtilityFunctions::is_instance_valid() could be used to validate an object. It's possible to use variant.stringify() == "<Freed Object>" as a hacky workaround although obviously not ideal.

Steps to reproduce

Add the following method to a GDExtension class.

// Must validate supplied object
godot_error MyLib::dummy(const Variant v) {
  // Side effects for other Variant types would necessitate accepting a Variant rather than Object*
  if (v.get_type() != Variant::OBJECT) return ERR_CANT_CREATE;
  // if (v.stringify() == "<Freed Object>") return ERR_CANT_CREATE; // Hacky solution
  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;
}

Use the GDExtension function via GDScript as follows.

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

Minimal reproduction project

N/A

[dsnopek]

Thanks!

Ack, yeah, in the case you give here (a bound function that takes a Variant argument), I think we are, in fact, missing something. :-(

The problem is that Variant::operator ObjectID() is implemented using Variant::operator Object*(), which will crash if the object is already freed. We need a way to get the ObjectID directly, which I don't think we actually have at the moment. (It would also would be nice to implement Variant::get_validated_object() in godot-cpp, that provides a nice API.)

I'm not sure how we can add that without adding something new in Godot, and so, this may need to be a Godot 4.4 thing. And if that's the case, then I think we'll probably need to revert the removal of is_instance_valid() until Godot 4.4 :-/

This is unfortunate, because I don't think it's very common to accept Variant arguments and check their validity. I think the "trap" is much more common, ie where folks accept and hold on to an Object *, and then later call is_instance_valid() on it, thinking that will actually check if the instance is valid (rather than crashing if the instance is invalid, which is what it'll actually do).

[ashtonmeuser]

@dsnopek Agreed that something like Variant::get_validated_object() would be useful. Also agree that while is_instance_valid() is a bit of a footgun, it's better than lacking an equivalent tool in v4.3.

A more realistic example than I provided above (and the case that revealed this issue to me) is a method that accepts a higher-order data structure e.g. Dictionary that predicate a Variant but is expected to contain an Object.

[dsnopek]

I just posted PR https://github.com/godotengine/godot-cpp/pull/1592 which reverts the removal of is_instance_valid(), targeting Godot 4.3 and earlier.

And also PR https://github.com/godotengine/godot-cpp/pull/1591 which adds what we need to support this use-case without is_instance_valid() that we can use in Godot 4.4+

[aaronfranke]

I also ran into this. I tried to use Variant::get_validated_object to match core, and it was not available.

[dsnopek]

This should be fixed now since https://github.com/godotengine/godot-cpp/pull/1591 and https://github.com/godotengine/godot-cpp/pull/1592 have been merged