Node pointers can't be passed into UtilityFunctions::is_instance_valid() to check if they have not been freed; is something needed for this?

[id01]

Godot version

4.2.1.stable

godot-cpp version

4.2.1.stable

System information

Any

Issue description

Currently, is_instance_valid() dereferences object pointers passed to it due to implicit cast to Variant. As such, executing is_instance_valid() on freed nodes, unlike in gdscript, causes a use-after-free crash. Currently, there doesn't seem to be a way to store nodes where UtilityFunctions::is_instance_valid() can work without sacrificing typing (by storing everything as Variant), and the example project simply uses pointers for passing around object pointers as well.

Steps to reproduce

Call UtilityFunctions::is_instance_valid() on a freed node pointer. The program crashes instead of returning false.

Minimal reproduction project

See steps to reproduce.

[dsnopek]

In a C++ module, rather than using is_instance_valid(), in situations where you need to store objects that might get freed, you'd store ObjectID and call ObjectDB::get_instance() to check if it's still valid. This is what I'd recommend doing in godot-cpp as well, since we are attempting to emulate modules (and it doesn't suffer from the problem you're encountering).

Regarding fixing is_instance_valid(): I wonder if we should just not expose it in godot-cpp?

There is no way to check if an Object * has been freed, and trying to create a Variant from a freed Object * will cause a crash. This is exactly what will happen if you call is_instance_valid() with a freed Object * (it's automatically converted to Variant), which that function is practically inviting developers to do. This isn't the first time this issue has come up. :-)

[TokisanGames]

After over a year of using UtilityFunctions::is_instance_valid() on a Camera3D *_camera in Terrain3D, I'm suddenly getting crashes on every few runs of my game in 4.2.2. It didn't start until I added MultiMeshInstancing to Terrain3D, but perhaps that's coincidental as the crash is no where near that code (or perhaps it changed internal timing).

My last bit of code in the callstack is: if (!UtilityFunctions::is_instance_valid(_camera)) Then it goes to Variant.cpp and crashes in the screenshot exactly as described above:

Regarding fixing is_instance_valid(): I wonder if we should just not expose it in godot-cpp?

If it doesn't work, and isn't going to work, then I suggest that you not expose it and in the gdextension documentation (👀?) say how to check objects so we don't have to use Issues and troubleshooting to figure things out. I assume that if there's a function available that it's safe to use.

Here's an example usage of the described solution and a replacement is_instance_valid() function.

// Additional instance id tracker
Camera3D *_camera = nullptr;
uint64_t _camera_instance_id = 0;

// Save instance id when setting object
void set_camera(Camera3D *p_camera) {
	_camera = p_camera;
	if (p_camera) {
		_camera_instance_id = _camera->get_instance_id();
	} else {
		_camera_instance_id = 0;
	}
}

// Check just the instance ID, or both your variables:
is_instance_valid(_camera_instance_id );
is_instance_valid(_camera_instance_id , _camera);

_FORCE_INLINE_ bool is_instance_valid(uint64_t p_instance_id, Object *p_object = nullptr) {
	Object *obj = ObjectDB::get_instance(p_instance_id);
	if (p_object != nullptr) {
		return p_instance_id> 0 && p_object == obj;
	} else {
		return p_instance_id> 0 && obj != nullptr;
	}
}

[dsnopek]

I agree, exposing this function is a trap. I've just posted PR https://github.com/godotengine/godot-cpp/pull/1513 to unexpose it