GNOME Bugzilla – Bug 784550
Segfault/hang after calling signal.clear() from signal handler
Last modified: 2017-07-17 13:51:11 UTC
The following code hangs on 2.99.8 compiled with Gcc 7.1.1: sigc::signal<void()> sig; sig.connect([&sig]() { sig.clear(); } ); sig(); Similar code (adopted to old syntax) crashes on 2.8.0 compiled with Gcc 5.2.0: sigc::signal<void> sig; sig.connect([&sig]() { sig.clear(); } ); sig(); The code works fine if instead of "sig.clear()" signal is destroyed like this "sig = decltype(sig)()" Expected result is that sig.clear() just clears all currently connected slots as documentation suggests without crashes or hangs.
Created attachment 355010 [details] [review] 0001-test_signal-Try-clearing-the-signal-during-its-handl.patch This adds the test code as a test to run during "make check". It does indeed segfault.
Here is the first problem seen by valgrind for this test case: [murrayc@murrayc-ThinkPad-X220 libsigcplusplus-master (master)]$ valgrind --num-callers=50 ./tests/.libs/test_signal ==29586== Memcheck, a memory error detector ==29586== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al. ==29586== Using Valgrind-3.12.0 and LibVEX; rerun with -h for copyright info ==29586== Command: ./tests/.libs/test_signal ==29586== ==29586== Invalid read of size 8 ==29586== at 0x11054F: std::_List_const_iterator<sigc::slot_base>::operator++() (stl_list.h:246) ==29586== by 0x110266: sigc::internal::signal_emit<void, void>::emit(std::shared_ptr<sigc::internal::signal_impl> const&) (signal.h:373) ==29586== by 0x1102E1: sigc::signal_with_accumulator<void, void>::emit() const (signal.h:464) ==29586== by 0x10CF75: (anonymous namespace)::test_clear_called_in_signal_handler() (test_signal.cc:120) ==29586== by 0x10D039: main (test_signal.cc:138) ==29586== Address 0x5cd8ed0 is 0 bytes inside a block of size 32 free'd ==29586== at 0x4C2F25B: operator delete(void*) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==29586== by 0x1120F1: __gnu_cxx::new_allocator<std::_List_node<sigc::slot_base> >::deallocate(std::_List_node<sigc::slot_base>*, unsigned long) (new_allocator.h:110) ==29586== by 0x11189D: std::allocator_traits<std::allocator<std::_List_node<sigc::slot_base> > >::deallocate(std::allocator<std::_List_node<sigc::slot_base> >&, std::_List_node<sigc::slot_base>*, unsigned long) (alloc_traits.h:462) ==29586== by 0x111059: std::__cxx11::_List_base<sigc::slot_base, std::allocator<sigc::slot_base> >::_M_put_node(std::_List_node<sigc::slot_base>*) (stl_list.h:387) ==29586== by 0x4E4D004: std::__cxx11::_List_base<sigc::slot_base, std::allocator<sigc::slot_base> >::_M_clear() (list.tcc:80) ==29586== by 0x4E4C7FB: std::__cxx11::list<sigc::slot_base, std::allocator<sigc::slot_base> >::clear() (stl_list.h:1375) ==29586== by 0x4E4B019: sigc::internal::signal_impl::clear() (signal_base.cc:81) ==29586== by 0x4E4B8DD: sigc::signal_base::clear() (signal_base.cc:236) ==29586== by 0x10CEF8: (anonymous namespace)::test_clear_called_in_signal_handler()::{lambda()#1}::operator()() const (test_signal.cc:119) ==29586== by 0x10DBFB: _ZNK4sigc15adaptor_functorIZN12_GLOBAL__N_135test_clear_called_in_signal_handlerEvEUlvE_EclIJEEEDcDpOT_ (adaptor_trait.h:99) ==29586== by 0x10DC29: sigc::internal::slot_call<(anonymous namespace)::test_clear_called_in_signal_handler()::{lambda()#1}, void>::call_it(sigc::internal::slot_rep*) (slot.h:130) ==29586== by 0x110257: sigc::internal::signal_emit<void, void>::emit(std::shared_ptr<sigc::internal::signal_impl> const&) (signal.h:378) ==29586== by 0x1102E1: sigc::signal_with_accumulator<void, void>::emit() const (signal.h:464) ==29586== by 0x10CF75: (anonymous namespace)::test_clear_called_in_signal_handler() (test_signal.cc:120) ==29586== by 0x10D039: main (test_signal.cc:138) ==29586== Block was alloc'd at ==29586== at 0x4C2E19F: operator new(unsigned long) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==29586== by 0x1121A7: __gnu_cxx::new_allocator<std::_List_node<sigc::slot_base> >::allocate(unsigned long, void const*) (new_allocator.h:104) ==29586== by 0x112024: std::allocator_traits<std::allocator<std::_List_node<sigc::slot_base> > >::allocate(std::allocator<std::_List_node<sigc::slot_base> >&, unsigned long) (alloc_traits.h:436) ==29586== by 0x11174C: std::__cxx11::_List_base<sigc::slot_base, std::allocator<sigc::slot_base> >::_M_get_node() (stl_list.h:383) ==29586== by 0x110ED3: std::_List_node<sigc::slot_base>* std::__cxx11::list<sigc::slot_base, std::allocator<sigc::slot_base> >::_M_create_node<sigc::slot_base>(sigc::slot_base&&) (stl_list.h:568) ==29586== by 0x110A50: std::_List_iterator<sigc::slot_base> std::__cxx11::list<sigc::slot_base, std::allocator<sigc::slot_base> >::emplace<sigc::slot_base>(std::_List_const_iterator<sigc::slot_base>, sigc::slot_base&&) (list.tcc:91) ==29586== by 0x110461: std::__cxx11::list<sigc::slot_base, std::allocator<sigc::slot_base> >::insert(std::_List_const_iterator<sigc::slot_base>, sigc::slot_base&&) (stl_list.h:1186) ==29586== by 0x4E4B51F: sigc::internal::signal_impl::insert(std::_List_iterator<sigc::slot_base>, sigc::slot_base&&) (signal_base.cc:157) ==29586== by 0x4E4B254: sigc::internal::signal_impl::connect(sigc::slot_base&&) (signal_base.cc:119) ==29586== by 0x4E4BAC4: sigc::signal_base::connect(sigc::slot_base&&) (signal_base.cc:274) ==29586== by 0x110906: sigc::signal_with_accumulator<void, void>::connect(sigc::slot<void ()>&&) (signal.h:446) ==29586== by 0x10CF51: (anonymous namespace)::test_clear_called_in_signal_handler() (test_signal.cc:119) ==29586== by 0x10D039: main (test_signal.cc:138)
This happens while iterating over the temp_slot_list: https://git.gnome.org/browse/libsigcplusplus/tree/sigc++/signal.h#n373 which is meant to avoid exactly this kind of problem: https://git.gnome.org/browse/libsigcplusplus/tree/sigc++/signal.h#n194 but it just has a reference to the original std::list of slots, hence the segfault when that list is made empty by clear(). As an experiment, this change makes the tests pass: - const temp_slot_list slots(impl->slots_); + const auto slots = impl->slots_; But presumably that incorrectly ignores other changes that can happen during signal emission (for which we should really have tests), and copy the std::list would be very inefficient during every emit().
I noticed that signal_impl::erase() is quite similar to signal_impl::clear(), and might also segfault. But when I looked into to code more closely, I noticed that signal_impl::erase() and the protected signal_base::erase() are not used in libsigc++3. They are left-overs from libsigc++2. In libsigc++2 they are not used by libsigc++ itself, but users can call them via a slot_list<>, returned from the deprecated signal<>.slots() methods. Looks like signal_impl::erase() and signal_base::erase() should be removed from libsigc++3.
Created attachment 355717 [details] [review] patch: signal_impl::clear(): Don't clear the slot list during signal emission This patch fixes the bug.
Created attachment 355718 [details] [review] patch: test_signal: Test calls to signal_base::clear() And here's a more complete test.
I have pushed patches very similar to the ones in comments 5 and 6. I added a comment to signal_impl::clear() and more connected slots to the new test cases in test_signal.cc. I have pushed both to the master branch and to the libsigc++-2-10 branch.
Thanks. > Looks like signal_impl::erase() and signal_base::erase() should be removed from libsigc++3. Sure. Feel free to do that. Thanks.
signal_impl::erase() and signal_base::erase() have been removed in the master branch. https://git.gnome.org/browse/libsigcplusplus/commit/?id=58521af49e448fd3d4b794eea19c886320ecd4a1