From e33e0d2ba569c8a7d6d74b5a8b39d804ec2a0169 Mon Sep 17 00:00:00 2001 From: Protobuf Team Bot Date: Fri, 29 Sep 2023 08:55:14 -0700 Subject: [PATCH] Add `ABSL_ATTRIBUTE_LIFETIME_BOUND` attribute on generated oneof accessors. This allows the compiler to statically detect use-after-free bugs. PiperOrigin-RevId: 569504371 --- .../java/com/google/protobuf/DescriptorsTest.java | 4 ++-- python/google/protobuf/internal/field_mask_test.py | 2 +- python/google/protobuf/internal/generator_test.py | 11 +++++++++-- .../compiler/cpp/field_generators/cord_field.cc | 3 ++- src/google/protobuf/generated_message_tctable_gen.cc | 5 +++++ src/google/protobuf/test_util.inc | 9 +++++++++ src/google/protobuf/unittest.proto | 3 +++ src/google/protobuf/util/field_mask_util_test.cc | 2 +- 8 files changed, 32 insertions(+), 7 deletions(-) diff --git a/java/core/src/test/java/com/google/protobuf/DescriptorsTest.java b/java/core/src/test/java/com/google/protobuf/DescriptorsTest.java index 608b175877da5..aae4a4dd13889 100644 --- a/java/core/src/test/java/com/google/protobuf/DescriptorsTest.java +++ b/java/core/src/test/java/com/google/protobuf/DescriptorsTest.java @@ -920,10 +920,10 @@ public void testOneofDescriptor() throws Exception { assertThat(messageType.getOneofs().get(0)).isSameInstanceAs(oneofDescriptor); assertThat(oneofDescriptor.getName()).isEqualTo("oneof_field"); - assertThat(oneofDescriptor.getFieldCount()).isEqualTo(4); + assertThat(oneofDescriptor.getFieldCount()).isEqualTo(7); assertThat(field).isSameInstanceAs(oneofDescriptor.getField(1)); - assertThat(oneofDescriptor.getFields()).hasSize(4); + assertThat(oneofDescriptor.getFields()).hasSize(7); assertThat(field).isEqualTo(oneofDescriptor.getFields().get(1)); } diff --git a/python/google/protobuf/internal/field_mask_test.py b/python/google/protobuf/internal/field_mask_test.py index e728f33d742f0..ed7c9ef60e264 100644 --- a/python/google/protobuf/internal/field_mask_test.py +++ b/python/google/protobuf/internal/field_mask_test.py @@ -53,7 +53,7 @@ def testDescriptorToFieldMask(self): mask = field_mask_pb2.FieldMask() msg_descriptor = unittest_pb2.TestAllTypes.DESCRIPTOR mask.AllFieldsFromDescriptor(msg_descriptor) - self.assertEqual(76, len(mask.paths)) + self.assertEqual(79, len(mask.paths)) self.assertTrue(mask.IsValidForDescriptor(msg_descriptor)) for field in msg_descriptor.fields: self.assertTrue(field.name in mask.paths) diff --git a/python/google/protobuf/internal/generator_test.py b/python/google/protobuf/internal/generator_test.py index b8f82a0035036..d11cb8d033350 100644 --- a/python/google/protobuf/internal/generator_test.py +++ b/python/google/protobuf/internal/generator_test.py @@ -344,8 +344,15 @@ def testOneof(self): self.assertEqual(0, desc.oneofs[0].index) self.assertIs(desc, desc.oneofs[0].containing_type) self.assertIs(desc.oneofs[0], desc.oneofs_by_name['oneof_field']) - nested_names = set(['oneof_uint32', 'oneof_nested_message', - 'oneof_string', 'oneof_bytes']) + nested_names = set([ + 'oneof_uint32', + 'oneof_nested_message', + 'oneof_string', + 'oneof_bytes', + 'oneof_cord', + 'oneof_string_piece', + 'oneof_lazy_nested_message', + ]) self.assertEqual( nested_names, set([field.name for field in desc.oneofs[0].fields])) diff --git a/src/google/protobuf/compiler/cpp/field_generators/cord_field.cc b/src/google/protobuf/compiler/cpp/field_generators/cord_field.cc index 1e7711a5aa0f0..8e5851da2882d 100644 --- a/src/google/protobuf/compiler/cpp/field_generators/cord_field.cc +++ b/src/google/protobuf/compiler/cpp/field_generators/cord_field.cc @@ -353,7 +353,8 @@ void CordOneofFieldGenerator::GenerateInlineAccessorDefinitions( } )cc"); printer->Emit(R"cc( - inline const ::absl::Cord& $classname$::$name$() const { + inline const ::absl::Cord& $classname$::$name$() const + ABSL_ATTRIBUTE_LIFETIME_BOUND { $annotate_get$; // @@protoc_insertion_point(field_get:$full_name$) return _internal_$name$(); diff --git a/src/google/protobuf/generated_message_tctable_gen.cc b/src/google/protobuf/generated_message_tctable_gen.cc index 3531ae5e0a255..7c1736d63ebd2 100644 --- a/src/google/protobuf/generated_message_tctable_gen.cc +++ b/src/google/protobuf/generated_message_tctable_gen.cc @@ -14,6 +14,7 @@ #include #include +#include "absl/log/absl_check.h" #include "absl/strings/str_cat.h" #include "google/protobuf/descriptor.h" #include "google/protobuf/descriptor.pb.h" @@ -731,6 +732,10 @@ TailCallTableInfo::TailCallTableInfo( const OptionProvider& option_provider, const std::vector& has_bit_indices, const std::vector& inlined_string_indices) { + ABSL_DCHECK(std::is_sorted(ordered_fields.begin(), ordered_fields.end(), + [](const auto* lhs, const auto* rhs) { + return lhs->number() < rhs->number(); + })); // If this message has any inlined string fields, store the donation state // offset in the first auxiliary entry, which is kInlinedStringAuxIdx. if (!inlined_string_indices.empty()) { diff --git a/src/google/protobuf/test_util.inc b/src/google/protobuf/test_util.inc index 9d50eba61cae2..77ea70ba2e905 100644 --- a/src/google/protobuf/test_util.inc +++ b/src/google/protobuf/test_util.inc @@ -295,7 +295,16 @@ inline void TestUtil::ModifyRepeatedFields(UNITTEST::TestAllTypes* message) { inline void TestUtil::SetOneofFields(UNITTEST::TestAllTypes* message) { message->set_oneof_uint32(601); message->mutable_oneof_nested_message()->set_bb(602); + message->mutable_oneof_lazy_nested_message()->set_bb(605); message->set_oneof_string("603"); +#ifndef PROTOBUF_TEST_NO_DESCRIPTORS + message->GetReflection()->SetString( + message, message->GetDescriptor()->FindFieldByName("oneof_cord"), "606"); + message->GetReflection()->SetString( + message, message->GetDescriptor()->FindFieldByName("oneof_string_piece"), + "607"); +#endif // !PROTOBUF_TEST_NO_DESCRIPTORS + // Must be last because tests are expecting it. message->set_oneof_bytes("604"); } diff --git a/src/google/protobuf/unittest.proto b/src/google/protobuf/unittest.proto index 218f7c6957e65..d6a64924901ec 100644 --- a/src/google/protobuf/unittest.proto +++ b/src/google/protobuf/unittest.proto @@ -157,6 +157,9 @@ message TestAllTypes { NestedMessage oneof_nested_message = 112; string oneof_string = 113; bytes oneof_bytes = 114; + string oneof_cord = 115 [ctype=CORD]; + string oneof_string_piece = 116 [ctype=STRING_PIECE]; + NestedMessage oneof_lazy_nested_message = 117 [lazy=true]; } } diff --git a/src/google/protobuf/util/field_mask_util_test.cc b/src/google/protobuf/util/field_mask_util_test.cc index edf52dac6526f..16b3bd2fab2f0 100644 --- a/src/google/protobuf/util/field_mask_util_test.cc +++ b/src/google/protobuf/util/field_mask_util_test.cc @@ -202,7 +202,7 @@ TEST(FieldMaskUtilTest, TestGetFieldMaskForAllFields) { EXPECT_TRUE(FieldMaskUtil::IsPathInFieldMask("bb", mask)); mask = FieldMaskUtil::GetFieldMaskForAllFields(); - EXPECT_EQ(76, mask.paths_size()); + EXPECT_EQ(79, mask.paths_size()); EXPECT_TRUE(FieldMaskUtil::IsPathInFieldMask("optional_int32", mask)); EXPECT_TRUE(FieldMaskUtil::IsPathInFieldMask("optional_int64", mask)); EXPECT_TRUE(FieldMaskUtil::IsPathInFieldMask("optional_uint32", mask));