Writing to freed memory in Network demo

[qarmin]

Godot version: 3.2 alpha 3 compiled with sanitizers support OS/device including version: Ubuntu 19.10

Issue describtion:

ERROR: _get_socket_error: Socket error: 111
   At: drivers/unix/net_socket_posix.cpp:211.
ERROR: connect_to_host: Connection to remote host failed!
   At: drivers/unix/net_socket_posix.cpp:429.
=================================================================
==29063==ERROR: AddressSanitizer: heap-use-after-free on address 0x621000268530 at pc 0x00000e646071 bp 0x7ffd1ef60960 sp 0x7ffd1ef60950
WRITE of size 4 at 0x621000268530 thread T0
    #0 0xe646070 in atomic_decrement<unsigned int> core/safe_refcount.h:118
    #1 0xe646070 in SafeRefCount::unref() core/safe_refcount.h:192
    #2 0xe646070 in _ObjectDebugLock::~_ObjectDebugLock() core/object.cpp:53
    #3 0xe623799 in Object::emit_signal(StringName const&, Variant const**, int) core/object.cpp:1178
    #4 0xe624235 in Object::emit_signal(StringName const&, Variant const&, Variant const&, Variant const&, Variant const&, Variant const&) core/object.cpp:1274
    #5 0x27b761b in WebSocketClient::_on_error() modules/websocket/websocket_client.cpp:139
    #6 0x283bb72 in WSLClient::poll() modules/websocket/wsl_client.cpp:272
    #7 0xef0ed6a in MultiplayerAPI::poll() core/io/multiplayer_api.cpp:100
    #8 0x94cf1bd in SceneTree::idle(float) scene/main/scene_tree.cpp:516
    #9 0x15ab862 in Main::iteration() main/main.cpp:1976
    #10 0x14a2ce4 in OS_X11::run() platform/x11/os_x11.cpp:3259
    #11 0x141d36c in main platform/x11/godot_x11.cpp:56
    #12 0x7f0852cbf1e2 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x271e2)
    #13 0x141cf4d in _start (/usr/bin/godots+0x141cf4d)

