Skip to content

Commit

Permalink
Fix static initialization order fiasco in audit / connections
Browse files Browse the repository at this point in the history
When running memcached_testapp under TSan (macOS), an exception is
thrown attempting to lock an invalid mutex:

    libc++abi.dylib: terminating with uncaught exception of type std::__1::system_error: mutex lock failed: Invalid argument

With the following backtrace:

    (lldb) bt
    * thread #1, queue = 'com.apple.main-thread', stop reason = breakpoint 1.1
      * frame #0: 0x00007fff6585e0f8 libc++abi.dylib` __cxa_throw
        frame #1: 0x00007fff6583855b libc++.1.dylib` std::__1::__throw_system_error(int, char const*)  + 77
        frame #2: 0x00007fff6582f54d libc++.1.dylib` std::__1::mutex::lock()  + 29
        frame #3: 0x00000001014139f1 memcached_testapp` std::__1::unique_lock<std::__1::mutex>::unique_lock(this=0x00007ffeefbff3e8, __m=<unavailable>)  + 65 at __mutex_base:130
        frame #4: 0x0000000101410811 memcached_testapp` std::__1::unique_lock<std::__1::mutex>::unique_lock(this=<unavailable>, __m=<unavailable>)  + 33 at __mutex_base:130
        frame #5: 0x000000010141053e memcached_testapp` folly::shared_mutex_detail::annotationGuard(ptr=<unavailable>)  + 110 at SharedMutex.cpp:33
        frame #6: 0x0000000101410445 memcached_testapp` folly::SharedMutexImpl<...>::annotateLazyCreate(this=0x0000000101984640)  + 53 at SharedMutex.h:719
        frame #7: 0x000000010140f6ca memcached_testapp` folly::SharedMutexImpl<...>::annotateDestroy(this=0x0000000101984640)  + 26 at SharedMutex.h:732
        frame #8: 0x000000010140f5af memcached_testapp` folly::SharedMutexImpl<...>::~SharedMutexImpl(this=0x0000000101984640)  + 63 at SharedMutex.h:338
        frame #9: 0x000000010140f71a memcached_testapp` folly::SharedMutexImpl<...>::~SharedMutexImpl(this=<unavailable>)  + 26 at SharedMutex.h:312
        frame #10: 0x0000000100d5549a memcached_testapp` folly::Synchronized<std::__1::unique_ptr<cb::audit::Audit, ...>, folly::SharedMutexImpl<...> >::~Synchronized(this=0x0000000101984638)  + 58 at Synchronized.h:489
        frame #11: 0x0000000100d511a9 memcached_testapp` folly::Synchronized<std::__1::unique_ptr<cb::audit::Audit, ...>, folly::SharedMutexImpl<...> >::~Synchronized(this=0x0000000101984638)  + 41 at Synchronized.h:489
        frame #12: 0x000000010ad4c721 libclang_rt.tsan_osx_dynamic.dylib` cxa_at_exit_wrapper(void*)  + 33
        frame #13: 0x00007fff685d813c libsystem_c.dylib` __cxa_finalize_ranges  + 319
        frame #14: 0x00007fff685d8412 libsystem_c.dylib` exit  + 55
        frame #15: 0x00007fff6852ecd0 libdyld.dylib` start  + 8

The crash is caused when destructing the static variable
`auditHandle`, the Synchronized dtor attempts to access another static
variable (kAnnotationMutexes) which has already been destructed.

The problem is that auditHandle is a static at file scope, so the
order it it initiailised (and destructed) relative to other statics is
undefined.

Switch to using a C++11 "magic static" via a getAuditHandle()
function, which defers construction until the type is used.

The same problem exists for `connections`, fix in the same way.

Change-Id: I58c925f2c71907ab22581bf462ce3c7d092cfefa
Reviewed-on: http://review.couchbase.org/c/kv_engine/+/138051
Tested-by: Build Bot <[email protected]>
Reviewed-by: Trond Norbye <[email protected]>
  • Loading branch information
daverigby authored and trondn committed Oct 16, 2020
1 parent a235230 commit aa9f7db
Show file tree
Hide file tree
Showing 2 changed files with 18 additions and 11 deletions.
11 changes: 7 additions & 4 deletions daemon/connections.cc
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,10 @@

