Skip to content

Commit

Permalink
Settings: Avoid static initialization fiasco
Browse files Browse the repository at this point in the history 
When building on macOS with clang 8 and ThreadSanitizer enabled,
memcached_testapp crashes when destructing the Settings object:

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

With the following backtrace from where the system_error exception is thrown:

* thread couchbase#1, queue = 'com.apple.main-thread', stop reason = breakpoint 1.1
    frame #0: 0x00007fff7b6411f6 libc++abi.dylib` __cxa_throw
    frame couchbase#1: 0x00007fff7b6147af libc++.1.dylib` std::__1::__throw_system_error(int, char const*)  + 77
    frame couchbase#2: 0x00007fff7b606c93 libc++.1.dylib` std::__1::mutex::lock()  + 29
    frame couchbase#3: 0x00000001009c27ff memcached_testapp` folly::SharedMutexImpl<...>::annotateLazyCreate() [inlined] std::__1::unique_lock<std::__1::mutex>::unique_lock(__m=<unavailable>)  + 191 at __mutex_base:131
    frame couchbase#4: 0x00000001009c27f7 memcached_testapp` folly::SharedMutexImpl<...>::annotateLazyCreate() [inlined] std::__1::unique_lock<std::__1::mutex>::unique_lock(__m=<unavailable>)  at __mutex_base:131
  * frame couchbase#5: 0x00000001009c27f7 memcached_testapp` folly::SharedMutexImpl<...>::annotateLazyCreate()  + 140 at SharedMutex.cpp:33
    frame couchbase#6: 0x00000001009c276b memcached_testapp` folly::SharedMutexImpl<...>::annotateLazyCreate(this=0x0000000100c7ddc8)  + 43 at SharedMutex.h:705
    frame #7: 0x00000001009c191a memcached_testapp` folly::SharedMutexImpl<...>::~SharedMutexImpl() [inlined] folly::SharedMutexImpl<...>::annotateDestroy(this=<unavailable>)  + 8 at SharedMutex.h:718
    frame #8: 0x00000001009c1912 memcached_testapp` folly::SharedMutexImpl<...>::~SharedMutexImpl()  + 24 at SharedMutex.h:324
    frame #9: 0x00000001009c18fa memcached_testapp` folly::SharedMutexImpl<...>::~SharedMutexImpl(this=0x0000000100c7ddc8)  + 26 at SharedMutex.h:300
    frame #10: 0x000000010076b837 memcached_testapp` folly::Synchronized<...>::~Synchronized(this=0x0000000100c7ddb0)  + 55 at Synchronized.h:484
    frame #11: 0x000000010075d1d9 memcached_testapp` folly::Synchronized<...>::~Synchronized(this=0x0000000100c7ddb0)  + 41 at Synchronized.h:484
    frame #12: 0x000000010075e8fc memcached_testapp` Settings::~Settings(this=0x0000000100c7db10)  + 76 at settings.h:51
    frame #13: 0x000000010075c8b9 memcached_testapp` Settings::~Settings(this=0x0000000100c7db10)  + 41 at settings.h:51
    frame #14: 0x0000000102b7d5c1 libclang_rt.tsan_osx_dynamic.dylib` cxa_at_exit_wrapper(void*)  + 33
    frame #15: 0x00007fff7d72beed libsystem_c.dylib` __cxa_finalize_ranges  + 351
    frame #16: 0x00007fff7d72c1fe libsystem_c.dylib` exit  + 55
    frame #17: 0x00007fff7d67f01c libdyld.dylib` start  + 8

The problem is at frame 5, where as part of the destruction of
SharedMutex (as used by a member variable of Settings) we are
attempting to acquire a mutex which has already been destructed (as it
is itself a function-local static) - i.e. we have encountered the
static initialization order fiasco :(

Address this by changing `settings` to no longer be a plain static
(global) variable, and instead be created on first use via a new
Settings::instance() static method.

Change-Id: I40bd44ed0ae32eb55271ce739fdf990d9869c32f
Reviewed-on: http://review.couchbase.org/113640
Tested-by: Build Bot <[email protected]>
Reviewed-by: Jim Walker <[email protected]>
  • Loading branch information
@daverigby
daverigby committed Aug 22, 2019
1 parent ac8385e commit 1974839
Show file tree
Hide file tree
Showing 27 changed files with 208 additions and 172 deletions.
10 changes: 5 additions & 5 deletions daemon/cmdline.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -79,19 +79,19 @@ void parse_arguments(int argc, char** argv) {
std::exit(EXIT_FAILURE);
}

load_config_file(config_file.c_str(), settings);
load_config_file(config_file.c_str(), Settings::instance());

if (verbosity > 0) {
settings.setVerbose(verbosity);
Settings::instance().setVerbose(verbosity);
}

if (threads > 0) {
settings.setNumWorkerThreads(threads);
Settings::instance().setNumWorkerThreads(threads);
}

if (requests > 0) {
settings.setRequestsPerEventNotification(requests,
EventPriority::Default);
Settings::instance().setRequestsPerEventNotification(
requests, EventPriority::Default);
}
}

