Fixed a missing check in wire format verification.

Also added some extra DCHECKs to more easily catch issues like this in the future.

PiperOrigin-RevId: 688347347

Author: haberman

src/google/protobuf/compiler/cpp/file_unittest.cc

@@ -7,13 +7,13 @@
 
 #include "google/protobuf/compiler/cpp/file.h"
 
-#include <algorithm>
-#include <cstddef>
+#include <string>
 #include <vector>
 
+#include <gmock/gmock.h>
 #include <gtest/gtest.h>
-#include "absl/strings/match.h"
 #include "absl/strings/string_view.h"
+#include "absl/strings/strip.h"
 #include "google/protobuf/descriptor.h"
 #include "google/protobuf/unittest.pb.h"
 
@@ -94,6 +94,7 @@ TEST(FileTest, TopologicallyOrderedDescriptors) {
       "TestExtensionInsideTable",
       "TestEmptyMessageWithExtensions",
       "TestEmptyMessage",
+      "TestEagerlyVerifiedLazyMessage.LazyMessage",
       "TestDynamicExtensions.DynamicMessageType",
       "TestDupFieldNumber.Foo",
       "TestDupFieldNumber.Bar",
@@ -149,6 +150,7 @@ TEST(FileTest, TopologicallyOrderedDescriptors) {
       "TestGroup",
       "TestForeignNested",
       "TestFieldOrderings",
+      "TestEagerlyVerifiedLazyMessage",
       "TestEagerMaybeLazy.NestedMessage",
       "TestDynamicExtensions",
       "TestDupFieldNumber",
@@ -196,23 +198,14 @@ TEST(FileTest, TopologicallyOrderedDescriptors) {
       "TestLazyMessageRepeated",
       "TestNestedRequiredForeign",
   };
-  static constexpr size_t kExpectedDescriptorCount =
-      std::end(kExpectedDescriptorOrder) - std::begin(kExpectedDescriptorOrder);
-  std::vector<const Descriptor*> actual_descriptor_order =
-      FileGeneratorFriendForTesting::MessagesInTopologicalOrder(fgen);
-  EXPECT_TRUE(kExpectedDescriptorCount == actual_descriptor_order.size())
-      << "Expected: " << kExpectedDescriptorCount
-      << ", got: " << actual_descriptor_order.size();
-
-  auto limit =
-      std::min(kExpectedDescriptorCount, actual_descriptor_order.size());
-  for (auto i = 0u; i < limit; ++i) {
-    const Descriptor* desc = actual_descriptor_order[i];
-    bool match = absl::EndsWith(desc->full_name(), kExpectedDescriptorOrder[i]);
-    EXPECT_TRUE(match) << "failed to match; expected "
-                       << kExpectedDescriptorOrder[i] << ", got "
-                       << desc->full_name();
+  std::vector<std::string> actual_order;
+  for (const Descriptor* desc :
+       FileGeneratorFriendForTesting::MessagesInTopologicalOrder(fgen)) {
+    actual_order.emplace_back(
+        absl::StripPrefix(desc->full_name(), "protobuf_unittest."));
   }
+  EXPECT_THAT(actual_order,
+              ::testing::ElementsAreArray(kExpectedDescriptorOrder));
 }
 
 }  // namespace

src/google/protobuf/edition_unittest.proto

@@ -1177,6 +1177,14 @@ message TestMessageSize {
   int64 m6 = 6;
 }
 
+// Tests eager verification of a lazy message field.
+message TestEagerlyVerifiedLazyMessage {
+  message LazyMessage {
+    bytes bytes_field = 1;
+  }
+  LazyMessage lazy_message = 1 [lazy = true];
+}
+
 // Test that RPC services work.
 message FooRequest  {}
 message FooResponse {}

src/google/protobuf/message_unittest.inc

@@ -51,6 +51,7 @@
 #include "google/protobuf/message.h"
 #include "google/protobuf/reflection_ops.h"
 #include "google/protobuf/test_util2.h"
+#include "google/protobuf/wire_format_lite.h"
 
 
 // Must be included last.
@@ -364,6 +365,29 @@ TEST(MESSAGE_TEST_NAME, ExplicitLazyExceedRecursionLimit) {
   EXPECT_NE(parsed.lazy_child().child().payload().optional_int32(), -1);
 }
 
+TEST(MESSAGE_TEST_NAME, ExplicitLazyBadLengthDelimitedSize) {
+  std::string serialized;
+
+  // This is a regression test for a bug in lazy field verification.  It
+  // requires invalid wire format to trigger the bug.
+
+  // NestedMessage optional_lazy_message = 27 [lazy=true];
+  uint32_t tag = internal::WireFormatLite::MakeTag(
+      1, internal::WireFormatLite::WIRETYPE_LENGTH_DELIMITED);
+  ASSERT_LT(tag, INT8_MAX);
+  serialized.push_back(tag);
+  serialized.push_back(6);
+
+  // bytes bytes_field = 1;
+  serialized.push_back(tag);
+
+  // To trigger this bug, we need an overlong size.
+  serialized.append(5, 0xff);
+
+  UNITTEST::TestLazyMessage parsed;
+  EXPECT_FALSE(parsed.ParseFromString(serialized));
+}
+
 TEST(MESSAGE_TEST_NAME, NestedLazyRecursionLimit) {
   UNITTEST::NestedTestAllTypes original, parsed;
   original.mutable_lazy_child()

src/google/protobuf/parse_context.h

@@ -8,13 +8,18 @@
 #ifndef GOOGLE_PROTOBUF_PARSE_CONTEXT_H__
 #define GOOGLE_PROTOBUF_PARSE_CONTEXT_H__
 
+#include <algorithm>
+#include <climits>
+#include <cstddef>
 #include <cstdint>
 #include <cstring>
+#include <limits>
 #include <string>
 #include <type_traits>
 #include <utility>
 
 #include "absl/base/config.h"
+#include "absl/base/prefetch.h"
 #include "absl/log/absl_check.h"
 #include "absl/log/absl_log.h"
 #include "absl/strings/cord.h"
@@ -27,9 +32,11 @@
 #include "google/protobuf/inlined_string_field.h"
 #include "google/protobuf/io/coded_stream.h"
 #include "google/protobuf/io/zero_copy_stream.h"
+#include "google/protobuf/message_lite.h"
 #include "google/protobuf/metadata_lite.h"
 #include "google/protobuf/port.h"
 #include "google/protobuf/repeated_field.h"
+#include "google/protobuf/repeated_ptr_field.h"
 #include "google/protobuf/wire_format_lite.h"
 
 
@@ -103,7 +110,7 @@ class PROTOBUF_EXPORT EpsCopyInputStream {
     ABSL_DCHECK(ptr <= buffer_end_ + kSlopBytes);
     int count;
     if (next_chunk_ == patch_buffer_) {
-      count = static_cast<int>(buffer_end_ + kSlopBytes - ptr);
+      count = BytesAvailable(ptr);
     } else {
       count = size_ + static_cast<int>(buffer_end_ - ptr);
     }
@@ -170,14 +177,14 @@ class PROTOBUF_EXPORT EpsCopyInputStream {
   }
 
   PROTOBUF_NODISCARD const char* Skip(const char* ptr, int size) {
-    if (size <= buffer_end_ + kSlopBytes - ptr) {
+    if (size <= BytesAvailable(ptr)) {
       return ptr + size;
     }
     return SkipFallback(ptr, size);
   }
   PROTOBUF_NODISCARD const char* ReadString(const char* ptr, int size,
                                             std::string* s) {
-    if (size <= buffer_end_ + kSlopBytes - ptr) {
+    if (size <= BytesAvailable(ptr)) {
       // Fundamentally we just want to do assign to the string.
       // However micro-benchmarks regress on string reading cases. So we copy
       // the same logic from the old CodedInputStream ReadString. Note: as of
@@ -191,7 +198,7 @@ class PROTOBUF_EXPORT EpsCopyInputStream {
   }
   PROTOBUF_NODISCARD const char* AppendString(const char* ptr, int size,
                                               std::string* s) {
-    if (size <= buffer_end_ + kSlopBytes - ptr) {
+    if (size <= BytesAvailable(ptr)) {
       s->append(ptr, size);
       return ptr + size;
     }
@@ -204,8 +211,7 @@ class PROTOBUF_EXPORT EpsCopyInputStream {
 
   PROTOBUF_NODISCARD const char* ReadCord(const char* ptr, int size,
                                           ::absl::Cord* cord) {
-    if (size <= std::min<int>(static_cast<int>(buffer_end_ + kSlopBytes - ptr),
-                              kMaxCordBytesToCopy)) {
+    if (size <= std::min<int>(BytesAvailable(ptr), kMaxCordBytesToCopy)) {
       *cord = absl::string_view(ptr, size);
       return ptr + size;
     }
@@ -345,6 +351,14 @@ class PROTOBUF_EXPORT EpsCopyInputStream {
   // systems. TODO do we need to set this as build flag?
   enum { kSafeStringSize = 50000000 };
 
+  int BytesAvailable(const char* ptr) const {
+    ABSL_DCHECK_NE(ptr, nullptr);
+    ptrdiff_t available = buffer_end_ + kSlopBytes - ptr;
+    ABSL_DCHECK_GE(available, 0);
+    ABSL_DCHECK_LE(available, INT_MAX);
+    return static_cast<int>(available);
+  }
+
   // Advances to next buffer chunk returns a pointer to the same logical place
   // in the stream as set by overrun. Overrun indicates the position in the slop
   // region the parse was left (0 <= overrun <= kSlopBytes). Returns true if at
@@ -382,7 +396,7 @@ class PROTOBUF_EXPORT EpsCopyInputStream {
 
   template <typename A>
   const char* AppendSize(const char* ptr, int size, const A& append) {
-    int chunk_size = static_cast<int>(buffer_end_ + kSlopBytes - ptr);
+    int chunk_size = BytesAvailable(ptr);
     do {
       ABSL_DCHECK(size > chunk_size);
       if (next_chunk_ == nullptr) return nullptr;
@@ -396,7 +410,7 @@ class PROTOBUF_EXPORT EpsCopyInputStream {
       ptr = Next();
       if (ptr == nullptr) return nullptr;  // passed the limit
       ptr += kSlopBytes;
-      chunk_size = static_cast<int>(buffer_end_ + kSlopBytes - ptr);
+      chunk_size = BytesAvailable(ptr);
     } while (size > chunk_size);
     append(ptr, size);
     return ptr + size;
@@ -411,7 +425,7 @@ class PROTOBUF_EXPORT EpsCopyInputStream {
   const char* AppendUntilEnd(const char* ptr, const A& append) {
     if (ptr - buffer_end_ > limit_) return nullptr;
     while (limit_ > kSlopBytes) {
-      size_t chunk_size = buffer_end_ + kSlopBytes - ptr;
+      size_t chunk_size = BytesAvailable(ptr);
       append(ptr, chunk_size);
       ptr = Next();
       if (ptr == nullptr) return limit_end_;
@@ -1163,7 +1177,7 @@ template <typename T>
 const char* EpsCopyInputStream::ReadPackedFixed(const char* ptr, int size,
                                                 RepeatedField<T>* out) {
   GOOGLE_PROTOBUF_PARSER_ASSERT(ptr);
-  int nbytes = static_cast<int>(buffer_end_ + kSlopBytes - ptr);
+  int nbytes = BytesAvailable(ptr);
   while (size > nbytes) {
     int num = nbytes / sizeof(T);
     int old_entries = out->size();
@@ -1181,7 +1195,7 @@ const char* EpsCopyInputStream::ReadPackedFixed(const char* ptr, int size,
     ptr = Next();
     if (ptr == nullptr) return nullptr;
     ptr += kSlopBytes - (nbytes - block_size);
-    nbytes = static_cast<int>(buffer_end_ + kSlopBytes - ptr);
+    nbytes = BytesAvailable(ptr);
   }
   int num = size / sizeof(T);
   int block_size = num * sizeof(T);

src/google/protobuf/unittest.proto

@@ -1725,6 +1725,14 @@ message OpenEnumMessage {
   repeated ForeignEnum repeated_closed = 4;
 }
 
+// Tests eager verification of a lazy message field.
+message TestEagerlyVerifiedLazyMessage {
+  message LazyMessage {
+    bytes bytes_field = 1;
+  }
+  LazyMessage lazy_message = 1 [lazy = true];
+}
+
 // Test that RPC services work.
 message FooRequest {
 }