Add test CI - Githubissues

qarmin commented 3 years ago

In this CI, Godex is compiled with address sanitizer.

Also editor is opened and closed.

As test project I choose - https://github.com/qarmin/RegressionTestProject/ - but it can be removed or changed(new project should be reproducible)

Not sure if I properly compiled Godex but I see this crash in unit tests

 =================================================================
==15399==ERROR: AddressSanitizer: heap-use-after-free on address 0x6060000132a0 at pc 0x000004494b39 bp 0x7ffef7b79940 sp 0x7ffef7b79930
READ of size 8 at 0x6060000132a0 thread T0
    #0 0x4494b38 in List<ExecutionGraph::StageNode, DefaultAllocator>::Element::next() core/templates/list.h:71
    #1 0x449086e in PipelineBuilder::optimize_stages(ExecutionGraph*) /home/runner/work/godex/godex/modules/godex/pipeline/pipeline_builder.cpp:932
    #2 0x447b2c0 in PipelineBuilder::build_graph(Vector<StringName> const&, Vector<StringName> const&, ExecutionGraph*, bool) /home/runner/work/godex/godex/modules/godex/pipeline/pipeline_builder.cpp:254
    #3 0x447b75a in PipelineBuilder::build_pipeline(Vector<StringName> const&, Vector<StringName> const&, Pipeline*) /home/runner/work/godex/godex/modules/godex/pipeline/pipeline_builder.cpp:267
    #4 0x2a88e5a in _DOCTEST_ANON_FUNC_4170 /home/runner/work/godex/godex/modules/godex/tests/test_ecs_pipeline_builder.h:1300
    #5 0x338639e in doctest::Context::run() thirdparty/doctest/doctest.h:6291
    #6 0x2c39672 in test_main(int, char**) tests/test_main.cpp:147
    #7 0x24d88f5 in Main::test_entrypoint(int, char**, bool&) main/main.cpp:495
    #8 0x23a6ec8 in main platform/linuxbsd/godot_linuxbsd.cpp:45
    #9 0x7f02abc8b0b2 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x270b2)
    #10 0x23a6cbd in _start (/home/runner/work/godex/godex/godot/bin/godot.linuxbsd.tools.64s+0x23a6cbd)

0x6060000132a0 is located 32 bytes inside of 56-byte region [0x606000013280,0x6060000132b8)
freed by thread T0 here:
    #0 0x7f02acbcd7cf in __interceptor_free (/lib/x86_64-linux-gnu/libasan.so.5+0x10d7cf)
    #1 0x17e32b2e in Memory::free_static(void*, bool) core/os/memory.cpp:168
    #2 0x23ab268 in DefaultAllocator::free(void*) core/os/memory.h:66
    #3 0x44a0e3b in void memdelete_allocator<List<ExecutionGraph::StageNode, DefaultAllocator>::Element, DefaultAllocator>(List<ExecutionGraph::StageNode, DefaultAllocator>::Element*) core/os/memory.h:124
    #4 0x449e830 in List<ExecutionGraph::StageNode, DefaultAllocator>::_Data::erase(List<ExecutionGraph::StageNode, DefaultAllocator>::Element const*) core/templates/list.h:241
    #5 0x449b070 in List<ExecutionGraph::StageNode, DefaultAllocator>::Element::erase() (/home/runner/work/godex/godex/godot/bin/godot.linuxbsd.tools.64s+0x449b070)
    #6 0x449083d in PipelineBuilder::optimize_stages(ExecutionGraph*) /home/runner/work/godex/godex/modules/godex/pipeline/pipeline_builder.cpp:934
    #7 0x447b2c0 in PipelineBuilder::build_graph(Vector<StringName> const&, Vector<StringName> const&, ExecutionGraph*, bool) /home/runner/work/godex/godex/modules/godex/pipeline/pipeline_builder.cpp:254
    #8 0x447b75a in PipelineBuilder::build_pipeline(Vector<StringName> const&, Vector<StringName> const&, Pipeline*) /home/runner/work/godex/godex/modules/godex/pipeline/pipeline_builder.cpp:267
    #9 0x2a88e5a in _DOCTEST_ANON_FUNC_4170 /home/runner/work/godex/godex/modules/godex/tests/test_ecs_pipeline_builder.h:1300
    #10 0x338639e in doctest::Context::run() thirdparty/doctest/doctest.h:6291
    #11 0x2c39672 in test_main(int, char**) tests/test_main.cpp:147
    #12 0x24d88f5 in Main::test_entrypoint(int, char**, bool&) main/main.cpp:495
    #13 0x23a6ec8 in main platform/linuxbsd/godot_linuxbsd.cpp:45
    #14 0x7f02abc8b0b2 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x270b2)

