From 8f5e43261531ceffb7f93b46bf134b75a55a28bb Mon Sep 17 00:00:00 2001 From: Protobuf Team Bot Date: Wed, 22 Feb 2023 12:45:42 -0800 Subject: [PATCH] Add regression test for invalid inline string code generation. A bug was found during code review that didn't trigger any test failure The fast TDP entry did not have the index in the aux_idx field. PiperOrigin-RevId: 511570831 --- .../protobuf/generated_message_tctable_impl.h | 2 +- src/google/protobuf/message_unittest.inc | 57 ++++++++++++++++++- src/google/protobuf/unittest.proto | 22 +++++-- 3 files changed, 74 insertions(+), 7 deletions(-) diff --git a/src/google/protobuf/generated_message_tctable_impl.h b/src/google/protobuf/generated_message_tctable_impl.h index 1adea2d9f9..13796053f9 100644 --- a/src/google/protobuf/generated_message_tctable_impl.h +++ b/src/google/protobuf/generated_message_tctable_impl.h @@ -271,7 +271,7 @@ inline void AlignFail(std::integral_constant, class PROTOBUF_EXPORT TcParser final { public: template - static constexpr const TcParseTableBase* GetTable() { + static constexpr auto GetTable() -> decltype(&T::_table_.header) { return &T::_table_.header; } diff --git a/src/google/protobuf/message_unittest.inc b/src/google/protobuf/message_unittest.inc index 24e67b5209..8cfe8c1d5a 100644 --- a/src/google/protobuf/message_unittest.inc +++ b/src/google/protobuf/message_unittest.inc @@ -62,11 +62,12 @@ #include "google/protobuf/arena.h" #include "google/protobuf/descriptor.h" #include "google/protobuf/generated_message_reflection.h" -#include "google/protobuf/message.h" +#include "google/protobuf/generated_message_tctable_impl.h" #include "google/protobuf/io/coded_stream.h" #include "google/protobuf/io/io_win32.h" #include "google/protobuf/io/zero_copy_stream.h" #include "google/protobuf/io/zero_copy_stream_impl.h" +#include "google/protobuf/message.h" #include "google/protobuf/test_util2.h" @@ -1684,6 +1685,60 @@ class TestInputStream final : public io::ZeroCopyInputStream { size_t break_pos_; }; +template +static const internal::TcParseTableBase* GetTableIfAvailable(...) { + return nullptr; +} + +template +static const internal::TcParseTableBase* GetTableIfAvailable( + decltype(internal::TcParser::GetTable())) { + return internal::TcParser::GetTable(); +} + +TEST(MESSAGE_TEST_NAME, TestRegressionInlinedStringAuxIdxMismatchOnFastParser) { + using Proto = UNITTEST::InlinedStringIdxRegressionProto; + + auto* table = GetTableIfAvailable(nullptr); + // Only test when TDP is on, and we have these fields inlined. + if (table != nullptr && + table->fast_entry(1)->target() == internal::TcParser::FastSiS1) { + // optional string str1 = 1; + EXPECT_EQ(table->fast_entry(1)->bits.aux_idx(), 1); + // optional InlinedStringIdxRegressionProto sub = 2; + EXPECT_EQ(table->fast_entry(2)->bits.aux_idx(), 2); + // optional string str2 = 3; + // The aux_idx points to the inlined_string_idx and not the actual aux_idx. + EXPECT_EQ(table->fast_entry(3)->bits.aux_idx(), 2); + // optional string str3 = 4; + // The aux_idx points to the inlined_string_idx and not the actual aux_idx. + EXPECT_EQ(table->fast_entry(0)->bits.aux_idx(), 3); + } + + std::string encoded; + { + Proto proto; + // We use strings longer than SSO. + proto.set_str1(std::string(100, 'a')); + proto.set_str2(std::string(100, 'a')); + proto.set_str3(std::string(100, 'a')); + encoded = proto.SerializeAsString(); + } + Arena arena; + auto* proto = Arena::CreateMessage(&arena); + // We don't alter donation here, so it works even if the idx are bad. + ASSERT_TRUE(proto->ParseFromString(encoded)); + // Now we alter donation bits. str2's bit (#2) will be off, but its aux_idx + // (#3) will point to a donated string. + proto = Arena::CreateMessage(&arena); + proto->mutable_str1(); + proto->mutable_str2(); + proto->mutable_str3(); + // With the bug, this breaks the cleanup list, causing UB on arena + // destruction. + ASSERT_TRUE(proto->ParseFromString(encoded)); +} + TEST(MESSAGE_TEST_NAME, TestRepeatedStringParsers) { google::protobuf::Arena arena; diff --git a/src/google/protobuf/unittest.proto b/src/google/protobuf/unittest.proto index 7ebd53f9a2..f6c57deb1b 100644 --- a/src/google/protobuf/unittest.proto +++ b/src/google/protobuf/unittest.proto @@ -1549,7 +1549,7 @@ message EnumParseTester { // An arbitrary field we can append to to break the runs of repeated fields. optional int32 other_field = 99; -}; +} // This message contains different kind of bool fields to exercise the different // parsers in table-drived. @@ -1566,7 +1566,7 @@ message BoolParseTester { // An arbitrary field we can append to to break the runs of repeated fields. optional int32 other_field = 99; -}; +} message Int32ParseTester { optional int32 optional_int32_lowfield = 1; @@ -1581,7 +1581,7 @@ message Int32ParseTester { // An arbitrary field we can append to to break the runs of repeated fields. optional int32 other_field = 99; -}; +} message Int64ParseTester { optional int64 optional_int64_lowfield = 1; @@ -1596,7 +1596,19 @@ message Int64ParseTester { // An arbitrary field we can append to to break the runs of repeated fields. optional int32 other_field = 99; -}; +} + +message InlinedStringIdxRegressionProto { + // We mix data to make sure aux ids and inlined string idx do not match. + // aux_idx == inlined_string_idx == 1 + optional string str1 = 1; + // aux_idx == 2 + optional InlinedStringIdxRegressionProto sub = 2; + // aux_idx == 3, inlined_string_idx == 2 + optional string str2 = 3; + // aux_idx == 4, inlined_string_idx == 3 + optional bytes str3 = 4; +} message StringParseTester { optional string optional_string_lowfield = 1; @@ -1605,7 +1617,7 @@ message StringParseTester { repeated string repeated_string_lowfield = 2; repeated string repeated_string_midfield = 1002; repeated string repeated_string_hifield = 1000002; -}; +} message BadFieldNames{ optional int32 OptionalInt32 = 1;