Improve fuzz testing of C++ JSON parser and serializer

PiperOrigin-RevId: 573826612

src/google/protobuf/io/test_zero_copy_stream.h

@@ -9,6 +9,7 @@
 #define GOOGLE_PROTOBUF_IO_TEST_ZERO_COPY_STREAM_H__
 
 #include <deque>
+#include <initializer_list>
 #include <memory>
 #include <string>
 #include <utility>
@@ -30,6 +31,8 @@ namespace internal {
 class TestZeroCopyInputStream final : public ZeroCopyInputStream {
  public:
   // The input stream will provide the buffers exactly as passed here.
+  TestZeroCopyInputStream(std::initializer_list<std::string> buffers)
+      : buffers_(buffers.begin(), buffers.end()) {}
   explicit TestZeroCopyInputStream(const std::vector<std::string>& buffers)
       : buffers_(buffers.begin(), buffers.end()) {}
 

src/google/protobuf/json/BUILD.bazel

@@ -54,19 +54,6 @@ cc_test(
     ],
 )
 
-cc_library(
-    name = "test_input_stream",
-    testonly = True,
-    hdrs = ["internal/test_input_stream.h"],
-    copts = COPTS,
-    strip_include_prefix = "/src",
-    visibility = ["//visibility:private"],
-    deps = [
-        "//src/google/protobuf/io",
-        "@com_google_absl//absl/strings",
-    ],
-)
-
 cc_library(
     name = "zero_copy_buffered_stream",
     srcs = ["internal/zero_copy_buffered_stream.cc"],
@@ -89,8 +76,8 @@ cc_test(
     srcs = ["internal/zero_copy_buffered_stream_test.cc"],
     copts = COPTS,
     deps = [
-        ":test_input_stream",
         ":zero_copy_buffered_stream",
+        "//src/google/protobuf/io:test_zero_copy_stream",
         "@com_google_absl//absl/strings",
         "@com_google_googletest//:gtest",
         "@com_google_googletest//:gtest_main",
@@ -147,8 +134,8 @@ cc_test(
     copts = COPTS,
     deps = [
         ":lexer",
-        ":test_input_stream",
         "//src/google/protobuf/io",
+        "//src/google/protobuf/io:test_zero_copy_stream",
         "@com_google_absl//absl/algorithm:container",
         "@com_google_absl//absl/status",
         "@com_google_absl//absl/status:statusor",

src/google/protobuf/json/internal/lexer_test.cc

@@ -27,9 +27,9 @@
 #include "absl/strings/str_replace.h"
 #include "absl/strings/string_view.h"
 #include "absl/types/variant.h"
+#include "google/protobuf/io/test_zero_copy_stream.h"
 #include "google/protobuf/io/zero_copy_stream.h"
 #include "google/protobuf/io/zero_copy_stream_impl_lite.h"
-#include "google/protobuf/json/internal/test_input_stream.h"
 #include "google/protobuf/stubs/status_macros.h"
 
 // Must be included last.
@@ -197,22 +197,19 @@ void Do(absl::string_view json,
       std::string second(json.substr(i, j));
       std::string third(json.substr(i + j));
 
-      TestInputStream in = {first, second, third};
+      io::internal::TestZeroCopyInputStream in{first, second, third};
       test(&in);
       if (testing::Test::HasFailure()) {
         return;
       }
 
       if (verify_all_consumed) {
-        if (!absl::c_all_of(third,
-                            [](char c) { return absl::ascii_isspace(c); })) {
-          ASSERT_GE(in.Consumed(), 3);
-        } else if (!absl::c_all_of(
-                       second, [](char c) { return absl::ascii_isspace(c); })) {
-          ASSERT_GE(in.Consumed(), 2);
-        } else {
-          ASSERT_GE(in.Consumed(), 1);
-        }
+        // Check that any unread bytes are just whitespace.
+        int64_t byte_count = in.ByteCount();
+        ASSERT_LE(byte_count, json.size());
+        EXPECT_TRUE(
+            absl::c_all_of(json.substr(byte_count, json.size() - byte_count),
+                           [](char c) { return absl::ascii_isspace(c); }));
       }
     }
   }

src/google/protobuf/json/internal/test_input_stream.h

@@ -1,82 +0,0 @@
-// Protocol Buffers - Google's data interchange format
-// Copyright 2008 Google Inc.  All rights reserved.
-//
-// Use of this source code is governed by a BSD-style
-// license that can be found in the LICENSE file or at
-// https://developers.google.com/open-source/licenses/bsd
-
-#ifndef GOOGLE_PROTOBUF_UTIL_TEST_INPUT_STREAM_H__
-#define GOOGLE_PROTOBUF_UTIL_TEST_INPUT_STREAM_H__
-
-#include <initializer_list>
-#include <string>
-#include <utility>
-#include <vector>
-
-#include "absl/log/absl_check.h"
-#include "absl/strings/string_view.h"
-#include "google/protobuf/io/zero_copy_stream.h"
-#include "google/protobuf/stubs/status_macros.h"
-
-namespace google {
-namespace protobuf {
-namespace json_internal {
-// A ZeroCopyInputStream for writing unit tests.
-class TestInputStream final : public io::ZeroCopyInputStream {
- public:
-  TestInputStream(std::initializer_list<std::string> strings)
-      : strings_(strings) {}
-  explicit TestInputStream(std::vector<std::string> strings)
-      : strings_(std::move(strings)) {}
-  ~TestInputStream() override = default;
-
-  size_t Consumed() const { return next_; }
-
-  bool Next(const void** data, int* size) override {
-    if (next_ == strings_.size()) {
-      return false;
-    }
-
-    if (next_ > 0) {
-      // Destroy the previous string so that ASAN can catch misbehavior
-      // correctly.
-      ReconstructAt(&strings_[next_ - 1]);
-    }
-
-    absl::string_view next = strings_[next_++];
-    *data = next.data();
-    *size = static_cast<int>(next.size());
-    return true;
-  }
-
-  // TestInputStream currently does not support these members.
-  void BackUp(int) override { ABSL_CHECK(false); }
-  bool Skip(int) override {
-    ABSL_CHECK(false);
-    return false;
-  }
-  int64_t ByteCount() const override {
-    ABSL_CHECK(false);
-    return 0;
-  }
-
- private:
-  // Some versions of Clang can't figure out that
-  //   x.std::string::~string()
-  // is valid syntax, so we indirect through a type param, instead.
-  //
-  // Of course, our luck has it that std::destroy_at is a C++17 feature. :)
-  template <typename T>
-  static void ReconstructAt(T* p) {
-    p->~T();
-    new (p) T;
-  }
-
-  std::vector<std::string> strings_;
-  size_t next_ = 0;
-};
-}  // namespace json_internal
-}  // namespace protobuf
-}  // namespace google
-
-#endif  // GOOGLE_PROTOBUF_UTIL_TEST_INPUT_STREAM_H__

src/google/protobuf/json/internal/zero_copy_buffered_stream.cc

@@ -74,6 +74,14 @@ void ZeroCopyBufferedStream::DownRefBuffer() {
     return;
   }
 
+  // If we have hit EOF then that means we might be buffering one or more
+  // chunks of data that we have not yet logically advanced through. We need to
+  // leave the buffer in place to ensure that we do not inadvertently drop such
+  // chunks.
+  if (eof_) {
+    return;
+  }
+
   // The "virtual length" is the size of the buffer cursor_ indexes into, which
   // is bigger than buf_.
   size_t virtual_buf_len = buf_.size() + buffer_start_;

src/google/protobuf/json/internal/zero_copy_buffered_stream_test.cc

@@ -14,7 +14,7 @@
 #include <gmock/gmock.h>
 #include <gtest/gtest.h>
 #include "absl/strings/string_view.h"
-#include "google/protobuf/json/internal/test_input_stream.h"
+#include "google/protobuf/io/test_zero_copy_stream.h"
 #include "google/protobuf/stubs/status_macros.h"
 
 namespace google {
@@ -46,7 +46,7 @@ MATCHER_P(StatusIs, status,
 #define ASSERT_OK(x) ASSERT_THAT(x, StatusIs(absl::StatusCode::kOk))
 
 TEST(ZcBufferTest, ReadUnbuffered) {
-  TestInputStream in{"foo", "bar", "baz"};
+  io::internal::TestZeroCopyInputStream in{"foo", "bar", "baz"};
   ZeroCopyBufferedStream stream(&in);
 
   {
@@ -69,7 +69,7 @@ TEST(ZcBufferTest, ReadUnbuffered) {
 }
 
 TEST(ZcBufferTest, ReadBuffered) {
-  TestInputStream in{"foo", "bar", "baz"};
+  io::internal::TestZeroCopyInputStream in{"foo", "bar", "baz"};
   ZeroCopyBufferedStream stream(&in);
 
   {
@@ -86,7 +86,7 @@ TEST(ZcBufferTest, ReadBuffered) {
 }
 
 TEST(ZcBufferTest, HoldAcrossSeam) {
-  TestInputStream in{"foo", "bar", "baz"};
+  io::internal::TestZeroCopyInputStream in{"foo", "bar", "baz"};
   ZeroCopyBufferedStream stream(&in);
 
   auto chunk = stream.Take(3);
@@ -100,7 +100,7 @@ TEST(ZcBufferTest, HoldAcrossSeam) {
 }
 
 TEST(ZcBufferTest, BufferAcrossSeam) {
-  TestInputStream in{"foo", "bar", "baz"};
+  io::internal::TestZeroCopyInputStream in{"foo", "bar", "baz"};
   ZeroCopyBufferedStream stream(&in);
 
   auto chunk = stream.Take(2);
@@ -113,8 +113,24 @@ TEST(ZcBufferTest, BufferAcrossSeam) {
   EXPECT_THAT(chunk, IsOkAndHolds("fo"));
 }
 
+TEST(ZcBufferTest, TakeEof) {
+  io::internal::TestZeroCopyInputStream in{"foo", "bar"};
+  ZeroCopyBufferedStream stream(&in);
+
+  // This should fail since there are not enough bytes available.
+  auto chunk = stream.Take(7);
+  EXPECT_FALSE(chunk.ok());
+  EXPECT_TRUE(stream.IsBuffering());
+
+  // Subsequent calls to Take() should still succeed.
+  auto chunk2 = stream.Take(2);
+  auto chunk3 = stream.Take(4);
+  EXPECT_THAT(chunk2, IsOkAndHolds("fo"));
+  EXPECT_THAT(chunk3, IsOkAndHolds("obar"));
+}
+
 TEST(ZcBufferTest, MarkUnbuffered) {
-  TestInputStream in{"foo", "bar", "baz"};
+  io::internal::TestZeroCopyInputStream in{"foo", "bar", "baz"};
   ZeroCopyBufferedStream stream(&in);
 
   ASSERT_OK(stream.Advance(1));
@@ -125,7 +141,7 @@ TEST(ZcBufferTest, MarkUnbuffered) {
 }
 
 TEST(ZcBufferTest, MarkBuffered) {
-  TestInputStream in{"foo", "bar", "baz"};
+  io::internal::TestZeroCopyInputStream in{"foo", "bar", "baz"};
   ZeroCopyBufferedStream stream(&in);
 
   ASSERT_OK(stream.Advance(1));