previously allocated by thread T0 here:
    #0 0x7f02acbcdbc8 in malloc (/lib/x86_64-linux-gnu/libasan.so.5+0x10dbc8)
    #1 0x17e31aeb in Memory::alloc_static(unsigned long, bool) core/os/memory.cpp:75
    #2 0x23ab245 in DefaultAllocator::alloc(unsigned long) core/os/memory.h:65
    #3 0x17e31a1f in operator new(unsigned long, void* (*)(unsigned long)) core/os/memory.cpp:44
    #4 0x449a653 in List<ExecutionGraph::StageNode, DefaultAllocator>::push_back(ExecutionGraph::StageNode const&) (/home/runner/work/godex/godex/godot/bin/godot.linuxbsd.tools.64s+0x449a653)
    #5 0x448ebd7 in internal_build_stages(Ref<ExecutionGraph::Dispatcher>) /home/runner/work/godex/godex/modules/godex/pipeline/pipeline_builder.cpp:819
    #6 0x448f813 in PipelineBuilder::build_stages(ExecutionGraph*) /home/runner/work/godex/godex/modules/godex/pipeline/pipeline_builder.cpp:844
    #7 0x447b2b1 in PipelineBuilder::build_graph(Vector<StringName> const&, Vector<StringName> const&, ExecutionGraph*, bool) /home/runner/work/godex/godex/modules/godex/pipeline/pipeline_builder.cpp:253
    #8 0x447b75a in PipelineBuilder::build_pipeline(Vector<StringName> const&, Vector<StringName> const&, Pipeline*) /home/runner/work/godex/godex/modules/godex/pipeline/pipeline_builder.cpp:267
    #9 0x2a88e5a in _DOCTEST_ANON_FUNC_4170 /home/runner/work/godex/godex/modules/godex/tests/test_ecs_pipeline_builder.h:1300
    #10 0x338639e in doctest::Context::run() thirdparty/doctest/doctest.h:6291
    #11 0x2c39672 in test_main(int, char**) tests/test_main.cpp:147
    #12 0x24d88f5 in Main::test_entrypoint(int, char**, bool&) main/main.cpp:495
    #13 0x23a6ec8 in main platform/linuxbsd/godot_linuxbsd.cpp:45
    #14 0x7f02abc8b0b2 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x270b2)

SUMMARY: AddressSanitizer: heap-use-after-free core/templates/list.h:71 in List<ExecutionGraph::StageNode, DefaultAllocator>::Element::next()

AndreaCatania commented 3 years ago

Really nice! In order to fix the editor crash, on close, it's necessary to merge this: https://github.com/godotengine/godot/pull/50179 or port Godex to GDExtension. To use GDExtension is needed this feature https://github.com/godotengine/godot/pull/42875 but for GDExtension, reduz is planning to add it at some point before 4.0 is out.

AndreaCatania commented 3 years ago

Really nice! In order to fix the editor crash, on close, it's necessary to merge this: https://github.com/godotengine/godot/pull/50179 or port Godex to GDExtension. To use GDExtension is needed this feature https://github.com/godotengine/godot/pull/42875 but for GDExtension, reduz is planning to add it at some point before 4.0 is out.

qarmin commented 3 years ago

This crash is related to unit test(editor crashes will be visible later) I think that this is related to this lines, because when iterating over collection, its elements are deleted https://github.com/GodotECS/godex/blob/855d1d7e1c1072faf10f8e751b0ad50fa9fb30ed/pipeline/pipeline_builder.cpp#L932-L936

qarmin commented 2 years ago

I added also fuzzer support, but for now crashes due to GDScript bug, because Godot used in Godex is a little too old(1 month behind Godot master branch)

