Fix logging segfault by leaking logger

There is a race between activity and logger teardown, so we
switch from a unique_ptr to a raw pointer and leak the logger so it
persists across all activity teardowns.

Signed-off-by: Lisanna Dettwyler <lisanna.dettwyler@gmail.com>

Author: lisanna-dettwyler

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

@@ -1047,14 +1047,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

@@ -1376,7 +1376,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/include/nix/util/logging.hh

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

src/libutil/logging.cc

@@ -30,7 +30,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)
 {
@@ -381,7 +385,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

@@ -233,16 +233,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

@@ -70,7 +70,7 @@ static int main_build_remote(int argc, char ** argv)
         if (pthread_sigmask(SIG_UNBLOCK, &set, nullptr))
             throw SysError("unblocking SIGTERM");
 
-        logger = makeJSONLogger(getStandardError());
+        logger = makeJSONLogger(getStandardError()).release();
 
         /* Ensure we don't get any SSH passphrase or host key popups. */
         unsetenv("DISPLAY");