Merge pull request #8670 from haberman/php-wkt-submsg

Fixed SEGV in sub-message getters for well-known types when message is unset.
protocolbuffers · May 27, 2021 · b42f237 · b42f237
2 parents 769826e + 75de6aa
commit b42f237
Show file tree

Hide file tree

Showing 17 changed files with 50 additions and 28 deletions.
diff --git a/php/ext/google/protobuf/message.c b/php/ext/google/protobuf/message.c
@@ -134,6 +134,10 @@ static void Message_get(Message *intern, const upb_fielddef *f, zval *rv) {
     RepeatedField_GetPhpWrapper(rv, msgval.array, TypeInfo_Get(f),
                                 &intern->arena);
   } else {
+    if (upb_fielddef_issubmsg(f) && !upb_msg_has(intern->msg, f)) {
+      ZVAL_NULL(rv);
+      return;
+    }
     upb_msgval msgval = upb_msg_get(intern->msg, f);
     Convert_UpbToPhp(msgval, rv, TypeInfo_Get(f), &intern->arena);
   }
@@ -280,7 +284,6 @@ static int Message_has_property(PROTO_VAL *obj, PROTO_STR *member,
  * Message_unset_property()
  *
  * Object handler for unsetting a property. Called when PHP code calls:
- * does any of:
  *
  *   unset($message->foobar);
  *

diff --git a/php/src/Google/Protobuf/Api.php b/php/src/Google/Protobuf/Api.php
diff --git a/php/src/Google/Protobuf/Enum.php b/php/src/Google/Protobuf/Enum.php
diff --git a/php/src/Google/Protobuf/Internal/DescriptorProto.php b/php/src/Google/Protobuf/Internal/DescriptorProto.php
diff --git a/php/src/Google/Protobuf/Internal/DescriptorProto/ExtensionRange.php b/php/src/Google/Protobuf/Internal/DescriptorProto/ExtensionRange.php
diff --git a/php/src/Google/Protobuf/Internal/EnumDescriptorProto.php b/php/src/Google/Protobuf/Internal/EnumDescriptorProto.php
diff --git a/php/src/Google/Protobuf/Internal/EnumValueDescriptorProto.php b/php/src/Google/Protobuf/Internal/EnumValueDescriptorProto.php
diff --git a/php/src/Google/Protobuf/Internal/FieldDescriptorProto.php b/php/src/Google/Protobuf/Internal/FieldDescriptorProto.php
diff --git a/php/src/Google/Protobuf/Internal/FileDescriptorProto.php b/php/src/Google/Protobuf/Internal/FileDescriptorProto.php
diff --git a/php/src/Google/Protobuf/Internal/MethodDescriptorProto.php b/php/src/Google/Protobuf/Internal/MethodDescriptorProto.php
diff --git a/php/src/Google/Protobuf/Internal/OneofDescriptorProto.php b/php/src/Google/Protobuf/Internal/OneofDescriptorProto.php
diff --git a/php/src/Google/Protobuf/Internal/ServiceDescriptorProto.php b/php/src/Google/Protobuf/Internal/ServiceDescriptorProto.php
diff --git a/php/src/Google/Protobuf/Option.php b/php/src/Google/Protobuf/Option.php
diff --git a/php/src/Google/Protobuf/Type.php b/php/src/Google/Protobuf/Type.php
diff --git a/php/tests/GeneratedClassTest.php b/php/tests/GeneratedClassTest.php
@@ -472,6 +472,8 @@ public function testMessageField()
     {
         $m = new TestMessage();
 
+        $this->assertNull($m->getOptionalMessage());
+
         $sub_m = new Sub();
         $sub_m->setA(1);
         $m->setOptionalMessage($sub_m);

diff --git a/php/tests/WellKnownTest.php b/php/tests/WellKnownTest.php
@@ -270,6 +270,8 @@ public function testStruct()
 
         $m = new Value();
 
+        $this->assertNull($m->getStructValue());
+
         $m->setNullValue(NullValue::NULL_VALUE);
         $this->assertSame(NullValue::NULL_VALUE, $m->getNullValue());
         $this->assertSame("null_value", $m->getKind());

diff --git a/src/google/protobuf/compiler/php/php_generator.cc b/src/google/protobuf/compiler/php/php_generator.cc
@@ -648,32 +648,21 @@ void GenerateFieldAccessor(const FieldDescriptor* field, const Options& options,
   std::string deprecation_trigger = (field->options().deprecated()) ? "@trigger_error('" +
       field->name() + " is deprecated.', E_USER_DEPRECATED);\n        " : "";
 
+  // Emit getter.
   if (oneof != NULL) {
     printer->Print(
         "public function get^camel_name^()\n"
         "{\n"
         "    ^deprecation_trigger^return $this->readOneof(^number^);\n"
-        "}\n\n"
-        "public function has^camel_name^()\n"
-        "{\n"
-        "    ^deprecation_trigger^return $this->hasOneof(^number^);\n"
         "}\n\n",
         "camel_name", UnderscoresToCamelCase(field->name(), true),
         "number", IntToString(field->number()),
         "deprecation_trigger", deprecation_trigger);
-  } else if (field->has_presence()) {
+  } else if (field->has_presence() && !field->message_type()) {
     printer->Print(
         "public function get^camel_name^()\n"
         "{\n"
         "    ^deprecation_trigger^return isset($this->^name^) ? $this->^name^ : ^default_value^;\n"
-        "}\n\n"
-        "public function has^camel_name^()\n"
-        "{\n"
-        "    ^deprecation_trigger^return isset($this->^name^);\n"
-        "}\n\n"
-        "public function clear^camel_name^()\n"
-        "{\n"
-        "    ^deprecation_trigger^unset($this->^name^);\n"
         "}\n\n",
         "camel_name", UnderscoresToCamelCase(field->name(), true),
         "name", field->name(),
@@ -690,6 +679,32 @@ void GenerateFieldAccessor(const FieldDescriptor* field, const Options& options,
         "deprecation_trigger", deprecation_trigger);
   }
 
+  // Emit hazzers/clear.
+  if (oneof) {
+    printer->Print(
+        "public function has^camel_name^()\n"
+        "{\n"
+        "    ^deprecation_trigger^return $this->hasOneof(^number^);\n"
+        "}\n\n",
+        "camel_name", UnderscoresToCamelCase(field->name(), true),
+        "number", IntToString(field->number()),
+        "deprecation_trigger", deprecation_trigger);
+  } else if (field->has_presence()) {
+    printer->Print(
+        "public function has^camel_name^()\n"
+        "{\n"
+        "    ^deprecation_trigger^return isset($this->^name^);\n"
+        "}\n\n"
+        "public function clear^camel_name^()\n"
+        "{\n"
+        "    ^deprecation_trigger^unset($this->^name^);\n"
+        "}\n\n",
+        "camel_name", UnderscoresToCamelCase(field->name(), true),
+        "name", field->name(),
+        "default_value", DefaultForField(field),
+        "deprecation_trigger", deprecation_trigger);
+  }
+
   // For wrapper types, generate an additional getXXXUnwrapped getter
   if (!field->is_map() &&
       !field->is_repeated() &&