Skip to content

Commit a9a4c3e

Browse files
committed
MB-35993: Add synchronization to auditHandle
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) couchbase#1 std::unique_ptr >::reset(cb::audit::Audit*) /usr/local/include/c++/7.3.0/bits/unique_ptr.h:374 (memcached+0x0000004f335a) couchbase#2 shutdown_audit() kv_engine/daemon/mcaudit.cc:348 (memcached+0x0000004f335a) couchbase#3 memcached_main kv_engine/daemon/memcached.cc:2503 (memcached+0x000000435a46) couchbase#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) couchbase#1 std::unique_ptr >::get() const /usr/local/include/c++/7.3.0/bits/unique_ptr.h:337 (memcached+0x0000004f3433) couchbase#2 std::unique_ptr >::operator->() const /usr/local/include/c++/7.3.0/bits/unique_ptr.h:331 (memcached+0x0000004f3433) couchbase#3 stats_audit(std::function)> const&, Cookie&) kv_engine/daemon/mcaudit.cc:361 (memcached+0x0000004f3433) couchbase#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 <build@couchbase.com> Reviewed-by: Richard de Mellow <richard.demellow@couchbase.com> Reviewed-by: Trond Norbye <trond.norbye@couchbase.com>
1 parent 3c221fd commit a9a4c3e

File tree

1 file changed

+36
-17
lines changed

1 file changed

+36
-17
lines changed

daemon/mcaudit.cc

+36-17
Original file line numberDiff line numberDiff line change
@@ -32,11 +32,12 @@
3232
#include <memcached/isotime.h>
3333
#include <platform/string_hex.h>
3434

35+
#include <folly/Synchronized.h>
3536
#include <nlohmann/json.hpp>
3637

3738
#include <sstream>
3839

39-
static cb::audit::UniqueAuditPtr auditHandle;
40+
static folly::Synchronized<cb::audit::UniqueAuditPtr> auditHandle;
4041

4142
static std::atomic_bool audit_enabled{false};
4243

@@ -113,9 +114,13 @@ static void do_audit(uint32_t id,
113114
const nlohmann::json& event,
114115
const char* warn) {
115116
auto text = event.dump();
116-
if (!auditHandle->put_event(id, text)) {
117-
LOG_WARNING("{}: {}", warn, text);
118-
}
117+
auditHandle.withRLock([id, warn, &text](auto& handle) {
118+
if (handle) {
119+
if (!handle->put_event(id, text)) {
120+
LOG_WARNING("{}: {}", warn, text);
121+
}
122+
}
123+
});
119124
}
120125

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

264269
cb::const_char_buffer buffer{reinterpret_cast<const char*>(payload.data()),
265270
payload.size()};
266-
return auditHandle->put_event(audit_eventid, buffer);
271+
return auditHandle.withRLock([audit_eventid, buffer](auto& handle) {
272+
if (!handle) {
273+
return false;
274+
}
275+
return handle->put_event(audit_eventid, buffer);
276+
});
267277
}
268278

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

336346
void initialize_audit() {
337347
/* Start the audit daemon */
338-
auditHandle = cb::audit::create_audit_daemon(
348+
auto audit = cb::audit::create_audit_daemon(
339349
Settings::instance().getAuditFile(), get_server_api()->cookie);
340-
if (!auditHandle) {
350+
if (!audit) {
341351
FATAL_ERROR(EXIT_FAILURE, "FATAL: Failed to start audit daemon");
342352
}
343-
auditHandle->add_event_state_listener(event_state_listener);
344-
auditHandle->notify_all_event_states();
353+
audit->add_event_state_listener(event_state_listener);
354+
audit->notify_all_event_states();
355+
*auditHandle.wlock() = std::move(audit);
345356
}
346357

347358
void shutdown_audit() {
348-
auditHandle.reset();
359+
auditHandle.wlock()->reset();
349360
}
350361

351362
ENGINE_ERROR_CODE reconfigure_audit(Cookie& cookie) {
352-
if (auditHandle->configure_auditdaemon(Settings::instance().getAuditFile(),
353-
static_cast<void*>(&cookie))) {
354-
return ENGINE_EWOULDBLOCK;
355-
}
356-
357-
return ENGINE_FAILED;
363+
return auditHandle.withRLock([&cookie](auto& handle) {
364+
if (!handle) {
365+
return ENGINE_FAILED;
366+
}
367+
if (handle->configure_auditdaemon(Settings::instance().getAuditFile(),
368+
static_cast<void*>(&cookie))) {
369+
return ENGINE_EWOULDBLOCK;
370+
}
371+
return ENGINE_FAILED;
372+
});
358373
}
359374

360375
void stats_audit(const AddStatFn& add_stats, Cookie& cookie) {
361-
auditHandle->stats(add_stats, &cookie);
376+
auditHandle.withRLock([&add_stats, &cookie](auto& handle) {
377+
if (handle) {
378+
handle->stats(add_stats, &cookie);
379+
}
380+
});
362381
}

0 commit comments

Comments
 (0)