0x621000268530 is located 48 bytes inside of 4600-byte region [0x621000268500,0x6210002696f8)
freed by thread T0 here:
    #0 0x7f08541e36ef in __interceptor_free (/lib/x86_64-linux-gnu/libasan.so.5+0x10d6ef)
    #1 0xebb6645 in Memory::free_static(void*, bool) core/os/memory.cpp:181
    #2 0x954e66a in void memdelete<NetworkedMultiplayerPeer>(NetworkedMultiplayerPeer*) core/os/memory.h:119
    #3 0x9539bdf in Ref<NetworkedMultiplayerPeer>::unref() core/reference.h:279
    #4 0x9539d22 in Ref<NetworkedMultiplayerPeer>::ref(Ref<NetworkedMultiplayerPeer> const&) core/reference.h:70
    #5 0xef4596f in Ref<NetworkedMultiplayerPeer>::operator=(Ref<NetworkedMultiplayerPeer> const&) core/reference.h:151
    #6 0xef113b5 in MultiplayerAPI::set_network_peer(Ref<NetworkedMultiplayerPeer> const&) core/io/multiplayer_api.cpp:151
    #7 0x94fdc82 in SceneTree::set_network_peer(Ref<NetworkedMultiplayerPeer> const&) scene/main/scene_tree.cpp:1775
    #8 0x956618d in MethodBind1<Ref<NetworkedMultiplayerPeer> const&>::call(Object*, Variant const**, int, Variant::CallError&) core/method_bind.gen.inc:775
    #9 0xe6180fe in Object::call(StringName const&, Variant const**, int, Variant::CallError&) core/object.cpp:921
    #10 0xe9068f8 in Variant::call_ptr(StringName const&, Variant const**, int, Variant*, Variant::CallError&) core/variant_call.cpp:1103
    #11 0x1a3ab26 in GDScriptFunction::call(GDScriptInstance*, Variant const**, int, Variant::CallError&, GDScriptFunction::CallState*) modules/gdscript/gdscript_function.cpp:1082
    #12 0x1872beb in GDScriptInstance::call(StringName const&, Variant const**, int, Variant::CallError&) modules/gdscript/gdscript.cpp:1164
    #13 0xe617c6b in Object::call(StringName const&, Variant const**, int, Variant::CallError&) core/object.cpp:900
    #14 0xe62224b in Object::emit_signal(StringName const&, Variant const**, int) core/object.cpp:1218
    #15 0xe624235 in Object::emit_signal(StringName const&, Variant const&, Variant const&, Variant const&, Variant const&, Variant const&) core/object.cpp:1274
    #16 0x94fa25d in SceneTree::_connection_failed() scene/main/scene_tree.cpp:1732
    #17 0x17a9e1e in MethodBind0::call(Object*, Variant const**, int, Variant::CallError&) core/method_bind.gen.inc:59
    #18 0xe6180fe in Object::call(StringName const&, Variant const**, int, Variant::CallError&) core/object.cpp:921
    #19 0xe62224b in Object::emit_signal(StringName const&, Variant const**, int) core/object.cpp:1218
    #20 0xe624235 in Object::emit_signal(StringName const&, Variant const&, Variant const&, Variant const&, Variant const&, Variant const&) core/object.cpp:1274
    #21 0xef295f5 in MultiplayerAPI::_connection_failed() core/io/multiplayer_api.cpp:625
    #22 0x17a9e1e in MethodBind0::call(Object*, Variant const**, int, Variant::CallError&) core/method_bind.gen.inc:59
    #23 0xe6180fe in Object::call(StringName const&, Variant const**, int, Variant::CallError&) core/object.cpp:921
    #24 0xe62224b in Object::emit_signal(StringName const&, Variant const**, int) core/object.cpp:1218
    #25 0xe624235 in Object::emit_signal(StringName const&, Variant const&, Variant const&, Variant const&, Variant const&, Variant const&) core/object.cpp:1274
    #26 0x27b761b in WebSocketClient::_on_error() modules/websocket/websocket_client.cpp:139
    #27 0x283bb72 in WSLClient::poll() modules/websocket/wsl_client.cpp:272
    #28 0xef0ed6a in MultiplayerAPI::poll() core/io/multiplayer_api.cpp:100
    #29 0x94cf1bd in SceneTree::idle(float) scene/main/scene_tree.cpp:516

