Fix a use-after-free bug in which the string passed to AtLocation may be

referenced after it is destroyed. While the string does live until the end of
the full statement, logging (previously occurred) in the destructor of the
`LogMessage` which may be constructed before the temporary string (and thus
destroyed after the temporary string's destructor).

This change moves logging to occur inside the internal `Voidify`. This does not
affect when logging occurs relative to any other observable, other than the
fact that it runs later than any temporary destructors in the log statement.

PiperOrigin-RevId: 729708145
Change-Id: I9b90367d7a5f4ed3cf091d4d33756262acc03ec6

Author: derekmauro

Diff for: absl/log/CMakeLists.txt

@@ -379,6 +379,7 @@ absl_cc_library(
     ${ABSL_DEFAULT_LINKOPTS}
   DEPS
     absl::config
+    absl::core_headers
 )
 
 absl_cc_library(

Diff for: absl/log/internal/BUILD.bazel

@@ -415,7 +415,10 @@ cc_library(
     hdrs = ["voidify.h"],
     copts = ABSL_DEFAULT_COPTS,
     linkopts = ABSL_DEFAULT_LINKOPTS,
-    deps = ["//absl/base:config"],
+    deps = [
+        "//absl/base:config",
+        "//absl/base:core_headers",
+    ],
 )
 
 cc_library(

Diff for: absl/log/internal/conditions.h

@@ -65,7 +65,7 @@
   switch (0)                                             \
   case 0:                                                \
   default:                                               \
-    !(condition) ? (void)0 : ::absl::log_internal::Voidify()&&
+    !(condition) ? (void)0 : ::absl::log_internal::Voidify() &&
 
 // `ABSL_LOG_INTERNAL_STATEFUL_CONDITION` applies a condition like
 // `ABSL_LOG_INTERNAL_STATELESS_CONDITION` but adds to that a series of variable
@@ -96,7 +96,8 @@
     for (const uint32_t COUNTER ABSL_ATTRIBUTE_UNUSED =                   \
              absl_log_internal_stateful_condition_state.counter();        \
          absl_log_internal_stateful_condition_do_log;                     \
-         absl_log_internal_stateful_condition_do_log = false)
+         absl_log_internal_stateful_condition_do_log = false)             \
+  ::absl::log_internal::Voidify() &&
 
 // `ABSL_LOG_INTERNAL_CONDITION_*` serve to combine any conditions from the
 // macro (e.g. `LOG_IF` or `VLOG`) with inherent conditions (e.g.

Diff for: absl/log/internal/log_message.cc

@@ -291,16 +291,8 @@ LogMessage::LogMessage(absl::Nonnull<const char*> file, int line, WarningTag)
 LogMessage::LogMessage(absl::Nonnull<const char*> file, int line, ErrorTag)
     : LogMessage(file, line, absl::LogSeverity::kError) {}
 
-LogMessage::~LogMessage() {
-#ifdef ABSL_MIN_LOG_LEVEL
-  if (data_->entry.log_severity() <
-          static_cast<absl::LogSeverity>(ABSL_MIN_LOG_LEVEL) &&
-      data_->entry.log_severity() < absl::LogSeverity::kFatal) {
-    return;
-  }
-#endif
-  Flush();
-}
+// This cannot go in the header since LogMessageData is defined in this file.
+LogMessage::~LogMessage() = default;
 
 LogMessage& LogMessage::AtLocation(absl::string_view file, int line) {
   data_->entry.full_filename_ = file;
@@ -691,7 +683,6 @@ LogMessageFatal::LogMessageFatal(absl::Nonnull<const char*> file, int line,
 }
 
 LogMessageFatal::~LogMessageFatal() {
-  Flush();
   FailWithoutStackTrace();
 }
 
@@ -700,7 +691,6 @@ LogMessageDebugFatal::LogMessageDebugFatal(absl::Nonnull<const char*> file,
     : LogMessage(file, line, absl::LogSeverity::kFatal) {}
 
 LogMessageDebugFatal::~LogMessageDebugFatal() {
-  Flush();
   FailWithoutStackTrace();
 }
 
@@ -711,7 +701,6 @@ LogMessageQuietlyDebugFatal::LogMessageQuietlyDebugFatal(
 }
 
 LogMessageQuietlyDebugFatal::~LogMessageQuietlyDebugFatal() {
-  Flush();
   FailQuietly();
 }
 
@@ -729,7 +718,6 @@ LogMessageQuietlyFatal::LogMessageQuietlyFatal(
 }
 
 LogMessageQuietlyFatal::~LogMessageQuietlyFatal() {
-  Flush();
   FailQuietly();
 }
 #if defined(_MSC_VER) && !defined(__clang__)

Diff for: absl/log/internal/log_message.h

@@ -17,12 +17,12 @@
 // -----------------------------------------------------------------------------
 //
 // This file declares `class absl::log_internal::LogMessage`. This class more or
-// less represents a particular log message. LOG/CHECK macros create a
-// temporary instance of `LogMessage` and then stream values to it.  At the end
-// of the LOG/CHECK statement, LogMessage instance goes out of scope and
-// `~LogMessage` directs the message to the registered log sinks.
-// Heap-allocation of `LogMessage` is unsupported.  Construction outside of a
-// `LOG` macro is unsupported.
+// less represents a particular log message. LOG/CHECK macros create a temporary
+// instance of `LogMessage` and then stream values to it.  At the end of the
+// LOG/CHECK statement, the LogMessage is voidified by operator&&, and `Flush()`
+// directs the message to the registered log sinks.  Heap-allocation of
+// `LogMessage` is unsupported.  Construction outside of a `LOG` macro is
+// unsupported.
 
 #ifndef ABSL_LOG_INTERNAL_LOG_MESSAGE_H_
 #define ABSL_LOG_INTERNAL_LOG_MESSAGE_H_
@@ -188,6 +188,9 @@ class LogMessage {
   template <typename T>
   LogMessage& operator<<(const T& v) ABSL_ATTRIBUTE_NOINLINE;
 
+  // Dispatches the completed `absl::LogEntry` to applicable `absl::LogSink`s.
+  void Flush();
+
   // Note: We explicitly do not support `operator<<` for non-const references
   // because it breaks logging of non-integer bitfield types (i.e., enums).
 
@@ -200,11 +203,6 @@ class LogMessage {
   // the process with an error exit code.
   [[noreturn]] static void FailQuietly();
 
-  // Dispatches the completed `absl::LogEntry` to applicable `absl::LogSink`s.
-  // This might as well be inlined into `~LogMessage` except that
-  // `~LogMessageFatal` needs to call it early.
-  void Flush();
-
   // After this is called, failures are done as quiet as possible for this log
   // message.
   void SetFailQuietly();

Diff for: absl/log/internal/nullstream.h

@@ -79,6 +79,7 @@ class NullStream {
     return *this;
   }
   NullStream& InternalStream() { return *this; }
+  void Flush() {}
 };
 template <typename T>
 inline NullStream& operator<<(NullStream& str, const T&) {

Diff for: absl/log/internal/voidify.h

@@ -16,13 +16,15 @@
 // File: log/internal/voidify.h
 // -----------------------------------------------------------------------------
 //
-// This class is used to explicitly ignore values in the conditional logging
-// macros. This avoids compiler warnings like "value computed is not used" and
-// "statement has no effect".
+// This class does the dispatching of the completed `absl::LogEntry` to
+// applicable `absl::LogSink`s, and is used to explicitly ignore values in the
+// conditional logging macros. This avoids compiler warnings like "value
+// computed is not used" and "statement has no effect".
 
 #ifndef ABSL_LOG_INTERNAL_VOIDIFY_H_
 #define ABSL_LOG_INTERNAL_VOIDIFY_H_
 
+#include "absl/base/attributes.h"
 #include "absl/base/config.h"
 
 namespace absl {
@@ -34,7 +36,11 @@ class Voidify final {
   // This has to be an operator with a precedence lower than << but higher than
   // ?:
   template <typename T>
-  void operator&&(const T&) const&& {}
+  ABSL_ATTRIBUTE_COLD void operator&&(T&& message) const&& {
+    // The dispatching of the completed `absl::LogEntry` to applicable
+    // `absl::LogSink`s happens here.
+    message.Flush();
+  }
 };
 
 }  // namespace log_internal

Diff for: absl/log/log_modifier_methods_test.cc

@@ -15,6 +15,8 @@
 
 #include <errno.h>
 
+#include <string>
+
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 #include "absl/log/internal/test_actions.h"
@@ -77,6 +79,14 @@ TEST(TailCallsModifiesTest, AtLocationFileLine) {
       << "hello world";
 }
 
+TEST(TailCallsModifiesTest, AtLocationFileLineLifetime) {
+  // The macro takes care to not use this temporary after its lifetime.
+  // The only salient expectation is "no sanitizer diagnostics".
+  LOG(INFO).AtLocation(std::string("/my/very/very/very_long_source_file.cc"),
+                       777)
+      << "hello world";
+}
+
 TEST(TailCallsModifiesTest, NoPrefix) {
   absl::ScopedMockLog test_sink(absl::MockLogDefault::kDisallowUnexpected);