AndreaCatania commented 2 years ago

Yup, I need to update it. Also the reason why I'm not merging this is because I'm waiting that the custom iterator is supported in godot. In theory it should arrive soon 🤞

AndreaCatania commented 2 years ago

I've updated to latest godot, can you please squash the commits? So we can also trigger again the CI, and see if it pass now.

qarmin commented 2 years ago

After rebasing, test project still crash when running project unit tests - https://github.com/GodotECS/godex/issues/272 (https://github.com/qarmin/godex/runs/4475133649?check_suite_focus=true)

Fuzzer crashes when running this code

    var temp_variable64 = ClassDB.instantiate("DatabagDynamicFetcher")
    temp_variable64.get_meta_list()
    temp_variable64.can_translate_messages()
    temp_variable64.connect(StringName("."), Callable(BoxMesh.new(),""), Array([]), 79)
    temp_variable64.is_class(".")
    temp_variable64.has_meta(StringName("3971291559"))
    temp_variable64.set_meta(StringName("1684871696"), null)
    temp_variable64.is_connected(StringName("1454454129"), Callable(BoxMesh.new(),""))
    temp_variable64.set(".", Array([]))
    temp_variable64.is_valid()
    temp_variable64.disconnect(StringName("3647961650"), Callable(BoxMesh.new(),""))
    temp_variable64.has_user_signal(StringName("."))
    temp_variable64.to_string()
    temp_variable64.get_class()
    temp_variable64.add_user_signal(".", Array([]))
    temp_variable64.set_indexed(NodePath("2420409340"), Array([]))
    temp_variable64.tr_n(StringName("."), StringName("2960212441"), 85, StringName("3784503530"))
    temp_variable64.get_signal_connection_list(".")
    temp_variable64.get_meta_list()
    temp_variable64.can_translate_messages()
    temp_variable64.connect(StringName("2370181585"), Callable(BoxMesh.new(),""), Array([]), -62)
    temp_variable64.has_method(StringName("."))
    temp_variable64.get("1286033485")

ERROR: Index p_index = 3200171710 is out of bounds (count = 23).
   at: operator[] (./core/templates/local_vector.h:163)

