Merge pull request protocolbuffers#9995 from haberman/php-segv-fix

Fixed PHP SEGV by not writing to shared memory for zend_class_entry.
tomerv · May 19, 2022 · fd3b5a3 · fd3b5a3
2 parents 171a6b1 + 8a46882
commit fd3b5a3
Show file tree

Hide file tree

Showing 3 changed files with 20 additions and 5 deletions.
diff --git a/php/ext/google/protobuf/message.c b/php/ext/google/protobuf/message.c
@@ -601,12 +601,11 @@ PHP_METHOD(Message, __construct) {
   //
   // However, if the user created their own class derived from Message, this
   // will trigger an infinite construction loop and blow the stack.  We
-  // temporarily clear create_object to break this loop (see check in
+  // store this `ce` in a global variable to break the cycle (see the check in
   // NameMap_GetMessage()).
-  PBPHP_ASSERT(ce->create_object == Message_create);
-  ce->create_object = NULL;
+  NameMap_EnterConstructor(ce);
   desc = Descriptor_GetFromClassEntry(ce);
-  ce->create_object = Message_create;
+  NameMap_ExitConstructor(ce);
 
   if (!desc) {
     zend_throw_exception_ex(

diff --git a/php/ext/google/protobuf/protobuf.c b/php/ext/google/protobuf/protobuf.c
@@ -56,6 +56,9 @@ ZEND_BEGIN_MODULE_GLOBALS(protobuf)
   // Set by the user to make the descriptor pool persist between requests.
   zend_bool keep_descriptor_pool_after_request;
 
+  // Set by the user to make the descriptor pool persist between requests.
+  zend_class_entry* constructing_class;
+
   // A upb_DefPool that we are saving for the next request so that we don't have
   // to rebuild it from scratch. When keep_descriptor_pool_after_request==true,
   // we steal the upb_DefPool from the global DescriptorPool object just before
@@ -173,6 +176,7 @@ static PHP_RINIT_FUNCTION(protobuf) {
 
   zend_hash_init(&PROTOBUF_G(object_cache), 64, NULL, NULL, 0);
   zend_hash_init(&PROTOBUF_G(descriptors), 64, NULL, ZVAL_PTR_DTOR, 0);
+  PROTOBUF_G(constructing_class) = NULL;
 
   return SUCCESS;
 }
@@ -253,7 +257,7 @@ const upb_MessageDef *NameMap_GetMessage(zend_class_entry *ce) {
   const upb_MessageDef *ret =
       zend_hash_find_ptr(&PROTOBUF_G(name_msg_cache), ce->name);
 
-  if (!ret && ce->create_object) {
+  if (!ret && ce->create_object && ce != PROTOBUF_G(constructing_class)) {
 #if PHP_VERSION_ID < 80000
     zval tmp;
     zval zv;
@@ -279,6 +283,16 @@ const upb_EnumDef *NameMap_GetEnum(zend_class_entry *ce) {
   return ret;
 }
 
+void NameMap_EnterConstructor(zend_class_entry* ce) {
+  assert(!PROTOBUF_G(constructing_class));
+  PROTOBUF_G(constructing_class) = ce;
+}
+
+void NameMap_ExitConstructor(zend_class_entry* ce) {
+  assert(PROTOBUF_G(constructing_class) == ce);
+  PROTOBUF_G(constructing_class) = NULL;
+}
+
 // -----------------------------------------------------------------------------
 // Module init.
 // -----------------------------------------------------------------------------

diff --git a/php/ext/google/protobuf/protobuf.h b/php/ext/google/protobuf/protobuf.h
@@ -155,6 +155,8 @@ void NameMap_AddMessage(const upb_MessageDef *m);
 void NameMap_AddEnum(const upb_EnumDef *m);
 const upb_MessageDef *NameMap_GetMessage(zend_class_entry *ce);
 const upb_EnumDef *NameMap_GetEnum(zend_class_entry *ce);
+void NameMap_EnterConstructor(zend_class_entry* ce);
+void NameMap_ExitConstructor(zend_class_entry* ce);
 
 // Add this descriptor object to the global list of descriptors that will be
 // kept alive for the duration of the request but destroyed when the request