From 537319e3b3f1900c6e734efc2ba9db08ef3e376a Mon Sep 17 00:00:00 2001 From: Alois Wohlschlager Date: Mon, 24 Jun 2024 19:02:56 +0200 Subject: [PATCH] fix the logger leaks Passing around the loggers as raw pointers or references everywhere is really error prone, so they were just leaked all over the place. Introduce unique ownership instead so that they are properly freed when no longer needed (for example, when a different logger is created or the program is terminated). Of course, actually exercising the destructors uncovered a bug in ProgressBar, where creating it in inactive state (on a non-TTY stderr) resulted in the update thread starting anyway, violating the invariant required by the destructor. This had to be fixed too. Change-Id: I372cb710aaca492c2adcd8241f1a8e57221b5d16 --- src/build-remote/build-remote.cc | 2 +- src/libmain/loggers.cc | 4 +-- src/libmain/progress-bar.cc | 31 +++++++++++---------- src/libmain/progress-bar.hh | 2 +- src/libstore/build/local-derivation-goal.cc | 2 +- src/libstore/daemon.cc | 8 ++++-- src/libutil/logging.cc | 16 +++++------ src/libutil/logging.hh | 6 ++-- tests/unit/libexpr/primops.cc | 13 ++++----- tests/unit/libmain/progress-bar.cc | 2 +- 10 files changed, 44 insertions(+), 42 deletions(-) diff --git a/src/build-remote/build-remote.cc b/src/build-remote/build-remote.cc index fb90403a0..88a829cc0 100644 --- a/src/build-remote/build-remote.cc +++ b/src/build-remote/build-remote.cc @@ -60,7 +60,7 @@ static bool allSupportedLocally(Store & store, const std::set& requ static int main_build_remote(int argc, char * * argv) { { - logger = makeJSONLogger(*logger); + logger = makeJSONLogger(std::move(logger)); /* Ensure we don't get any SSH passphrase or host key popups. */ unsetenv("DISPLAY"); diff --git a/src/libmain/loggers.cc b/src/libmain/loggers.cc index 8c3c4e355..85ffe4a8a 100644 --- a/src/libmain/loggers.cc +++ b/src/libmain/loggers.cc @@ -24,14 +24,14 @@ LogFormat parseLogFormat(const std::string & logFormatStr) { throw Error("option 'log-format' has an invalid value '%s'", logFormatStr); } -Logger * makeDefaultLogger() { +std::unique_ptr makeDefaultLogger() { switch (defaultLogFormat) { case LogFormat::raw: return makeSimpleLogger(false); case LogFormat::rawWithLogs: return makeSimpleLogger(true); case LogFormat::internalJSON: - return makeJSONLogger(*makeSimpleLogger(true)); + return makeJSONLogger(makeSimpleLogger(true)); case LogFormat::bar: return makeProgressBar(); case LogFormat::barWithLogs: { diff --git a/src/libmain/progress-bar.cc b/src/libmain/progress-bar.cc index e36bc0b01..3151ac9ec 100644 --- a/src/libmain/progress-bar.cc +++ b/src/libmain/progress-bar.cc @@ -44,17 +44,20 @@ static std::string_view storePathToName(std::string_view path) ProgressBar::ProgressBar(bool isTTY) : isTTY(isTTY) { - state_.lock()->active = isTTY; - updateThread = std::thread([&]() { - auto state(state_.lock()); - auto nextWakeup = A_LONG_TIME; - while (state->active) { - if (!state->haveUpdate) - state.wait_for(updateCV, nextWakeup); - nextWakeup = draw(*state, {}); - state.wait_for(quitCV, std::chrono::milliseconds(50)); - } - }); + if ((state_.lock()->active = isTTY)) { + // Only start the update thread when the logger is set to active. + // Otherwise, the destructor will std::terminate trying to destroy a joinable thread. + updateThread = std::thread([&]() { + auto state(state_.lock()); + auto nextWakeup = A_LONG_TIME; + while (state->active) { + if (!state->haveUpdate) + state.wait_for(updateCV, nextWakeup); + nextWakeup = draw(*state, {}); + state.wait_for(quitCV, std::chrono::milliseconds(50)); + } + }); + } } ProgressBar::~ProgressBar() @@ -554,9 +557,9 @@ void ProgressBar::setPrintMultiline(bool printMultiline) this->printMultiline = printMultiline; } -Logger * makeProgressBar() +std::unique_ptr makeProgressBar() { - return new ProgressBar(shouldANSI()); + return std::make_unique(shouldANSI()); } void startProgressBar() @@ -566,7 +569,7 @@ void startProgressBar() void stopProgressBar() { - auto progressBar = dynamic_cast(logger); + auto progressBar = dynamic_cast(logger.get()); if (progressBar) progressBar->stop(); } diff --git a/src/libmain/progress-bar.hh b/src/libmain/progress-bar.hh index e682d75fe..bf05c89de 100644 --- a/src/libmain/progress-bar.hh +++ b/src/libmain/progress-bar.hh @@ -111,7 +111,7 @@ struct ProgressBar : public Logger void setPrintMultiline(bool printMultiline) override; }; -Logger * makeProgressBar(); +std::unique_ptr makeProgressBar(); void startProgressBar(); diff --git a/src/libstore/build/local-derivation-goal.cc b/src/libstore/build/local-derivation-goal.cc index c7d7d06d6..acc8f7cf1 100644 --- a/src/libstore/build/local-derivation-goal.cc +++ b/src/libstore/build/local-derivation-goal.cc @@ -2636,7 +2636,7 @@ void LocalDerivationGoal::runChild() /* Execute the program. This should not return. */ if (drv->isBuiltin()) { try { - logger = makeJSONLogger(*logger); + logger = makeJSONLogger(std::move(logger)); BasicDerivation & drv2(*drv); for (auto & e : drv2.env) diff --git a/src/libstore/daemon.cc b/src/libstore/daemon.cc index 6d64644d1..d96d92d01 100644 --- a/src/libstore/daemon.cc +++ b/src/libstore/daemon.cc @@ -991,11 +991,13 @@ void processConnection( if (clientVersion < MIN_SUPPORTED_WORKER_PROTO_VERSION) throw Error("the Nix client version is too old"); - auto tunnelLogger = new TunnelLogger(to, clientVersion); - auto prevLogger = nix::logger; + std::unique_ptr savedPrevLogger; + auto prevLogger = nix::logger.get(); + auto ownedTunnelLogger = std::make_unique(to, clientVersion); + auto tunnelLogger = ownedTunnelLogger.get(); // FIXME if (!recursive) - logger = tunnelLogger; + savedPrevLogger = std::exchange(logger, std::move(ownedTunnelLogger)); unsigned int opCount = 0; diff --git a/src/libutil/logging.cc b/src/libutil/logging.cc index fecbc89ac..ccd1f1ea7 100644 --- a/src/libutil/logging.cc +++ b/src/libutil/logging.cc @@ -27,7 +27,7 @@ void setCurActivity(const ActivityId activityId) curActivity = activityId; } -Logger * logger = makeSimpleLogger(true); +std::unique_ptr logger = makeSimpleLogger(true); void Logger::warn(const std::string & msg) { @@ -129,9 +129,9 @@ void writeToStderr(std::string_view s) } } -Logger * makeSimpleLogger(bool printBuildLogs) +std::unique_ptr makeSimpleLogger(bool printBuildLogs) { - return new SimpleLogger(printBuildLogs); + return std::make_unique(printBuildLogs); } std::atomic nextId{0}; @@ -159,9 +159,9 @@ void to_json(nlohmann::json & json, std::shared_ptr pos) } struct JSONLogger : Logger { - Logger & prevLogger; + std::unique_ptr prevLogger; - JSONLogger(Logger & prevLogger) : prevLogger(prevLogger) { } + JSONLogger(std::unique_ptr prevLogger) : prevLogger(std::move(prevLogger)) { } bool isVerbose() override { return true; @@ -182,7 +182,7 @@ struct JSONLogger : Logger { void write(const nlohmann::json & json) { - prevLogger.log(lvlError, "@nix " + json.dump(-1, ' ', false, nlohmann::json::error_handler_t::replace)); + prevLogger->log(lvlError, "@nix " + json.dump(-1, ' ', false, nlohmann::json::error_handler_t::replace)); } void log(Verbosity lvl, std::string_view s) override @@ -254,9 +254,9 @@ struct JSONLogger : Logger { } }; -Logger * makeJSONLogger(Logger & prevLogger) +std::unique_ptr makeJSONLogger(std::unique_ptr prevLogger) { - return new JSONLogger(prevLogger); + return std::make_unique(std::move(prevLogger)); } static Logger::Fields getFields(nlohmann::json & json) diff --git a/src/libutil/logging.hh b/src/libutil/logging.hh index 3cead4296..6089f1585 100644 --- a/src/libutil/logging.hh +++ b/src/libutil/logging.hh @@ -227,11 +227,11 @@ struct PushActivity ~PushActivity() { setCurActivity(prevAct); } }; -extern Logger * logger; +extern std::unique_ptr logger; -Logger * makeSimpleLogger(bool printBuildLogs = true); +std::unique_ptr makeSimpleLogger(bool printBuildLogs = true); -Logger * makeJSONLogger(Logger & prevLogger); +std::unique_ptr makeJSONLogger(std::unique_ptr prevLogger); /** * suppress msgs > this diff --git a/tests/unit/libexpr/primops.cc b/tests/unit/libexpr/primops.cc index bd174a6c0..f623cc896 100644 --- a/tests/unit/libexpr/primops.cc +++ b/tests/unit/libexpr/primops.cc @@ -27,20 +27,17 @@ namespace nix { }; class CaptureLogging { - Logger * oldLogger; - std::unique_ptr tempLogger; + std::unique_ptr oldLogger; public: - CaptureLogging() : tempLogger(std::make_unique()) { - oldLogger = logger; - logger = tempLogger.get(); - } + CaptureLogging() : oldLogger(std::exchange(logger, std::make_unique())) {} ~CaptureLogging() { - logger = oldLogger; + logger = std::move(oldLogger); } std::string get() const { - return tempLogger->get(); + auto captureLogger = dynamic_cast(logger.get()); + return captureLogger ? captureLogger->get() : ""; } }; diff --git a/tests/unit/libmain/progress-bar.cc b/tests/unit/libmain/progress-bar.cc index e44a8b37e..778256d0b 100644 --- a/tests/unit/libmain/progress-bar.cc +++ b/tests/unit/libmain/progress-bar.cc @@ -24,7 +24,7 @@ namespace nix initGC(); startProgressBar(); - ASSERT_NE(dynamic_cast(logger), nullptr); + ASSERT_NE(dynamic_cast(logger.get()), nullptr); ProgressBar & progressBar = dynamic_cast(*logger); Activity act(