paullouisageneau
commented
4 years ago Good catch...
Actually onRecv(nullptr) unregisters the callback set by the upper layer (here the PeerConnection). The issue is actually caused by the order of shutdown() and mLower->onRecv(nullptr) in Transport::stop(), since it seems usrsctp_close() is not thread safe.
However doing so we need to take care of a potential deadlock if the lower layer thread is blocked in incoming() while we call Transport::stop(). I'm fixing that.
X-Kent
Hi. I am running a libdatachannel under valgrind and it detects a use-after-free error. Valgrind output:
==19020== Thread 21: ==19020== Invalid read of size 4 ==19020== at 0x8C38AA: sctp_free_assoc (sctp_pcb.c:5663) ==19020== by 0x88939C: sctp_handle_abort (sctp_input.c:910) ==19020== by 0x8861F3: sctp_process_control (sctp_input.c:5256) ==19020== by 0x8839A5: sctp_common_input_processing (sctp_input.c:5913) ==19020== by 0x87F139: usrsctp_conninput (user_socket.c:3353) ==19020== by 0x594445: rtc::SctpTransport::incoming(std::shared_ptr) (sctptransport.cpp:295)
==19020== by 0x56678B: void std::invoke_impl<void, void (rtc::Transport::&)(std::shared_ptr), rtc::Transport &, std::shared_ptr >(std::__invoke_memfun_deref, void (rtc::Transport::&)(std::shared_ptr), rtc::Transport &, std::shared_ptr&&) (invoke.h:73)
==19020== by 0x566AF4: operator() (std_function.h:706)
==19020== by 0x566AF4: rtc::synchronized_callback<std::shared_ptr >::operator()(std::shared_ptr) const (include.hpp:97)
==19020== by 0x565CEF: rtc::Transport::recv(std::shared_ptr) (transport.hpp:63)
==19020== by 0x5635CE: rtc::DtlsTransport::runRecvLoop() (dtlstransport.cpp:450)
==19020== by 0x55216DE: ??? (in /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.25)
==19020== by 0x5DAA6DA: start_thread (pthread_create.c:463)
==19020== Address 0x1159c374 is 484 bytes inside a block of size 584 free'd
==19020== at 0x4C30D3B: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==19020== by 0x8788C8: sodealloc (user_socket.c:241)
==19020== by 0x878848: sofree (user_socket.c:302)
==19020== by 0x87C8F9: usrsctp_close (user_socket.c:2020)
==19020== by 0x591B6E: rtc::SctpTransport::shutdown() (sctptransport.cpp:244)
==19020== by 0x5912DC: rtc::SctpTransport::stop() (sctptransport.cpp:202)
==19020== by 0x57A205: operator() (peerconnection.cpp:451)
==19020== by 0x57A205: invoke_impl<void, (lambda at src/thirdparty/libdatachannel/peerconnection.cpp:449:17)> (invoke.h:60)
==19020== by 0x57A205: invoke<(lambda at src/thirdparty/libdatachannel/peerconnection.cpp:449:17)> (invoke.h:95)
==19020== by 0x57A205: _M_invoke<0> (thread:234)
==19020== by 0x57A205: operator() (thread:243)
==19020== by 0x57A205: std::thread::_State_impl<std::thread::_Invoker<std::tuple<rtc::PeerConnection::closeTransports()::$_5> > >::_M_run() (thread:186)
==19020== by 0x55216DE: ??? (in /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.25)
==19020== by 0x5DAA6DA: start_thread (pthread_create.c:463)
==19020== by 0x60E388E: clone (clone.S:95)
==19020== Block was alloc'd at
==19020== at 0x4C2FB0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==19020== by 0x879463: soalloc (user_socket.c:206)
==19020== by 0x87B34C: socreate (user_socket.c:1251)
==19020== by 0x87B5FC: usrsctp_socket (user_socket.c:1346)
==19020== by 0x58F603: rtc::SctpTransport::SctpTransport(std::shared_ptr, unsigned short, std::function<void (std::shared_ptr)>, std::function<void (unsigned short, unsigned long)>, std::function<void (rtc::Transport::State)>) (sctptransport.cpp:95)
==19020== by 0x5753E1: construct<rtc::SctpTransport, std::shared_ptr &, unsigned short &, (lambda at include/thirdparty/rtc/include.hpp:73:9), (lambda at include/thirdparty/rtc/include.hpp:73:9), (lambda at src/thirdparty/libdatachannel/peerconnection.cpp:390:7)> (new_allocator.h:136)
==19020== by 0x5753E1: construct<rtc::SctpTransport, std::shared_ptr &, unsigned short &, (lambda at include/thirdparty/rtc/include.hpp:73:9), (lambda at include/thirdparty/rtc/include.hpp:73:9), (lambda at src/thirdparty/libdatachannel/peerconnection.cpp:390:7)> (alloc_traits.h:475)
==19020== by 0x5753E1: _Sp_counted_ptr_inplace<std::shared_ptr &, unsigned short &, (lambda at include/thirdparty/rtc/include.hpp:73:9), (lambda at include/thirdparty/rtc/include.hpp:73:9), (lambda at src/thirdparty/libdatachannel/peerconnection.cpp:390:7)> (shared_ptr_base.h:526)
==19020== by 0x5753E1: shared_count<rtc::SctpTransport, std::allocator, std::shared_ptr &, unsigned short &, (lambda at include/thirdparty/rtc/include.hpp:73:9), (lambda at include/thirdparty/rtc/include.hpp:73:9), (lambda at src/thirdparty/libdatachannel/peerconnection.cpp:390:7)> (shared_ptr_base.h:637)
==19020== by 0x5753E1: shared_ptr<std::allocator, std::shared_ptr &, unsigned short &, (lambda at include/thirdparty/rtc/include.hpp:73:9), (lambda at include/thirdparty/rtc/include.hpp:73:9), (lambda at src/thirdparty/libdatachannel/peerconnection.cpp:390:7)> (shared_ptr_base.h:1294)
==19020== by 0x5753E1: shared_ptr<std::allocator, std::shared_ptr &, unsigned short &, (lambda at include/thirdparty/rtc/include.hpp:73:9), (lambda at include/thirdparty/rtc/include.hpp:73:9), (lambda at src/thirdparty/libdatachannel/peerconnection.cpp:390:7)> (shared_ptr.h:344)
==19020== by 0x5753E1: allocate_shared<rtc::SctpTransport, std::allocator, std::shared_ptr &, unsigned short &, (lambda at include/thirdparty/rtc/include.hpp:73:9), (lambda at include/thirdparty/rtc/include.hpp:73:9), (lambda at src/thirdparty/libdatachannel/peerconnection.cpp:390:7)> (shared_ptr.h:690)
==19020== by 0x5753E1: make_shared<rtc::SctpTransport, std::shared_ptr &, unsigned short &, (lambda at include/thirdparty/rtc/include.hpp:73:9), (lambda at include/thirdparty/rtc/include.hpp:73:9), (lambda at src/thirdparty/libdatachannel/peerconnection.cpp:390:7)> (shared_ptr.h:706)
==19020== by 0x5753E1: rtc::PeerConnection::initSctpTransport() (peerconnection.cpp:387)
==19020== by 0x5795CA: operator() (peerconnection.cpp:331)
==19020== by 0x5795CA: std::_Function_handler<void (rtc::Transport::State), rtc::PeerConnection::initDtlsTransport()::$_3>::_M_invoke(std::_Any_data const&, rtc::Transport::State&&) (std_function.h:316)
==19020== by 0x5633C6: operator() (std_function.h:706)
==19020== by 0x5633C6: operator() (include.hpp:97)
==19020== by 0x5633C6: changeState (transport.hpp:66)
==19020== by 0x5633C6: rtc::DtlsTransport::runRecvLoop() (dtlstransport.cpp:441)
==19020== by 0x55216DE: ??? (in /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.25)
==19020== by 0x5DAA6DA: start_thread (pthread_create.c:463)
==19020== by 0x60E388E: clone (clone.S:95)
==19020==
==19020== Invalid write of size 4
==19020== at 0x8C38E4: sctp_free_assoc (sctp_pcb.c:5663)
==19020== by 0x88939C: sctp_handle_abort (sctp_input.c:910)
==19020== by 0x8861F3: sctp_process_control (sctp_input.c:5256)
==19020== by 0x8839A5: sctp_common_input_processing (sctp_input.c:5913)
==19020== by 0x87F139: usrsctp_conninput (user_socket.c:3353)
==19020== by 0x594445: rtc::SctpTransport::incoming(std::shared_ptr) (sctptransport.cpp:295)
==19020== by 0x56678B: void std:: invoke_impl<void, void (rtc::Transport::&)(std::shared_ptr), rtc::Transport &, std::shared_ptr >(std::invoke_memfun_deref, void (rtc::Transport::&)(std::shared_ptr), rtc::Transport &, std::shared_ptr&&) (invoke.h:73)
==19020== by 0x566AF4: operator() (std_function.h:706)
==19020== by 0x566AF4: rtc::synchronized_callback<std::shared_ptr >::operator()(std::shared_ptr) const (include.hpp:97)
==19020== by 0x565CEF: rtc::Transport::recv(std::shared_ptr) (transport.hpp:63)
==19020== by 0x5635CE: rtc::DtlsTransport::runRecvLoop() (dtlstransport.cpp:450)
==19020== by 0x55216DE: ??? (in /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.25)
==19020== by 0x5DAA6DA: start_thread (pthread_create.c:463)
==19020== Address 0x1159c374 is 484 bytes inside a block of size 584 free'd
==19020== at 0x4C30D3B: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==19020== by 0x8788C8: sodealloc (user_socket.c:241)
==19020== by 0x878848: sofree (user_socket.c:302)
==19020== by 0x87C8F9: usrsctp_close (user_socket.c:2020)
==19020== by 0x591B6E: rtc::SctpTransport::shutdown() (sctptransport.cpp:244)
==19020== by 0x5912DC: rtc::SctpTransport::stop() (sctptransport.cpp:202)
==19020== by 0x57A205: operator() (peerconnection.cpp:451)
==19020== by 0x57A205: __invoke_impl<void, (lambda at src/thirdparty/libdatachannel/peerconnection.cpp:449:17)> (invoke.h:60)
==19020== by 0x57A205: invoke<(lambda at src/thirdparty/libdatachannel/peerconnection.cpp:449:17)> (invoke.h:95)
==19020== by 0x57A205: _M_invoke<0> (thread:234)
==19020== by 0x57A205: operator() (thread:243)
==19020== by 0x57A205: std::thread::_State_impl<std::thread::_Invoker<std::tuple<rtc::PeerConnection::closeTransports()::$_5> > >::_M_run() (thread:186)
==19020== by 0x55216DE: ??? (in /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.25)
==19020== by 0x5DAA6DA: start_thread (pthread_create.c:463)
==19020== by 0x60E388E: clone (clone.S:95)
==19020== Block was alloc'd at
==19020== at 0x4C2FB0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==19020== by 0x879463: soalloc (user_socket.c:206)
==19020== by 0x87B34C: socreate (user_socket.c:1251)
==19020== by 0x87B5FC: usrsctp_socket (user_socket.c:1346)
==19020== by 0x58F603: rtc::SctpTransport::SctpTransport(std::shared_ptr, unsigned short, std::function<void (std::shared_ptr)>, std::function<void (unsigned short, unsigned long)>, std::function<void (rtc::Transport::State)>) (sctptransport.cpp:95)
==19020== by 0x5753E1: construct<rtc::SctpTransport, std::shared_ptr &, unsigned short &, (lambda at include/thirdparty/rtc/include.hpp:73:9), (lambda at include/thirdparty/rtc/include.hpp:73:9), (lambda at src/thirdparty/libdatachannel/peerconnection.cpp:390:7)> (new_allocator.h:136)
==19020== by 0x5753E1: construct<rtc::SctpTransport, std::shared_ptr &, unsigned short &, (lambda at include/thirdparty/rtc/include.hpp:73:9), (lambda at include/thirdparty/rtc/include.hpp:73:9), (lambda at src/thirdparty/libdatachannel/peerconnection.cpp:390:7)> (alloc_traits.h:475)
==19020== by 0x5753E1: _Sp_counted_ptr_inplace<std::shared_ptr &, unsigned short &, (lambda at include/thirdparty/rtc/include.hpp:73:9), (lambda at include/thirdparty/rtc/include.hpp:73:9), (lambda at src/thirdparty/libdatachannel/peerconnection.cpp:390:7)> (shared_ptr_base.h:526)
==19020== by 0x5753E1: shared_count<rtc::SctpTransport, std::allocator, std::shared_ptr &, unsigned short &, (lambda at include/thirdparty/rtc/include.hpp:73:9), (lambda at include/thirdparty/rtc/include.hpp:73:9), (lambda at src/thirdparty/libdatachannel/peerconnection.cpp:390:7)> (shared_ptr_base.h:637)
==19020== by 0x5753E1: shared_ptr<std::allocator, std::shared_ptr &, unsigned short &, (lambda at include/thirdparty/rtc/include.hpp:73:9), (lambda at include/thirdparty/rtc/include.hpp:73:9), (lambda at src/thirdparty/libdatachannel/peerconnection.cpp:390:7)> (shared_ptr_base.h:1294)
==19020== by 0x5753E1: shared_ptr<std::allocator, std::shared_ptr &, unsigned short &, (lambda at include/thirdparty/rtc/include.hpp:73:9), (lambda at include/thirdparty/rtc/include.hpp:73:9), (lambda at src/thirdparty/libdatachannel/peerconnection.cpp:390:7)> (shared_ptr.h:344)
==19020== by 0x5753E1: allocate_shared<rtc::SctpTransport, std::allocator, std::shared_ptr &, unsigned short &, (lambda at include/thirdparty/rtc/include.hpp:73:9), (lambda at include/thirdparty/rtc/include.hpp:73:9), (lambda at src/thirdparty/libdatachannel/peerconnection.cpp:390:7)> (shared_ptr.h:690)
==19020== by 0x5753E1: make_shared<rtc::SctpTransport, std::shared_ptr &, unsigned short &, (lambda at include/thirdparty/rtc/include.hpp:73:9), (lambda at include/thirdparty/rtc/include.hpp:73:9), (lambda at src/thirdparty/libdatachannel/peerconnection.cpp:390:7)> (shared_ptr.h:706)
==19020== by 0x5753E1: rtc::PeerConnection::initSctpTransport() (peerconnection.cpp:387)
==19020== by 0x5795CA: operator() (peerconnection.cpp:331)
==19020== by 0x5795CA: std::_Function_handler<void (rtc::Transport::State), rtc::PeerConnection::initDtlsTransport()::$_3>::_M_invoke(std::_Any_data const&, rtc::Transport::State&&) (std_function.h:316)
==19020== by 0x5633C6: operator() (std_function.h:706)
==19020== by 0x5633C6: operator() (include.hpp:97)
==19020== by 0x5633C6: changeState (transport.hpp:66)
==19020== by 0x5633C6: rtc::DtlsTransport::runRecvLoop() (dtlstransport.cpp:441)
==19020== by 0x55216DE: ??? (in /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.25)
==19020== by 0x5DAA6DA: start_thread (pthread_create.c:463)
==19020== by 0x60E388E: clone (clone.S:95)
==19020==
in sctptransport::stop() there is: shutdown(); onRecv(nullptr); Shouldn't it be in reverse order ?
usrctp/libjuice/libdatachannel are from current master.