Bug #2313
closed
Undefined behavior in UtilSignal::DestructInHandler test case
100%
Description
Reduced testcase:
unique_ptr<SignalOwner0> so(new SignalOwner0);
so->sig.connect([&] {
so.reset();
});
BOOST_CHECK_NO_THROW(so->emitSignal(sig));
The .reset() in the handler causes the deallocation of the signal, therefore everything that happens in Signal::operator() after this handler has run is undefined behavior.
Deallocating a signal from one of its handler should be illegal since there's always at least one statement that uses the this pointer (m_isExecuting = false;) after the execution of any handler.
Updated by Alex Afanasyev almost 10 years ago
- Related to Bug #2302: UtilSignal/DisconnectSelfInHandler segfault added
Updated by Alex Afanasyev almost 10 years ago
I believe this is exactly the problem I was having in #2302
Updated by Davide Pesavento almost 10 years ago
I don't see an apparent similarity between the two test cases (in DisconnectSelfInHandler the SignalOwner0 instance is statically allocated on the stack), can you clarify?
Updated by Junxiao Shi almost 10 years ago
- Related to deleted (Bug #2302: UtilSignal/DisconnectSelfInHandler segfault)
Updated by Junxiao Shi almost 10 years ago
- Category changed from Tests to Utils
- Estimated time set to 1.00 h
This is unrelated to #2302. #2302 is an isolated problem in a test case, while this Bug is more than a test case problem.
Allow destructing a Signal from its handler comes from the following scenario:
In NFD Face::onFail, a handler from FaceTable may want to remove the failing face from the FaceTable.
FaceTable may hold the last shared_ptr to the face. Removing it causes the destruction of the face and thus the Signal.
We can't drop this feature without finding a solution to this scenario.
Updated by Davide Pesavento almost 10 years ago
Maybe the onFail part of NFD's face system should be redesigned so that it no longer depends on this "feature"..?
Updated by Junxiao Shi almost 10 years ago
I found a way to solve note-5 problem: FaceTable::remove can use io.post to delay deleting the Face from container, thus Signal isn't destructed during its execution.
Updated by Junxiao Shi almost 10 years ago
Confirmed with valgrind.
==2596== at 0x6C2891: ndn::util::signal::Signal<ndn::util::signal::tests::UtilSignal::SignalOwner0, >::operator()() (signal-signal.hpp:239)
==2596== by 0x6C0CCB: ndn::util::signal::Signal<ndn::util::signal::tests::UtilSignal::SignalOwner0, >::operator()(ndn::util::signal::DummyExtraArg const&) (signal-signal.hpp:246)
==2596== by 0x6BE6D8: void ndn::util::signal::tests::UtilSignal::SignalOwner0::emit_sig<ndn::util::signal::DummyExtraArg>(ndn::util::signal::DummyExtraArg const&) (signal.cpp:39)
==2596== by 0x6B8C75: ndn::util::signal::tests::UtilSignal::DestructInHandler::test_method() (signal.cpp:435)
==2596== by 0x6B8B43: ndn::util::signal::tests::UtilSignal::DestructInHandler_invoker() (signal.cpp:425)
==2596== by 0x4D6879: boost::unit_test::ut_detail::unused boost::unit_test::ut_detail::invoker<boost::unit_test::ut_detail::unused>::invoke<void (*)()>(void (*&)()) (callback.hpp:56)
==2596== by 0x4D5E63: boost::unit_test::ut_detail::callback0_impl_t<boost::unit_test::ut_detail::unused, void (*)()>::invoke() (callback.hpp:89)
==2596== by 0x5BDF2F0: ??? (in /usr/lib/libboost_unit_test_framework.so.1.48.0)
==2596== by 0x5BC34FD: boost::execution_monitor::catch_signals(boost::unit_test::callback0<int> const&) (in /usr/lib/libboost_unit_test_framework.so.1.48.0)
==2596== by 0x5BC3DFA: boost::execution_monitor::execute(boost::unit_test::callback0<int> const&) (in /usr/lib/libboost_unit_test_framework.so.1.48.0)
==2596== by 0x5BDF3EA: boost::unit_test::unit_test_monitor_t::execute_and_translate(boost::unit_test::test_case const&) (in /usr/lib/libboost_unit_test_framework.so.1.48.0)
==2596== by 0x5BCA8A0: boost::unit_test::framework_impl::visit(boost::unit_test::test_case const&) (in /usr/lib/libboost_unit_test_framework.so.1.48.0)
==2596== Address 0x97e6c60 is 16 bytes inside a block of size 32 free'd
==2596== at 0x4C2A4BC: operator delete(void*) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==2596== by 0x6C206D: std::default_delete<ndn::util::signal::tests::UtilSignal::SignalOwner0>::operator()(ndn::util::signal::tests::UtilSignal::SignalOwner0*) const (unique_ptr.h:63)
==2596== by 0x6BFE1B: std::unique_ptr<ndn::util::signal::tests::UtilSignal::SignalOwner0, std::default_delete<ndn::util::signal::tests::UtilSignal::SignalOwner0> >::reset(ndn::util::signal::tests::UtilSignal::SignalOwner0*) (unique_ptr.h:245)
==2596== by 0x6B8B74: _ZZN3ndn4util6signal5tests10UtilSignal17DestructInHandler11test_methodEvENKUlvE_clEv (signal.cpp:432)
==2596== by 0x6BB8C9: _ZNSt17_Function_handlerIFvvEZN3ndn4util6signal5tests10UtilSignal17DestructInHandler11test_methodEvEUlvE_E9_M_invokeERKSt9_Any_data (functional:1778)
==2596== by 0x69C8F1: std::function<void ()()>::operator()() const (functional:2161)
==2596== by 0x6C2881: ndn::util::signal::Signal<ndn::util::signal::tests::UtilSignal::SignalOwner0, >::operator()() (signal-signal.hpp:232)
==2596== by 0x6C0CCB: ndn::util::signal::Signal<ndn::util::signal::tests::UtilSignal::SignalOwner0, >::operator()(ndn::util::signal::DummyExtraArg const&) (signal-signal.hpp:246)
==2596== by 0x6BE6D8: void ndn::util::signal::tests::UtilSignal::SignalOwner0::emit_sig<ndn::util::signal::DummyExtraArg>(ndn::util::signal::DummyExtraArg const&) (signal.cpp:39)
==2596== by 0x6B8C75: ndn::util::signal::tests::UtilSignal::DestructInHandler::test_method() (signal.cpp:435)
==2596== by 0x6B8B43: ndn::util::signal::tests::UtilSignal::DestructInHandler_invoker() (signal.cpp:425)
==2596== by 0x4D6879: boost::unit_test::ut_detail::unused boost::unit_test::ut_detail::invoker<boost::unit_test::ut_detail::unused>::invoke<void (*)()>(void (*&)()) (callback.hpp:56)
==2596==
Updated by Davide Pesavento over 9 years ago
- Blocks Task #2589: CI: enable AddressSanitizer for unit tests added
Updated by Junxiao Shi over 9 years ago
- Status changed from New to In Progress
- Target version changed from v0.3 to v0.4
Updated by Junxiao Shi over 9 years ago
- Status changed from In Progress to Code review
- % Done changed from 0 to 100
http://gerrit.named-data.net/1820 forbids destruction of Signal during signal emission.
nfd::FaceTable doesn't seem affected.
Updated by Junxiao Shi over 9 years ago
- Status changed from Code review to Closed