/// List of all of the connections created in the system (so that we can
/// iterate over them)
folly::Synchronized<std::deque<Connection*>> connections;
folly::Synchronized<std::deque<Connection*>>& getConnections() {
static folly::Synchronized<std::deque<Connection*>> connections;
return connections;
}

int signal_idle_clients(FrontEndThread& me, bool dumpConnection) {
int connected = 0;
Expand All @@ -50,7 +53,7 @@ void iterate_thread_connections(FrontEndThread* thread,
std::function<void(Connection&)> callback) {
// Deny modifications to the connection map while we're iterating
// over it
auto locked = connections.rlock();
auto locked = getConnections().rlock();
for (auto* c : *locked) {
if (&c->getThread() == thread) {
callback(*c);
Expand All @@ -67,7 +70,7 @@ Connection* conn_new(SOCKET sfd,

try {
ret = std::make_unique<Connection>(sfd, base, interface, thread);
connections.wlock()->push_back(ret.get());
getConnections().wlock()->push_back(ret.get());
c = ret.release();
stats.total_conns++;
} catch (const std::bad_alloc&) {
Expand Down Expand Up @@ -100,7 +103,7 @@ Connection* conn_new(SOCKET sfd,
* and freeing the Connection object.
*/
void conn_destroy(Connection* c) {
connections.withWLock([c](auto& conns) {
getConnections().withWLock([c](auto& conns) {
auto iter = std::find(conns.begin(), conns.end(), c);
// The connection should be one I know about
cb_assert(iter != conns.end());
Expand Down
18 changes: 11 additions & 7 deletions daemon/mcaudit.cc
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,11 @@

#include <sstream>

static folly::Synchronized<cb::audit::UniqueAuditPtr> auditHandle;
/// @returns the singleton audit handle.
folly::Synchronized<cb::audit::UniqueAuditPtr>& getAuditHandle() {
static folly::Synchronized<cb::audit::UniqueAuditPtr> handle;
return handle;
}

static std::atomic_bool audit_enabled{false};

Expand Down Expand Up @@ -117,7 +121,7 @@ static void do_audit(uint32_t id,
const nlohmann::json& event,
const char* warn) {
auto text = event.dump();
auditHandle.withRLock([id, warn, &text](auto& handle) {
getAuditHandle().withRLock([id, warn, &text](auto& handle) {
if (handle) {
if (!handle->put_event(id, text)) {
LOG_WARNING("{}: {}", warn, text);
Expand Down Expand Up @@ -272,7 +276,7 @@ bool mc_audit_event(uint32_t audit_eventid, cb::const_byte_buffer payload) {

std::string_view buffer{reinterpret_cast<const char*>(payload.data()),
payload.size()};
return auditHandle.withRLock([audit_eventid, buffer](auto& handle) {
return getAuditHandle().withRLock([audit_eventid, buffer](auto& handle) {
if (!handle) {
return false;
}
Expand Down Expand Up @@ -372,15 +376,15 @@ void initialize_audit() {
}
audit->add_event_state_listener(event_state_listener);
audit->notify_all_event_states();
*auditHandle.wlock() = std::move(audit);
*getAuditHandle().wlock() = std::move(audit);
}

void shutdown_audit() {
auditHandle.wlock()->reset();
getAuditHandle().wlock()->reset();
}

ENGINE_ERROR_CODE reconfigure_audit(Cookie& cookie) {
return auditHandle.withRLock([&cookie](auto& handle) {
return getAuditHandle().withRLock([&cookie](auto& handle) {
if (!handle) {
return ENGINE_FAILED;
}
Expand All @@ -393,7 +397,7 @@ ENGINE_ERROR_CODE reconfigure_audit(Cookie& cookie) {
}

void stats_audit(StatCollector& collector) {
auditHandle.withRLock([&collector](auto& handle) {
getAuditHandle().withRLock([&collector](auto& handle) {
if (handle) {
handle->stats(collector);
}
Expand Down

0 comments on commit aa9f7db

Please sign in to comment.