Bug #2523
closed
UtilSignal/DisconnectSelfInHandler use-after-free
100%
Description
Looks like #2302 was not really fixed... or maybe it's something else.
==12651==ERROR: AddressSanitizer: heap-use-after-free on address 0x6030008aaea8 at pc 0x79ecea bp 0x7fffd51f71d0 sp 0x7fffd51f71c0
READ of size 8 at 0x6030008aaea8 thread T0
#0 0x79ece9 in operator() ../tests/unit-tests/util/signal.cpp:388
#1 0x79ece9 in __call<void, 0ul> /usr/include/c++/4.9/functional:1264
#2 0x79ece9 in operator()<, void> /usr/include/c++/4.9/functional:1323
#3 0x79ece9 in _M_invoke /usr/include/c++/4.9/functional:2039
#4 0x78b1e1 in std::function<void ()>::operator()() const /usr/include/c++/4.9/functional:2439
#5 0x7aca4f in ndn::util::signal::Signal<ndn::util::signal::tests::UtilSignal::SignalOwner0>::operator()() /home/davide/ndn-cxx/src/util/signal-signal.hpp:232
#6 0x7a4976 in ndn::util::signal::Signal<ndn::util::signal::tests::UtilSignal::SignalOwner0>::operator()(ndn::util::signal::DummyExtraArg const&) /home/davide/ndn-cxx/src/util/signal-signal.hpp:246
#7 0x7a4976 in emit_sig<ndn::util::signal::DummyExtraArg> ../tests/unit-tests/util/signal.cpp:39
#8 0x7a4976 in ndn::util::signal::tests::UtilSignal::DisconnectSelfInHandler::test_method() ../tests/unit-tests/util/signal.cpp:394
#9 0x7a4f52 in DisconnectSelfInHandler_invoker ../tests/unit-tests/util/signal.cpp:377
#10 0x44b018 in invoke<void (*)()> /usr/include/boost/test/utils/callback.hpp:56
#11 0x44b018 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
#12 0x7fbf4996b5a0 (/usr/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.55.0+0x685a0)
#13 0x7fbf49946865 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)
#14 0x7fbf499470a2 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)
#15 0x7fbf4996b6a1 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)
#16 0x7fbf499552f3 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)
#17 0x7fbf499842d2 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)
#18 0x7fbf499842d2 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)
#19 0x7fbf49950819 in boost::unit_test::framework::run(unsigned long, bool) (/usr/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.55.0+0x4d819)
#20 0x7fbf49969283 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)
#21 0x43e8eb in main /usr/include/boost/test/unit_test.hpp:59
#22 0x7fbf4845aec4 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21ec4)
#23 0x43e2c8 (/home/davide/ndn-cxx/build/unit-tests+0x43e2c8)
0x6030008aaea8 is located 8 bytes inside of 24-byte region [0x6030008aaea0,0x6030008aaeb8)
freed by thread T0 here:
#0 0x7fbf4a72c63f in operator delete(void*) (/usr/lib/x86_64-linux-gnu/libasan.so.1+0x5863f)
#1 0x79c650 in _M_destroy /usr/include/c++/4.9/functional:1894
#2 0x79c650 in _M_manager /usr/include/c++/4.9/functional:1918
previously allocated by thread T0 here:
#0 0x7fbf4a72c13f in operator new(unsigned long) (/usr/lib/x86_64-linux-gnu/libasan.so.1+0x5813f)
#1 0x79c59b in _M_clone /usr/include/c++/4.9/functional:1878
#2 0x79c59b in _M_manager /usr/include/c++/4.9/functional:1914
SUMMARY: AddressSanitizer: heap-use-after-free ../tests/unit-tests/util/signal.cpp:388 operator()
Shadow bytes around the buggy address:
0x0c068010d580: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x0c068010d590: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x0c068010d5a0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x0c068010d5b0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x0c068010d5c0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fd fd
=>0x0c068010d5d0: fd fd fa fa fd[fd]fd fa fa fa fd fd fd fa fa fa
0x0c068010d5e0: 00 00 00 fa fa fa fd fd fd fd fa fa fd fd fd fd
0x0c068010d5f0: fa fa fd fd fd fd fa fa fd fd fd fd fa fa fd fd
0x0c068010d600: fd fa fa fa fd fd fd fd fa fa fd fd fd fa fa fa
0x0c068010d610: fd fd fd fd fa fa fd fd fd fa fa fa fd fd fd fd
0x0c068010d620: fa fa fd fd fd fa fa fa fd fd fd fd fa fa 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
==12651==ABORTING
Updated by Davide Pesavento almost 10 years ago
- Related to Bug #2302: UtilSignal/DisconnectSelfInHandler segfault added
Updated by Davide Pesavento almost 10 years ago
- Related to Bug #2333: Signal: disconnecting a handler while it is executing is unsafe added
Updated by Junxiao Shi almost 10 years ago
Confirmed on Ubuntu 14.10 using steps in #2307 note-6.
Updated by Junxiao Shi almost 10 years ago
- Status changed from New to In Progress
- Assignee set to Junxiao Shi
This Bug is similar to #2302: the reference to connection is part of ClosureType, which is destructed upon disconnecting.
Updated by Junxiao Shi almost 10 years ago
- Status changed from In Progress to Code review
- % Done changed from 0 to 100
- Estimated time set to 0.50 h
Updated by Davide Pesavento almost 10 years ago
As I already argued in the related bugs, this "functionality" is extremely error-prone and should just be prohibited by the API.
Updated by Junxiao Shi almost 10 years ago
Disconnecting self on handler has many use cases including connectSingleShot.
commit:508bda86c862a3be7d0003111dc7b19e08275fee is a correct solution to this Bug.
The handler is responsible for maintaining its own validity.
Updated by Davide Pesavento almost 10 years ago
Junxiao Shi wrote:
Disconnecting self on handler has many use cases including
connectSingleShot.
connectSingleShot could be handled specially, e.g. it could use a private method of Signal similar to disconnect but which doesn't assert if m_isExecuting is true.
I doubt anything else uses this functionality.
commit:508bda86c862a3be7d0003111dc7b19e08275fee is a correct solution to this Bug.
The handler is responsible for maintaining its own validity.
You keep saying that but you already got it wrong twice, and you're the author of Signal so you're probably the person that knows it best...
Updated by Junxiao Shi almost 10 years ago
Disconnecting self on handler has many use cases including
connectSingleShot.
connectSingleShotcould be handled specially, e.g. it could use a private method ofSignalsimilar todisconnectbut which doesn't assert ifm_isExecutingis true.I doubt anything else uses this functionality.
Consider this use case:
- Some module emits the signal whenever a message is received.
- The callback processes the message, and disconnects itself when the message contains "END".
connectSingleShot is insufficient.
You keep saying that but you already got it wrong twice, and you're the author of
Signalso you're probably the person that knows it best...
But I got it right this time.
Updated by Davide Pesavento almost 10 years ago
Junxiao Shi wrote:
Consider this use case: [...]
Sure, I'm not saying that there are no use cases. But is this needed by current code? If not, we can have this discussion if and when it will be required, and not support this usage pattern for the time being. (I actually proposed a better solution in #2333 but IIRC you didn't like it for some reason)
You keep saying that but you already got it wrong twice, and you're the author of
Signalso you're probably the person that knows it best...But I got it right this time.
My point (that the API is easy to misuse) stands.
Updated by Junxiao Shi almost 10 years ago
Sure, I'm not saying that there are no use cases. But is this needed by current code? If not, we can have this discussion if and when it will be required, and not support this usage pattern for the time being. (I actually proposed a better solution in #2333 but IIRC you didn't like it for some reason)
No. Signal already supports "disconnect self in handler" feature, and there's no reason to drop it.
My point (that the API is easy to misuse) stands.
Yes, but that's separate issue #2333. commit:508bda86c862a3be7d0003111dc7b19e08275fee is sufficient to solve the immediate Bug.
Updated by Alex Afanasyev over 9 years ago
I think Davide didn't mean to do anything special to "drop support", except maybe documentation that lambdas should not be used in such cases.
My opinion is that more examples is better than no examples and, assuming no problems exist, we could proceed with merging the code and closing the issue. We could add additional doc statement that such use case is dangerous and needs to be avoided if possible.
Updated by Davide Pesavento over 9 years ago
Alex Afanasyev wrote:
I think Davide didn't mean to do anything special to "drop support", except maybe documentation that lambdas should not be used in such cases.
Correct. I said "prohibit" in a previous comment, but "not supported" is more accurate.
My opinion is that more examples is better than no examples and, assuming no problems exist, we could proceed with merging the code and closing the issue. We could add additional doc statement that such use case is dangerous and needs to be avoided if possible.
I could agree to this. (Not sure what you mean by "examples" though)
Updated by Alex Afanasyev over 9 years ago
"Examples" = "test cases" (in case somebody is looking). This at least for us to know the extent of how things can be used.
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 Code review to Closed