From: Remi Gacogne Date: Tue, 25 May 2021 13:37:59 +0000 (+0200) Subject: dnsdist: Fix a data race reported by TSAN in SNMP metrics X-Git-Tag: auth-4.5.0-beta1~25^2 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=refs%2Fpull%2F10439%2Fhead;p=thirdparty%2Fpdns.git dnsdist: Fix a data race reported by TSAN in SNMP metrics Unless I'm mistaken, the report (below) is a false positive since `std::atomic::operator T` is equivalent to `load()` and `std::atomic::operator=` to `store()`, but perhaps I'm missing something. Note that I could not reproduce the issue using clang++ 11.1.0's TSAN, only with the one from g++ 11.1.0. ``` ================== WARNING: ThreadSanitizer: data race (pid=11157) Atomic read of size 8 at 0x7b7400002580 by thread T2: #0 __tsan_atomic64_load (dnsdist+0x7a6eb0) #1 std::atomic::load(std::memory_order) const /usr/bin/../lib/gcc/x86_64-linux-gnu/8/../../../../include/c++/8/atomic:250:2 (dnsdist+0x83c4da) #2 pdns::stat_t_trait::operator double() const /opt/project/pdns/dnsdistdist/./stat_t.hh:67:60 (dnsdist+0xd05b2e) #3 backendStatTable_handler(netsnmp_mib_handler_s*, netsnmp_handler_registration_s*, netsnmp_agent_request_info_s*, netsnmp_request_info_s*) /opt/project/pdns/dnsdistdist/dnsdist-snmp.cc:356:54 (dnsdist+0xeceab8) #4 netsnmp_call_next_handler (libnetsnmpagent.so.30+0x2a0cc) #5 SNMPAgent::handleSNMPQueryCB(int, boost::any&) /opt/project/pdns/dnsdistdist/snmp-agent.cc:96:13 (dnsdist+0xfb2417) #6 boost::detail::function::void_function_invoker2::invoke(boost::detail::function::function_buffer&, int, boost::any&) /usr/include/boost/function/function_template.hpp:118:11 (dnsdist+0x8956e8) #7 boost::function2::operator()(int, boost::any&) const /usr/include/boost/function/function_template.hpp:768:14 (dnsdist+0xf9f13c) #8 EpollFDMultiplexer::run(timeval*, int) /opt/project/pdns/dnsdistdist/epollmplexer.cc:176:7 (dnsdist+0xfd1feb) #9 SNMPAgent::worker() /opt/project/pdns/dnsdistdist/snmp-agent.cc:141:24 (dnsdist+0xfb273f) #10 void std::__invoke_impl(std::__invoke_memfun_deref, void (SNMPAgent::*&&)(), SNMPAgent*&&) /usr/bin/../lib/gcc/x86_64-linux-gnu/8/../../../../include/c++/8/bits/invoke.h:73:14 (dnsdist+0xf36079) #11 std::__invoke_result::type std::__invoke(void (SNMPAgent::*&&)(), SNMPAgent*&&) /usr/bin/../lib/gcc/x86_64-linux-gnu/8/../../../../include/c++/8/bits/invoke.h:95:14 (dnsdist+0xf35f71) #12 decltype(std::__invoke(_S_declval<0ul>(), _S_declval<1ul>())) std::thread::_Invoker >::_M_invoke<0ul, 1ul>(std::_Index_tuple<0ul, 1ul>) /usr/bin/../lib/gcc/x86_64-linux-gnu/8/../../../../include/c++/8/thread:244:13 (dnsdist+0xf35f1e) #13 std::thread::_Invoker >::operator()() /usr/bin/../lib/gcc/x86_64-linux-gnu/8/../../../../include/c++/8/thread:253:11 (dnsdist+0xf35ec5) #14 std::thread::_State_impl > >::_M_run() /usr/bin/../lib/gcc/x86_64-linux-gnu/8/../../../../include/c++/8/thread:196:13 (dnsdist+0xf35c29) #15 (libstdc++.so.6+0xbbb2e) Previous write of size 8 at 0x7b7400002580 by thread T18: [failed to restore the stack] Location is heap block of size 2304 at 0x7b7400001e00 allocated by main thread: #0 operator new(unsigned long, std::align_val_t) (dnsdist+0x7e8c57) #1 __gnu_cxx::new_allocator, (__gnu_cxx::_Lock_policy)2> >::allocate(unsigned long, void const*) /usr/bin/../lib/gcc/x86_64-linux-gnu/8/../../../../include/c++/8/ext/new_allocator.h:108:31 (dnsdist+0xe6b4e2) #2 std::allocator_traits, (__gnu_cxx::_Lock_policy)2> > >::allocate(std::allocator, (__gnu_cxx::_Lock_policy)2> >&, unsigned long) /usr/bin/../lib/gcc/x86_64-linux-gnu/8/../../../../include/c++/8/bits/alloc_traits.h:436:20 (dnsdist+0xe6b430) #3 std::__allocated_ptr, (__gnu_cxx::_Lock_policy)2> > > std::__allocate_guarded, (__gnu_cxx::_Lock_policy)2> > >(std::allocator, (__gnu_cxx::_Lock_policy)2> >&) /usr/bin/../lib/gcc/x86_64-linux-gnu/8/../../../../include/c++/8/bits/allocated_ptr.h:97:21 (dnsdist+0xe6b1d0) #4 std::__shared_count<(__gnu_cxx::_Lock_policy)2>::__shared_count, ComboAddress&, ComboAddress&, unsigned int&, std::__cxx11::basic_string, std::allocator >&, unsigned long&, bool>(DownstreamState*&, std::_Sp_alloc_shared_tag >, ComboAddress&, ComboAddress&, unsigned int&, std::__cxx11::basic_string, std::allocator >&, unsigned long&, bool&&) /usr/bin/../lib/gcc/x86_64-linux-gnu/8/../../../../include/c++/8/bits/shared_ptr_base.h:675:19 (dnsdist+0xe6fb0a) #5 std::__shared_ptr::__shared_ptr, ComboAddress&, ComboAddress&, unsigned int&, std::__cxx11::basic_string, std::allocator >&, unsigned long&, bool>(std::_Sp_alloc_shared_tag >, ComboAddress&, ComboAddress&, unsigned int&, std::__cxx11::basic_string, std::allocator >&, unsigned long&, bool&&) /usr/bin/../lib/gcc/x86_64-linux-gnu/8/../../../../include/c++/8/bits/shared_ptr_base.h:1342:14 (dnsdist+0xe6fa7a) #6 std::shared_ptr::shared_ptr, ComboAddress&, ComboAddress&, unsigned int&, std::__cxx11::basic_string, std::allocator >&, unsigned long&, bool>(std::_Sp_alloc_shared_tag >, ComboAddress&, ComboAddress&, unsigned int&, std::__cxx11::basic_string, std::allocator >&, unsigned long&, bool&&) /usr/bin/../lib/gcc/x86_64-linux-gnu/8/../../../../include/c++/8/bits/shared_ptr.h:359:4 (dnsdist+0xe6f99d) #7 std::shared_ptr std::allocate_shared, ComboAddress&, ComboAddress&, unsigned int&, std::__cxx11::basic_string, std::allocator >&, unsigned long&, bool>(std::allocator const&, ComboAddress&, ComboAddress&, unsigned int&, std::__cxx11::basic_string, std::allocator >&, unsigned long&, bool&&) /usr/bin/../lib/gcc/x86_64-linux-gnu/8/../../../../include/c++/8/bits/shared_ptr.h:705:14 (dnsdist+0xe6f8e0) #8 std::shared_ptr std::make_shared, std::allocator >&, unsigned long&, bool>(ComboAddress&, ComboAddress&, unsigned int&, std::__cxx11::basic_string, std::allocator >&, unsigned long&, bool&&) /usr/bin/../lib/gcc/x86_64-linux-gnu/8/../../../../include/c++/8/bits/shared_ptr.h:721:14 (dnsdist+0xe6a726) #9 setupLuaConfig(LuaContext&, bool, bool)::$_2::operator()(boost::variant, std::allocator >, std::unordered_map, std::allocator >, boost::variant, std::allocator >, std::vector, std::allocator > >, std::allocator, std::allocator > > > >, std::function (DNSName const&, unsigned short, unsigned short, dnsheader*)> >, std::hash, std::allocator > >, std::equal_to, std::allocator > >, std::allocator, std::allocator > const, boost::variant, std::allocator >, std::vector, std::allocator > >, std::allocator, std::allocator > > > >, std::function (DNSName const&, unsigned short, unsigned short, dnsheader*)> > > > > >, boost::optional) const /opt/project/pdns/dnsdistdist/dnsdist-lua.cc:356:11 (dnsdist+0xdf9166) #10 _ZN10LuaContext6BinderIRZL14setupLuaConfigRS_bbE3$_2RKN5boost7variantINSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEEJSt13unordered_mapISB_NS5_IbJSB_St6vectorISt4pairIiSB_ESaISF_EESt8functionIFSt5tupleIJ7DNSNamettEERKSK_ttP9dnsheaderEEEEESt4hashISB_ESt8equal_toISB_ESaISE_IKSB_SS_EEEEEEEclIJRKNS4_8optionalIiEEEEEDTcldtdefpT8functiondtdefpT5paramspclsr3stdE7forwardIT_Efp_EEEDpOS1A_ /opt/project/pdns/dnsdistdist/./ext/luawrapper/include/LuaContext.hpp:1846:20 (dnsdist+0xdf89af) #11 _ZN10LuaContext6BinderIRNS0_IRZL14setupLuaConfigRS_bbE3$_2RKN5boost7variantINSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEEJSt13unordered_mapISB_NS5_IbJSB_St6vectorISt4pairIiSB_ESaISF_EESt8functionIFSt5tupleIJ7DNSNamettEERKSK_ttP9dnsheaderEEEEESt4hashISB_ESt8equal_toISB_ESaISE_IKSB_SS_EEEEEEEERKNS4_8optionalIiEEEclIJEEEDTcldtdefpT8functiondtdefpT5paramspclsr3stdE7forwardIT_Efp_EEEDpOS1C_ /opt/project/pdns/dnsdistdist/./ext/luawrapper/include/LuaContext.hpp:1846:20 (dnsdist+0xdf8917) #12 std::shared_ptr LuaContext::readIntoFunction, LuaContext::Binder, std::allocator >, std::unordered_map, std::allocator >, boost::variant, std::allocator >, std::vector, std::allocator > >, std::allocator, std::allocator > > > >, std::function (DNSName const&, unsigned short, unsigned short, dnsheader*)> >, std::hash, std::allocator > >, std::equal_to, std::allocator > >, std::allocator, std::allocator > const, boost::variant, std::allocator >, std::vector, std::allocator > >, std::allocator, std::allocator > > > >, std::function (DNSName const&, unsigned short, unsigned short, dnsheader*)> > > > > > const&>&, boost::optional const&>&>(lua_State*, LuaContext::tag >, LuaContext::Binder, std::allocator >, std::unordered_map, std::allocator >, boost::variant, std::allocator >, std::vector, std::allocator > >, std::allocator, std::allocator > > > >, std::function (DNSName const&, unsigned short, unsigned short, dnsheader*)> >, std::hash, std::allocator > >, std::equal_to, std::allocator > >, std::allocator, std::allocator > const, boost::variant, std::allocator >, std::vector, std::allocator > >, std::allocator, std::allocator > > > >, std::function (DNSName const&, unsigned short, unsigned short, dnsheader*)> > > > > > const&>&, boost::optional const&>&, int) /opt/project/pdns/dnsdistdist/./ext/luawrapper/include/LuaContext.hpp:1766:16 (dnsdist+0xdf88be) #13 std::enable_if >::value, std::shared_ptr >::type LuaContext::readIntoFunction, LuaContext::Binder, std::allocator >, std::unordered_map, std::allocator >, boost::variant, std::allocator >, std::vector, std::allocator > >, std::allocator, std::allocator > > > >, std::function (DNSName const&, unsigned short, unsigned short, dnsheader*)> >, std::hash, std::allocator > >, std::equal_to, std::allocator > >, std::allocator, std::allocator > const, boost::variant, std::allocator >, std::vector, std::allocator > >, std::allocator, std::allocator > > > >, std::function (DNSName const&, unsigned short, unsigned short, dnsheader*)> > > > > > const&>&, boost::optional >(lua_State*, LuaContext::tag >, LuaContext::Binder, std::allocator >, std::unordered_map, std::allocator >, boost::variant, std::allocator >, std::vector, std::allocator > >, std::allocator, std::allocator > > > >, std::function (DNSName const&, unsigned short, unsigned short, dnsheader*)> >, std::hash, std::allocator > >, std::equal_to, std::allocator > >, std::allocator, std::allocator > const, boost::variant, std::allocator >, std::vector, std::allocator > >, std::allocator, std::allocator > > > >, std::function (DNSName const&, unsigned short, unsigned short, dnsheader*)> > > > > > const&>&, int, LuaContext::tag >) /opt/project/pdns/dnsdistdist/./ext/luawrapper/include/LuaContext.hpp:1774:20 (dnsdist+0xdf8624) #14 std::enable_if, std::allocator >, std::unordered_map, std::allocator >, boost::variant, std::allocator >, std::vector, std::allocator > >, std::allocator, std::allocator > > > >, std::function (DNSName const&, unsigned short, unsigned short, dnsheader*)> >, std::hash, std::allocator > >, std::equal_to, std::allocator > >, std::allocator, std::allocator > const, boost::variant, std::allocator >, std::vector, std::allocator > >, std::allocator, std::allocator > > > >, std::function (DNSName const&, unsigned short, unsigned short, dnsheader*)> > > > > > >::value), std::shared_ptr >::type LuaContext::readIntoFunction, setupLuaConfig(LuaContext&, bool, bool)::$_2&, boost::variant, std::allocator >, std::unordered_map, std::allocator >, boost::variant, std::allocator >, std::vector, std::allocator > >, std::allocator, std::allocator > > > >, std::function (DNSName const&, unsigned short, unsigned short, dnsheader*)> >, std::hash, std::allocator > >, std::equal_to, std::allocator > >, std::allocator, std::allocator > const, boost::variant, std::allocator >, std::vector, std::allocator > >, std::allocator, std::allocator > > > >, std::function (DNSName const&, unsigned short, unsigned short, dnsheader*)> > > > > >, boost::optional >(lua_State*, LuaContext::tag >, setupLuaConfig(LuaContext&, bool, bool)::$_2&, int, LuaContext::tag, std::allocator >, std::unordered_map, std::allocator >, boost::variant, std::allocator >, std::vector, std::allocator > >, std::allocator, std::allocator > > > >, std::function (DNSName const&, unsigned short, unsigned short, dnsheader*)> >, std::hash, std::allocator > >, std::equal_to, std::allocator > >, std::allocator, std::allocator > const, boost::variant, std::allocator >, std::vector, std::allocator > >, std::allocator, std::allocator > > > >, std::function (DNSName const&, unsigned short, unsigned short, dnsheader*)> > > > > > >, LuaContext::tag >) /opt/project/pdns/dnsdistdist/./ext/luawrapper/include/LuaContext.hpp:1796:16 (dnsdist+0xdf848d) #15 std::enable_if<(!(std::integral_constant::value)) && (!(std::is_void::value)), LuaContext::PushedObject>::type LuaContext::Pusher (boost::variant, std::allocator >, std::unordered_map, std::allocator >, boost::variant, std::allocator >, std::vector, std::allocator > >, std::allocator, std::allocator > > > >, std::function (DNSName const&, unsigned short, unsigned short, dnsheader*)> >, std::hash, std::allocator > >, std::equal_to, std::allocator > >, std::allocator, std::allocator > const, boost::variant, std::allocator >, std::vector, std::allocator > >, std::allocator, std::allocator > > > >, std::function (DNSName const&, unsigned short, unsigned short, dnsheader*)> > > > > >, boost::optional), void>::callback2(lua_State*, bool&&, int) /opt/project/pdns/dnsdistdist/./ext/luawrapper/include/LuaContext.hpp:2429:31 (dnsdist+0xdf83c2) #16 LuaContext::PushedObject LuaContext::Pusher (boost::variant, std::allocator >, std::unordered_map, std::allocator >, boost::variant, std::allocator >, std::vector, std::allocator > >, std::allocator, std::allocator > > > >, std::function (DNSName const&, unsigned short, unsigned short, dnsheader*)> >, std::hash, std::allocator > >, std::equal_to, std::allocator > >, std::allocator, std::allocator > const, boost::variant, std::allocator >, std::vector, std::allocator > >, std::allocator, std::allocator > > > >, std::function (DNSName const&, unsigned short, unsigned short, dnsheader*)> > > > > >, boost::optional), void>::callback(lua_State*, setupLuaConfig(LuaContext&, bool, bool)::$_2*, int) /opt/project/pdns/dnsdistdist/./ext/luawrapper/include/LuaContext.hpp:2398:20 (dnsdist+0xdf8169) #17 std::enable_if::value, LuaContext::PushedObject>::type LuaContext::Pusher (boost::variant, std::allocator >, std::unordered_map, std::allocator >, boost::variant, std::allocator >, std::vector, std::allocator > >, std::allocator, std::allocator > > > >, std::function (DNSName const&, unsigned short, unsigned short, dnsheader*)> >, std::hash, std::allocator > >, std::equal_to, std::allocator > >, std::allocator, std::allocator > const, boost::variant, std::allocator >, std::vector, std::allocator > >, std::allocator, std::allocator > > > >, std::function (DNSName const&, unsigned short, unsigned short, dnsheader*)> > > > > >, boost::optional), void>::push(lua_State*, setupLuaConfig(LuaContext&, bool, bool)::$_2)::'lambda'(lua_State*)::operator()(lua_State*) const /opt/project/pdns/dnsdistdist/./ext/luawrapper/include/LuaContext.hpp:2327:20 (dnsdist+0xdf80c9) #18 std::enable_if::value, LuaContext::PushedObject>::type LuaContext::Pusher (boost::variant, std::allocator >, std::unordered_map, std::allocator >, boost::variant, std::allocator >, std::vector, std::allocator > >, std::allocator, std::allocator > > > >, std::function (DNSName const&, unsigned short, unsigned short, dnsheader*)> >, std::hash, std::allocator > >, std::equal_to, std::allocator > >, std::allocator, std::allocator > const, boost::variant, std::allocator >, std::vector, std::allocator > >, std::allocator, std::allocator > > > >, std::function (DNSName const&, unsigned short, unsigned short, dnsheader*)> > > > > >, boost::optional), void>::push(lua_State*, setupLuaConfig(LuaContext&, bool, bool)::$_2)::'lambda'(lua_State*)::__invoke(lua_State*) /opt/project/pdns/dnsdistdist/./ext/luawrapper/include/LuaContext.hpp:2323:31 (dnsdist+0xdf8055) #19 _init (libluajit-5.1.so.2+0x9dd6) #20 std::tuple<> LuaContext::call >(lua_State*, LuaContext::PushedObject) /opt/project/pdns/dnsdistdist/./ext/luawrapper/include/LuaContext.hpp:1413:29 (dnsdist+0xc9ae67) #21 LuaContext::executeCode(std::istream&) /opt/project/pdns/dnsdistdist/./ext/luawrapper/include/LuaContext.hpp:267:9 (dnsdist+0xe6077a) #22 setupLua(LuaContext&, bool, bool, std::__cxx11::basic_string, std::allocator > const&) /opt/project/pdns/dnsdistdist/dnsdist-lua.cc:2599:10 (dnsdist+0xdf4ff3) #23 main /opt/project/pdns/dnsdistdist/dnsdist.cc:2235:17 (dnsdist+0xf282ea) Thread T2 'dnsdist/SNMP' (tid=11160, running) created by main thread at: #0 pthread_create (dnsdist+0x75f2d5) #1 std::thread::_M_start_thread(std::unique_ptr >, void (*)()) (libstdc++.so.6+0xbbdb4) #2 SNMPAgent::run() /opt/project/pdns/dnsdistdist/./snmp-agent.hh:36:14 (dnsdist+0xf3087c) #3 main /opt/project/pdns/dnsdistdist/dnsdist.cc:2373:20 (dnsdist+0xf28f32) Thread T18 'dnsdist/healthC' (tid=11176, running) created by main thread at: #0 pthread_create (dnsdist+0x75f2d5) #1 std::thread::_M_start_thread(std::unique_ptr >, void (*)()) (libstdc++.so.6+0xbbdb4) #2 main /opt/project/pdns/dnsdistdist/dnsdist.cc:2463:12 (dnsdist+0xf29769) SUMMARY: ThreadSanitizer: data race (/opt/dnsdist-with-tsan/bin/dnsdist+0x7a6eb0) in __tsan_atomic64_load ``` --- diff --git a/pdns/dnsdist-snmp.cc b/pdns/dnsdist-snmp.cc index 6aedb06ee6..d4f4937d6d 100644 --- a/pdns/dnsdist-snmp.cc +++ b/pdns/dnsdist-snmp.cc @@ -311,14 +311,14 @@ static int backendStatTable_handler(netsnmp_mib_handler* handler, break; case COLUMN_BACKENDOUTSTANDING: DNSDistSNMPAgent::setCounter64Value(request, - server->outstanding); + server->outstanding.load()); break; case COLUMN_BACKENDQPSLIMIT: DNSDistSNMPAgent::setCounter64Value(request, server->qps.getRate()); break; case COLUMN_BACKENDREUSED: - DNSDistSNMPAgent::setCounter64Value(request, server->reuseds); + DNSDistSNMPAgent::setCounter64Value(request, server->reuseds.load()); break; case COLUMN_BACKENDSTATE: { @@ -353,10 +353,10 @@ static int backendStatTable_handler(netsnmp_mib_handler* handler, break; } case COLUMN_BACKENDQPS: - DNSDistSNMPAgent::setCounter64Value(request, server->queryLoad); + DNSDistSNMPAgent::setCounter64Value(request, server->queryLoad.load()); break; case COLUMN_BACKENDQUERIES: - DNSDistSNMPAgent::setCounter64Value(request, server->queries); + DNSDistSNMPAgent::setCounter64Value(request, server->queries.load()); break; case COLUMN_BACKENDORDER: DNSDistSNMPAgent::setCounter64Value(request, server->order); diff --git a/pdns/dnsdist.cc b/pdns/dnsdist.cc index 0b4c5b316b..9a2ac0a23f 100644 --- a/pdns/dnsdist.cc +++ b/pdns/dnsdist.cc @@ -1709,8 +1709,8 @@ static void healthChecksThread() } auto delta = dss->sw.udiffAndSet()/1000000.0; - dss->queryLoad = 1.0*(dss->queries.load() - dss->prev.queries.load())/delta; - dss->dropRate = 1.0*(dss->reuseds.load() - dss->prev.reuseds.load())/delta; + dss->queryLoad.store(1.0*(dss->queries.load() - dss->prev.queries.load())/delta); + dss->dropRate.store(1.0*(dss->reuseds.load() - dss->prev.reuseds.load())/delta); dss->prev.queries.store(dss->queries.load()); dss->prev.reuseds.store(dss->reuseds.load());