Project

General

Profile

Actions

Bug #2213

closed

EventEmitter::clear() leads to double free when called from Face::fail()

Added by Davide Pesavento over 9 years ago. Updated about 9 years ago.

Status:
Rejected
Priority:
Normal
Assignee:
-
Category:
Utils
Target version:
Start date:
Due date:
% Done:

0%

Estimated time:

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 using std::_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 of shared_ptr pointing at the EthernetFace that initially called fail
  • ...which triggers the destruction of the EthernetFace, i.e. ~EthernetFace and ~Face are called
  • ...which means that ~EventEmitter for the Face::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, therefore std::_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

Related issues 2 (0 open2 closed)

Blocked by ndn-cxx - Task #2116: Rewrite EventEmitterClosedJunxiao Shi

Actions
Blocked by NFD - Task #2300: Face: use SignalClosedJunxiao Shi

Actions
Actions #1

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

Actions #2

Updated by Junxiao Shi over 9 years ago

  • Blocked by Task #2116: Rewrite EventEmitter added
Actions #3

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.

Actions #4

Updated by Davide Pesavento over 9 years ago

Is the onFail.clear() also going away as part of the EventEmitter rewrite?

Actions #5

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

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().

Actions #7

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?

Actions #8

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().

Actions #9

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)

Actions #10

Updated by Davide Pesavento over 9 years ago

  • Target version set to v0.3

Davide Pesavento wrote:

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

I abandoned the change but redmine doesn't let me move the status of this task back to "New" or "In progress".

Actions #11

Updated by Junxiao Shi over 9 years ago

  • Status changed from Code review to New
Actions #12

Updated by Junxiao Shi over 9 years ago

Actions #13

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.

Actions

Also available in: Atom PDF