From 84c1ec46015855d7ecf76d90ab582e05b2bd9627 Mon Sep 17 00:00:00 2001
From: Thomas Goyne <tg@realm.io>
Date: Mon, 20 Mar 2023 12:41:53 -0700
Subject: [PATCH] Add locking to setting and getting the default Logger

There's no (reasonable) way to externally guarantee that calls to
Logger:get_default_logger() and Logger::set_default_logger() won't race, so
they need internal locking.
---
 CHANGELOG.md                        |  2 +-
 src/realm/object-store/sync/app.cpp |  5 ++---
 src/realm/object-store/sync/app.hpp |  2 +-
 src/realm/util/logger.cpp           | 13 ++++++++++---
 src/realm/util/logger.hpp           |  4 +---
 5 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 3e1ca790838..86e5266ec80 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -6,7 +6,7 @@
 
 ### Fixed
 * <How do the end-user experience this issue? what was the impact?> ([#????](https://github.com/realm/realm-core/issues/????), since v?.?.?)
-* None.
+* `Logger::set_default_logger()` did not perform any locking, resulting in data races if it was called while the default logger was being read on another thread ([PR #6398](https://github.com/realm/realm-core/pull/6398), since v13.7.0).
 
 ### Breaking changes
 * None.
diff --git a/src/realm/object-store/sync/app.cpp b/src/realm/object-store/sync/app.cpp
index 0ec3dd7d3fc..302ea713490 100644
--- a/src/realm/object-store/sync/app.cpp
+++ b/src/realm/object-store/sync/app.cpp
@@ -269,7 +269,7 @@ void App::configure(const SyncClientConfig& sync_client_config)
     }
 }
 
-inline bool App::init_logger()
+bool App::init_logger()
 {
     if (!m_logger_ptr) {
         m_logger_ptr = util::Logger::get_default_logger();
@@ -279,8 +279,7 @@ inline bool App::init_logger()
 
 bool App::would_log(util::Logger::Level level)
 {
-    init_logger();
-    return m_logger_ptr && m_logger_ptr->would_log(level);
+    return init_logger() && m_logger_ptr->would_log(level);
 }
 
 template <class... Params>
diff --git a/src/realm/object-store/sync/app.hpp b/src/realm/object-store/sync/app.hpp
index 194e2eb9a09..199e3787bfb 100644
--- a/src/realm/object-store/sync/app.hpp
+++ b/src/realm/object-store/sync/app.hpp
@@ -390,7 +390,7 @@ class App : public std::enable_shared_from_this<App>,
     std::shared_ptr<SyncManager> m_sync_manager;
     std::shared_ptr<util::Logger> m_logger_ptr;
 
-    /// m_Logger_ptr is not set until the first call to one of these functions.
+    /// m_logger_ptr is not set until the first call to one of these functions.
     /// If configure() not been called, a logger will not be available yet.
     /// @returns true if the logger was set, otherwise false.
     bool init_logger();
diff --git a/src/realm/util/logger.cpp b/src/realm/util/logger.cpp
index 5e0f1b9af26..174a130475d 100644
--- a/src/realm/util/logger.cpp
+++ b/src/realm/util/logger.cpp
@@ -19,19 +19,25 @@
 #include <realm/util/logger.hpp>
 
 #include <iostream>
+#include <mutex>
 
 namespace realm::util {
 
-std::shared_ptr<util::Logger> Logger::s_default_logger;
-Logger::Level Logger::s_default_level = Level::info;
+namespace {
+auto& s_logger_mutex = *new std::mutex;
+std::shared_ptr<util::Logger> s_default_logger;
+std::atomic<Logger::Level> s_default_level = Logger::Level::info;
+} // anonymous namespace
 
 void Logger::set_default_logger(std::shared_ptr<util::Logger> logger) noexcept
 {
+    std::lock_guard lock(s_logger_mutex);
     s_default_logger = logger;
 }
 
 std::shared_ptr<util::Logger>& Logger::get_default_logger() noexcept
 {
+    std::lock_guard lock(s_logger_mutex);
     if (!s_default_logger) {
         s_default_logger = std::make_shared<StderrLogger>();
         s_default_logger->set_level_threshold(s_default_level);
@@ -42,6 +48,7 @@ std::shared_ptr<util::Logger>& Logger::get_default_logger() noexcept
 
 void Logger::set_default_level_threshold(Level level) noexcept
 {
+    std::lock_guard lock(s_logger_mutex);
     s_default_level = level;
     if (s_default_logger)
         s_default_logger->set_level_threshold(level);
@@ -49,7 +56,7 @@ void Logger::set_default_level_threshold(Level level) noexcept
 
 Logger::Level Logger::get_default_level_threshold() noexcept
 {
-    return s_default_level;
+    return s_default_level.load(std::memory_order_relaxed);
 }
 
 const char* Logger::get_level_prefix(Level level) noexcept
diff --git a/src/realm/util/logger.hpp b/src/realm/util/logger.hpp
index 54e348c4a26..ee5e2040ffd 100644
--- a/src/realm/util/logger.hpp
+++ b/src/realm/util/logger.hpp
@@ -117,7 +117,7 @@ class Logger {
 
     Logger() noexcept
         : m_level_threshold{m_threshold_base}
-        , m_threshold_base{Logger::s_default_level}
+        , m_threshold_base{get_default_level_threshold()}
     {
     }
 
@@ -140,8 +140,6 @@ class Logger {
     static const char* get_level_prefix(Level) noexcept;
 
 private:
-    static std::shared_ptr<util::Logger> s_default_logger;
-    static Level s_default_level;
     // Only used by the base Logger class
     std::atomic<Level> m_threshold_base;