Fail building descriptors if ctype is used for fields other than string or bytes.

protobuf-github-bot · mkruskal-google · commit 624d65da6aa8 · 2024-01-29T11:55:18.000-08:00
Instead of silently ignoring `[ctype = XXX]` for non string or bytes fields,
this CL starts failing to build descriptors to call out the issue. This may
cause failures to existing proto schemas but fixing them should be straightforward.

PiperOrigin-RevId: 602441330
diff --git a/src/google/protobuf/descriptor.cc b/src/google/protobuf/descriptor.cc
@@ -7721,6 +7721,29 @@ void DescriptorBuilder::ValidateOptions(const FieldDescriptor* field,
 
   ValidateFieldFeatures(field, proto);
 
+  // The following check is temporarily OSS only till we fix all affected
+  // google3 TAP tests.
+  if (field->options().has_ctype()) {
+    if (field->cpp_type() != FieldDescriptor::CPPTYPE_STRING) {
+      AddError(
+          field->full_name(), proto, DescriptorPool::ErrorCollector::TYPE,
+          absl::StrFormat(
+              "Field %s specifies ctype, but is not a string nor bytes field.",
+              field->full_name())
+              .c_str());
+    }
+    if (field->options().ctype() == FieldOptions::CORD) {
+      if (field->is_extension()) {
+        AddError(field->full_name(), proto,
+                 DescriptorPool::ErrorCollector::TYPE,
+                 absl::StrFormat("Extension %s specifies ctype=CORD which is "
+                                 "not supported for extensions.",
+                                 field->full_name())
+                     .c_str());
+      }
+    }
+  }
+
   // Only message type fields may be lazy.
   if (field->options().lazy() || field->options().unverified_lazy()) {
     if (field->type() != FieldDescriptor::TYPE_MESSAGE) {
diff --git a/src/google/protobuf/descriptor_unittest.cc b/src/google/protobuf/descriptor_unittest.cc
@@ -2943,6 +2943,26 @@ TEST_F(MiscTest, DefaultValues) {
   EXPECT_EQ(enum_value_a, message->field(22)->default_value_enum());
 }
 
+TEST_F(MiscTest, InvalidFieldOptions) {
+  FileDescriptorProto file_proto;
+  file_proto.set_name("foo.proto");
+
+  DescriptorProto* message_proto = AddMessage(&file_proto, "TestMessage");
+  AddField(message_proto, "foo", 1, FieldDescriptorProto::LABEL_OPTIONAL,
+           FieldDescriptorProto::TYPE_INT32);
+  FieldDescriptorProto* bar_proto =
+      AddField(message_proto, "bar", 2, FieldDescriptorProto::LABEL_OPTIONAL,
+               FieldDescriptorProto::TYPE_INT32);
+
+  FieldOptions* options = bar_proto->mutable_options();
+  options->set_ctype(FieldOptions::CORD);
+
+  // Expects it to fail as int32 fields cannot have ctype.
+  DescriptorPool pool;
+  const FileDescriptor* file = pool.BuildFile(file_proto);
+  EXPECT_EQ(file, nullptr);
+}
+
 TEST_F(MiscTest, FieldOptions) {
   // Try setting field options.
 
@@ -2954,7 +2974,7 @@ TEST_F(MiscTest, FieldOptions) {
            FieldDescriptorProto::TYPE_INT32);
   FieldDescriptorProto* bar_proto =
       AddField(message_proto, "bar", 2, FieldDescriptorProto::LABEL_OPTIONAL,
-               FieldDescriptorProto::TYPE_INT32);
+               FieldDescriptorProto::TYPE_BYTES);
 
   FieldOptions* options = bar_proto->mutable_options();
   options->set_ctype(FieldOptions::CORD);
