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
diff --git a/absl/log/CMakeLists.txt b/absl/log/CMakeLists.txt
index a511f03..01e2b28 100644
--- a/absl/log/CMakeLists.txt
+++ b/absl/log/CMakeLists.txt
@@ -379,6 +379,7 @@
${ABSL_DEFAULT_LINKOPTS}
DEPS
absl::config
+ absl::core_headers
)
absl_cc_library(
diff --git a/absl/log/internal/BUILD.bazel b/absl/log/internal/BUILD.bazel
index 594f867..d3f4a8f 100644
--- a/absl/log/internal/BUILD.bazel
+++ b/absl/log/internal/BUILD.bazel
@@ -415,7 +415,10 @@
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 --git a/absl/log/internal/conditions.h b/absl/log/internal/conditions.h
index 9dc15db..97edf65 100644
--- a/absl/log/internal/conditions.h
+++ b/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 --git a/absl/log/internal/log_message.cc b/absl/log/internal/log_message.cc
index 9e7722d..8dc5722 100644
--- a/absl/log/internal/log_message.cc
+++ b/absl/log/internal/log_message.cc
@@ -291,16 +291,8 @@
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() {
- Flush();
FailWithoutStackTrace();
}
@@ -700,7 +691,6 @@
: LogMessage(file, line, absl::LogSeverity::kFatal) {}
LogMessageDebugFatal::~LogMessageDebugFatal() {
- Flush();
FailWithoutStackTrace();
}
@@ -711,7 +701,6 @@
}
LogMessageQuietlyDebugFatal::~LogMessageQuietlyDebugFatal() {
- Flush();
FailQuietly();
}
@@ -729,7 +718,6 @@
}
LogMessageQuietlyFatal::~LogMessageQuietlyFatal() {
- Flush();
FailQuietly();
}
#if defined(_MSC_VER) && !defined(__clang__)
diff --git a/absl/log/internal/log_message.h b/absl/log/internal/log_message.h
index 4986e7e..8acc4a1 100644
--- a/absl/log/internal/log_message.h
+++ b/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 @@
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 @@
// 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 --git a/absl/log/internal/nullstream.h b/absl/log/internal/nullstream.h
index 973e91a..c87f9aa 100644
--- a/absl/log/internal/nullstream.h
+++ b/absl/log/internal/nullstream.h
@@ -79,6 +79,7 @@
return *this;
}
NullStream& InternalStream() { return *this; }
+ void Flush() {}
};
template <typename T>
inline NullStream& operator<<(NullStream& str, const T&) {
diff --git a/absl/log/internal/voidify.h b/absl/log/internal/voidify.h
index 8f62da2..f42859e 100644
--- a/absl/log/internal/voidify.h
+++ b/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 @@
// 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 --git a/absl/log/log_modifier_methods_test.cc b/absl/log/log_modifier_methods_test.cc
index fc98e1f..4cee0c0 100644
--- a/absl/log/log_modifier_methods_test.cc
+++ b/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 @@
<< "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);
