serenity icon indicating copy to clipboard operation
serenity copied to clipboard
verified publisher icon SerenityOS

LibWeb: Illegal instruction when destroying main thread VM

Open ADKaster opened this issue 3 years ago • 0 comments

A month or so ago, Andreas leaked a ref of the main thread vm "to prevent unecessary GC on process exit", but in doing so, we can now enjoy extra bugs!

Commenting out the offending reference leak:

https://github.com/SerenityOS/serenity/blob/a934fa3d28f15ff01722b927e94c20265fd38277/Userland/Libraries/LibWeb/Bindings/MainThreadVM.cpp#L56-L58

Results in some fun behavior in Ladybird in particular, when run through valgrind:

The log vvvv 
==456627== Invalid read of size 8
==456627==    at 0x5EEE1F9: remove_first_matching<(lambda at /home/andrew/serenity/Userland/Libraries/LibWeb/HTML/EventLoop/EventLoop.cpp:363:84)> (serenity/AK/Vector.h:420)
==456627==    by 0x5EEE1F9: Web::HTML::EventLoop::unregister_environment_settings_object(AK::Badge<Web::HTML::EnvironmentSettingsObject>, Web::HTML::EnvironmentSettingsObject&) (serenity/Userland/Libraries/LibWeb/HTML/EventLoop/EventLoop.cpp:363)
==456627==    by 0x5F3EB26: Web::HTML::EnvironmentSettingsObject::~EnvironmentSettingsObject() (serenity/Userland/Libraries/LibWeb/HTML/Scripting/Environments.cpp:27)
==456627==    by 0x5F4341D: Web::HTML::WindowEnvironmentSettingsObject::~WindowEnvironmentSettingsObject() (serenity/Userland/Libraries/LibWeb/HTML/Scripting/WindowEnvironmentSettingsObject.cpp:21)
==456627==    by 0x5940A3B: AK::OwnPtr<Web::HTML::EnvironmentSettingsObject>::clear() (serenity/AK/OwnPtr.h:108)
==456627==    by 0x59406CD: Web::Bindings::HostDefined::~HostDefined() (serenity/Userland/Libraries/LibWeb/Bindings/HostDefined.cpp:21)
==456627==    by 0x595FB8B: AK::OwnPtr<JS::Realm::HostDefined>::clear() (serenity/AK/OwnPtr.h:108)
==456627==    by 0x8E02651: ~OwnPtr (serenity/AK/OwnPtr.h:44)
==456627==    by 0x8E02651: JS::Realm::~Realm() (serenity/Userland/Libraries/LibJS/Runtime/Realm.h:20)
==456627==    by 0x8C447DB: JS::HeapBlock::deallocate(JS::Cell*) (serenity/Userland/Libraries/LibJS/Heap/HeapBlock.cpp:49)
==456627==    by 0x8C3A30D: operator() (serenity/Userland/Libraries/LibJS/Heap/Heap.cpp:251)
==456627==    by 0x8C3A30D: operator()<JS::Cell> (serenity/Userland/Libraries/LibJS/Heap/HeapBlock.h:65)
==456627==    by 0x8C3A30D: for_each_cell<(lambda at /home/andrew/serenity/Userland/Libraries/LibJS/Heap/HeapBlock.h:63:23)> (serenity/Userland/Libraries/LibJS/Heap/HeapBlock.h:57)
==456627==    by 0x8C3A30D: for_each_cell_in_state<JS::Cell::State::Live, (lambda at /home/andrew/serenity/Userland/Libraries/LibJS/Heap/Heap.cpp:248:66)> (serenity/Userland/Libraries/LibJS/Heap/HeapBlock.h:63)
==456627==    by 0x8C3A30D: auto JS::Heap::sweep_dead_cells(bool, Core::ElapsedTimer const&)::$_2::operator()<JS::HeapBlock>(JS::HeapBlock&) const (serenity/Userland/Libraries/LibJS/Heap/Heap.cpp:248)
==456627==    by 0x8C38C30: for_each_block<(lambda at /home/andrew/serenity/Userland/Libraries/LibJS/Heap/Heap.cpp:245:20)> (serenity/Userland/Libraries/LibJS/Heap/CellAllocator.h:29)
==456627==    by 0x8C38C30: for_each_block<(lambda at /home/andrew/serenity/Userland/Libraries/LibJS/Heap/Heap.cpp:245:20)> (serenity/Userland/Libraries/LibJS/Heap/Heap.h:95)
==456627==    by 0x8C38C30: JS::Heap::sweep_dead_cells(bool, Core::ElapsedTimer const&) (serenity/Userland/Libraries/LibJS/Heap/Heap.cpp:245)
==456627==    by 0x8C38297: JS::Heap::collect_garbage(JS::Heap::CollectionType, bool) (serenity/Userland/Libraries/LibJS/Heap/Heap.cpp:109)
==456627==    by 0x8C381B9: JS::Heap::~Heap() (serenity/Userland/Libraries/LibJS/Heap/Heap.cpp:61)
==456627==  Address 0x13618a08 is 152 bytes inside a block of size 248 free'd
==456627==    at 0x484B8AF: operator delete(void*) (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==456627==    by 0x595E08B: AK::OwnPtr<JS::VM::CustomData>::clear() (serenity/AK/OwnPtr.h:108)
==456627==    by 0x5959D04: ~OwnPtr (serenity/AK/OwnPtr.h:44)
==456627==    by 0x5959D04: JS::VM::~VM() (serenity/Userland/Libraries/LibJS/Runtime/VM.h:42)
==456627==    by 0x5959C9C: AK::RefCounted<JS::VM>::unref() const (serenity/AK/RefCounted.h:66)
==456627==    by 0x5958EF3: unref_if_not_null<JS::VM> (serenity/AK/NonnullRefPtr.h:35)
==456627==    by 0x5958EF3: clear (serenity/AK/RefPtr.h:216)
==456627==    by 0x5958EF3: AK::RefPtr<JS::VM>::~RefPtr() (serenity/AK/RefPtr.h:102)
==456627==    by 0xAF1F494: __run_exit_handlers (exit.c:113)
==456627==    by 0xAF1F60F: exit (exit.c:143)
==456627==    by 0xAF03D96: (below main) (libc_start_call_main.h:74)
==456627==  Block was alloc'd at
==456627==    at 0x4849013: operator new(unsigned long) (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==456627==    by 0x5953283: make<Web::Bindings::WebEngineCustomData> (serenity/AK/NonnullOwnPtr.h:161)
==456627==    by 0x5953283: Web::Bindings::main_thread_vm() (serenity/Userland/Libraries/LibWeb/Bindings/MainThreadVM.cpp:56)
==456627==    by 0x5EEC0A7: Web::HTML::main_thread_event_loop() (serenity/Userland/Libraries/LibWeb/HTML/EventLoop/EventLoop.cpp:49)
==456627==    by 0x5B4126F: Web::HTML::BrowsingContext::create_a_new_browsing_context(Web::Page&, JS::GCPtr<Web::DOM::Document>, JS::GCPtr<Web::DOM::Element>, Web::HTML::BrowsingContextGroup&) (serenity/Userland/Libraries/LibWeb/HTML/BrowsingContext.cpp:111)
==456627==    by 0x5B62731: Web::HTML::BrowsingContextGroup::create_a_new_browsing_context_group(Web::Page&) (serenity/Userland/Libraries/LibWeb/HTML/BrowsingContextGroup.cpp:38)
==456627==    by 0x5B4100E: Web::HTML::BrowsingContext::create_a_new_top_level_browsing_context(Web::Page&) (serenity/Userland/Libraries/LibWeb/HTML/BrowsingContext.cpp:97)
==456627==    by 0x5DC42B1: Web::Page::Page(Web::PageClient&) (serenity/Userland/Libraries/LibWeb/Page/Page.cpp:16)
==456627==    by 0x204D44: AK::NonnullOwnPtr<Web::Page> AK::make<Web::Page, Ladybird::PageClientLadybird&>(Ladybird::PageClientLadybird&) (serenity/AK/NonnullOwnPtr.h:161)
==456627==    by 0x200816: Ladybird::PageClientLadybird::PageClientLadybird(SimpleWebView&) (PageClientLadybird.cpp:33)
==456627==    by 0x200794: Ladybird::PageClientLadybird::create(SimpleWebView&) (PageClientLadybird.cpp:28)
==456627==    by 0x213036: SimpleWebView::SimpleWebView() (SimpleWebView.cpp:79)
==456627==    by 0x226D9C: Tab::Tab(BrowserWindow*) (Tab.cpp:29)
==456627== 
==456627== valgrind: Unrecognised instruction at address 0x5eee32a.
==456627==    at 0x5EEE32A: Web::HTML::EventLoop::unregister_environment_settings_object(AK::Badge<Web::HTML::EnvironmentSettingsObject>, Web::HTML::EnvironmentSettingsObject&) (serenity/Userland/Libraries/LibWeb/HTML/EventLoop/EventLoop.cpp:364)
==456627==    by 0x5F3EB26: Web::HTML::EnvironmentSettingsObject::~EnvironmentSettingsObject() (serenity/Userland/Libraries/LibWeb/HTML/Scripting/Environments.cpp:27)
==456627==    by 0x5F4341D: Web::HTML::WindowEnvironmentSettingsObject::~WindowEnvironmentSettingsObject() (serenity/Userland/Libraries/LibWeb/HTML/Scripting/WindowEnvironmentSettingsObject.cpp:21)
==456627==    by 0x5940A3B: AK::OwnPtr<Web::HTML::EnvironmentSettingsObject>::clear() (serenity/AK/OwnPtr.h:108)
==456627==    by 0x59406CD: Web::Bindings::HostDefined::~HostDefined() (serenity/Userland/Libraries/LibWeb/Bindings/HostDefined.cpp:21)
==456627==    by 0x595FB8B: AK::OwnPtr<JS::Realm::HostDefined>::clear() (serenity/AK/OwnPtr.h:108)
==456627==    by 0x8E02651: ~OwnPtr (serenity/AK/OwnPtr.h:44)
==456627==    by 0x8E02651: JS::Realm::~Realm() (serenity/Userland/Libraries/LibJS/Runtime/Realm.h:20)
==456627==    by 0x8C447DB: JS::HeapBlock::deallocate(JS::Cell*) (serenity/Userland/Libraries/LibJS/Heap/HeapBlock.cpp:49)
==456627==    by 0x8C3A30D: operator() (serenity/Userland/Libraries/LibJS/Heap/Heap.cpp:251)
==456627==    by 0x8C3A30D: operator()<JS::Cell> (serenity/Userland/Libraries/LibJS/Heap/HeapBlock.h:65)
==456627==    by 0x8C3A30D: for_each_cell<(lambda at /home/andrew/serenity/Userland/Libraries/LibJS/Heap/HeapBlock.h:63:23)> (serenity/Userland/Libraries/LibJS/Heap/HeapBlock.h:57)
==456627==    by 0x8C3A30D: for_each_cell_in_state<JS::Cell::State::Live, (lambda at /home/andrew/serenity/Userland/Libraries/LibJS/Heap/Heap.cpp:248:66)> (serenity/Userland/Libraries/LibJS/Heap/HeapBlock.h:63)
==456627==    by 0x8C3A30D: auto JS::Heap::sweep_dead_cells(bool, Core::ElapsedTimer const&)::$_2::operator()<JS::HeapBlock>(JS::HeapBlock&) const (serenity/Userland/Libraries/LibJS/Heap/Heap.cpp:248)
==456627==    by 0x8C38C30: for_each_block<(lambda at /home/andrew/serenity/Userland/Libraries/LibJS/Heap/Heap.cpp:245:20)> (serenity/Userland/Libraries/LibJS/Heap/CellAllocator.h:29)
==456627==    by 0x8C38C30: for_each_block<(lambda at /home/andrew/serenity/Userland/Libraries/LibJS/Heap/Heap.cpp:245:20)> (serenity/Userland/Libraries/LibJS/Heap/Heap.h:95)
==456627==    by 0x8C38C30: JS::Heap::sweep_dead_cells(bool, Core::ElapsedTimer const&) (serenity/Userland/Libraries/LibJS/Heap/Heap.cpp:245)
==456627==    by 0x8C38297: JS::Heap::collect_garbage(JS::Heap::CollectionType, bool) (serenity/Userland/Libraries/LibJS/Heap/Heap.cpp:109)
==456627==    by 0x8C381B9: JS::Heap::~Heap() (serenity/Userland/Libraries/LibJS/Heap/Heap.cpp:61)
==456627== Your program just tried to execute an instruction that Valgrind
==456627== did not recognise.  There are two possible reasons for this.
==456627== 1. Your program has a bug and erroneously jumped to a non-code
==456627==    location.  If you are running Memcheck and you just saw a
==456627==    warning about a bad jump, it's probably your program's fault.
==456627== 2. The instruction is legitimate but Valgrind doesn't handle it,
==456627==    i.e. it's Valgrind's fault.  If you think this is the case or
==456627==    you are not sure, please let us know and we'll try to fix it.
==456627== Either way, Valgrind will now raise a SIGILL signal which will
==456627== probably kill your program.

ADKaster avatar Oct 01 '22 03:10 ADKaster