Fix logging segfault by leaking logger

src/libexpr-tests/primops.cc

@@ -32,17 +32,17 @@ class CaptureLogger : public Logger
 
 class CaptureLogging
 {
-    std::unique_ptr<Logger> oldLogger;
+    Logger * oldLogger;
 public:
     CaptureLogging()
     {
-        oldLogger = std::move(logger);
-        logger = std::make_unique<CaptureLogger>();
+        oldLogger = logger;
+        logger = new CaptureLogger();
     }
 
     ~CaptureLogging()
     {
-        logger = std::move(oldLogger);
+        logger = oldLogger;
     }
 };
 
@@ -144,7 +144,7 @@ TEST_F(PrimOpTest, trace)
     CaptureLogging l;
     auto v = eval("builtins.trace \"test string 123\" 123");
     ASSERT_THAT(v, IsIntEq(123));
-    auto text = (dynamic_cast<CaptureLogger *>(logger.get()))->get();
+    auto text = (dynamic_cast<CaptureLogger *>(logger))->get();
     ASSERT_NE(text.find("test string 123"), std::string::npos);
 }
 

src/libmain/loggers.cc

@@ -50,7 +50,7 @@ void setLogFormat(const std::string & logFormatStr)
 void setLogFormat(const LogFormat & logFormat)
 {
     defaultLogFormat = logFormat;
-    logger = makeDefaultLogger();
+    logger = makeDefaultLogger().release();
 }
 
 } // namespace nix

src/libstore/daemon.cc

