Bug #2313: Undefined behavior in UtilSignal::DestructInHandler test case - ndn-cxx - NDN project issue tracking system

Bug #2313

closed

Undefined behavior in UtilSignal::DestructInHandler test case

Added by Davide Pesavento over 9 years ago. Updated over 8 years ago.

Status:

Closed

Priority:

Normal

Assignee:

Junxiao Shi

Category:

Utils

Target version:

v0.4

Start date:

Due date:

% Done:

100%

Estimated time:

1.00 h

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.

Related issues 1 (0 open — 1 closed)

Blocks NFD - Task #2589: CI: enable AddressSanitizer for unit tests

Closed

Davide Pesavento

Actions

Issue # Delay: days Cancel

History
Notes
Property changes

Actions

Copy link

Updated by Alex Afanasyev over 9 years ago

Related to Bug #2302: UtilSignal/DisconnectSelfInHandler segfault added

Actions

Copy link

Updated by Alex Afanasyev over 9 years ago

I believe this is exactly the problem I was having in #2302

Actions

Copy link

Updated by Davide Pesavento over 9 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?

Actions

Copy link

Updated by Junxiao Shi over 9 years ago

Related to deleted (Bug #2302: UtilSignal/DisconnectSelfInHandler segfault)

Actions

Copy link

Updated by Junxiao Shi over 9 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.

Actions

Copy link

Updated by Davide Pesavento over 9 years ago

Maybe the onFail part of NFD's face system should be redesigned so that it no longer depends on this "feature"..?

Actions

Copy link

Updated by Junxiao Shi over 9 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.

Actions

Copy link

Updated by Junxiao Shi about 9 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==

Actions

Copy link