================================================================
handle_crash: Program crashed with signal 4
Engine version: Godot Engine v4.0.dev.custom_build (534e1d525189bd2aa80e0408eeda3b4898b1f977)
Dumping the backtrace. Please include this when reporting the bug on https://github.com/godotengine/godot/issues
[1] ./godot/bin/godot.linuxbsd.tools.64s(+0x342b0170) [0x5653634f0170] (??:0)
[2] /lib/x86_64-linux-gnu/libc.so.6(+0x46210) [0x7ff9314dc210] (??:0)
[3] LocalVector<DatabagInfo, unsigned int, false>::operator[](unsigned int) (??:0)
[4] ECS::unsafe_databag_get_by_name(unsigned int, void const*, StringName const&, Variant&) (??:0)
[5] DatabagDynamicFetcher::_get(StringName const&, Variant&) const (??:0)
[6] DatabagDynamicFetcher::_getv(StringName const&, Variant&) const (??:0)
[7] Object::get(StringName const&, bool*) const (??:0)
[8] Object::_get_bind(String const&) const (??:0)
[9] void call_with_variant_args_retc_helper<__UnexistingClass, Variant, String const&, 0ul>(__UnexistingClass*, Variant (__UnexistingClass::*)(String const&) const, Variant const**, Variant&, Callable::CallError&, IndexSequence<0ul>) (??:0)
[10] void call_with_variant_args_retc_dv<__UnexistingClass, Variant, String const&>(__UnexistingClass*, Variant (__UnexistingClass::*)(String const&) const, Variant const**, int, Variant&, Callable::CallError&, Vector<Variant> const&) (??:0)
[11] MethodBindTRC<Variant, String const&>::call(Object*, Variant const**, int, Callable::CallError&) (??:0)
[12] Object::call(StringName const&, Variant const**, int, Callable::CallError&) (??:0)
[13] DatabagDynamicFetcher::call(StringName const&, Variant const**, int, Callable::CallError&) (??:0)
[14] Object::callv(StringName const&, Array const&) (??:0)
[15] void call_with_variant_args_ret_helper<__UnexistingClass, Variant, StringName const&, Array const&, 0ul, 1ul>(__UnexistingClass*, Variant (__UnexistingClass::*)(StringName const&, Array const&), Variant const**, Variant&, Callable::CallError&, IndexSequence<0ul, 1ul>) (??:0)
[16] void call_with_variant_args_ret_dv<__UnexistingClass, Variant, StringName const&, Array const&>(__UnexistingClass*, Variant (__UnexistingClass::*)(StringName const&, Array const&), Variant const**, int, Variant&, Callable::CallError&, Vector<Variant> const&) (??:0)
[17] MethodBindTR<Variant, StringName const&, Array const&>::call(Object*, Variant const**, int, Callable::CallError&) (??:0)
[18] GDScriptFunction::call(GDScriptInstance*, Variant const**, int, Callable::CallError&, GDScriptFunction::CallState*) (??:0)
[19] GDScriptInstance::call(StringName const&, Variant const**, int, Callable::CallError&) (??:0)
[20] Object::call(StringName const&, Variant const**, int, Callable::CallError&) (??:0)
[21] Variant::call(StringName const&, Variant const**, int, Variant&, Callable::CallError&) (??:0)
[22] GDScriptFunction::call(GDScriptInstance*, Variant const**, int, Callable::CallError&, GDScriptFunction::CallState*) (??:0)
[23] GDScriptInstance::call(StringName const&, Variant const**, int, Callable::CallError&) (??:0)
[24] Node::_gdvirtual__process_call(double) (??:0)
[25] Node::_notification(int) (??:0)
[26] Node::_notificationv(int, bool) (??:0)
[27] Object::notification(int, bool) (??:0)
[28] SceneTree::_notify_group_pause(StringName const&, int) (??:0)
[29] SceneTree::process(double) (??:0)
[30] Main::iteration() (??:0)
[31] OS_LinuxBSD::run() (??:0)
[32] ./godot/bin/godot.linuxbsd.tools.64s(main+0x414) [0x5653634eda7d] (??:0)
[33] /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf3) [0x7ff9314bd0b3] (??:0)
[34] ./godot/bin/godot.linuxbsd.tools.64s(_start+0x2e) [0x5653634ed5ae] (??:0)
-- END OF BACKTRACE --
================================================================

https://github.com/qarmin/godex/actions/runs/1560321377

AndreaCatania commented 2 years ago

It's kind of expected thought, it's calling the function unsafe_databag_get_by_name, and it's expected that if the name doesn't exist it crashes. Those APIs are supposed to be called by internal stuff, like the DynamicQuery, that knows how to use it. I think the test should not use that API, at lest not in that way or directly.

Do you think it's possible to change that?

qarmin commented 2 years ago

I added get to excluded functions(fuzzer_config.txt file) Other crash

    var temp_variable119 = ClassDB.instantiate("DynamicQuery")
    temp_variable119.is_connected(StringName("2437906285"), Callable(BoxMesh.new(),""))
    temp_variable119.fetch(89)
    temp_variable119.not_component(16)
    temp_variable119.get_signal_list()
    temp_variable119.can_translate_messages()
    temp_variable119.maybe_component(88, false)
    temp_variable119.notify_property_list_changed()
    temp_variable119.reset()
    temp_variable119.tr(StringName("."), StringName("."))
    temp_variable119.set_message_translation(true)
    temp_variable119.end()
    temp_variable119.get_component(-30)
    temp_variable119.get_meta(StringName("149950933"))
    temp_variable119.changed_component(8, true)
    temp_variable119.to_string()
    temp_variable119.get_script()
    temp_variable119.get_current_entity_id()
    temp_variable119.has_user_signal(StringName("1481896810"))
    temp_variable119.get_signal_connection_list("3824683422")
    temp_variable119.set_deferred(StringName("."), Translation.new())
    temp_variable119.is_blocking_signals()
    temp_variable119.set_space(-28)
    temp_variable119.is_class(".")
    temp_variable119.has(-1)
    temp_variable119.with_component(62, false)
    temp_variable119.prepare_world(null)
    temp_variable119.has_signal(StringName("878858018"))
    temp_variable119.get_meta_list()
    temp_variable119.maybe_component(-98, false)
    temp_variable119.connect(StringName("."), Callable(BoxMesh.new(),""), Array([]), -60)
    temp_variable119.get_class()
    temp_variable119.is_queued_for_deletion()
    temp_variable119.tr(StringName("2009425479"), StringName("1737654357"))
    temp_variable119.set_message_translation(true)
    temp_variable119.end()

