Bug #2213
closedEventEmitter::clear() leads to double free when called from Face::fail()
0%
Description
I'm not sure who to blame exactly... this is what happens (simplified), platform is 64-bit Ubuntu 14.10 with gcc-4.9.1
- NFD's EthernetFace calls
Face::fail
- ...which at the end executes
onFail.clear()
, i.e.EventEmitter::clear
- ...which clears the underlying vector of handlers, i.e. calls
std::vector::clear
- Now, the behavior of
vector::clear
is that it first destroys all its elements usingstd::_Destroy
, and only after that it resets its internal pointers to the new vector size (0) - The
std::_Destroy
invocation on the various handlers ends up destroying the last instance ofshared_ptr
pointing at the EthernetFace that initially calledfail
- ...which triggers the destruction of the EthernetFace, i.e.
~EthernetFace
and~Face
are called - ...which means that
~EventEmitter
for theFace::onFail
member is invoked - ...which calls
std::vector::~vector
- ...which calls
std::_Destroy
on all its elements (again) - Note that at this point in the call chain, the
vector<Handler>
in EventEmitter hasn't had the chance to set its end pointer (_M_finish
) to the correct value yet, thereforestd::_Destroy
will be called on the same elements that were already deallocated, leading to a double free or worse.
Basically the issue is that std::vector
is not reentrant, and we're violating this condition.
The bug itself can manifest in different ways: a glibc-detected double free, a segfault in libc.so, random aborts later in the execution, 100% cpu usage, and so on. With -fsanitize=address
I get a reproducible heap-use-after-free error.
==26624==ERROR: AddressSanitizer: heap-use-after-free on address 0x60400001cee8 at pc 0x65a45f bp 0x7fff52445a90 sp 0x7fff52445a80
READ of size 8 at 0x60400001cee8 thread T0
#0 0x65a45e in std::string::_M_data() const /usr/include/c++/4.9/bits/basic_string.h:293
#1 0x65a45e in std::string::_M_rep() const /usr/include/c++/4.9/bits/basic_string.h:301
#2 0x65a45e in ~basic_string /usr/include/c++/4.9/bits/basic_string.h:547
#3 0x65a45e in ~_Head_base /usr/include/c++/4.9/tuple:128
#4 0x65a45e in ~_Tuple_impl /usr/include/c++/4.9/tuple:229
#5 0x65a45e in ~_Tuple_impl /usr/include/c++/4.9/tuple:229
#6 0x65a45e in ~tuple /usr/include/c++/4.9/tuple:388
#7 0x65a45e in ~_Bind /usr/include/c++/4.9/functional:1248
#8 0x65a45e in std::_Function_base::_Base_manager<std::_Bind<std::_Mem_fn<void (nfd::EthernetFactory::*)(std::string const&, ndn::util::ethernet::Address const&)> (nfd::EthernetFactory*, std::string, ndn::util::ethernet::Address)> >::_M_destroy(std::_Any_data&, std::integral_constant<bool, false>) /usr/include/c++/4.9/functional:1894
#9 0x65a45e in std::_Function_base::_Base_manager<std::_Bind<std::_Mem_fn<void (nfd::EthernetFactory::*)(std::string const&, ndn::util::ethernet::Address const&)> (nfd::EthernetFactory*, std::string, ndn::util::ethernet::Address)> >::_M_manager(std::_Any_data&, std::_Any_data const&, std::_Manager_operation) /usr/include/c++/4.9/functional:1918
#10 0x64ad76 in ~_Function_base /usr/include/c++/4.9/functional:1998
#11 0x64ad76 in ~function /usr/include/c++/4.9/functional:2142
#12 0x64ad76 in _Destroy<std::function<void(const std::basic_string<char>&)> > /usr/include/c++/4.9/bits/stl_construct.h:93
#13 0x64ad76 in __destroy<std::function<void(const std::basic_string<char>&)>*> /usr/include/c++/4.9/bits/stl_construct.h:103
#14 0x64ad76 in _Destroy<std::function<void(const std::basic_string<char>&)>*> /usr/include/c++/4.9/bits/stl_construct.h:126
#15 0x64ad76 in _Destroy<std::function<void(const std::basic_string<char>&)>*, std::function<void(const std::basic_string<char>&)> > /usr/include/c++/4.9/bits/stl_construct.h:151
#16 0x64ad76 in std::vector<std::function<void (std::string const&)>, std::allocator<std::function<void (std::string const&)> > >::_M_erase_at_end(std::function<void (std::string const&)>*) /usr/include/c++/4.9/bits/stl_vector.h:1438
#17 0x64ad76 in std::vector<std::function<void (std::string const&)>, std::allocator<std::function<void (std::string const&)> > >::clear() /usr/include/c++/4.9/bits/stl_vector.h:1212
#18 0x64ad76 in ndn::util::EventEmitter<std::string>::clear() /usr/local/include/ndn-cxx/util/event-emitter.hpp:94
#19 0x64ad76 in nfd::EthernetFace::~EthernetFace() ../daemon/face/ethernet-face.cpp:94
#20 0x659e92 in destroy<nfd::EthernetFace> /usr/include/c++/4.9/ext/new_allocator.h:124
#21 0x659e92 in _S_destroy<nfd::EthernetFace> /usr/include/c++/4.9/bits/alloc_traits.h:282
#22 0x659e92 in destroy<nfd::EthernetFace> /usr/include/c++/4.9/bits/alloc_traits.h:411
#23 0x659e92 in std::_Sp_counted_ptr_inplace<nfd::EthernetFace, std::allocator<nfd::EthernetFace>, (__gnu_cxx::_Lock_policy)2>::_M_dispose() /usr/include/c++/4.9/bits/shared_ptr_base.h:524
#24 0x51955a in std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release() /usr/include/c++/4.9/bits/shared_ptr_base.h:149
#25 0x51955a in ~__shared_count /usr/include/c++/4.9/bits/shared_ptr_base.h:666
#26 0x51955a in ~__shared_ptr /usr/include/c++/4.9/bits/shared_ptr_base.h:914
#27 0x51955a in ~shared_ptr /usr/include/c++/4.9/bits/shared_ptr.h:93
#28 0x51955a in ~_Head_base /usr/include/c++/4.9/tuple:128
#29 0x51955a in ~_Tuple_impl /usr/include/c++/4.9/tuple:229
#30 0x51955a in ~_Tuple_impl /usr/include/c++/4.9/tuple:229
#31 0x51955a in ~tuple /usr/include/c++/4.9/tuple:521
#32 0x51955a in ~_Bind /usr/include/c++/4.9/functional:1248
#33 0x51955a in std::_Function_base::_Base_manager<std::_Bind<std::_Mem_fn<void (nfd::FaceTable::*)(std::shared_ptr<nfd::Face>)> (nfd::FaceTable*, std::shared_ptr<nfd::Face>)> >::_M_destroy(std::_Any_data&, std::integral_constant<bool, false>) /usr/include/c++/4.9/functional:1894
#34 0x51955a in std::_Function_base::_Base_manager<std::_Bind<std::_Mem_fn<void (nfd::FaceTable::*)(std::shared_ptr<nfd::Face>)> (nfd::FaceTable*, std::shared_ptr<nfd::Face>)> >::_M_manager(std::_Any_data&, std::_Any_data const&, std::_Manager_operation) /usr/include/c++/4.9/functional:1918
#35 0x45a8cd in ~_Function_base /usr/include/c++/4.9/functional:1998
#36 0x45a8cd in ~function /usr/include/c++/4.9/functional:2142
#37 0x45a8cd in _Destroy<std::function<void(const std::basic_string<char>&)> > /usr/include/c++/4.9/bits/stl_construct.h:93
#38 0x45a8cd in __destroy<std::function<void(const std::basic_string<char>&)>*> /usr/include/c++/4.9/bits/stl_construct.h:103
#39 0x45a8cd in _Destroy<std::function<void(const std::basic_string<char>&)>*> /usr/include/c++/4.9/bits/stl_construct.h:126
#40 0x45a8cd in _Destroy<std::function<void(const std::basic_string<char>&)>*, std::function<void(const std::basic_string<char>&)> > /usr/include/c++/4.9/bits/stl_construct.h:151
#41 0x45a8cd in std::vector<std::function<void (std::string const&)>, std::allocator<std::function<void (std::string const&)> > >::_M_erase_at_end(std::function<void (std::string const&)>*) /usr/include/c++/4.9/bits/stl_vector.h:1438
#42 0x45a8cd in std::vector<std::function<void (std::string const&)>, std::allocator<std::function<void (std::string const&)> > >::clear() /usr/include/c++/4.9/bits/stl_vector.h:1212
#43 0x45a8cd in ndn::util::EventEmitter<std::string>::clear() /usr/local/include/ndn-cxx/util/event-emitter.hpp:94
#44 0x45a8cd in nfd::Face::fail(std::string const&) ../daemon/face/face.cpp:124
#45 0x653d8a in nfd::EthernetFace::handleRead(boost::system::error_code const&, unsigned long) ../daemon/face/ethernet-face.cpp:237
#46 0x6577b4 in boost::_mfi::mf2<void, nfd::EthernetFace, boost::system::error_code const&, unsigned long>::operator()(nfd::EthernetFace*, boost::system::error_code const&, unsigned long) const /usr/include/boost/bind/mem_fn_template.hpp:280
#47 0x6577b4 in operator()<boost::_mfi::mf2<void, nfd::EthernetFace, const boost::system::error_code&, long unsigned int>, boost::_bi::list2<const boost::system::error_code&, long unsigned int const&> > /usr/include/boost/bind/bind.hpp:392
#48 0x6577b4 in operator()<boost::system::error_code, long unsigned int> /usr/include/boost/bind/bind_template.hpp:102
#49 0x6577b4 in boost::asio::detail::binder2<boost::_bi::bind_t<void, boost::_mfi::mf2<void, nfd::EthernetFace, boost::system::error_code const&, unsigned long>, boost::_bi::list3<boost::_bi::value<nfd::EthernetFace*>, boost::arg<1> (*)(), boost::arg<2> (*)()> >, boost::system::error_code, unsigned long>::operator()() /usr/include/boost/asio/detail/bind_handler.hpp:127
#50 0x6577b4 in asio_handler_invoke<boost::asio::detail::binder2<boost::_bi::bind_t<void, boost::_mfi::mf2<void, nfd::EthernetFace, const boost::system::error_code&, long unsigned int>, boost::_bi::list3<boost::_bi::value<nfd::EthernetFace*>, boost::arg<1> (*)(), boost::arg<2> (*)()> >, boost::system::error_code, long unsigned int> > /usr/include/boost/asio/handler_invoke_hook.hpp:69
#51 0x6577b4 in invoke<boost::asio::detail::binder2<boost::_bi::bind_t<void, boost::_mfi::mf2<void, nfd::EthernetFace, const boost::system::error_code&, long unsigned int>, boost::_bi::list3<boost::_bi::value<nfd::EthernetFace*>, boost::arg<1> (*)(), boost::arg<2> (*)()> >, boost::system::error_code, long unsigned int>, boost::_bi::bind_t<void, boost::_mfi::mf2<void, nfd::EthernetFace, const boost::system::error_code&, long unsigned int>, boost::_bi::list3<boost::_bi::value<nfd::EthernetFace*>, boost::arg<1> (*)(), boost::arg<2> (*)()> > > /usr/include/boost/asio/detail/handler_invoke_helpers.hpp:37
#52 0x6577b4 in boost::asio::detail::reactive_null_buffers_op<boost::_bi::bind_t<void, boost::_mfi::mf2<void, nfd::EthernetFace, boost::system::error_code const&, unsigned long>, boost::_bi::list3<boost::_bi::value<nfd::EthernetFace*>, boost::arg<1> (*)(), boost::arg<2> (*)()> > >::do_complete(boost::asio::detail::task_io_service*, boost::asio::detail::task_io_service_operation*, boost::system::error_code const&, unsigned long) /usr/include/boost/asio/detail/reactive_null_buffers_op.hpp:75
#53 0x4480c0 in boost::asio::detail::task_io_service_operation::complete(boost::asio::detail::task_io_service&, boost::system::error_code const&, unsigned long) /usr/include/boost/asio/detail/task_io_service_operation.hpp:38
#54 0x4480c0 in boost::asio::detail::epoll_reactor::descriptor_state::do_complete(boost::asio::detail::task_io_service*, boost::asio::detail::task_io_service_operation*, boost::system::error_code const&, unsigned long) /usr/include/boost/asio/detail/impl/epoll_reactor.ipp:651
#55 0x44c9f4 in boost::asio::detail::task_io_service_operation::complete(boost::asio::detail::task_io_service&, boost::system::error_code const&, unsigned long) /usr/include/boost/asio/detail/task_io_service_operation.hpp:38
#56 0x44c9f4 in boost::asio::detail::task_io_service::do_run_one(boost::asio::detail::scoped_lock<boost::asio::detail::posix_mutex>&, boost::asio::detail::task_io_service_thread_info&, boost::system::error_code const&) /usr/include/boost/asio/detail/impl/task_io_service.ipp:384
#57 0x44c9f4 in boost::asio::detail::task_io_service::run(boost::system::error_code&) /usr/include/boost/asio/detail/impl/task_io_service.ipp:153
#58 0x43fb43 in boost::asio::io_service::run() /usr/include/boost/asio/impl/io_service.ipp:59
#59 0x43fb43 in main ../daemon/main.cpp:396
#60 0x7f4f1a8caec4 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21ec4)
#61 0x43ec6e (/home/davide/NFD/build/bin/nfd+0x43ec6e)
0x60400001cee8 is located 24 bytes inside of 40-byte region [0x60400001ced0,0x60400001cef8)
freed by thread T0 here:
#0 0x7f4f1cb3963f in operator delete(void*) (/usr/lib/x86_64-linux-gnu/libasan.so.1+0x5863f)
#1 0x65a4fd in std::_Function_base::_Base_manager<std::_Bind<std::_Mem_fn<void (nfd::EthernetFactory::*)(std::string const&, ndn::util::ethernet::Address const&)> (nfd::EthernetFactory*, std::string, ndn::util::ethernet::Address)> >::_M_destroy(std::_Any_data&, std::integral_constant<bool, false>) /usr/include/c++/4.9/functional:1894
#2 0x65a4fd in std::_Function_base::_Base_manager<std::_Bind<std::_Mem_fn<void (nfd::EthernetFactory::*)(std::string const&, ndn::util::ethernet::Address const&)> (nfd::EthernetFactory*, std::string, ndn::util::ethernet::Address)> >::_M_manager(std::_Any_data&, std::_Any_data const&, std::_Manager_operation) /usr/include/c++/4.9/functional:1918
previously allocated by thread T0 here:
#0 0x7f4f1cb3913f in operator new(unsigned long) (/usr/lib/x86_64-linux-gnu/libasan.so.1+0x5813f)
#1 0x65a352 in std::_Function_base::_Base_manager<std::_Bind<std::_Mem_fn<void (nfd::EthernetFactory::*)(std::string const&, ndn::util::ethernet::Address const&)> (nfd::EthernetFactory*, std::string, ndn::util::ethernet::Address)> >::_M_clone(std::_Any_data&, std::_Any_data const&, std::integral_constant<bool, false>) /usr/include/c++/4.9/functional:1878
#2 0x65a352 in std::_Function_base::_Base_manager<std::_Bind<std::_Mem_fn<void (nfd::EthernetFactory::*)(std::string const&, ndn::util::ethernet::Address const&)> (nfd::EthernetFactory*, std::string, ndn::util::ethernet::Address)> >::_M_manager(std::_Any_data&, std::_Any_data const&, std::_Manager_operation) /usr/include/c++/4.9/functional:1914
SUMMARY: AddressSanitizer: heap-use-after-free /usr/include/c++/4.9/bits/basic_string.h:293 std::string::_M_data() const
Shadow bytes around the buggy address:
0x0c087fffb980: fa fa fd fd fd fd fd fa fa fa fd fd fd fd fd fa
0x0c087fffb990: fa fa fd fd fd fd fd fa fa fa fd fd fd fd fd fa
0x0c087fffb9a0: fa fa fd fd fd fd fd fa fa fa fd fd fd fd fd fa
0x0c087fffb9b0: fa fa fd fd fd fd fd fa fa fa fd fd fd fd fd fd
0x0c087fffb9c0: fa fa fd fd fd fd fd fa fa fa fd fd fd fd fd fa
=>0x0c087fffb9d0: fa fa fd fd fd fd fd fa fa fa fd fd fd[fd]fd fa
0x0c087fffb9e0: fa fa 00 00 00 00 00 fa fa fa fd fd fd fd fd fa
0x0c087fffb9f0: fa fa fd fd fd fd fd fa fa fa fd fd fd fd fd fa
0x0c087fffba00: fa fa fd fd fd fd fd fd fa fa fd fd fd fd fd fd
0x0c087fffba10: fa fa fd fd fd fd fd fd fa fa fd fd fd fd fd fd
0x0c087fffba20: fa fa 00 00 00 00 00 02 fa fa 00 00 00 00 00 00
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
==26624==ABORTING
Updated by Davide Pesavento over 9 years ago
A simple fix would be replacing
return m_handlers.clear();
with
std::vector<Handler>().swap(m_handlers);
in EventEmitter<>::clear
Updated by Junxiao Shi over 9 years ago
- Blocked by Task #2116: Rewrite EventEmitter added
Updated by Junxiao Shi over 9 years ago
EventEmitter is being rewritten in #2116. After that, usage of .clear
should be avoided in many cases.
Updated by Davide Pesavento over 9 years ago
Is the onFail.clear()
also going away as part of the EventEmitter rewrite?
Updated by Davide Pesavento over 9 years ago
- Category set to Utils
- Status changed from New to Code review
- Start date deleted (
11/23/2014)
Updated by Junxiao Shi over 9 years ago
I disagree with the solution given in commit 6de9884f2e1a5c68b6de95ee609f32b86deed9d6.
This bug is caused by EthernetFace's incorrect usage of EventEmitter. EventEmitter itself is perfectly fine.
Please be patient and wait for #2116 to close. After that, individual handlers can be unsubscribed without using .clear()
.
Updated by Davide Pesavento over 9 years ago
Junxiao Shi wrote:
I disagree with the solution given in commit 6de9884f2e1a5c68b6de95ee609f32b86deed9d6.
It's not a solution, it's a temporary workaround.
This bug is caused by EthernetFace's incorrect usage of EventEmitter. EventEmitter itself is perfectly fine.
Please explain how EthernetFace incorrectly uses EventEmitter.
Please be patient and wait for #2116 to close. After that, individual handlers can be unsubscribed without using
.clear()
.
But even after #2116, calling .clear()
under the same circumstances would still lead to a crash, wouldn't it?
Updated by Junxiao Shi over 9 years ago
It's not a solution, it's a temporary workaround.
Don't add this workaround. #2116 will bring a complete solution.
Please explain how EthernetFace incorrectly uses EventEmitter.
The caller of any EventEmitter operator / method must guarantee that EventEmitter itself stays valid during this call.
EthernetFace shouldn't cause its last reference to be held by some object inside EventEmitter.
But even after #2116, calling
.clear()
under the same circumstances would still lead to a crash, wouldn't it?
Yes, but EthernetFace won't call .clear()
anymore.
To be precise, #2116 provides a way to unsubscribe a specific handler. After that, #2213 will be moved to NFD, and the solution is to unsubscribe the handler instead of calling .clear()
.
Updated by Davide Pesavento over 9 years ago
Junxiao Shi wrote:
The caller of any EventEmitter operator / method must guarantee that EventEmitter itself stays valid during this call.
EthernetFace shouldn't cause its last reference to be held by some object inside EventEmitter.
This is a problem of the Face base class then, not of EthernetFace.
But even after #2116, calling
.clear()
under the same circumstances would still lead to a crash, wouldn't it?Yes, but EthernetFace won't call
.clear()
anymore.
To be precise, #2116 provides a way to unsubscribe a specific handler. After that, #2213 will be moved to NFD, and the solution is to unsubscribe the handler instead of calling.clear()
.
Again, I suppose you're talking about the Face class (Face::fail
)
Updated by Davide Pesavento over 9 years ago
- Target version set to v0.3
Davide Pesavento wrote:
I abandoned the change but redmine doesn't let me move the status of this task back to "New" or "In progress".
Updated by Junxiao Shi over 9 years ago
- Blocked by Task #2300: Face: use Signal added
Updated by Junxiao Shi about 9 years ago
- Status changed from New to Rejected
This Bug is no longer applicable because Face
doesn't use EventEmitter
anymore.