From b955165ebdcc5a8ba9c267230d6305f4e3d9c118 Mon Sep 17 00:00:00 2001 From: Adam Cozzette Date: Fri, 13 Oct 2023 15:20:54 -0700 Subject: [PATCH] Internal change PiperOrigin-RevId: 573332237 --- .../protobuf/io/test_zero_copy_stream.h | 22 ++++++++++++------- src/google/protobuf/json/BUILD.bazel | 1 + src/google/protobuf/json/internal/parser.cc | 2 +- src/google/protobuf/json/json_test.cc | 20 +++++++++++++++++ 4 files changed, 36 insertions(+), 9 deletions(-) diff --git a/src/google/protobuf/io/test_zero_copy_stream.h b/src/google/protobuf/io/test_zero_copy_stream.h index 4c5a06db400ef..1a56d7038c964 100644 --- a/src/google/protobuf/io/test_zero_copy_stream.h +++ b/src/google/protobuf/io/test_zero_copy_stream.h @@ -9,12 +9,12 @@ #define GOOGLE_PROTOBUF_IO_TEST_ZERO_COPY_STREAM_H__ #include +#include #include #include #include #include "absl/log/absl_check.h" -#include "absl/types/optional.h" #include "google/protobuf/io/zero_copy_stream.h" // Must be included last. @@ -37,18 +37,22 @@ class TestZeroCopyInputStream final : public ZeroCopyInputStream { TestZeroCopyInputStream(const TestZeroCopyInputStream& other) : ZeroCopyInputStream(), buffers_(other.buffers_), - last_returned_buffer_(other.last_returned_buffer_), + last_returned_buffer_( + other.last_returned_buffer_ + ? std::make_unique(*other.last_returned_buffer_) + : nullptr), byte_count_(other.byte_count_) {} bool Next(const void** data, int* size) override { ABSL_CHECK(data) << "data must not be null"; ABSL_CHECK(size) << "size must not be null"; - last_returned_buffer_ = absl::nullopt; + last_returned_buffer_ = nullptr; // We are done if (buffers_.empty()) return false; - last_returned_buffer_ = std::move(buffers_.front()); + last_returned_buffer_ = + std::make_unique(std::move(buffers_.front())); buffers_.pop_front(); *data = last_returned_buffer_->data(); *size = static_cast(last_returned_buffer_->size()); @@ -58,19 +62,19 @@ class TestZeroCopyInputStream final : public ZeroCopyInputStream { void BackUp(int count) override { ABSL_CHECK_GE(count, 0) << "count must not be negative"; - ABSL_CHECK(last_returned_buffer_.has_value()) + ABSL_CHECK(last_returned_buffer_ != nullptr) << "The last call was not a successful Next()"; ABSL_CHECK_LE(count, last_returned_buffer_->size()) << "count must be within bounds of last buffer"; buffers_.push_front( last_returned_buffer_->substr(last_returned_buffer_->size() - count)); - last_returned_buffer_ = absl::nullopt; + last_returned_buffer_ = nullptr; byte_count_ -= count; } bool Skip(int count) override { ABSL_CHECK_GE(count, 0) << "count must not be negative"; - last_returned_buffer_ = absl::nullopt; + last_returned_buffer_ = nullptr; while (true) { if (count == 0) return true; if (buffers_.empty()) return false; @@ -96,7 +100,9 @@ class TestZeroCopyInputStream final : public ZeroCopyInputStream { // move them to `last_returned_buffer_`. It makes it simpler to keep track of // the state of the object. The extra cost is not relevant for testing. std::deque buffers_; - absl::optional last_returned_buffer_; + // absl::optional could work here, but std::unique_ptr makes it more likely + // for sanitizers to detect if the string is used after it is destroyed. + std::unique_ptr last_returned_buffer_; int64_t byte_count_ = 0; }; diff --git a/src/google/protobuf/json/BUILD.bazel b/src/google/protobuf/json/BUILD.bazel index dece74e4d0f00..6ec8184e0e09e 100644 --- a/src/google/protobuf/json/BUILD.bazel +++ b/src/google/protobuf/json/BUILD.bazel @@ -41,6 +41,7 @@ cc_test( "//src/google/protobuf:cc_test_protos", "//src/google/protobuf:port_def", "//src/google/protobuf/io", + "//src/google/protobuf/io:test_zero_copy_stream", "//src/google/protobuf/util:json_format_cc_proto", "//src/google/protobuf/util:json_format_proto3_cc_proto", "//src/google/protobuf/util:type_resolver_util", diff --git a/src/google/protobuf/json/internal/parser.cc b/src/google/protobuf/json/internal/parser.cc index 17e8fcc07c421..fbf492afa7153 100644 --- a/src/google/protobuf/json/internal/parser.cc +++ b/src/google/protobuf/json/internal/parser.cc @@ -1273,7 +1273,7 @@ absl::Status ParseMessage(JsonLexer& lex, const Desc& desc, } } - return ParseField(lex, desc, name.value.AsView(), msg); + return ParseField(lex, desc, name.value.ToString(), msg); }); } } // namespace diff --git a/src/google/protobuf/json/json_test.cc b/src/google/protobuf/json/json_test.cc index 48379ceeb5f9e..2ff1e87a90fe6 100644 --- a/src/google/protobuf/json/json_test.cc +++ b/src/google/protobuf/json/json_test.cc @@ -26,6 +26,7 @@ #include "absl/strings/string_view.h" #include "google/protobuf/descriptor_database.h" #include "google/protobuf/dynamic_message.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/util/json_format.pb.h" @@ -50,6 +51,7 @@ using ::proto3::TestMap; using ::proto3::TestMessage; using ::proto3::TestOneof; using ::proto3::TestWrapper; +using ::testing::ContainsRegex; using ::testing::ElementsAre; using ::testing::IsEmpty; using ::testing::Not; @@ -1331,6 +1333,24 @@ TEST_P(JsonTest, ClearPreExistingRepeatedInJsonValues) { EXPECT_THAT(s.fields(), IsEmpty()); } +TEST(JsonErrorTest, FieldNameAndSyntaxErrorInSeparateChunks) { + std::unique_ptr resolver{ + google::protobuf::util::NewTypeResolverForDescriptorPool( + "type.googleapis.com", DescriptorPool::generated_pool())}; + io::internal::TestZeroCopyInputStream input_stream( + {"{\"bool_value\":", "5}"}); + std::string result; + io::StringOutputStream output_stream(&result); + absl::Status s = JsonToBinaryStream( + resolver.get(), "type.googleapis.com/proto3.TestMessage", &input_stream, + &output_stream, ParseOptions{}); + ASSERT_FALSE(s.ok()); + EXPECT_THAT( + s.message(), + ContainsRegex("invalid *JSON *in *type.googleapis.com/proto3.TestMessage " + "*@ *bool_value")); +} + } // namespace } // namespace json } // namespace protobuf