From 85f36210321b492f698fa9acf6a79ae4eab03903 Mon Sep 17 00:00:00 2001 From: Andrei Polushin Date: Fri, 12 Aug 2022 18:28:53 +0700 Subject: [PATCH 1/2] SetLogger should delete previously set custom logger. As specified in the doc comment for SetLogger, "the logger becomes the property of the logging module and should not be deleted by the caller". Not only should the LogDestination delete a custom logger in its destructor, but it should also delete a previous logger when another logger is passed to SetLogger(). --- src/logging.cc | 15 +++++++++++++-- src/logging_custom_prefix_unittest.cc | 19 +++++++++++++++---- src/logging_unittest.cc | 19 +++++++++++++++---- 3 files changed, 43 insertions(+), 10 deletions(-) diff --git a/src/logging.cc b/src/logging.cc index 1df1034ae..ad8292d41 100644 --- a/src/logging.cc +++ b/src/logging.cc @@ -592,6 +592,10 @@ class LogDestination { static LogDestination* log_destination(LogSeverity severity); + base::Logger* GetLoggerImpl() const { return logger_; } + void SetLoggerImpl(base::Logger* logger); + void ResetLoggerImpl() { SetLoggerImpl(&fileobject_); } + LogFileObject fileobject_; base::Logger* logger_; // Either &fileobject_, or wrapper around it @@ -641,10 +645,17 @@ LogDestination::LogDestination(LogSeverity severity, } LogDestination::~LogDestination() { + ResetLoggerImpl(); +} + +void LogDestination::SetLoggerImpl(base::Logger* logger) { + if (logger_ == logger) return; + if (logger_ && logger_ != &fileobject_) { // Delete user-specified logger set via SetLogger(). delete logger_; } + logger_ = logger; } inline void LogDestination::FlushLogFilesUnsafe(int min_severity) { @@ -2017,12 +2028,12 @@ void LogMessage::SendToSyslogAndLog() { base::Logger* base::GetLogger(LogSeverity severity) { MutexLock l(&log_mutex); - return LogDestination::log_destination(severity)->logger_; + return LogDestination::log_destination(severity)->GetLoggerImpl(); } void base::SetLogger(LogSeverity severity, base::Logger* logger) { MutexLock l(&log_mutex); - LogDestination::log_destination(severity)->logger_ = logger; + LogDestination::log_destination(severity)->SetLoggerImpl(logger); } // L < log_mutex. Acquires and releases mutex_. diff --git a/src/logging_custom_prefix_unittest.cc b/src/logging_custom_prefix_unittest.cc index 615dce789..5888a21a9 100644 --- a/src/logging_custom_prefix_unittest.cc +++ b/src/logging_custom_prefix_unittest.cc @@ -842,6 +842,11 @@ static void TestExtension() { struct MyLogger : public base::Logger { string data; + explicit MyLogger(bool* set_on_destruction) + : set_on_destruction_(set_on_destruction) {} + + ~MyLogger() { *set_on_destruction_ = true; } + virtual void Write(bool /* should_flush */, time_t /* timestamp */, const char* message, @@ -852,19 +857,25 @@ struct MyLogger : public base::Logger { virtual void Flush() { } virtual uint32 LogSize() { return data.length(); } + + private: + bool* set_on_destruction_; }; static void TestWrapper() { fprintf(stderr, "==== Test log wrapper\n"); - MyLogger my_logger; + bool custom_logger_deleted = false; + MyLogger* my_logger = new MyLogger(&custom_logger_deleted); base::Logger* old_logger = base::GetLogger(GLOG_INFO); - base::SetLogger(GLOG_INFO, &my_logger); + base::SetLogger(GLOG_INFO, my_logger); LOG(INFO) << "Send to wrapped logger"; + CHECK(strstr(my_logger->data.c_str(), "Send to wrapped logger") != NULL); FlushLogFiles(GLOG_INFO); - base::SetLogger(GLOG_INFO, old_logger); - CHECK(strstr(my_logger.data.c_str(), "Send to wrapped logger") != NULL); + EXPECT_FALSE(custom_logger_deleted); + base::SetLogger(GLOG_INFO, old_logger); + EXPECT_TRUE(custom_logger_deleted); } static void TestErrno() { diff --git a/src/logging_unittest.cc b/src/logging_unittest.cc index 728b5fe3c..c71caebda 100644 --- a/src/logging_unittest.cc +++ b/src/logging_unittest.cc @@ -853,6 +853,11 @@ static void TestExtension() { struct MyLogger : public base::Logger { string data; + explicit MyLogger(bool* set_on_destruction) + : set_on_destruction_(set_on_destruction) {} + + ~MyLogger() { *set_on_destruction_ = true; } + virtual void Write(bool /* should_flush */, time_t /* timestamp */, const char* message, @@ -863,19 +868,25 @@ struct MyLogger : public base::Logger { virtual void Flush() { } virtual uint32 LogSize() { return data.length(); } + + private: + bool* set_on_destruction_; }; static void TestWrapper() { fprintf(stderr, "==== Test log wrapper\n"); - MyLogger my_logger; + bool custom_logger_deleted = false; + MyLogger* my_logger = new MyLogger(&custom_logger_deleted); base::Logger* old_logger = base::GetLogger(GLOG_INFO); - base::SetLogger(GLOG_INFO, &my_logger); + base::SetLogger(GLOG_INFO, my_logger); LOG(INFO) << "Send to wrapped logger"; + CHECK(strstr(my_logger->data.c_str(), "Send to wrapped logger") != NULL); FlushLogFiles(GLOG_INFO); - base::SetLogger(GLOG_INFO, old_logger); - CHECK(strstr(my_logger.data.c_str(), "Send to wrapped logger") != NULL); + EXPECT_FALSE(custom_logger_deleted); + base::SetLogger(GLOG_INFO, old_logger); + EXPECT_TRUE(custom_logger_deleted); } static void TestErrno() { From c0fc8703f7105c125d9b8239024f494c2fa8eb51 Mon Sep 17 00:00:00 2001 From: Andrei Polushin Date: Sat, 13 Aug 2022 16:10:18 +0700 Subject: [PATCH 2/2] Update src/logging.cc Co-authored-by: Sergiu Deitsch --- src/logging.cc | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/logging.cc b/src/logging.cc index ad8292d41..0f8b81504 100644 --- a/src/logging.cc +++ b/src/logging.cc @@ -649,7 +649,10 @@ LogDestination::~LogDestination() { } void LogDestination::SetLoggerImpl(base::Logger* logger) { - if (logger_ == logger) return; + if (logger_ == logger) { + // Prevent releasing currently held sink on reset + return; + } if (logger_ && logger_ != &fileobject_) { // Delete user-specified logger set via SetLogger().