async_msg: take ownership of filename/funcname

the async logger cannot ensure that filename/funcname pointers are still valid at the time when the async worker is executed: * filename/funcname may be provided by a VM * filename/funcname may point to a readonly text field, in a shared object that had been `dlclose`d we add an api that can be enabled via `SPDLOG_ASYNC_OWNING_SOURCELOC_STRINGS` to take ownership of the strings
timblechmann · Nov 9, 2023 · 38ff02b · 38ff02b
1 parent ac55e60
commit 38ff02b
Show file tree

Hide file tree

Showing 3 changed files with 41 additions and 8 deletions.
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
@@ -18,6 +18,9 @@ jobs:
           - { compiler: gcc, version: 12, build_type: Release, cppstd: 20 }
           - { compiler: clang, version: 12, build_type: Debug, cppstd: 17, asan: OFF }
           - { compiler: clang, version: 15, build_type: Release, cppstd: 20, asan: OFF }
+        async_owning_sourceloc_strings:
+          - ON
+          - OFF
     container:
       image: ${{ matrix.config.compiler == 'clang' && 'teeks99/clang-ubuntu' || matrix.config.compiler }}:${{ matrix.config.version }}
     name: "${{ matrix.config.compiler}} ${{ matrix.config.version }} (C++${{ matrix.config.cppstd }}, ${{ matrix.config.build_type }})"
@@ -51,6 +54,7 @@ jobs:
             -DSPDLOG_BUILD_BENCH=OFF \
             -DSPDLOG_BUILD_TESTS=ON \
             -DSPDLOG_BUILD_TESTS_HO=OFF \
+            -DSPDLOG_ASYNC_OWNING_SOURCELOC_STRINGS=${{ matrix.async_owning_sourceloc_strings }} \
             -DSPDLOG_SANITIZE_ADDRESS=${{ matrix.config.asan || 'ON' }}
           make -j2
           ctest -j2 --output-on-failure

diff --git a/CMakeLists.txt b/CMakeLists.txt
@@ -113,6 +113,8 @@ else()
     set(SPDLOG_WCHAR_FILENAMES OFF CACHE BOOL "non supported option" FORCE)
 endif()
 
+set(SPDLOG_ASYNC_OWNING_SOURCELOC_STRINGS BOOL "Async loggers take owership of source_loc strings" OFF)
+
 if(${CMAKE_SYSTEM_NAME} STREQUAL "Linux")
     option(SPDLOG_CLOCK_COARSE "Use CLOCK_REALTIME_COARSE instead of the regular clock," OFF)
 else()
@@ -248,6 +250,7 @@ foreach(
     SPDLOG_NO_TLS
     SPDLOG_NO_ATOMIC_LEVELS
     SPDLOG_DISABLE_DEFAULT_LOGGER
+    SPDLOG_ASYNC_OWNING_SOURCELOC_STRINGS
     SPDLOG_USE_STD_FORMAT)
     if(${SPDLOG_OPTION})
         target_compile_definitions(spdlog PUBLIC ${SPDLOG_OPTION})

diff --git a/include/spdlog/details/thread_pool.h b/include/spdlog/details/thread_pool.h
@@ -10,6 +10,7 @@
 #include <chrono>
 #include <functional>
 #include <memory>
+#include <string>
 #include <thread>
 #include <vector>
 
@@ -34,29 +35,48 @@ struct async_msg : log_msg_buffer {
     // should only be moved in or out of the queue..
     async_msg(const async_msg &) = delete;
 
-// support for vs2013 move
-#if defined(_MSC_VER) && _MSC_VER <= 1800
     async_msg(async_msg &&other)
         : log_msg_buffer(std::move(other)),
           msg_type(other.msg_type),
-          worker_ptr(std::move(other.worker_ptr)) {}
+          worker_ptr(std::move(other.worker_ptr)) {
+#ifdef SPDLOG_ASYNC_OWNING_SOURCELOC_STRINGS
+        source.filename = filename_string.c_str();
+        source.funcname = function_string.c_str();
+#endif
+    }
 
     async_msg &operator=(async_msg &&other) {
         *static_cast<log_msg_buffer *>(this) = std::move(other);
         msg_type = other.msg_type;
         worker_ptr = std::move(other.worker_ptr);
+#ifdef SPDLOG_ASYNC_OWNING_SOURCELOC_STRINGS
+        source.filename = filename_string.c_str();
+        source.funcname = function_string.c_str();
+#endif
         return *this;
     }
-#else  // (_MSC_VER) && _MSC_VER <= 1800
-    async_msg(async_msg &&) = default;
-    async_msg &operator=(async_msg &&) = default;
-#endif
 
     // construct from log_msg with given type
     async_msg(async_logger_ptr &&worker, async_msg_type the_type, const details::log_msg &m)
         : log_msg_buffer{m},
           msg_type{the_type},
-          worker_ptr{std::move(worker)} {}
+          worker_ptr{std::move(worker)} {
+#ifdef SPDLOG_ASYNC_OWNING_SOURCELOC_STRINGS
+        // take ownership of filename/funcname
+        if (m.source.filename) {
+            filename_string = std::string{m.source.filename};
+            source.filename = filename_string.c_str();
+        } else {
+            source.filename = nullptr;
+        }
+        if (m.source.funcname) {
+            function_string = std::string{m.source.funcname};
+            source.funcname = function_string.c_str();
+        } else {
+            source.funcname = nullptr;
+        }
+#endif
+    }
 
     async_msg(async_logger_ptr &&worker, async_msg_type the_type)
         : log_msg_buffer{},
@@ -65,6 +85,12 @@ struct async_msg : log_msg_buffer {
 
     explicit async_msg(async_msg_type the_type)
         : async_msg{nullptr, the_type} {}
+
+#ifdef SPDLOG_ASYNC_OWNING_SOURCELOC_STRINGS
+    // source.filename/funcname always need to point to the member strings
+    std::string filename_string;
+    std::string function_string;
+#endif
 };
 
 class SPDLOG_API thread_pool {