previously allocated by thread T0 here:
    #0 0x7f08541e3ae8 in malloc (/lib/x86_64-linux-gnu/libasan.so.5+0x10dae8)
    #1 0xebb563f in Memory::alloc_static(unsigned long, bool) core/os/memory.cpp:85
    #2 0xebb553e in operator new(unsigned long, char const*) core/os/memory.cpp:42
    #3 0x27b0353 in WSLClient::_create() (/usr/bin/godots+0x27b0353)
    #4 0x27b028f in WebSocketClient::create() (/usr/bin/godots+0x27b028f)
    #5 0x27b25da in Object* ClassDB::_create_ptr_func<WebSocketClient>() core/class_db.h:197
    #6 0xe3a5749 in ClassDB::instance(StringName const&) core/class_db.cpp:541
    #7 0x184c532 in GDScriptNativeClass::instance() modules/gdscript/gdscript.cpp:82
    #8 0x184be1d in GDScriptNativeClass::_new() modules/gdscript/gdscript.cpp:69
    #9 0x191d5af in MethodBind0R<Variant>::call(Object*, Variant const**, int, Variant::CallError&) core/method_bind.gen.inc:237
    #10 0xe6180fe in Object::call(StringName const&, Variant const**, int, Variant::CallError&) core/object.cpp:921
    #11 0xe9068f8 in Variant::call_ptr(StringName const&, Variant const**, int, Variant*, Variant::CallError&) core/variant_call.cpp:1103
    #12 0x1a3aaa5 in GDScriptFunction::call(GDScriptInstance*, Variant const**, int, Variant::CallError&, GDScriptFunction::CallState*) modules/gdscript/gdscript_function.cpp:1079
    #13 0x1872beb in GDScriptInstance::call(StringName const&, Variant const**, int, Variant::CallError&) modules/gdscript/gdscript.cpp:1164
    #14 0xe617c6b in Object::call(StringName const&, Variant const**, int, Variant::CallError&) core/object.cpp:900
    #15 0xe62224b in Object::emit_signal(StringName const&, Variant const**, int) core/object.cpp:1218
    #16 0xe624235 in Object::emit_signal(StringName const&, Variant const&, Variant const&, Variant const&, Variant const&, Variant const&) core/object.cpp:1274
    #17 0x96dd0df in BaseButton::_pressed() scene/gui/base_button.cpp:135
    #18 0x96e0a4c in BaseButton::on_action_event(Ref<InputEvent>) scene/gui/base_button.cpp:169
    #19 0x96d99b9 in BaseButton::_gui_input(Ref<InputEvent>) scene/gui/base_button.cpp:64
    #20 0x65593e7 in MethodBind1<Ref<InputEvent> >::call(Object*, Variant const**, int, Variant::CallError&) core/method_bind.gen.inc:775
    #21 0xe61383e in Object::call_multilevel(StringName const&, Variant const**, int) core/object.cpp:763
    #22 0xe616b5f in Object::call_multilevel(StringName const&, Variant const&, Variant const&, Variant const&, Variant const&, Variant const&) core/object.cpp:863
    #23 0x95e079e in Viewport::_gui_call_input(Control*, Ref<InputEvent> const&) scene/main/viewport.cpp:1525
    #24 0x95effcf in Viewport::_gui_input_event(Ref<InputEvent>) scene/main/viewport.cpp:1905
    #25 0x96141d7 in Viewport::input(Ref<InputEvent> const&) scene/main/viewport.cpp:2680
    #26 0x95d40d6 in Viewport::_vp_input(Ref<InputEvent> const&) scene/main/viewport.cpp:1302
    #27 0x238177d in MethodBind1<Ref<InputEvent> const&>::call(Object*, Variant const**, int, Variant::CallError&) core/method_bind.gen.inc:775
    #28 0xe6180fe in Object::call(StringName const&, Variant const**, int, Variant::CallError&) core/object.cpp:921
    #29 0xe616591 in Object::call(StringName const&, Variant const&, Variant const&, Variant const&, Variant const&, Variant const&) core/object.cpp:847

Steps to reproduce:

Minimal reproduction project: https://github.com/godotengine/godot-demo-projects/tree/master/networking/websocket_multiplayer

[Faless]

