Bug #2302: UtilSignal/DisconnectSelfInHandler segfault - ndn-cxx - NDN project issue tracking system

Actions

Copy link

Bug #2302

closed

UtilSignal/DisconnectSelfInHandler segfault

Added by Alex Afanasyev over 9 years ago. Updated over 9 years ago.

Status:

Closed

Priority:

Normal

Assignee:

Junxiao Shi

Category:

Utils

Target version:

v0.3

Start date:

Due date:

% Done:

100%

Estimated time:

0.50 h

Description

On cygwin64, UtilSignal/DisconnectSelfInHandler is for some reason segfaulting.

Entering test case "DisconnectSelfInHandler"

Program received signal SIGSEGV, Segmentation fault.
operator() (__closure=0x6000850b0) at ../tests/unit-tests/util/signal.cpp:292
292         BOOST_CHECK_EQUAL(so.isSigEmpty(), false); // disconnecting hasn't taken effect
(gdb) bt
#0  operator() (__closure=0x6000850b0) at ../tests/unit-tests/util/signal.cpp:292
#1  std::_Function_handler<void(), ndn::util::signal::tests::UtilSignal::DisconnectSelfInHandler::test_method()::__lambda15>::_M_invoke(const std::_Any_data &) (
    __functor=...) at /usr/lib/gcc/x86_64-pc-cygwin/4.8.3/include/c++/functional:2071
#2  0x00000001007a1bc6 in std::function<void ()>::operator()() const (this=0x40) at /usr/lib/gcc/x86_64-pc-cygwin/4.8.3/include/c++/functional:2471
#3  0x00000001006b1dd4 in ndn::util::signal::Signal<ndn::util::signal::tests::UtilSignal::SignalOwner0>::operator() (this=this@entry=0x23a160)
    at /home/cawka/ndn-cxx/src/util/signal-signal.hpp:200
#4  0x00000001005270eb in emit_sig<> (this=0x23a160) at ../tests/unit-tests/util/signal.cpp:42
#5  ndn::util::signal::tests::UtilSignal::DisconnectSelfInHandler::test_method (this=<optimized out>) at ../tests/unit-tests/util/signal.cpp:295
#6  0x00000001005277c1 in ndn::util::signal::tests::UtilSignal::DisconnectSelfInHandler_invoker () at ../tests/unit-tests/util/signal.cpp:283
#7  0x0000000100704f97 in invoke<void (*)()> (f=<optimized out>, this=<optimized out>) at /usr/include/boost/test/utils/callback.hpp:56
#8  boost::unit_test::ut_detail::callback0_impl_t<boost::unit_test::ut_detail::unused, void (*)()>::invoke (this=<optimized out>)
    at /usr/include/boost/test/utils/callback.hpp:89
#9  0x000000051aa8d1b1 in cygboost_unit_test_framework-1_55!_ZN5boost9unit_test14unit_test_mainEPFbvEiPPc () from /usr/bin/cygboost_unit_test_framework-1_55.dll
#10 0x000000051aa7188e in cygboost_unit_test_framework-1_55!_ZN5boost17execution_monitor13catch_signalsERKNS_9unit_test9callback0IiEE ()
   from /usr/bin/cygboost_unit_test_framework-1_55.dll
#11 0x000000051aa71f53 in cygboost_unit_test_framework-1_55!_ZN5boost17execution_monitor7executeERKNS_9unit_test9callback0IiEE ()
   from /usr/bin/cygboost_unit_test_framework-1_55.dll
#12 0x000000051aa8d290 in cygboost_unit_test_framework-1_55!_ZN5boost9unit_test19unit_test_monitor_t21execute_and_translateERKNS0_9test_caseE ()
   from /usr/bin/cygboost_unit_test_framework-1_55.dll
#13 0x000000051aaabc20 in cygboost_unit_test_framework-1_55!_ZN5boost9unit_test14framework_impl5visitERKNS0_9test_caseE ()
   from /usr/bin/cygboost_unit_test_framework-1_55.dll
#14 0x000000051aa96dad in cygboost_unit_test_framework-1_55!_ZN5boost9unit_test18traverse_test_treeERKNS0_10test_suiteERNS0_17test_tree_visitorE ()
   from /usr/bin/cygboost_unit_test_framework-1_55.dll
#15 0x000000051aa96d9f in cygboost_unit_test_framework-1_55!_ZN5boost9unit_test18traverse_test_treeERKNS0_10test_suiteERNS0_17test_tree_visitorE ()
   from /usr/bin/cygboost_unit_test_framework-1_55.dll
#16 0x000000051aa7855a in cygboost_unit_test_framework-1_55!_ZN5boost9unit_test9framework3runEmb () from /usr/bin/cygboost_unit_test_framework-1_55.dll
#17 0x000000051aa8cfc4 in cygboost_unit_test_framework-1_55!_ZN5boost9unit_test14unit_test_mainEPFbvEiPPc () from /usr/bin/cygboost_unit_test_framework-1_55.dll
#18 0x00000001005768ff in main (argc=5, argv=0x23aab0) at /usr/include/boost/test/unit_test.hpp:59