Expand Down
4 changes: 2 additions & 2 deletions daemon/config_parse.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ boost::optional<nlohmann::json> validate_proposed_config_changes(
try {
auto json = nlohmann::json::parse(new_cfg);
Settings new_settings(json);
settings.updateSettings(new_settings, false);
Settings::instance().updateSettings(new_settings, false);
return {};
} catch (const std::exception& exception) {
errors.push_back(exception.what());
Expand All @@ -58,7 +58,7 @@ void reload_config_file() {
try {
auto content = cb::io::loadFile(get_config_file());
Settings new_settings(nlohmann::json::parse(content));
settings.updateSettings(new_settings, true);
Settings::instance().updateSettings(new_settings, true);
} catch (const std::exception& exception) {
LOG_WARNING(
"Validation failed while reloading config file '{}'. Error: {}",
Expand Down
29 changes: 17 additions & 12 deletions daemon/connection.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -357,7 +357,7 @@ cb::rbac::PrivilegeAccess Connection::checkPrivilege(
const std::string privilege_string = cb::rbac::to_string(privilege);
const std::string context = privilegeContext.to_string();

if (settings.isPrivilegeDebug()) {
if (Settings::instance().isPrivilegeDebug()) {
audit_privilege_debug(*this,
command,
all_buckets[bucketIndex].name,
Expand Down Expand Up @@ -743,7 +743,8 @@ int Connection::sslPreConnection() {
disconnect = true;
break;
case cb::x509::Status::NotPresent:
if (settings.getClientCertMode() == cb::x509::Mode::Mandatory) {
if (Settings::instance().getClientCertMode() ==
cb::x509::Mode::Mandatory) {
disconnect = true;
} else if (is_default_bucket_enabled()) {
associate_bucket(*this, "default");
Expand Down Expand Up @@ -1122,7 +1123,7 @@ int Connection::sslRead(char* dest, size_t nbytes) {
int Connection::sslWrite(const char* src, size_t nbytes) {
int ret = 0;

int chunksize = settings.getBioDrainBufferSize();
int chunksize = Settings::instance().getBioDrainBufferSize();

while (ret < int(nbytes)) {
int n;
Expand Down Expand Up @@ -1249,7 +1250,7 @@ void Connection::ensureIovSpace() {

bool Connection::enableSSL(const std::string& cert, const std::string& pkey) {
if (ssl.enable(cert, pkey)) {
if (settings.getVerbose() > 1) {
if (Settings::instance().getVerbose() > 1) {
ssl.dumpCipherList(getId());
}

Expand All @@ -1267,7 +1268,7 @@ Connection::Connection(FrontEndThread& thr)
peername("unknown"),
sockname("unknown"),
stateMachine(*this),
max_reqs_per_event(settings.getRequestsPerEventNotification(
max_reqs_per_event(Settings::instance().getRequestsPerEventNotification(
EventPriority::Default)) {
updateDescription();
cookies.emplace_back(std::unique_ptr<Cookie>{new Cookie(*this)});
Expand All @@ -1286,7 +1287,7 @@ Connection::Connection(SOCKET sfd,
peername(cb::net::getpeername(socketDescriptor)),
sockname(cb::net::getsockname(socketDescriptor)),
stateMachine(*this),
max_reqs_per_event(settings.getRequestsPerEventNotification(
max_reqs_per_event(Settings::instance().getRequestsPerEventNotification(
EventPriority::Default)) {
setTcpNoDelay(true);
updateDescription();
Expand Down Expand Up @@ -1330,7 +1331,7 @@ void Connection::setState(StateMachine::State next_state) {
}

void Connection::runStateMachinery() {
if (settings.getVerbose() > 1) {
if (Settings::instance().getVerbose() > 1) {
do {
LOG_DEBUG("{} - Running task: {}",
getId(),
Expand Down Expand Up @@ -1557,15 +1558,18 @@ void Connection::setPriority(Connection::Priority priority) {
switch (priority) {
case Priority::High:
max_reqs_per_event =
settings.getRequestsPerEventNotification(EventPriority::High);
Settings::instance().getRequestsPerEventNotification(
EventPriority::High);
return;
case Priority::Medium:
max_reqs_per_event =
settings.getRequestsPerEventNotification(EventPriority::Medium);
Settings::instance().getRequestsPerEventNotification(
EventPriority::Medium);
return;
case Priority::Low:
max_reqs_per_event =
settings.getRequestsPerEventNotification(EventPriority::Low);
Settings::instance().getRequestsPerEventNotification(
EventPriority::Low);
return;
}
throw std::invalid_argument("Unkown priority: " +
Expand All @@ -1575,9 +1579,10 @@ void Connection::setPriority(Connection::Priority priority) {
bool Connection::selectedBucketIsXattrEnabled() const {
auto* bucketEngine = getBucketEngine();
if (bucketEngine) {
return settings.isXattrEnabled() && bucketEngine->isXattrEnabled();
return Settings::instance().isXattrEnabled() &&
bucketEngine->isXattrEnabled();
}
return settings.isXattrEnabled();
return Settings::instance().isXattrEnabled();
}

ENGINE_ERROR_CODE Connection::add_packet_to_send_pipe(
Expand Down
2 changes: 1 addition & 1 deletion daemon/connections.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,7 @@ Connection* conn_new(SOCKET sfd,
bucket.name);
}

if (settings.getVerbose() > 1) {
if (Settings::instance().getVerbose() > 1) {
LOG_DEBUG("<{} new client connection", sfd);
}

Expand Down
6 changes: 3 additions & 3 deletions daemon/cookie.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -251,7 +251,7 @@ void Cookie::sendDynamicBuffer() {
void Cookie::sendNotMyVBucket() {
auto pair = connection.getBucket().clusterConfiguration.getConfiguration();
if (pair.first == -1 || (pair.first == connection.getClustermapRevno() &&
settings.isDedupeNmvbMaps())) {
Settings::instance().isDedupeNmvbMaps())) {
// We don't have a vbucket map, or we've already sent it to the
// client
mcbp_add_header(*this,
Expand Down Expand Up @@ -414,7 +414,7 @@ std::string Cookie::getPrintableRequestKey() const {
}

void Cookie::logCommand() const {
if (settings.getVerbose() == 0) {
if (Settings::instance().getVerbose() == 0) {
// Info is not enabled.. we don't want to try to format
// output
return;
Expand All @@ -437,7 +437,7 @@ void Cookie::logResponse(const char* reason) const {
}

void Cookie::logResponse(ENGINE_ERROR_CODE code) const {
if (settings.getVerbose() == 0) {
if (Settings::instance().getVerbose() == 0) {
// Info is not enabled.. we don't want to try to format
// output
return;
Expand Down
6 changes: 3 additions & 3 deletions daemon/datatype.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,11 @@
bool Datatype::isSupported(cb::mcbp::Feature feature) {
switch (feature) {
case cb::mcbp::Feature::XATTR:
return settings.isXattrEnabled();
return Settings::instance().isXattrEnabled();
case cb::mcbp::Feature::JSON:
return settings.isDatatypeJsonEnabled();
return Settings::instance().isDatatypeJsonEnabled();
case cb::mcbp::Feature::SNAPPY:
return settings.isDatatypeSnappyEnabled();
return Settings::instance().isDatatypeSnappyEnabled();
case cb::mcbp::Feature::TLS:
case cb::mcbp::Feature::TCPNODELAY:
case cb::mcbp::Feature::MUTATION_SEQNO:
Expand Down
6 changes: 3 additions & 3 deletions daemon/mcaudit.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -335,8 +335,8 @@ static void event_state_listener(uint32_t id, bool enabled) {

void initialize_audit() {
/* Start the audit daemon */
auditHandle = cb::audit::create_audit_daemon(settings.getAuditFile(),
get_server_api()->cookie);
auditHandle = cb::audit::create_audit_daemon(
Settings::instance().getAuditFile(), get_server_api()->cookie);
if (!auditHandle) {
FATAL_ERROR(EXIT_FAILURE, "FATAL: Failed to start audit daemon");
}
Expand All @@ -349,7 +349,7 @@ void shutdown_audit() {
}

ENGINE_ERROR_CODE reconfigure_audit(Cookie& cookie) {
if (auditHandle->configure_auditdaemon(settings.getAuditFile(),
if (auditHandle->configure_auditdaemon(Settings::instance().getAuditFile(),
static_cast<void*>(&cookie))) {
return ENGINE_EWOULDBLOCK;
}
Expand Down
2 changes: 1 addition & 1 deletion daemon/mcbp.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ void mcbp_add_header(Cookie& cookie,
header.getOpaque(),
cookie.getCas());

if (settings.getVerbose() > 1) {
if (Settings::instance().getVerbose() > 1) {
auto* header = reinterpret_cast<const cb::mcbp::Header*>(wbuf.data());
try {
LOG_TRACE("<{} Sending: {}",
Expand Down
30 changes: 16 additions & 14 deletions daemon/mcbp_executors.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,7 @@ static void verbosity_executor(Cookie& cookie) {
if (level < 0 || level > MAX_VERBOSITY_LEVEL) {
level = MAX_VERBOSITY_LEVEL;
}
settings.setVerbose(level);
Settings::instance().setVerbose(level);
cookie.sendResponse(cb::mcbp::Status::Success);
}

Expand Down Expand Up @@ -254,7 +254,7 @@ static void sasl_list_mech_executor(Cookie& cookie) {
return;
}

if (settings.isExternalAuthServiceEnabled()) {
if (Settings::instance().isExternalAuthServiceEnabled()) {
// If the system is configured to use ns_server for authentication
// we should only advertise PLAIN as the mechanism (as it can't use
// SCRAM to connect to LDAP)
Expand All @@ -267,16 +267,18 @@ static void sasl_list_mech_executor(Cookie& cookie) {
return;
}

if (connection.isSslEnabled() && settings.has.ssl_sasl_mechanisms) {
const auto& mechs = settings.getSslSaslMechanisms();
if (connection.isSslEnabled() &&
Settings::instance().has.ssl_sasl_mechanisms) {
const auto& mechs = Settings::instance().getSslSaslMechanisms();
cookie.sendResponse(cb::mcbp::Status::Success,
{},
{},
mechs,
cb::mcbp::Datatype::Raw,
0);
} else if (!connection.isSslEnabled() && settings.has.sasl_mechanisms) {
const auto& mechs = settings.getSaslMechanisms();
} else if (!connection.isSslEnabled() &&
Settings::instance().has.sasl_mechanisms) {
const auto& mechs = Settings::instance().getSaslMechanisms();
cookie.sendResponse(cb::mcbp::Status::Success,
{},
{},
Expand Down Expand Up @@ -453,11 +455,11 @@ static void config_reload_executor(Cookie& cookie) {
// We need to audit that the privilege debug mode changed and
// in order to do that we need the "connection" object so we can't
// do this by using the common "changed_listener"-interface.
const bool old_priv_debug = settings.isPrivilegeDebug();
const bool old_priv_debug = Settings::instance().isPrivilegeDebug();
reload_config_file();
if (settings.isPrivilegeDebug() != old_priv_debug) {
if (Settings::instance().isPrivilegeDebug() != old_priv_debug) {
audit_set_privilege_debug_mode(cookie.getConnection(),
settings.isPrivilegeDebug());
Settings::instance().isPrivilegeDebug());
}
cookie.sendResponse(cb::mcbp::Status::Success);
}
Expand Down Expand Up @@ -487,7 +489,7 @@ static void get_errmap_executor(Cookie& cookie) {
auto value = cookie.getRequest(Cookie::PacketContent::Full).getValue();
auto* req = reinterpret_cast<const cb::mcbp::request::GetErrmapPayload*>(
value.data());
auto const& errormap = settings.getErrorMap(req->getVersion());
auto const& errormap = Settings::instance().getErrorMap(req->getVersion());
if (errormap.empty()) {
cookie.sendResponse(cb::mcbp::Status::KeyEnoent);
} else {
Expand Down Expand Up @@ -543,7 +545,7 @@ static void rbac_refresh_executor(Cookie& cookie) {
}

static void auth_provider_executor(Cookie& cookie) {
if (!settings.isExternalAuthServiceEnabled()) {
if (!Settings::instance().isExternalAuthServiceEnabled()) {
cookie.setErrorContext(
"Support for external authentication service is disabled");
cookie.sendResponse(cb::mcbp::Status::NotSupported);
Expand Down Expand Up @@ -982,7 +984,7 @@ void try_read_mcbp_command(Cookie& cookie) {
return;
}

if (settings.getVerbose() > 1) {
if (Settings::instance().getVerbose() > 1) {
try {
LOG_TRACE(">{} Read command {}",
c.getId(),
Expand All @@ -997,13 +999,13 @@ void try_read_mcbp_command(Cookie& cookie) {

// Protect ourself from someone trying to kill us by sending insanely
// large packets.
if (header.getBodylen() > settings.getMaxPacketSize()) {
if (header.getBodylen() > Settings::instance().getMaxPacketSize()) {
LOG_WARNING(
"{}: The package size ({}) exceeds the limit ({}) for what "
"the system accepts.. Disconnecting client",
c.getId(),
header.getBodylen(),
settings.getMaxPacketSize());
Settings::instance().getMaxPacketSize());
c.setState(StateMachine::State::closing);
return;
}
Expand Down
Loading

0 comments on commit 1974839

Please sign in to comment.