Skip to content

Commit

Permalink
MB-35993: Add synchronization to auditHandle
Browse files Browse the repository at this point in the history
The audit daemon handle (auditHandle) does not have any
synchronization around it, resulting in TSan warnings during shutdown
as the Audit object may have been deleted:

ThreadSanitizer: data race

  Write of size 8 at 0x0000009b0130 by main thread:
    #0 std::swap(cb::audit::Audit*&, cb::audit::Audit*&) /usr/local/include/c++/7.3.0/bits/move.h:199 (memcached+0x0000004f335a)
    #1 std::unique_ptr >::reset(cb::audit::Audit*) /usr/local/include/c++/7.3.0/bits/unique_ptr.h:374 (memcached+0x0000004f335a)
    #2 shutdown_audit() kv_engine/daemon/mcaudit.cc:348 (memcached+0x0000004f335a)
    #3 memcached_main kv_engine/daemon/memcached.cc:2503 (memcached+0x000000435a46)
    #4 main kv_engine/daemon/main.cc:33 (memcached+0x00000042145e)

  Previous read of size 8 at 0x0000009b0130 by thread T8 (mutexes: write M1049192993527234104):
    #0 std::__uniq_ptr_impl >::_M_ptr() const /usr/local/include/c++/7.3.0/bits/unique_ptr.h:147 (memcached+0x0000004f3433)
    #1 std::unique_ptr >::get() const /usr/local/include/c++/7.3.0/bits/unique_ptr.h:337 (memcached+0x0000004f3433)
    #2 std::unique_ptr >::operator->() const /usr/local/include/c++/7.3.0/bits/unique_ptr.h:331 (memcached+0x0000004f3433)
    #3 stats_audit(std::function)> const&, Cookie&) kv_engine/daemon/mcaudit.cc:361 (memcached+0x0000004f3433)
    #4 stat_audit_executor kv_engine/daemon/protocol/mcbp/stats_context.cc:446 (memcached+0x000000528f72)

  Location is global 'auditHandle' of size 8 at 0x0000009b0130 (memcached+0x0000009b0130)

Fix by using folly::Synchronized<> for the auditHandle. Note that
exclusive access is only needed during initialization & shutdown, so
there should be no additional contention for actually adding audit
events (where shared access is sufficient).

Change-Id: I34966f08674c6363fde4592b5c4bede48747fb2f
Reviewed-on: http://review.couchbase.org/114945
Tested-by: Build Bot <[email protected]>
Reviewed-by: Richard de Mellow <[email protected]>
Reviewed-by: Trond Norbye <[email protected]>
  • Loading branch information
daverigby committed Sep 18, 2019
1 parent 3c221fd commit a9a4c3e
Showing 1 changed file with 36 additions and 17 deletions.
53 changes: 36 additions & 17 deletions daemon/mcaudit.cc
Original file line number Diff line number Diff line change
Expand Up @@ -32,11 +32,12 @@
#include <memcached/isotime.h>
#include <platform/string_hex.h>

#include <folly/Synchronized.h>
#include <nlohmann/json.hpp>

#include <sstream>

static cb::audit::UniqueAuditPtr auditHandle;
static folly::Synchronized<cb::audit::UniqueAuditPtr> auditHandle;

static std::atomic_bool audit_enabled{false};

Expand Down Expand Up @@ -113,9 +114,13 @@ static void do_audit(uint32_t id,
const nlohmann::json& event,
const char* warn) {
auto text = event.dump();
if (!auditHandle->put_event(id, text)) {
LOG_WARNING("{}: {}", warn, text);
}
auditHandle.withRLock([id, warn, &text](auto& handle) {
if (handle) {
if (!handle->put_event(id, text)) {
LOG_WARNING("{}: {}", warn, text);
}
}
});
}

void audit_auth_failure(const Connection& c, const char* reason) {
Expand Down Expand Up @@ -263,7 +268,12 @@ bool mc_audit_event(uint32_t audit_eventid, cb::const_byte_buffer payload) {

cb::const_char_buffer buffer{reinterpret_cast<const char*>(payload.data()),
payload.size()};
return auditHandle->put_event(audit_eventid, buffer);
return auditHandle.withRLock([audit_eventid, buffer](auto& handle) {
if (!handle) {
return false;
}
return handle->put_event(audit_eventid, buffer);
});
}

namespace cb {
Expand Down Expand Up @@ -335,28 +345,37 @@ static void event_state_listener(uint32_t id, bool enabled) {

void initialize_audit() {
/* Start the audit daemon */
auditHandle = cb::audit::create_audit_daemon(
auto audit = cb::audit::create_audit_daemon(
Settings::instance().getAuditFile(), get_server_api()->cookie);
if (!auditHandle) {
if (!audit) {
FATAL_ERROR(EXIT_FAILURE, "FATAL: Failed to start audit daemon");
}
auditHandle->add_event_state_listener(event_state_listener);
auditHandle->notify_all_event_states();
audit->add_event_state_listener(event_state_listener);
audit->notify_all_event_states();
*auditHandle.wlock() = std::move(audit);
}

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

ENGINE_ERROR_CODE reconfigure_audit(Cookie& cookie) {
if (auditHandle->configure_auditdaemon(Settings::instance().getAuditFile(),
static_cast<void*>(&cookie))) {
return ENGINE_EWOULDBLOCK;
}

return ENGINE_FAILED;
return auditHandle.withRLock([&cookie](auto& handle) {
if (!handle) {
return ENGINE_FAILED;
}
if (handle->configure_auditdaemon(Settings::instance().getAuditFile(),
static_cast<void*>(&cookie))) {
return ENGINE_EWOULDBLOCK;
}
return ENGINE_FAILED;
});
}

void stats_audit(const AddStatFn& add_stats, Cookie& cookie) {
auditHandle->stats(add_stats, &cookie);
auditHandle.withRLock([&add_stats, &cookie](auto& handle) {
if (handle) {
handle->stats(add_stats, &cookie);
}
});
}

0 comments on commit a9a4c3e

Please sign in to comment.