Related issues 2 (0 open — 2 closed)

Actions

Copy link

Updated by Alex Afanasyev over 9 years ago

Project changed from NFD to ndn-cxx
Category set to Utils
Target version changed from Unsupported to Unsupported

Actions

Copy link

Updated by Alex Afanasyev over 9 years ago

Related to Bug #2313: Undefined behavior in UtilSignal::DestructInHandler test case added

Actions

Copy link

Updated by Davide Pesavento over 9 years ago

Target version changed from Unsupported to v0.3

On Ubuntu 14.10, ASan reports a use-after-free on a heap-allocated object.

==8346==ERROR: AddressSanitizer: heap-use-after-free on address 0x6030000175a0 at pc 0x6e89a0 bp 0x7fffb349a790 sp 0x7fffb349a780
READ of size 8 at 0x6030000175a0 thread T0
    #0 0x6e899f in operator() ../tests/unit-tests/util/signal.cpp:292
    #1 0x6e899f in _M_invoke /usr/include/c++/4.9/functional:2039
    #2 0x6d55e5 in std::function<void ()>::operator()() const /usr/include/c++/4.9/functional:2439
    #3 0x6f49bb in ndn::util::signal::Signal<ndn::util::signal::tests::UtilSignal::SignalOwner0>::operator()() /home/davide/ndn-cxx/src/util/signal-signal.hpp:200
    #4 0x6ed969 in emit_sig<> ../tests/unit-tests/util/signal.cpp:42
    #5 0x6ed969 in ndn::util::signal::tests::UtilSignal::DisconnectSelfInHandler::test_method() ../tests/unit-tests/util/signal.cpp:295
    #6 0x6ee140 in DisconnectSelfInHandler_invoker ../tests/unit-tests/util/signal.cpp:283
    #7 0x44e2fe in invoke<void (*)()> /usr/include/boost/test/utils/callback.hpp:56
    #8 0x44e2fe in boost::unit_test::ut_detail::callback0_impl_t<boost::unit_test::ut_detail::unused, void (*)()>::invoke() /usr/include/boost/test/utils/callback.hpp:89
    #9 0x7fd10c6355a0 (/usr/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.55.0+0x685a0)
    #10 0x7fd10c610865 in boost::execution_monitor::catch_signals(boost::unit_test::callback0<int> const&) (/usr/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.55.0+0x43865)
    #11 0x7fd10c6110a2 in boost::execution_monitor::execute(boost::unit_test::callback0<int> const&) (/usr/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.55.0+0x440a2)
    #12 0x7fd10c6356a1 in boost::unit_test::unit_test_monitor_t::execute_and_translate(boost::unit_test::test_case const&) (/usr/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.55.0+0x686a1)
    #13 0x7fd10c61f2f3 in boost::unit_test::framework_impl::visit(boost::unit_test::test_case const&) (/usr/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.55.0+0x522f3)
    #14 0x7fd10c64e2d2 in boost::unit_test::traverse_test_tree(boost::unit_test::test_suite const&, boost::unit_test::test_tree_visitor&) (/usr/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.55.0+0x812d2)
    #15 0x7fd10c64e2d2 in boost::unit_test::traverse_test_tree(boost::unit_test::test_suite const&, boost::unit_test::test_tree_visitor&) (/usr/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.55.0+0x812d2)
    #16 0x7fd10c61a819 in boost::unit_test::framework::run(unsigned long, bool) (/usr/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.55.0+0x4d819)
    #17 0x7fd10c633283 in boost::unit_test::unit_test_main(bool (*)(), int, char**) (/usr/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.55.0+0x66283)
    #18 0x821ee8 in main /usr/include/boost/test/unit_test.hpp:59
    #19 0x7fd10b124ec4 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21ec4)
    #20 0x43b6d8 (/home/davide/ndn-cxx/build/unit-tests+0x43b6d8)

0x6030000175a0 is located 16 bytes inside of 24-byte region [0x603000017590,0x6030000175a8)
freed by thread T0 here:
    #0 0x7fd10d3f663f in operator delete(void*) (/usr/lib/x86_64-linux-gnu/libasan.so.1+0x5863f)
    #1 0x6e4929 in _M_destroy /usr/include/c++/4.9/functional:1894
    #2 0x6e4929 in _M_manager /usr/include/c++/4.9/functional:1918

previously allocated by thread T0 here:
    #0 0x7fd10d3f613f in operator new(unsigned long) (/usr/lib/x86_64-linux-gnu/libasan.so.1+0x5813f)
    #1 0x6e48d8 in _M_clone /usr/include/c++/4.9/functional:1878
    #2 0x6e48d8 in _M_manager /usr/include/c++/4.9/functional:1914

SUMMARY: AddressSanitizer: heap-use-after-free ../tests/unit-tests/util/signal.cpp:292 operator()
Shadow bytes around the buggy address:
  0x0c067fffae60: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c067fffae70: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c067fffae80: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c067fffae90: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c067fffaea0: fa fa fa fa fa fa fa fa fa fa fa fa fd fd fd fd