@@ -1074,14 +1074,11 @@ void processConnection(ref<Store> store, FdSource && from, FdSink && to, Trusted
     conn.to = std::move(to);
     conn.from = std::move(from);
 
-    auto tunnelLogger_ = std::make_unique<TunnelLogger>(conn.to, conn.protoVersion);
-    auto tunnelLogger = tunnelLogger_.get();
-    std::unique_ptr<Logger> prevLogger_;
-    auto prevLogger = logger.get();
+    auto tunnelLogger = new TunnelLogger(conn.to, conn.protoVersion);
+    auto prevLogger = logger;
     // FIXME
     if (!recursive) {
-        prevLogger_ = std::move(logger);
-        logger = std::move(tunnelLogger_);
+        logger = tunnelLogger;
         applyJSONLogger();
     }
 

src/libstore/unix/build/child.cc

@@ -9,7 +9,7 @@ namespace nix {
 
 void commonChildInit()
 {
-    logger = makeSimpleLogger();
+    logger = makeSimpleLogger().release();
 
     const static std::string pathNullDevice = "/dev/null";
     restoreProcessContext(false);

src/libstore/unix/build/derivation-builder.cc

@@ -1408,7 +1408,7 @@ void DerivationBuilderImpl::runChild(RunChildArgs args)
         /* If this is a builtin builder, call it now. This should not return. */
         if (drv.isBuiltin()) {
             try {
-                logger = makeJSONLogger(getStandardError());
+                logger = makeJSONLogger(getStandardError()).release();
 
                 for (auto & e : drv.outputs)
                     ctx.outputs.insert_or_assign(e.first, store.printStorePath(scratchOutputs.at(e.first)));

src/libutil-c/nix_api_util.cc

@@ -257,7 +257,7 @@ nix_err nix_set_logger(nix_c_context * context, const nix_logger * vtable, void
     if (vtable == nullptr)
         return nix_set_err_msg(context, NIX_ERR_UNKNOWN, "logger vtable is null");
     try {
-        nix::logger = std::make_unique<CallbackLogger>(*vtable, userdata);
+        nix::logger = new CallbackLogger(*vtable, userdata);
     }
     NIXC_CATCH_ERRS
 }

src/libutil-tests/nix_api_util.cc

@@ -145,7 +145,7 @@ TEST_F(nix_api_util_context, nix_set_logger_routes_log_calls)
     LogCapture capture;
     // Restore the default logger before `capture` goes out of scope so
     // the destroy callback runs while `capture` is still alive.
-    Finally restoreLogger([] { nix::logger = nix::makeSimpleLogger(); });
+    Finally restoreLogger([] { nix::logger = nix::makeSimpleLogger().release(); });
 
     ASSERT_EQ(nix_set_logger(ctx, &captureLoggerVtable, &capture), NIX_OK);
 
@@ -163,7 +163,7 @@ TEST_F(nix_api_util_context, nix_set_logger_routes_log_calls)
 TEST_F(nix_api_util_context, nix_set_logger_routes_activities_and_results)
 {
     LogCapture capture;
-    Finally restoreLogger([] { nix::logger = nix::makeSimpleLogger(); });
+    Finally restoreLogger([] { nix::logger = nix::makeSimpleLogger().release(); });
 
     ASSERT_EQ(nix_set_logger(ctx, &captureLoggerVtable, &capture), NIX_OK);
 
@@ -191,20 +191,20 @@ TEST_F(nix_api_util_context, nix_set_logger_routes_activities_and_results)
     EXPECT_EQ(capture.stops[0], actId);
 }
 
-TEST_F(nix_api_util_context, nix_set_logger_invokes_destroy_when_replaced)
+TEST_F(nix_api_util_context, nix_set_logger_does_not_invoke_destroy_when_replaced)
 {
     // Both captures must outlive the Finally so that the logger held
     // by `nix::logger` at teardown can call destroy on a live capture.
     LogCapture capture;
     LogCapture capture2;
-    Finally restoreLogger([] { nix::logger = nix::makeSimpleLogger(); });
+    Finally restoreLogger([] { nix::logger = nix::makeSimpleLogger().release(); });
 
     ASSERT_EQ(nix_set_logger(ctx, &captureLoggerVtable, &capture), NIX_OK);
     EXPECT_FALSE(capture.destroyed);
 
-    // Replacing the logger must fire destroy on the old userdata.
+    // Replacing the logger won't destroy the old logger.
     ASSERT_EQ(nix_set_logger(ctx, &captureLoggerVtable, &capture2), NIX_OK);
-    EXPECT_TRUE(capture.destroyed);
+    EXPECT_FALSE(capture.destroyed);
     EXPECT_FALSE(capture2.destroyed);
 }
 

src/libutil/include/nix/util/logging.hh

@@ -273,7 +273,7 @@ struct PushActivity
     }
 };
 
-extern std::unique_ptr<Logger> logger;
+extern Logger * logger;
 
 std::unique_ptr<Logger> makeSimpleLogger(bool printBuildLogs = true);
 

src/libutil/logging.cc

@@ -36,7 +36,11 @@ void setCurActivity(const ActivityId activityId)
     curActivity = activityId;
 }
 
-std::unique_ptr<Logger> logger = makeSimpleLogger(true);
+/**
+ * This is a raw pointer to allow it to leak.
+ * Avoids races in activity teardown.
+ */
+Logger * logger = makeSimpleLogger(true).release();
 
 void Logger::warn(const std::string & msg)
 {
@@ -403,7 +407,7 @@ void applyJSONLogger()
             std::vector<std::unique_ptr<Logger>> loggers;
             loggers.push_back(makeJSONLogger(*opt, false));
             try {
-                logger = makeTeeLogger(std::move(logger), std::move(loggers));
+                logger = makeTeeLogger(std::unique_ptr<Logger>(logger), std::move(loggers)).release();
             } catch (...) {
                 // `logger` is now gone so give up.
                 abort();

src/libutil/unix/processes.cc

@@ -210,16 +210,15 @@ static int childEntry(void * arg)
 
 pid_t startProcess(fun<void()> processMain, const ProcessOptions & options)
 {
-    auto newLogger = makeSimpleLogger();
+    auto newLogger = makeSimpleLogger().release();
     ChildWrapperFunction wrapper = [&] {
         if (!options.allowVfork) {
-            /* Set a simple logger, while releasing (not destroying)
+            /* Set a simple logger, while leaking (not destroying)
                the parent logger. We don't want to run the parent
                logger's destructor since that will crash (e.g. when
                ~ProgressBar() tries to join a thread that doesn't
                exist. */
-            logger.release();
-            logger = std::move(newLogger);
+            logger = newLogger;
         }
         try {
 #ifdef __linux__

src/nix/build-remote/build-remote.cc

@@ -54,7 +54,7 @@ static bool allSupportedLocally(Store & store, const StringSet & requiredFeature
 static int main_build_remote(int argc, char ** argv)
 {
     {
-        logger = makeJSONLogger(getStandardError());
+        logger = makeJSONLogger(getStandardError()).release();
 
         /* Ensure we don't get any SSH passphrase or host key popups. */
         unsetenv("DISPLAY");