So, if I get this right, OBJ_DEBUG_LOCK is referencing the object that is emitting the signal (by pointer, so it's generic enough). Given the peer object is a reference, if the object loses the last Ref (via multiplayer.network_peer = null) during the signal, then _ObjectDebugLock tries to access it right after the signal emission (via obj->_lock_index in ~_ObjectDebugLock), and we get the crash. Am I getting this right? Tagging people with a bit more knowledge on the internals (@reduz @hpvb ?).

Should we just use an extra reference to it while polling? Sounds more like an hack then a fix :(

[Faless]

Well, that seems to do it, at least according to valgrind... @qarmin can you try this patch (again, pretty hacky, we'll need to likely discuss something better):

diff --git a/core/io/multiplayer_api.cpp b/core/io/multiplayer_api.cpp
index 1426dbbd4d..d12b5c52a5 100644
--- a/core/io/multiplayer_api.cpp
+++ b/core/io/multiplayer_api.cpp
@@ -97,7 +97,10 @@ void MultiplayerAPI::poll() {
        if (!network_peer.is_valid() || network_peer->get_connection_status() == NetworkedMultiplayerPeer::CONNECTION_DISCONNECTED)
                return;

-       network_peer->poll();
+       // Avoid the network peer going out of scope during polling (when signals are emitted)
+       Ref<NetworkedMultiplayerPeer> safe_peer = network_peer;
+       safe_peer->poll();
+       safe_peer = Ref<NetworkedMultiplayerPeer>();

        if (!network_peer.is_valid()) // It's possible that polling might have resulted in a disconnection, so check here.
                return;

[Faless]

A more "appropriate" fix might be to prevent references from going out of scope when the object->_lock_index is > 0, but then we'll have to make sure to properly free them when _lock_index goes back to 0, and no reference is left.

EDIT: assuming of course, I got the culprit right

[qarmin]

This patch fixed invalid write

[qarmin]

Errors from Godot 3.2 beta 4

ERROR: _disconnect: Attempt to disconnect signal 'connection_failed' while in emission callback. Use CONNECT_DEFERRED (to be able to safely disconnect) or CONNECT_ONESHOT (for automatic disconnection) as connection flags.
   At: core/object.cpp:1522.
ERROR: _disconnect: Attempt to disconnect signal 'connection_failed' while in emission callback. Use CONNECT_DEFERRED (to be able to safely disconnect) or CONNECT_ONESHOT (for automatic disconnection) as connection flags.
   At: core/object.cpp:1522.
ERROR: ~Object: Object was freed or unreferenced while signal 'connection_failed' is being emitted from it. Try connecting to the signal using 'CONNECT_DEFERRED' flag, or use queue_free() to free the object (if this object is a Node) to avoid this error and potential crashes.
   At: core/object.cpp:1955.

[qarmin]

Still happens with Godot 3.2.3 rc4 with a little different backtrace

drivers/unix/net_socket_posix.cpp:714:9: runtime error: implicit conversion from type 'unsigned long' of value 4294969390 (64-bit, unsigned) to type 'int' changed the value to 2094 (32-bit, signed)
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior drivers/unix/net_socket_posix.cpp:714:9 in 
ERROR: _verify_headers: Invalid protocol or status code.
   At: modules/websocket/wsl_client.cpp:112.
ERROR: ~Object: Object [Object:1954] was freed or unreferenced while a signal is being emitted from it. Try connecting to the signal using 'CONNECT_DEFERRED' flag, or use queue_free() to free the object (if this object is a Node) to avoid this error and potential crashes.
   At: core/object.cpp:2012.
=================================================================
==45707==ERROR: AddressSanitizer: heap-use-after-free on address 0x621000070969 at pc 0x0000122c8170 bp 0x7fff4295b350 sp 0x7fff4295b348
WRITE of size 1 at 0x621000070969 thread T0
    #0 0x122c816f in Object::emit_signal(StringName const&, Variant const**, int) /mnt/Miecz/godot3.2/core/object.cpp:1250:14
    #1 0x122c12c2 in Object::emit_signal(StringName const&, Variant const&, Variant const&, Variant const&, Variant const&, Variant const&) /mnt/Miecz/godot3.2/core/object.cpp:1306:9
    #2 0x349db0f in WebSocketClient::_on_error() /mnt/Miecz/godot3.2/modules/websocket/websocket_client.cpp:139:3
    #3 0x352ae92 in WSLClient::_do_handshake() /mnt/Miecz/godot3.2/modules/websocket/wsl_client.cpp:82:6
    #4 0x3535067 in WSLClient::poll() /mnt/Miecz/godot3.2/modules/websocket/wsl_client.cpp:269:4
    #5 0x12e152f8 in MultiplayerAPI::poll() /mnt/Miecz/godot3.2/core/io/multiplayer_api.cpp:100:16
    #6 0xbde6d0e in SceneTree::idle(float) /mnt/Miecz/godot3.2/scene/main/scene_tree.cpp:515:16
    #7 0x1f97b4b in Main::iteration() /mnt/Miecz/godot3.2/main/main.cpp:2107:44
    #8 0x1e5f130 in OS_X11::run() /mnt/Miecz/godot3.2/platform/x11/os_x11.cpp:3233:7
    #9 0x1dd853d in main /mnt/Miecz/godot3.2/platform/x11/godot_x11.cpp:56:6
    #10 0x7f805f3030b2 in __libc_start_main /build/glibc-YYA7BZ/glibc-2.31/csu/../csu/libc-start.c:308:16
    #11 0x1d2d84d in _start (/usr/bin/godots+0x1d2d84d)

[Faless]

@qarmin this can't be fixed in godot, must be fixed in demos

[qarmin]

For me it is strange that GDscript code(even invalid) may crash project in editor due invalid using of memory.

[Faless]

@qarmin but there isn't much we can do, it's like calling free in the script of a Node during processing. The only thing we can do, is warn the user not to do that.