=>0x0c067fffaeb0: fa fa fd fd[fd]fa fa fa fd fd fd fa fa fa 00 00
  0x0c067fffaec0: 00 fa fa fa fd fd fd fd fa fa fd fd fd fd fa fa
  0x0c067fffaed0: fd fd fd fd fa fa fd fd fd fd fa fa fd fd fd fa
  0x0c067fffaee0: fa fa fd fd fd fd fa fa fd fd fd fa fa fa fd fd
  0x0c067fffaef0: fd fd fa fa fd fd fd fa fa fa fd fd fd fd fa fa
  0x0c067fffaf00: fd fd fd fa fa fa fd fd fd fd fa fa fd fd fd fd
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Heap right redzone:      fb
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack partial redzone:   f4
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Contiguous container OOB:fc
  ASan internal:           fe
==8346==ABORTING

If I delete line 292 -- BOOST_CHECK_EQUAL(so.isSigEmpty(), false); -- the error disappears.

Actions

Copy link

Updated by Junxiao Shi over 9 years ago

Subject changed from Segfault of UtilSignal/DisconnectSelfInHandler test case to UtilSignal/DisconnectSelfInHandler segfault
Status changed from New to In Progress
Assignee set to Junxiao Shi
% Done changed from 0 to 20
Estimated time set to 0.50 h

From C++ reference - lambda functions:

The lambda expression constructs an unnamed prvalue temporary object of unique unnamed non-union non-aggregate type, known as closure type.

When connection.disconnect is invoked, the ClosureType instance is destructed.
After that, accessing so results in undefined behavior.

The following snippet shows the lifetime of ClosureType.

valgrind complains "Invalid read" at the second a.x() call.

#include <iostream>
#include <functional>

struct A
{
  A()
    : i(0)
  {
    std::cout << "ctor" << std::endl;
  }

  A(const A& other)
    : i(other.i)
  {
    std::cout << "ctor1" << std::endl;
  }

  ~A()
  {
    std::cout << "dtor" << std::endl;
  }

  void
  x() const
  {
    std::cout << i << std::endl;
  }

  int i;
};

int
main()
{
  A a;

  std::function<void()> f;
  f = [a, &f] {
    std::cout << "1" << std::endl;
    a.x();
    std::cout << "2" << std::endl;
    f = []{};
    std::cout << "3" << std::endl;
    a.x();
    std::cout << "4" << std::endl;
  };
  f();

  return 0;
}

Actions

Copy link

Updated by Junxiao Shi over 9 years ago

Status changed from In Progress to Code review
% Done changed from 20 to 100

http://gerrit.named-data.net/1564

Actions

Copy link

Updated by Junxiao Shi over 9 years ago

Related to deleted (Bug #2313: Undefined behavior in UtilSignal::DestructInHandler test case)

Actions

Copy link

Updated by Davide Pesavento over 9 years ago

Random thought for a possible solution.

In Signal<>::disconnect, if m_isExecuting is true, do not call m_slots.erase. In Signal<>::operator() delete line 198 and, after the handler invocation, add:

if (m_currentSlot == m_slots.end())
  it = m_slots.erase(it);
else
  ++it;

This should keep the closure alive until the handler has finished executing.

Actions

Copy link

Updated by Junxiao Shi over 9 years ago

I disagree with note-7 solution because it complicates signal emission procedure which is performance critical.

Actions

Copy link

Updated by Davide Pesavento over 9 years ago

complicates? I seriously doubt that an additional "==" comparison would have a measurable performance impact.

Actions

Copy link

#10

Updated by Junxiao Shi over 9 years ago

@Davide, please do a benchmark to show that the overhead of note-7 design is small. See #2279 note-9 for benchmark code snippet.

Actions

Copy link

#11

Updated by Davide Pesavento over 9 years ago

Before: 3.37 ns
After:  4.86 ns

Actions

Copy link

#12

Updated by Junxiao Shi over 9 years ago

I disagree with note-7 solution because its overhead is 44% as measured in note-11.

Actions

Copy link

#13

Updated by Davide Pesavento over 9 years ago

44% doesn't mean anything in this case. The benchmark is a worst-case scenario where the handler does almost nothing. If the handler did some actual work, the overhead would be much lower. The above results should be interpreted as: the additional logic costs 1.5 ns per iteration, which means it has a negligible impact, while making the disconnection feature a lot safer to use (less error-prone).

Actions

Copy link

#14

Updated by Junxiao Shi over 9 years ago

No.
The handler is responsible for its own validity.
If the handler is a lambda expression, the handler itself, instead of Signal::operator(), is responsible to ensure the ClosureType instance is valid.

Actions

Copy link

#15

Updated by Davide Pesavento over 9 years ago

This is not just a failing test case, it's about API design. And Signal is a library feature, thus good API design is even more important in my opinion. The current API is extremely error-prone and easy to misuse, also because this potential pitfall is undocumented. But even if it was documented, there's no reason to leave the burden of ensuring the "validity" of the handler to every API consumer, when it can easily be done on the library side at a negligible cost. Moreover, the existing pitfall creates bugs that are, like most memory management bugs in C++, both hard to reproduce and hard to notice during code reviews.

Actions

Copy link

#16