Zero encoded_remaining when a string field doesn't fit, so that we don't leave…

Zero encoded_remaining when a string field doesn't fit, so that we don't leave partial data in the buffer (all decoders should ignore it anyway) and to be sure that we don't try to put any subsequent operands in either (there shouldn't be enough space).

PiperOrigin-RevId: 490143656
Change-Id: I4d743dd9214013fbd151478ef662d50affd5ff7a

absl/log/BUILD.bazel

@@ -332,6 +332,7 @@ cc_test(
     copts = ABSL_TEST_COPTS,
     linkopts = ABSL_DEFAULT_LINKOPTS,
     deps = [
+        ":check",
         ":log",
         ":scoped_mock_log",
         "//absl/log/internal:test_matchers",

absl/log/CMakeLists.txt

@@ -745,6 +745,7 @@ absl_cc_test(
   LINKOPTS
     ${ABSL_DEFAULT_LINKOPTS}
   DEPS
+    absl::check
     absl::log
     absl::log_internal_test_matchers
     absl::scoped_mock_log

absl/log/internal/log_message.cc

@@ -345,11 +345,13 @@ void LogMessage::FailQuietly() {
 }
 
 LogMessage& LogMessage::operator<<(const std::string& v) {
-  return LogString(false, v);
+  CopyToEncodedBuffer(v, StringType::kNotLiteral);
+  return *this;
 }
 
 LogMessage& LogMessage::operator<<(absl::string_view v) {
-  return LogString(false, v);
+  CopyToEncodedBuffer(v, StringType::kNotLiteral);
+  return *this;
 }
 LogMessage& LogMessage::operator<<(std::ostream& (*m)(std::ostream& os)) {
   OstreamView view(*data_);
@@ -431,20 +433,21 @@ LogMessage::OstreamView::OstreamView(LogMessageData& message_data)
 }
 
 LogMessage::OstreamView::~OstreamView() {
+  data_.manipulated.rdbuf(nullptr);
+  if (!string_start_.data()) {
+    // The second field header didn't fit.  Whether the first one did or not, we
+    // shouldn't commit `encoded_remaining_copy_`, and we also need to zero the
+    // size of `data_->encoded_remaining` so that no more data are encoded.
+    data_.encoded_remaining.remove_suffix(data_.encoded_remaining.size());
+    return;
+  }
   const absl::Span<const char> contents(pbase(),
                                         static_cast<size_t>(pptr() - pbase()));
+  if (contents.empty()) return;
   encoded_remaining_copy_.remove_prefix(contents.size());
-  if (!string_start_.data()) {
-    // The headers didn't fit; we won't write anything to the buffer, but we
-    // also need to zero the size of `data_->encoded_remaining` so that no more
-    // data is encoded.
-    data_.encoded_remaining.remove_suffix(data_.encoded_remaining.size());
-  } else if (!contents.empty()) {
   EncodeMessageLength(string_start_, &encoded_remaining_copy_);
   EncodeMessageLength(message_start_, &encoded_remaining_copy_);
   data_.encoded_remaining = encoded_remaining_copy_;
-  }
-  data_.manipulated.rdbuf(nullptr);
 }
 
 std::ostream& LogMessage::OstreamView::stream() { return data_.manipulated; }
@@ -511,21 +514,31 @@ void LogMessage::LogBacktraceIfNeeded() {
   view.stream() << ") ";
 }
 
-// Encodes a partial `logging.proto.Event` containing the specified string data
-// into `data_->encoded_remaining`.
-LogMessage& LogMessage::LogString(bool literal, absl::string_view str) {
-  // Don't commit the MessageStart if the String tag_type and length don't fit.
+// Encodes into `data_->encoded_remaining` a partial `logging.proto.Event`
+// containing the specified string data using a `Value` field appropriate to
+// `str_type`.  Truncates `str` if necessary, but emits nothing and marks the
+// buffer full if  even the field headers do not fit.
+void LogMessage::CopyToEncodedBuffer(absl::string_view str,
+                                     StringType str_type) {
   auto encoded_remaining_copy = data_->encoded_remaining;
   auto start = EncodeMessageStart(
       EventTag::kValue, BufferSizeFor(WireType::kLengthDelimited) + str.size(),
       &encoded_remaining_copy);
-  if (EncodeStringTruncate(
-          literal ? ValueTag::kStringLiteral : ValueTag::kString, str,
-          &encoded_remaining_copy)) {
+  // If the `logging.proto.Event.value` field header did not fit,
+  // `EncodeMessageStart` will have zeroed `encoded_remaining_copy`'s size and
+  // `EncodeStringTruncate` will fail too.
+  if (EncodeStringTruncate(str_type == StringType::kLiteral
+                               ? ValueTag::kStringLiteral
+                               : ValueTag::kString,
+                           str, &encoded_remaining_copy)) {
+    // The string may have been truncated, but the field header fit.
     EncodeMessageLength(start, &encoded_remaining_copy);
     data_->encoded_remaining = encoded_remaining_copy;
+  } else {
+    // The field header(s) did not fit; zero `encoded_remaining` so we don't
+    // write anything else later.
+    data_->encoded_remaining.remove_suffix(data_->encoded_remaining.size());
   }
-  return *this;
 }
 
 LogMessageFatal::LogMessageFatal(const char* file, int line)

absl/log/internal/log_message.h

@@ -216,6 +216,13 @@ class LogMessage {
     absl::Span<char> string_start_;
   };
 
+  enum class StringType {
+    kLiteral,
+    kNotLiteral,
+  };
+  void CopyToEncodedBuffer(absl::string_view str,
+                           StringType str_type) ABSL_ATTRIBUTE_NOINLINE;
+
   // Returns `true` if the message is fatal or enabled debug-fatal.
   bool IsFatal() const;
 
@@ -228,9 +235,6 @@ class LogMessage {
   // Checks `FLAGS_log_backtrace_at` and appends a backtrace if appropriate.
   void LogBacktraceIfNeeded();
 
-  LogMessage& LogString(bool literal,
-                        absl::string_view str) ABSL_ATTRIBUTE_NOINLINE;
-
   // This should be the first data member so that its initializer captures errno
   // before any other initializers alter it (e.g. with calls to new) and so that
   // no other destructors run afterward an alter it (e.g. with calls to delete).
@@ -282,15 +286,15 @@ LogMessage& LogMessage::operator<<(const T& v) {
 
 template <int SIZE>
 LogMessage& LogMessage::operator<<(const char (&buf)[SIZE]) {
-  const bool literal = true;
-  return LogString(literal, buf);
+  CopyToEncodedBuffer(buf, StringType::kLiteral);
+  return *this;
 }
 
 // Note: the following is declared `ABSL_ATTRIBUTE_NOINLINE`
 template <int SIZE>
 LogMessage& LogMessage::operator<<(char (&buf)[SIZE]) {
-  const bool literal = false;
-  return LogString(literal, buf);
+  CopyToEncodedBuffer(buf, StringType::kNotLiteral);
+  return *this;
 }
 // We instantiate these specializations in the library's TU to save space in
 // other TUs.  Since the template is marked `ABSL_ATTRIBUTE_NOINLINE` we will be

absl/log/internal/proto.cc

@@ -106,7 +106,8 @@ bool EncodeBytesTruncate(uint64_t tag, absl::Span<const char> value,
   const uint64_t tag_type = MakeTagType(tag, WireType::kLengthDelimited);
   const uint64_t tag_type_size = VarintSize(tag_type);
   uint64_t length = value.size();
-  const uint64_t length_size = VarintSize(length);
+  const uint64_t length_size =
+      VarintSize(std::min<uint64_t>(length, buf->size()));
   if (tag_type_size + length_size <= buf->size() &&
       tag_type_size + length_size + value.size() > buf->size()) {
     value.remove_suffix(tag_type_size + length_size + value.size() -

absl/log/internal/structured.h

@@ -41,12 +41,13 @@ class ABSL_MUST_USE_RESULT AsLiteralImpl final {
   friend std::ostream& operator<<(std::ostream& os, AsLiteralImpl as_literal) {
     return os << as_literal.str_;
   }
-  log_internal::LogMessage& AddToMessage(log_internal::LogMessage& m) {
-    return m.LogString(/* literal = */ true, str_);
+  void AddToMessage(log_internal::LogMessage& m) {
+    m.CopyToEncodedBuffer(str_, log_internal::LogMessage::StringType::kLiteral);
   }
   friend log_internal::LogMessage& operator<<(log_internal::LogMessage& m,
                                               AsLiteralImpl as_literal) {
-    return as_literal.AddToMessage(m);
+    as_literal.AddToMessage(m);
+    return m;
   }
 };
 

absl/log/internal/test_actions.cc

@@ -18,6 +18,7 @@
 #include <cassert>
 #include <iostream>
 #include <string>
+#include <type_traits>
 
 #include "absl/base/attributes.h"
 #include "absl/base/config.h"

absl/log/internal/test_matchers.cc

@@ -15,6 +15,7 @@
 
 #include "absl/log/internal/test_matchers.h"
 
+#include <ostream>
 #include <sstream>
 #include <string>
 #include <type_traits>
@@ -32,74 +33,138 @@
 namespace absl {
 ABSL_NAMESPACE_BEGIN
 namespace log_internal {
+namespace {
+using ::testing::_;
+using ::testing::AllOf;
+using ::testing::Ge;
+using ::testing::HasSubstr;
+using ::testing::MakeMatcher;
+using ::testing::Matcher;
+using ::testing::MatcherInterface;
+using ::testing::MatchResultListener;
+using ::testing::Not;
+using ::testing::Property;
+using ::testing::ResultOf;
+using ::testing::Truly;
+
+class AsStringImpl final
+    : public MatcherInterface<absl::string_view> {
+ public:
+  explicit AsStringImpl(
+      const Matcher<const std::string&>& str_matcher)
+      : str_matcher_(str_matcher) {}
+  bool MatchAndExplain(
+      absl::string_view actual,
+      MatchResultListener* listener) const override {
+    return str_matcher_.MatchAndExplain(std::string(actual), listener);
+  }
+  void DescribeTo(std::ostream* os) const override {
+    return str_matcher_.DescribeTo(os);
+  }
+
+  void DescribeNegationTo(std::ostream* os) const override {
+    return str_matcher_.DescribeNegationTo(os);
+  }
+
+ private:
+  const Matcher<const std::string&> str_matcher_;
+};
+
+class MatchesOstreamImpl final
+    : public MatcherInterface<absl::string_view> {
+ public:
+  explicit MatchesOstreamImpl(std::string expected)
+      : expected_(std::move(expected)) {}
+  bool MatchAndExplain(absl::string_view actual,
+                       MatchResultListener*) const override {
+    return actual == expected_;
+  }
+  void DescribeTo(std::ostream* os) const override {
+    *os << "matches the contents of the ostringstream, which are \""
+        << expected_ << "\"";
+  }
+
+  void DescribeNegationTo(std::ostream* os) const override {
+    *os << "does not match the contents of the ostringstream, which are \""
+        << expected_ << "\"";
+  }
 
-::testing::Matcher<const absl::LogEntry&> SourceFilename(
-    const ::testing::Matcher<absl::string_view>& source_filename) {
+ private:
+  const std::string expected_;
+};
+}  // namespace
+
+Matcher<absl::string_view> AsString(
+    const Matcher<const std::string&>& str_matcher) {
+  return MakeMatcher(new AsStringImpl(str_matcher));
+}
+
+Matcher<const absl::LogEntry&> SourceFilename(
+    const Matcher<absl::string_view>& source_filename) {
   return Property("source_filename", &absl::LogEntry::source_filename,
                   source_filename);
 }
 
-::testing::Matcher<const absl::LogEntry&> SourceBasename(
-    const ::testing::Matcher<absl::string_view>& source_basename) {
+Matcher<const absl::LogEntry&> SourceBasename(
+    const Matcher<absl::string_view>& source_basename) {
   return Property("source_basename", &absl::LogEntry::source_basename,
                   source_basename);
 }
 
-::testing::Matcher<const absl::LogEntry&> SourceLine(
-    const ::testing::Matcher<int>& source_line) {
+Matcher<const absl::LogEntry&> SourceLine(
+    const Matcher<int>& source_line) {
   return Property("source_line", &absl::LogEntry::source_line, source_line);
 }
 
-::testing::Matcher<const absl::LogEntry&> Prefix(
-    const ::testing::Matcher<bool>& prefix) {
+Matcher<const absl::LogEntry&> Prefix(
+    const Matcher<bool>& prefix) {
   return Property("prefix", &absl::LogEntry::prefix, prefix);
 }
 
-::testing::Matcher<const absl::LogEntry&> LogSeverity(
-    const ::testing::Matcher<absl::LogSeverity>& log_severity) {
+Matcher<const absl::LogEntry&> LogSeverity(
+    const Matcher<absl::LogSeverity>& log_severity) {
   return Property("log_severity", &absl::LogEntry::log_severity, log_severity);
 }
 
-::testing::Matcher<const absl::LogEntry&> Timestamp(
-    const ::testing::Matcher<absl::Time>& timestamp) {
+Matcher<const absl::LogEntry&> Timestamp(
+    const Matcher<absl::Time>& timestamp) {
   return Property("timestamp", &absl::LogEntry::timestamp, timestamp);
 }
 
-::testing::Matcher<const absl::LogEntry&> TimestampInMatchWindow() {
+Matcher<const absl::LogEntry&> TimestampInMatchWindow() {
   return Property("timestamp", &absl::LogEntry::timestamp,
-                  ::testing::AllOf(::testing::Ge(absl::Now()),
-                                   ::testing::Truly([](absl::Time arg) {
+                  AllOf(Ge(absl::Now()), Truly([](absl::Time arg) {
                           return arg <= absl::Now();
                         })));
 }
 
-::testing::Matcher<const absl::LogEntry&> ThreadID(
-    const ::testing::Matcher<absl::LogEntry::tid_t>& tid) {
+Matcher<const absl::LogEntry&> ThreadID(
+    const Matcher<absl::LogEntry::tid_t>& tid) {
   return Property("tid", &absl::LogEntry::tid, tid);
 }
 
-::testing::Matcher<const absl::LogEntry&> TextMessageWithPrefixAndNewline(
-    const ::testing::Matcher<absl::string_view>&
+Matcher<const absl::LogEntry&> TextMessageWithPrefixAndNewline(
+    const Matcher<absl::string_view>&
         text_message_with_prefix_and_newline) {
   return Property("text_message_with_prefix_and_newline",
                   &absl::LogEntry::text_message_with_prefix_and_newline,
                   text_message_with_prefix_and_newline);
 }
 
-::testing::Matcher<const absl::LogEntry&> TextMessageWithPrefix(
-    const ::testing::Matcher<absl::string_view>& text_message_with_prefix) {
+Matcher<const absl::LogEntry&> TextMessageWithPrefix(
+    const Matcher<absl::string_view>& text_message_with_prefix) {
   return Property("text_message_with_prefix",
                   &absl::LogEntry::text_message_with_prefix,
                   text_message_with_prefix);
 }
 
-::testing::Matcher<const absl::LogEntry&> TextMessage(
-    const ::testing::Matcher<absl::string_view>& text_message) {
+Matcher<const absl::LogEntry&> TextMessage(
+    const Matcher<absl::string_view>& text_message) {
   return Property("text_message", &absl::LogEntry::text_message, text_message);
 }
 
-::testing::Matcher<const absl::LogEntry&> TextPrefix(
-    const ::testing::Matcher<absl::string_view>& text_prefix) {
+Matcher<const absl::LogEntry&> TextPrefix(
+    const Matcher<absl::string_view>& text_prefix) {
   return ResultOf(
       [](const absl::LogEntry& entry) {
         absl::string_view msg = entry.text_message_with_prefix();
@@ -108,42 +173,25 @@ namespace log_internal {
       },
       text_prefix);
 }
+Matcher<const absl::LogEntry&> RawEncodedMessage(
+    const Matcher<absl::string_view>& raw_encoded_message) {
+  return Property("encoded_message", &absl::LogEntry::encoded_message,
+                  raw_encoded_message);
+}
 
-::testing::Matcher<const absl::LogEntry&> Verbosity(
-    const ::testing::Matcher<int>& verbosity) {
+Matcher<const absl::LogEntry&> Verbosity(
+    const Matcher<int>& verbosity) {
   return Property("verbosity", &absl::LogEntry::verbosity, verbosity);
 }
 
-::testing::Matcher<const absl::LogEntry&> Stacktrace(
-    const ::testing::Matcher<absl::string_view>& stacktrace) {
+Matcher<const absl::LogEntry&> Stacktrace(
+    const Matcher<absl::string_view>& stacktrace) {
   return Property("stacktrace", &absl::LogEntry::stacktrace, stacktrace);
 }
 
-class MatchesOstreamImpl final
-    : public ::testing::MatcherInterface<absl::string_view> {
- public:
-  explicit MatchesOstreamImpl(std::string expected)
-      : expected_(std::move(expected)) {}
-  bool MatchAndExplain(absl::string_view actual,
-                       ::testing::MatchResultListener*) const override {
-    return actual == expected_;
-  }
-  void DescribeTo(std::ostream* os) const override {
-    *os << "matches the contents of the ostringstream, which are \""
-        << expected_ << "\"";
-  }
-
-  void DescribeNegationTo(std::ostream* os) const override {
-    *os << "does not match the contents of the ostringstream, which are \""
-        << expected_ << "\"";
-  }
-
- private:
-  const std::string expected_;
-};
-::testing::Matcher<absl::string_view> MatchesOstream(
+Matcher<absl::string_view> MatchesOstream(
     const std::ostringstream& stream) {
-  return ::testing::MakeMatcher(new MatchesOstreamImpl(stream.str()));
+  return MakeMatcher(new MatchesOstreamImpl(stream.str()));
 }
 
 // We need to validate what is and isn't logged as the process dies due to
@@ -152,16 +200,16 @@ class MatchesOstreamImpl final
 // Instead, we use the mock actions `DeathTestExpectedLogging` and
 // `DeathTestUnexpectedLogging` to write specific phrases to `stderr` that we
 // can validate in the parent process using this matcher.
-::testing::Matcher<const std::string&> DeathTestValidateExpectations() {
+Matcher<const std::string&> DeathTestValidateExpectations() {
   if (log_internal::LoggingEnabledAt(absl::LogSeverity::kFatal)) {
-    return ::testing::Matcher<const std::string&>(::testing::AllOf(
-        ::testing::HasSubstr("Mock received expected entry"),
-        Not(::testing::HasSubstr("Mock received unexpected entry"))));
+    return Matcher<const std::string&>(
+        AllOf(HasSubstr("Mock received expected entry"),
+              Not(HasSubstr("Mock received unexpected entry"))));
   }
   // If `FATAL` logging is disabled, neither message should have been written.
-  return ::testing::Matcher<const std::string&>(::testing::AllOf(
-      Not(::testing::HasSubstr("Mock received expected entry")),
-      Not(::testing::HasSubstr("Mock received unexpected entry"))));
+  return Matcher<const std::string&>(
+      AllOf(Not(HasSubstr("Mock received expected entry")),
+            Not(HasSubstr("Mock received unexpected entry"))));
 }
 
 }  // namespace log_internal

absl/log/internal/test_matchers.h

@@ -38,6 +38,10 @@
 namespace absl {
 ABSL_NAMESPACE_BEGIN
 namespace log_internal {
+// In some configurations, Googletest's string matchers (e.g.
+// `::testing::EndsWith`) need help to match `absl::string_view`.
+::testing::Matcher<absl::string_view> AsString(
+    const ::testing::Matcher<const std::string&>& str_matcher);
 
 // These matchers correspond to the components of `absl::LogEntry`.
 ::testing::Matcher<const absl::LogEntry&> SourceFilename(
@@ -79,6 +83,8 @@ namespace log_internal {
     const std::ostringstream& stream);
 ::testing::Matcher<const std::string&> DeathTestValidateExpectations();
 
+::testing::Matcher<const absl::LogEntry&> RawEncodedMessage(
+    const ::testing::Matcher<absl::string_view>& raw_encoded_message);
 #define ENCODED_MESSAGE(message_matcher) ::testing::_
 
 }  // namespace log_internal