From 70120f910d1538a67430e23e5c36d9e8d52e76d7 Mon Sep 17 00:00:00 2001 From: Richard de Mellow Date: Thu, 24 Jun 2021 16:16:37 +0100 Subject: [PATCH] Fix TSAN race when destroying a MockCookie Fix race of more than one thread trying to free a MockCookie. This is due to MockServerCookieApi::release() and destroy_mock_cookie() racing with each other. For instance, T1 could dec the ref count then pause. T2 gets run which decs the ref count, reads the ref count as 0 and frees the MockCookie. Then T1 gets run again, it tries to call getRefcount() which causes it to deref an invalid pointer as the MockCookie has been freed. To fix this make the read+write of the ref count atomic. RNING: ThreadSanitizer: data race on vptr (ctor/dtor vs virtual call) (pid=107468) Write of size 8 at 0x7b4800030a80 by thread T2 (mutexes: write M1121813589057925128): #0 cb::tracing::Traceable::~Traceable() ../kv_engine/include/memcached/tracer.h:132 (ep_testsuite_dcp+0x5aab16) #1 MockCookie::~MockCookie() ../kv_engine/programs/engine_testapp/mock_cookie.cc:22 (ep_testsuite_dcp+0x5a9ddd) #2 MockCookie::~MockCookie() ../kv_engine/programs/engine_testapp/mock_cookie.cc:18 (ep_testsuite_dcp+0x5a9e45) #3 MockServerCookieApi::release(CookieIface const&) ../kv_engine/programs/engine_testapp/mock_server.cc:235 (ep_testsuite_dcp+0x5b86c4) #4 EventuallyPersistentEngine::releaseCookie(CookieIface const*) /home/couchbase/jenkins/workspace/kv_engine.threadsanitizer_master/kv_engine/engines/ep/src/ep_engine.cc:1793 (ep_testsuite_dcp+0x765c89) #5 ConnHandler::releaseReference() /home/couchbase/jenkins/workspace/kv_engine.threadsanitizer_master/kv_engine/engines/ep/src/connhandler.cc:343 (ep_testsuite_dcp+0x86cb6d) #6 DcpConnMap::manageConnections() /home/couchbase/jenkins/workspace/kv_engine.threadsanitizer_master/kv_engine/engines/ep/src/dcp/dcpconnmap.cc:392 (ep_testsuite_dcp+0x7f415e) ... Previous read of size 8 at 0x7b4800030a80 by main thread: #0 destroy_mock_cookie(CookieIface*) ../kv_engine/programs/engine_testapp/mock_cookie.cc:48 (ep_testsuite_dcp+0x5a9fa6) #1 MockTestHarness::destroy_cookie(CookieIface*) /home/couchbase/jenkins/workspace/kv_engine.threadsanitizer_master/kv_engine/programs/engine_testapp/engine_testapp.cc:159 (ep_testsuite_dcp+0x490ae5) #2 test_dcp_producer_stream_req_backfill(EngineIface*) ../kv_engine/engines/ep/tests/ep_testsuite_dcp.cc:2306 (ep_testsuite_dcp+0x4d3e3f) ... Location is heap block of size 336 at 0x7b4800030a80 allocated by main thread: #0 operator new(unsigned long) (libtsan.so.0+0x87c5c) #1 create_mock_cookie(EngineIface*) ../kv_engine/programs/engine_testapp/mock_cookie.cc:32 (ep_testsuite_dcp+0x5a9f0a) #2 MockTestHarness::create_cookie(EngineIface*) /home/couchbase/jenkins/workspace/kv_engine.threadsanitizer_master/kv_engine/programs/engine_testapp/engine_testapp.cc:155 (ep_testsuite_dcp+0x490aa5) #3 test_dcp_producer_stream_req_backfill(EngineIface*) ../kv_engine/engines/ep/tests/ep_testsuite_dcp.cc:2287 (ep_testsuite_dcp+0x4d3d31) #4 execute_test(test, char const*, char const*)::$_1::operator()() const /home/couchbase/jenkins/workspace/kv_engine.threadsanitizer_master/kv_engine/programs/engine_testapp/engine_testapp.cc:445 (ep_testsuite_dcp+0x48f988) #5 test_result std::__invoke_impl(std::__invoke_other, execute_test(test, char const*, char const*)::$_1&) /opt/gcc-10.2.0/lib/gcc/x86_64-pc-linux-gnu/10.2.0/../../../../include/c++/10.2.0/bits/invoke.h:60 (ep_testsuite_dcp+0x48f90d) #6 std::enable_if, test_result>::type std::__invoke_r(execute_test(test, char const*, char const*)::$_1&) /opt/gcc-10.2.0/lib/gcc/x86_64-pc-linux-gnu/10.2.0/../../../../include/c++/10.2.0/bits/invoke.h:113 (ep_testsuite_dcp+0x48f89d) #7 std::_Function_handler::_M_invoke(std::_Any_data const&) /opt/gcc-10.2.0/lib/gcc/x86_64-pc-linux-gnu/10.2.0/../../../../include/c++/10.2.0/bits/std_function.h:291 (ep_testsuite_dcp+0x48f78d) #8 std::function::operator()() const /opt/gcc-10.2.0/lib/gcc/x86_64-pc-linux-gnu/10.2.0/../../../../include/c++/10.2.0/bits/std_function.h:622 (ep_testsuite_dcp+0x4900d8) #9 try_run_test(std::function) /home/couchbase/jenkins/workspace/kv_engine.threadsanitizer_master/kv_engine/programs/engine_testapp/engine_testapp.cc:288 (ep_testsuite_dcp+0x48d368) #10 execute_test(test, char const*, char const*) /home/couchbase/jenkins/workspace/kv_engine.threadsanitizer_master/kv_engine/programs/engine_testapp/engine_testapp.cc:445 (ep_testsuite_dcp+0x48ea37) #11 main /home/couchbase/jenkins/workspace/kv_engine.threadsanitizer_master/kv_engine/programs/engine_testapp/engine_testapp.cc:698 (ep_testsuite_dcp+0x48dbfb) Mutex M1121813589057925128 is already destroyed. SUMMARY: ThreadSanitizer: data race on vptr (ctor/dtor vs virtual call) ../kv_engine/include/memcached/tracer.h:132 in cb::tracing::Traceable::~Traceable() Change-Id: I5cc6959ee9644c8c780b239cd63a6071c10c6c45 Reviewed-on: http://review.couchbase.org/c/kv_engine/+/156420 Reviewed-by: Dave Rigby Tested-by: Build Bot --- daemon/cookie.h | 8 ++++---- include/memcached/cookie_iface.h | 6 ++++-- programs/engine_testapp/mock_cookie.cc | 3 +-- programs/engine_testapp/mock_cookie.h | 8 ++++---- programs/engine_testapp/mock_server.cc | 3 +-- 5 files changed, 14 insertions(+), 14 deletions(-) diff --git a/daemon/cookie.h b/daemon/cookie.h index 59e37577a5..270baf4cfc 100644 --- a/daemon/cookie.h +++ b/daemon/cookie.h @@ -393,19 +393,19 @@ class Cookie : public CookieIface { uint8_t getRefcount() override { return refcount; } - void incrementRefcount() override { + uint8_t incrementRefcount() override { if (refcount == 255) { throw std::logic_error( "Cookie::incrementRefcount(): refcount will wrap"); } - refcount++; + return ++refcount; } - void decrementRefcount() override { + uint8_t decrementRefcount() override { if (refcount == 0) { throw std::logic_error( "Cookie::decrementRefcount(): refcount will wrap"); } - refcount--; + return --refcount; } void* getEngineStorage() const override { diff --git a/include/memcached/cookie_iface.h b/include/memcached/cookie_iface.h index 381949c676..a842aa1bb3 100644 --- a/include/memcached/cookie_iface.h +++ b/include/memcached/cookie_iface.h @@ -40,9 +40,11 @@ class CookieIface : public cb::tracing::Traceable { /// Get the current reference count virtual uint8_t getRefcount() = 0; /// Add a reference to the cookie - virtual void incrementRefcount() = 0; + /// returns the incremented ref count + virtual uint8_t incrementRefcount() = 0; /// Release a reference to the cookie - virtual void decrementRefcount() = 0; + /// returns the decremented ref count + virtual uint8_t decrementRefcount() = 0; // The underlying engine may store information bound to the given cookie // in an opaque pointer. The framework will _NOT_ take ownership of the diff --git a/programs/engine_testapp/mock_cookie.cc b/programs/engine_testapp/mock_cookie.cc index d5af2a8fdd..2c553050f9 100644 --- a/programs/engine_testapp/mock_cookie.cc +++ b/programs/engine_testapp/mock_cookie.cc @@ -37,7 +37,7 @@ void destroy_mock_cookie(CookieIface* cookie) { } c->disconnect(); - if (c->getRefcount() == 0) { + if (c->decrementRefcount() == 0) { delete c; } } @@ -100,7 +100,6 @@ void MockCookie::waitForNotifications(std::unique_lock& lock) { } void MockCookie::disconnect() { - decrementRefcount(); if (engine) { engine->disconnect(*this); } diff --git a/programs/engine_testapp/mock_cookie.h b/programs/engine_testapp/mock_cookie.h index ef940ac56f..241966d475 100644 --- a/programs/engine_testapp/mock_cookie.h +++ b/programs/engine_testapp/mock_cookie.h @@ -52,12 +52,12 @@ class MockCookie : public CookieIface { return references; } - void incrementRefcount() override { - ++references; + uint8_t incrementRefcount() override { + return ++references; } - void decrementRefcount() override { - --references; + uint8_t decrementRefcount() override { + return --references; } void* getEngineStorage() const override { diff --git a/programs/engine_testapp/mock_server.cc b/programs/engine_testapp/mock_server.cc index 1b0b95d477..1a5986d67a 100644 --- a/programs/engine_testapp/mock_server.cc +++ b/programs/engine_testapp/mock_server.cc @@ -230,8 +230,7 @@ struct MockServerCookieApi : public ServerCookieIface { void release(const CookieIface& cookie) override { auto* c = cookie_to_mock_cookie(&cookie); - c->decrementRefcount(); - if (c->getRefcount() == 0) { + if (c->decrementRefcount() == 0) { delete c; } }