GDExtension/SurfaceTool crash on commit.

[Kev]

Godot version

4.0 (various, including b50acebd48d7e45a1444e553a5ea9878ef87592a)

System information

MBP Silicon

Issue description

I get a crash when I call commit on a surface tool from C++. I don't know if this is a problem with Ref, with the bridge or with SurfaceTool itself. It sounds a little similar to https://github.com/godotengine/godot/issues/53191 but seems to be distinct.

handle_crash: Program crashed with signal 11
Engine version: Godot Engine v4.0.alpha.custom_build (9904a9db5a90f2f1896b6b7e1603ca2dd2745bad)
Dumping the backtrace. Please include this when reporting the bug on: https://github.com/godotengine/godot/issues
[1] 1   libsystem_platform.dylib            0x00000001862c34a4 _sigtramp + 56
[2] godot::SurfaceTool::commit(godot::Ref<godot::ArrayMesh> const&, unsigned int)
[3] godot::SurfaceTool::commit(godot::Ref<godot::ArrayMesh> const&, unsigned int)
[4] Game::Ground::_ready()
[5] void godot::call_with_ptr_args_helper<Game::Ground>(Game::Ground*, void (Game::Ground::*)(), void* const*, IndexSequence<>)
[6] void godot::call_with_ptr_args<Game::Ground>(Game::Ground*, void (Game::Ground::*)(), void* const*, void*)
[7] void godot::Node::register_virtuals<Game::Ground>()::'lambda0'(void*, void* const*, void*)::operator()(void*, void* const*, void*) const
[8] void godot::Node::register_virtuals<Game::Ground>()::'lambda0'(void*, void* const*, void*)::__invoke(void*, void* const*, void*)

or

