Bug #2302
closed
UtilSignal/DisconnectSelfInHandler segfault
Added by Alex Afanasyev over 9 years ago.
Updated over 9 years ago.
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
- Project changed from NFD to ndn-cxx
- Category set to Utils
- Target version changed from Unsupported to Unsupported
- Related to Bug #2313: Undefined behavior in UtilSignal::DestructInHandler test case added
- 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.
- 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;
}
- Status changed from In Progress to Code review
- % Done changed from 20 to 100
- Related to deleted (Bug #2313: Undefined behavior in UtilSignal::DestructInHandler test case)
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.
I disagree with note-7 solution because it complicates signal emission procedure which is performance critical.
complicates? I seriously doubt that an additional "==" comparison would have a measurable performance impact.
@Davide, please do a benchmark to show that the overhead of note-7 design is small. See #2279 note-9 for benchmark code snippet.
Before: 3.37 ns
After: 4.86 ns
I disagree with note-7 solution because its overhead is 44% as measured in note-11.
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).
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.
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.
Commit d22a7baa0e6cca8c494f42c3b35bfed126b23dfc is a correct solution that fixes the bug.
API design change, if any, should be handled in a different commit.
Junxiao Shi wrote:
Commit d22a7baa0e6cca8c494f42c3b35bfed126b23dfc is a correct solution that fixes the bug.
Yes, but it's unnecessary if my proposed solution (or a similar one) is implemented.
As I said: API design change, if any, should be handled in a different commit.
Fair enough. I've opened #2333 to track the issue.
- Related to Bug #2333: Signal: disconnecting a handler while it is executing is unsafe added
- Status changed from Code review to Closed
- Related to Bug #2523: UtilSignal/DisconnectSelfInHandler use-after-free added
Also available in: Atom
PDF
Updated by 
Updated by 
Updated by