fix: make log severity type safe (#1025)

CMakeLists.txt

@@ -793,6 +793,69 @@ if (BUILD_TESTING)
     striplog10
     PROPERTIES WILL_FAIL ON
   )
+
+  set (_glog_TEST_ENVIRONMENT
+    glog_ROOT=${glog_BINARY_DIR}
+  )
+
+  add_test (NAME log_severity_constants COMMAND ${CMAKE_CTEST_COMMAND}
+    --build-config $<CONFIG>
+    --build-and-test
+    "${glog_SOURCE_DIR}/src/log_severity_unittest"
+    "${glog_BINARY_DIR}/Tests/log_severity_constants"
+    --build-generator ${CMAKE_GENERATOR}
+    --build-makeprogram ${CMAKE_MAKE_PROGRAM}
+    --build-target glog_log_severity_constants
+    --build-options
+    -DCMAKE_BUILD_TYPE=$<CONFIG>
+    -DCMAKE_CXX_COMPILER=${CMAKE_CXX_COMPILER}
+    -DCMAKE_GENERATOR_PLATFORM=${CMAKE_GENERATOR_PLATFORM}
+    -DCMAKE_GENERATOR_TOOLSET=${CMAKE_GENERATOR_TOOLSET}
+  )
+  set_tests_properties (log_severity_constants PROPERTIES
+    ENVIRONMENT "${_glog_TEST_ENVIRONMENT}"
+    PASS_REGULAR_EXPRESSION "COMPACT_GOOGLE_LOG_[1-3]"
+  )
+
+  add_test (NAME log_severity_conversion COMMAND ${CMAKE_CTEST_COMMAND}
+    --build-config $<CONFIG>
+    --build-and-test
+    "${glog_SOURCE_DIR}/src/log_severity_unittest"
+    "${glog_BINARY_DIR}/Tests/log_severity_conversion"
+    --build-generator ${CMAKE_GENERATOR}
+    --build-makeprogram ${CMAKE_MAKE_PROGRAM}
+    --build-target glog_log_severity_conversion
+    --build-options
+    -DCMAKE_BUILD_TYPE=$<CONFIG>
+    -DCMAKE_CXX_COMPILER=${CMAKE_CXX_COMPILER}
+    -DCMAKE_GENERATOR_PLATFORM=${CMAKE_GENERATOR_PLATFORM}
+    -DCMAKE_GENERATOR_TOOLSET=${CMAKE_GENERATOR_TOOLSET}
+  )
+  set_tests_properties (log_severity_conversion PROPERTIES
+    ENVIRONMENT "${_glog_TEST_ENVIRONMENT}"
+  )
+
+  if (CMAKE_COMPILER_IS_GNUCXX)
+    set_tests_properties (log_severity_conversion PROPERTIES
+      PASS_REGULAR_EXPRESSION "error: invalid conversion from (‘|')int(’|')"
+    )
+  elseif (CMAKE_CXX_COMPILER_ID MATCHES Clang)
+    set_tests_properties (log_severity_conversion PROPERTIES
+      PASS_REGULAR_EXPRESSION "no known conversion from 'int'"
+    )
+  elseif (MSVC)
+    set_tests_properties (log_severity_conversion PROPERTIES
+      PASS_REGULAR_EXPRESSION "error C2440"
+    )
+  else (CMAKE_COMPILER_IS_GNUCXX)
+    message (AUTHOR_WARNING
+      "Unsuuported C++ compiler ${CMAKE_CXX_COMPILER_ID}: "
+      "log_severity_conversion test will be disabled"
+    )
+    set_tests_properties (log_severity_conversion DISABLED ON)
+  endif (CMAKE_COMPILER_IS_GNUCXX)
+
+  unset (_glog_TEST_ENVIRONMENT)
 endif (BUILD_TESTING)
 
 install (TARGETS glog

src/glog/log_severity.h

@@ -1,4 +1,4 @@
-// Copyright (c) 2023, Google Inc.
+// Copyright (c) 2024, Google Inc.
 // All rights reserved.
 //
 // Redistribution and use in source and binary forms, with or without
@@ -48,17 +48,31 @@
 // Variables of type LogSeverity are widely taken to lie in the range
 // [0, NUM_SEVERITIES-1].  Be careful to preserve this assumption if
 // you ever need to change their values or add a new severity.
-using LogSeverity = int;
 
-const int GLOG_INFO = 0, GLOG_WARNING = 1, GLOG_ERROR = 2, GLOG_FATAL = 3,
+enum LogSeverity {
-          NUM_SEVERITIES = 4;
+  GLOG_INFO = 0,
+  GLOG_WARNING = 1,
+  GLOG_ERROR = 2,
+  GLOG_FATAL = 3,
 #ifndef GLOG_NO_ABBREVIATED_SEVERITIES
 #  ifdef ERROR
 #  error ERROR macro is defined. Define GLOG_NO_ABBREVIATED_SEVERITIES before including logging.h. See the document for detail.
 #  endif
-const int INFO = GLOG_INFO, WARNING = GLOG_WARNING, ERROR = GLOG_ERROR,
+  INFO = GLOG_INFO,
-          FATAL = GLOG_FATAL;
+  WARNING = GLOG_WARNING,
+  ERROR = GLOG_ERROR,
+  FATAL = GLOG_FATAL
 #endif
+};
+
+#if defined(__cpp_inline_variables)
+#  if (__cpp_inline_variables >= 201606L)
+inline
+#  endif  // (__cpp_inline_variables >= 201606L)
+#endif    // defined(__cpp_inline_variables)
+          // clang-format off
+constexpr int NUM_SEVERITIES = 4;
+// clang-format on
 
 // DFATAL is FATAL in debug mode, ERROR in normal mode
 #ifdef NDEBUG

src/glog/logging.h

@@ -1526,7 +1526,7 @@ class GLOG_EXPORT LogMessageFatal : public LogMessage {
 
 // A non-macro interface to the log facility; (useful
 // when the logging level is not a compile-time constant).
-inline void LogAtLevel(int const severity, std::string const& msg) {
+inline void LogAtLevel(LogSeverity severity, std::string const& msg) {
   LogMessage(__FILE__, __LINE__, severity).stream() << msg;
 }
 

src/log_severity_unittest/CMakeLists.txt

@@ -0,0 +1,10 @@
+cmake_minimum_required (VERSION 3.16)
+project (glog_log_severity LANGUAGES CXX)
+
+find_package (glog REQUIRED NO_MODULE)
+
+add_executable (glog_log_severity_constants glog_log_severity_constants.cc)
+target_link_libraries (glog_log_severity_constants PRIVATE glog::glog)
+
+add_executable (glog_log_severity_conversion glog_log_severity_conversion.cc)
+target_link_libraries (glog_log_severity_conversion PRIVATE glog::glog)

src/log_severity_unittest/glog_log_severity_constants.cc

@@ -0,0 +1,40 @@
+// Copyright (c) 2024, Google Inc.
+// All rights reserved.
+//
+// Redistribution and use in source and binary forms, with or without
+// modification, are permitted provided that the following conditions are
+// met:
+//
+//     * Redistributions of source code must retain the above copyright
+// notice, this list of conditions and the following disclaimer.
+//     * Redistributions in binary form must reproduce the above
+// copyright notice, this list of conditions and the following disclaimer
+// in the documentation and/or other materials provided with the
+// distribution.
+//     * Neither the name of Google Inc. nor the names of its
+// contributors may be used to endorse or promote products derived from
+// this software without specific prior written permission.
+//
+// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+//
+// Author: Sergiu Deitsch
+
+#include <glog/logging.h>
+
+int main() {
+  // Must not compile
+  LOG(0) << "type unsafe info";
+  LOG(1) << "type unsafe info";
+  LOG(2) << "type unsafe info";
+  LOG(3) << "type unsafe info";
+}

src/log_severity_unittest/glog_log_severity_conversion.cc

@@ -0,0 +1,42 @@
+// Copyright (c) 2024, Google Inc.
+// All rights reserved.
+//
+// Redistribution and use in source and binary forms, with or without
+// modification, are permitted provided that the following conditions are
+// met:
+//
+//     * Redistributions of source code must retain the above copyright
+// notice, this list of conditions and the following disclaimer.
+//     * Redistributions in binary form must reproduce the above
+// copyright notice, this list of conditions and the following disclaimer
+// in the documentation and/or other materials provided with the
+// distribution.
+//     * Neither the name of Google Inc. nor the names of its
+// contributors may be used to endorse or promote products derived from
+// this software without specific prior written permission.
+//
+// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+//
+// Author: Sergiu Deitsch
+
+#include <glog/logging.h>
+
+int main() {
+  // Must not compile
+  google::LogMessage{__FILE__, __LINE__, -1};
+  // Cast to int to avoid implicit conversoin to nullptr
+  google::LogMessage{__FILE__, __LINE__, static_cast<int>(0)};
+  google::LogMessage{__FILE__, __LINE__, 1};
+  google::LogMessage{__FILE__, __LINE__, 2};
+  google::LogMessage{__FILE__, __LINE__, 3};
+}

src/logging.cc

@@ -27,14 +27,13 @@
 // (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
 // OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 
-#include <chrono>
-#include <system_error>
 #define _GNU_SOURCE 1  // needed for O_NOFOLLOW and pread()/pwrite()
 
 #include "glog/logging.h"
 
 #include <algorithm>
 #include <cassert>
+#include <chrono>
 #include <cstddef>
 #include <iomanip>
 #include <iterator>
@@ -42,6 +41,8 @@
 #include <shared_mutex>
 #include <string>
 #include <thread>
+#include <type_traits>
+#include <utility>
 
 #include "base/commandlineflags.h"  // to get the program name
 #include "base/googleinit.h"
@@ -304,29 +305,43 @@ static bool TerminalSupportsColor() {
   return term_supports_color;
 }
 
+#if defined(__cpp_lib_unreachable) && (__cpp_lib_unreachable >= 202202L)
+#  define GLOG_UNREACHABLE std::unreachable()
+#elif !defined(NDEBUG)
+#  define GLOG_UNREACHABLE assert(false)
+#else
+#  if defined(_MSC_VER)
+#    define GLOG_UNREACHABLE __assume(false)
+#  elif defined(__has_builtin)
+#    if __has_builtin(unreachable)
+#      define GLOG_UNREACHABLE __builtin_unreachable()
+#    endif
+#  endif
+#  if !defined(GLOG_UNREACHABLE) && defined(__GNUG__)
+#    define GLOG_UNREACHABLE __builtin_unreachable()
+#  endif
+#  if !defined(GLOG_UNREACHABLE)
+#    define GLOG_UNREACHABLE
+#  endif
+#endif
+
 namespace google {
 
 enum GLogColor { COLOR_DEFAULT, COLOR_RED, COLOR_GREEN, COLOR_YELLOW };
 
 static GLogColor SeverityToColor(LogSeverity severity) {
-  assert(severity >= 0 && severity < NUM_SEVERITIES);
-  GLogColor color = COLOR_DEFAULT;
   switch (severity) {
     case GLOG_INFO:
-      color = COLOR_DEFAULT;
+      return COLOR_DEFAULT;
-      break;
     case GLOG_WARNING:
-      color = COLOR_YELLOW;
+      return COLOR_YELLOW;
-      break;
     case GLOG_ERROR:
     case GLOG_FATAL:
-      color = COLOR_RED;
+      return COLOR_RED;
-      break;
-    default:
-      // should never get here.
-      assert(false);
   }
-  return color;
+
+  // should never get here.
+  GLOG_UNREACHABLE;
 }
 
 #ifdef GLOG_OS_WINDOWS
@@ -340,9 +355,10 @@ static WORD GetColorAttribute(GLogColor color) {
       return FOREGROUND_GREEN;
     case COLOR_YELLOW:
       return FOREGROUND_RED | FOREGROUND_GREEN;
-    default:
+    case COLOR_DEFAULT:
-      return 0;
+      break;
   }
+  return 0;
 }
 
 #else
@@ -382,8 +398,8 @@ struct LogMessage::LogMessageData {
   // Buffer space; contains complete message text.
   char message_text_[LogMessage::kMaxLogMessageLen + 1];
   LogStream stream_;
-  char severity_;  // What level is this LogMessage logged at?
+  LogSeverity severity_;  // What level is this LogMessage logged at?
-  int line_;       // line number where logging call is.
+  int line_;              // line number where logging call is.
   void (LogMessage::*send_method_)();  // Call this in destructor to send
   union {  // At most one of these is used: union to keep the size low.
     LogSink* sink_;  // nullptr or sink to send message to
@@ -621,7 +637,7 @@ class LogDestination {
   base::Logger* logger_;  // Either &fileobject_, or wrapper around it
 
   static std::unique_ptr<LogDestination> log_destinations_[NUM_SEVERITIES];
-  static LogSeverity email_logging_severity_;
+  static std::underlying_type_t<LogSeverity> email_logging_severity_;
   static string addresses_;
   static string hostname_;
   static bool terminal_supports_color_;
@@ -639,7 +655,8 @@ class LogDestination {
 };
 
 // Errors do not get logged to email by default.
-LogSeverity LogDestination::email_logging_severity_ = 99999;
+std::underlying_type_t<LogSeverity> LogDestination::email_logging_severity_ =
+    99999;
 
 string LogDestination::addresses_;
 string LogDestination::hostname_;
@@ -696,7 +713,7 @@ inline void LogDestination::FlushLogFiles(int min_severity) {
   // all this stuff.
   std::lock_guard<std::mutex> l{log_mutex};
   for (int i = min_severity; i < NUM_SEVERITIES; i++) {
-    LogDestination* log = log_destination(i);
+    LogDestination* log = log_destination(static_cast<LogSeverity>(i));
     if (log != nullptr) {
       log->logger_->Flush();
     }
@@ -705,7 +722,6 @@ inline void LogDestination::FlushLogFiles(int min_severity) {
 
 inline void LogDestination::SetLogDestination(LogSeverity severity,
                                               const char* base_filename) {
-  assert(severity >= 0 && severity < NUM_SEVERITIES);
   // Prevent any subtle race conditions by wrapping a mutex lock around
   // all this stuff.
   std::lock_guard<std::mutex> l{log_mutex};
@@ -744,12 +760,12 @@ inline void LogDestination::SetLogFilenameExtension(const char* ext) {
   // all this stuff.
   std::lock_guard<std::mutex> l{log_mutex};
   for (int severity = 0; severity < NUM_SEVERITIES; ++severity) {
-    log_destination(severity)->fileobject_.SetExtension(ext);
+    log_destination(static_cast<LogSeverity>(severity))
+        ->fileobject_.SetExtension(ext);
   }
 }
 
 inline void LogDestination::SetStderrLogging(LogSeverity min_severity) {
-  assert(min_severity >= 0 && min_severity < NUM_SEVERITIES);
   // Prevent any subtle race conditions by wrapping a mutex lock around
   // all this stuff.
   std::lock_guard<std::mutex> l{log_mutex};
@@ -759,15 +775,15 @@ inline void LogDestination::SetStderrLogging(LogSeverity min_severity) {
 inline void LogDestination::LogToStderr() {
   // *Don't* put this stuff in a mutex lock, since SetStderrLogging &
   // SetLogDestination already do the locking!
-  SetStderrLogging(0);  // thus everything is "also" logged to stderr
+  SetStderrLogging(GLOG_INFO);  // thus everything is "also" logged to stderr
   for (int i = 0; i < NUM_SEVERITIES; ++i) {
-    SetLogDestination(i, "");  // "" turns off logging to a logfile
+    SetLogDestination(static_cast<LogSeverity>(i),
+                      "");  // "" turns off logging to a logfile
   }
 }
 
 inline void LogDestination::SetEmailLogging(LogSeverity min_severity,
                                             const char* addresses) {
-  assert(min_severity >= 0 && min_severity < NUM_SEVERITIES);
   // Prevent any subtle race conditions by wrapping a mutex lock around
   // all this stuff.
   std::lock_guard<std::mutex> l{log_mutex};
@@ -907,7 +923,8 @@ inline void LogDestination::LogToAllLogfiles(LogSeverity severity,
     ColoredWriteToStderr(severity, message, len);
   } else {
     for (int i = severity; i >= 0; --i) {
-      LogDestination::MaybeLogToLogfile(i, timestamp, message, len);
+      LogDestination::MaybeLogToLogfile(static_cast<LogSeverity>(i), timestamp,
+                                        message, len);
     }
   }
 }
@@ -946,7 +963,6 @@ std::unique_ptr<LogDestination>
     LogDestination::log_destinations_[NUM_SEVERITIES];
 
 inline LogDestination* LogDestination::log_destination(LogSeverity severity) {
-  assert(severity >= 0 && severity < NUM_SEVERITIES);
   if (log_destinations_[severity] == nullptr) {
     log_destinations_[severity] =
         std::make_unique<LogDestination>(severity, nullptr);
@@ -999,10 +1015,7 @@ LogFileObject::LogFileObject(LogSeverity severity, const char* base_filename)
       filename_extension_(),
       severity_(severity),
       rollover_attempt_(kRolloverAttemptFrequency - 1),
-      start_time_(std::chrono::system_clock::now()) {
+      start_time_(std::chrono::system_clock::now()) {}
-  assert(severity >= 0);
-  assert(severity < NUM_SEVERITIES);
-}
 
 LogFileObject::~LogFileObject() {
   std::lock_guard<std::mutex> l{mutex_};
@@ -1819,7 +1832,6 @@ void ReprintFatalMessage() {
 void LogMessage::SendToLog() EXCLUSIVE_LOCKS_REQUIRED(log_mutex) {
   static bool already_warned_before_initgoogle = false;
 
-
   RAW_DCHECK(data_->num_chars_to_log_ > 0 &&
                  data_->message_text_[data_->num_chars_to_log_ - 1] == '\n',
              "");

src/logging_unittest.cc

@@ -1419,7 +1419,7 @@ TEST(LogAtLevel, Basic) {
   EXPECT_CALL(log, Log(GLOG_WARNING, StrNe(__FILE__), "function version"));
   EXPECT_CALL(log, Log(GLOG_INFO, __FILE__, "macro version"));
 
-  int severity = GLOG_WARNING;
+  LogSeverity severity = GLOG_WARNING;
   LogAtLevel(severity, "function version");
 
   severity = GLOG_INFO;

src/signalhandler.cc

@@ -353,7 +353,7 @@ void FailureSignalHandler(int signal_number, siginfo_t* signal_info,
 
   // Flush the logs before we do anything in case 'anything'
   // causes problems.
-  FlushLogFilesUnsafe(0);
+  FlushLogFilesUnsafe(GLOG_INFO);
 
   // Kill ourself by the default signal handler.
   InvokeDefaultSignalHandler(signal_number);