cause

ERROR: Index p_index = 4294967295 is out of bounds (count = 0).
   at: operator[] (./core/templates/local_vector.h:163)

================================================================
handle_crash: Program crashed with signal 4
Engine version: Godot Engine v4.0.dev.custom_build (534e1d525189bd2aa80e0408eeda3b4898b1f977)
Dumping the backtrace. Please include this when reporting the bug on https://github.com/godotengine/godot/issues
[1] ./godot/bin/godot.linuxbsd.tools.64s(+0x342b0170) [0x55ef44505170] (??:0)
[2] /lib/x86_64-linux-gnu/libc.so.6(+0x46210) [0x7fd4b2803210] (??:0)
[3] LocalVector<EntityList, unsigned int, false>::operator[](unsigned int) (??:0)
[4] godex::DynamicQuery::conclude_process(World*) (??:0)
[5] godex::DynamicQuery::end_script() (??:0)
[6] void call_with_variant_args_helper<__UnexistingClass>(__UnexistingClass*, void (__UnexistingClass::*)(), Variant const**, Callable::CallError&, IndexSequence<>) (??:0)
[7] void call_with_variant_args_dv<__UnexistingClass>(__UnexistingClass*, void (__UnexistingClass::*)(), Variant const**, int, Callable::CallError&, Vector<Variant> const&) (??:0)
[8] MethodBindT<>::call(Object*, Variant const**, int, Callable::CallError&) (??:0)
[9] Object::call(StringName const&, Variant const**, int, Callable::CallError&) (??:0)
[10] Object::callv(StringName const&, Array const&) (??:0)
[11] void call_with_variant_args_ret_helper<__UnexistingClass, Variant, StringName const&, Array const&, 0ul, 1ul>(__UnexistingClass*, Variant (__UnexistingClass::*)(StringName const&, Array const&), Variant const**, Variant&, Callable::CallError&, IndexSequence<0ul, 1ul>) (??:0)
[12] void call_with_variant_args_ret_dv<__UnexistingClass, Variant, StringName const&, Array const&>(__UnexistingClass*, Variant (__UnexistingClass::*)(StringName const&, Array const&), Variant const**, int, Variant&, Callable::CallError&, Vector<Variant> const&) (??:0)
[13] MethodBindTR<Variant, StringName const&, Array const&>::call(Object*, Variant const**, int, Callable::CallError&) (??:0)
[14] GDScriptFunction::call(GDScriptInstance*, Variant const**, int, Callable::CallError&, GDScriptFunction::CallState*) (??:0)
[15] GDScriptInstance::call(StringName const&, Variant const**, int, Callable::CallError&) (??:0)
[16] Object::call(StringName const&, Variant const**, int, Callable::CallError&) (??:0)
[17] Variant::call(StringName const&, Variant const**, int, Variant&, Callable::CallError&) (??:0)
[18] GDScriptFunction::call(GDScriptInstance*, Variant const**, int, Callable::CallError&, GDScriptFunction::CallState*) (??:0)
[19] GDScriptInstance::call(StringName const&, Variant const**, int, Callable::CallError&) (??:0)
[20] Node::_gdvirtual__process_call(double) (??:0)
[21] Node::_notification(int) (??:0)
[22] Node::_notificationv(int, bool) (??:0)
[23] Object::notification(int, bool) (??:0)
[24] SceneTree::_notify_group_pause(StringName const&, int) (??:0)
[25] SceneTree::process(double) (??:0)
[26] Main::iteration() (??:0)
[27] OS_LinuxBSD::run() (??:0)
[28] ./godot/bin/godot.linuxbsd.tools.64s(main+0x414) [0x55ef44502a7d] (??:0)
[29] /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf3) [0x7fd4b27e40b3] (??:0)
[30] ./godot/bin/godot.linuxbsd.tools.64s(_start+0x2e) [0x55ef445025ae] (??:0)

