Project

General

Profile

Actions

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:
Category:
Utils
Target version:
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 open1 closed)

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

Actions
Actions #1

Updated by Alex Afanasyev over 9 years ago

  • Related to Bug #2302: UtilSignal/DisconnectSelfInHandler segfault added
Actions #2

Updated by Alex Afanasyev over 9 years ago

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

Actions #3

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 #4

Updated by Junxiao Shi over 9 years ago

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

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 #6

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 #7

Updated by Junxiao Shi about 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 #8

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 #9

Updated by Davide Pesavento about 9 years ago

  • Blocks Task #2589: CI: enable AddressSanitizer for unit tests added
Actions #10

Updated by Junxiao Shi about 9 years ago

  • Status changed from New to In Progress
  • Target version changed from v0.3 to v0.4
Actions #11

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

Actions #12

Updated by Junxiao Shi about 9 years ago

  • Status changed from Code review to Closed
Actions #13

Updated by Davide Pesavento over 8 years ago

  • Start date deleted (12/19/2014)
Actions

Also available in: Atom PDF