Process 34791 stopped
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x8)
    frame #0: 0x000000012b60a12c libresort.osx.debug`godot::ArrayMesh* godot::internal::_call_native_mb_ret_obj<godot::ArrayMesh, void*, long long*>(mb=0x000000014a92c840, instance=0x000000014ab5ab80, args=0x0000000000000008, args=0x000000016fdfc8d0) at engine_ptrcall.hpp:49:90 [opt]
   46   template <class O, class... Args>
   47   O *_call_native_mb_ret_obj(const GDNativeMethodBindPtr mb, void *instance, const Args &...args) {
   48           GodotObject *ret = nullptr;
-> 49           std::array<const GDNativeTypePtr, sizeof...(Args)> mb_args = { { (const GDNativeTypePtr)args... } };
   50           internal::gdn_interface->object_method_bind_ptrcall(mb, instance, mb_args.data(), &ret);
   51           if (ret == nullptr) {
   52                   return nullptr;
Target 0: (Godot) stopped.
Process 34791 launched: '/Users/me/devel/godot/Godot.app/Contents/MacOS/Godot' (arm64)
(lldb) bt
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x8)
  * frame #0: 0x000000012b60a12c libresort.osx.debug`godot::ArrayMesh* godot::internal::_call_native_mb_ret_obj<godot::ArrayMesh, void*, long long*>(mb=0x000000014a92c840, instance=0x000000014ab5ab80, args=0x0000000000000008, args=0x000000016fdfc8d0) at engine_ptrcall.hpp:49:90 [opt]
    frame #1: 0x000000012b609fd8 libresort.osx.debug`godot::SurfaceTool::commit(this=<unavailable>, existing=<unavailable>, flags=<unavailable>) at surface_tool.cpp:257:49 [opt]

depending on your preferred backtrace style.

Steps to reproduce

In _ready on a class inheriting MeshInstance3D, created with auto ground = memnew(Ground()); and inserted into the tree with parentForScenes->call_deferred("add_child", ground);

    godot::Ref<godot::SurfaceTool> surfaceTool;
    surfaceTool.instantiate();

    surfaceTool->begin(godot::Mesh::PRIMITIVE_TRIANGLES);
//    ...assorted times...
    surfaceTool->add_vertex(godot::Vector3(x * cellSize_ + xOffset_, z * cellSize_ + yOffset_, y * cellSize_ + zOffset_));
//    etc.

    surfaceTool->add_index(topLeftIndex(x, y));
    surfaceTool->add_index(topRightIndex(x, y));
    surfaceTool->add_index(midIndex(x, y));
//    etc.

    surfaceTool->generate_normals();
    set_mesh(surfaceTool->commit());

It only crashes if I use the return style, rather than passing in a mesh. The following does not crash

    surfaceTool->generate_normals();
    godot::Ref<godot::ArrayMesh> arrayMesh;
    arrayMesh.instantiate();
    surfaceTool->commit(arrayMesh); // Crash here
    set_mesh(arrayMesh);  

Minimal reproduction project

No response

[Kev]

Apologies, a screwup on my side meant I was running old code. It only crashes if I use the return style, not if I use the pass style. i.e.

set_mesh(surfaceTool->commit());

crashes and

    godot::Ref<godot::ArrayMesh> arrayMesh;
    arrayMesh.instantiate();
    surfaceTool->commit(arrayMesh);
    set_mesh(arrayMesh);

does not. Possibly lending weight to Zylann's theory on Discord that this could be down to Ref being freed early through the bridge.

I'll update the original description now.

[playmyskay]

The following happens:

1. Calling surfaceTool->commit() creates a Ref<ArrayMesh> with an internal nullptr as reference.

   Ref<ArrayMesh> commit(const Ref<ArrayMesh> &existing = nullptr, uint32_t flags = 0);

2. Variable existing of type Ref<ArrayMesh> contains (as said before) a nullptr but is still being accessed by existing->_owner. operator->() is overriden in class Ref<> and returns the internal pointer (variable reference) which is a nullptr. So this is simply a nullpointer access. In step 3 it will finally crash.

Ref<ArrayMesh> SurfaceTool::commit(const Ref<ArrayMesh> &existing, uint32_t flags) {
    static GDNativeMethodBindPtr ___method_bind = internal::gdn_interface->classdb_get_method_bind("SurfaceTool", "commit", 4107864055);
    CHECK_METHOD_BIND_RET(___method_bind, Ref<ArrayMesh>());
    int64_t flags_encoded;
    PtrToArg<int64_t>::encode(flags, &flags_encoded);
    return Ref<ArrayMesh>::___internal_constructor(internal::_call_native_mb_ret_obj<ArrayMesh>(___method_bind, _owner, existing->_owner, &flags_encoded));
}

3. Now call_native_mb_ret_obj<ArrayMesh>(...) is called. The std::array creation leads to a segmentation fault.

   template <class O, class... Args>
   O *_call_native_mb_ret_obj(const GDNativeMethodBindPtr mb, void *instance, const Args &...args) {
   GodotObject *ret = nullptr;
   std::array<const GDNativeTypePtr, sizeof...(Args)> mb_args = { { (const GDNativeTypePtr)args... } };
   internal::gdn_interface->object_method_bind_ptrcall(mb, instance, mb_args.data(), &ret);
   if (ret == nullptr) {
       return nullptr;
   }
   return reinterpret_cast<O *>(internal::gdn_interface->object_get_instance_binding(ret, internal::token, &O::___binding_callbacks));
   }

Solution (for engine devs): In step 2 has to be a nullpointer check on the internal reference of variable existing. If the internal pointer is null, then avoid the operator->() access and pass a null pointer. Something like: existing.is_valid() ? existing->_owner : nullptr)

As far as I have seen, this also affects other functions that work with default arguments.

[akien-mga]

Can you still reproduce this in 4.0.3 and 4.1-beta3 or later?

[KajSoerenGrunow]

Can you still reproduce this in 4.0.3 and 4.1-beta3 or later?

Yes, can still reproduce this issue with Godot 4.2.1-stable (Arch Linux) in combination with Rust/Gdext. Not (yet) tested with GDScript or C++.