AndreaCatania commented 2 years ago

Same, godex::DynamicQuery is created assuming that it's impossible to use it directly. It's almost always wrapped by something else, so you can't mess with it. If i make it safe, will pass this CI but will be slower for no reason.

qarmin commented 2 years ago

So which classes from this can user use?

Component
ComponentDynamicExposer
Components3DGizmoPlugin
DatabagDynamicFetcher
DynamicQuery
ECS
Entity2D
Entity3D
EventsEmitterDynamicFetcher
EventsReceiverDynamicFetcher
GodexWorldFetcher
PipelineECS
ScriptEcs
SharedComponentResource
StorageDynamicFetcher
System
SystemBundle
TextServerFallback
WorldECS

AndreaCatania commented 2 years ago

Component: All the static functions are not supposed to be used by the user. So I would say all are unsafe. https://github.com/GodotECS/godex/blob/main/components/component.h
ComponentDynamicExposer: This is safe to use when passed by a WorldECS, World, Query. (There is no other way to obtain that object in GDS, if not using those classes) https://github.com/GodotECS/godex/blob/main/utils/fetchers.h
Components3DGizmoPlugin: Not for user usage. https://github.com/GodotECS/godex/blob/main/modules/godot/editor_plugins/components_gizmo_3d.h
DatabagDynamicFetcher: This is safe to use when passed by a WorldECS, World, Query. (There is no other way to obtain that object in GDS, if not using those classes) https://github.com/GodotECS/godex/blob/main/utils/fetchers.h
DynamicQuery: This is always safe. (I already saw that we need some extra null pointer checks however). https://github.com/GodotECS/godex/blob/main/iterators/dynamic_query.h
ECS: All are safe with exception of the one marked with unsafe_ (those are internal for internal use only). https://github.com/GodotECS/godex/blob/main/ecs.h
Entity2D: Safe https://github.com/GodotECS/godex/blob/main/modules/godot/nodes/entity.h
Entity3D: Safe https://github.com/GodotECS/godex/blob/main/modules/godot/nodes/entity.h
EventsEmitterDynamicFetcher: This is safe to use when passed by a WorldECS, World, Query. (There is no other way to obtain that object in GDS, if not using those classes) https://github.com/GodotECS/godex/blob/main/utils/fetchers.h
EventsReceiverDynamicFetcher: This is safe to use when passed by a WorldECS, World, Query. (There is no other way to obtain that object in GDS, if not using those classes) https://github.com/GodotECS/godex/blob/main/utils/fetchers.h
GodexWorldFetcher: This is safe to use when passed by a WorldECS, World, Query. (There is no other way to obtain that object in GDS, if not using those classes) https://github.com/GodotECS/godex/blob/main/utils/fetchers.h
PipelineECS: This is special. Generally we have to treat this as always safe. If some API crashes, we have to evaluate if worth make the function safe. https://github.com/GodotECS/godex/blob/main/modules/godot/nodes/ecs_world.h
ScriptEcs: The user doesn't have access to this class. https://github.com/GodotECS/godex/blob/main/modules/godot/nodes/script_ecs.h
SharedComponentResource:
StorageDynamicFetcher: This is safe to use when passed by a WorldECS, World, Query. (There is no other way to obtain that object in GDS, if not using those classes) https://github.com/GodotECS/godex/blob/main/utils/fetchers.h
System: This should be created inside the _prepare function. However we can assume all functions are safe, with exception of the static methods. https://github.com/GodotECS/godex/blob/main/modules/godot/nodes/ecs_utilities.h
SystemBundle: This should be created inside the _prepare function. However we can assume all functions are safe, with exception of the static methods. https://github.com/GodotECS/godex/blob/main/modules/godot/nodes/ecs_utilities.h
TextServerFallback: Not sure about this. Do you have the link to the file?
WorldECS: Safe. https://github.com/GodotECS/godex/blob/main/modules/godot/nodes/ecs_world.h

GodotECS / godex

Add test CI #271