Merge pull request #12168 from stanley-cheung/php-fix-segfault

PHP: fix some segfaults

src/php/ext/grpc/call_credentials.c

@@ -194,7 +194,7 @@ void plugin_get_metadata(void *ptr, grpc_auth_metadata_context context,
   grpc_metadata_array metadata;
   bool cleanup = true;
 
-  if (Z_TYPE_P(retval) != IS_ARRAY) {
+  if (retval == NULL || Z_TYPE_P(retval) != IS_ARRAY) {
     cleanup = false;
     code = GRPC_STATUS_INVALID_ARGUMENT;
   } else if (!create_metadata_array(retval, &metadata)) {

src/php/ext/grpc/channel.c

@@ -81,6 +81,7 @@ PHP_GRPC_FREE_WRAPPED_FUNC_START(wrapped_grpc_channel)
       if (!(PHP_GRPC_PERSISTENT_LIST_FIND(&EG(persistent_list), p->wrapper->key,
                                           key_len, rsrc))) {
         grpc_channel_destroy(p->wrapper->wrapped);
+        free(p->wrapper->target);
       }
       gpr_mu_unlock(&global_persistent_list_mu);
     }
@@ -202,10 +203,8 @@ void create_and_add_channel_to_persistent_list(
  * credentials.
  *
  * If the $args array contains a "force_new" key mapping to a boolean value
- * of "true", a new underlying grpc_channel will be created regardless. If
- * there are any opened channels on the same hostname, user must manually
- * call close() on those dangling channels before the end of the PHP
- * script.
+ * of "true", a new and separate underlying grpc_channel will be created
+ * regardless. This will not affect existing channels.
  *
  * @param string $target The hostname to associate with this channel
  * @param array $args_array The arguments to pass to the Channel
@@ -288,7 +287,7 @@ PHP_METHOD(Channel, __construct) {
   }
   channel->wrapper = malloc(sizeof(grpc_channel_wrapper));
   channel->wrapper->key = key;
-  channel->wrapper->target = target;
+  channel->wrapper->target = strdup(target);
   channel->wrapper->args_hashstr = sha1str;
   if (creds != NULL && creds->hashstr != NULL) {
     channel->wrapper->creds_hashstr = creds->hashstr;
@@ -296,11 +295,7 @@ PHP_METHOD(Channel, __construct) {
   gpr_mu_init(&channel->wrapper->mu);
   smart_str_free(&buf);
 
-  if (force_new) {
-    php_grpc_delete_persistent_list_entry(key, key_len TSRMLS_CC);
-  }
-
-  if (creds != NULL && creds->has_call_creds) {
+  if (force_new || (creds != NULL && creds->has_call_creds)) {
     // If the ChannelCredentials object was composed with a CallCredentials
     // object, there is no way we can tell them apart. Do NOT persist
     // them. They should be individually destroyed.
@@ -431,6 +426,7 @@ PHP_METHOD(Channel, close) {
   gpr_mu_lock(&channel->wrapper->mu);
   if (channel->wrapper->wrapped != NULL) {
     grpc_channel_destroy(channel->wrapper->wrapped);
+    free(channel->wrapper->target);
     channel->wrapper->wrapped = NULL;
   }
 
@@ -464,6 +460,7 @@ static void php_grpc_channel_plink_dtor(php_grpc_zend_resource *rsrc
     gpr_mu_lock(&le->channel->mu);
     if (le->channel->wrapped != NULL) {
       grpc_channel_destroy(le->channel->wrapped);
+      free(le->channel->target);
       free(le->channel->key);
       free(le->channel);
     }

src/php/ext/grpc/channel_credentials.c

@@ -57,15 +57,28 @@ zend_class_entry *grpc_ce_channel_credentials;
 #if PHP_MAJOR_VERSION >= 7
 static zend_object_handlers channel_credentials_ce_handlers;
 #endif
-static char *default_pem_root_certs = NULL;
+static gpr_mu cc_persistent_list_mu;
+int le_cc_plink;
+const char *persistent_list_key = "default_root_certs";
 
 static grpc_ssl_roots_override_result get_ssl_roots_override(
     char **pem_root_certs) {
-  *pem_root_certs = default_pem_root_certs;
-  if (default_pem_root_certs == NULL) {
+  TSRMLS_FETCH();
+  php_grpc_zend_resource *rsrc;
+  php_grpc_int key_len = strlen(persistent_list_key);
+
+  gpr_mu_lock(&cc_persistent_list_mu);
+  if (PHP_GRPC_PERSISTENT_LIST_FIND(&EG(persistent_list), persistent_list_key,
+                                    key_len, rsrc)) {
+    channel_credentials_persistent_le_t *le =
+      (channel_credentials_persistent_le_t *)rsrc->ptr;
+    *pem_root_certs = le->default_root_certs;
+    gpr_mu_unlock(&cc_persistent_list_mu);
+    return GRPC_SSL_ROOTS_OVERRIDE_OK;
+  } else {
+    gpr_mu_unlock(&cc_persistent_list_mu);
     return GRPC_SSL_ROOTS_OVERRIDE_FAIL;
   }
-  return GRPC_SSL_ROOTS_OVERRIDE_OK;
 }
 
 /* Frees and destroys an instance of wrapped_grpc_channel_credentials */
@@ -100,6 +113,26 @@ zval *grpc_php_wrap_channel_credentials(grpc_channel_credentials *wrapped,
   return credentials_object;
 }
 
+void update_root_certs_persistent_list(char *pem_roots,
+                                       php_grpc_int pem_roots_length
+                                       TSRMLS_DC) {
+
+  php_grpc_zend_resource new_rsrc;
+  channel_credentials_persistent_le_t *le;
+  php_grpc_int key_len = strlen(persistent_list_key);
+  new_rsrc.type = le_cc_plink;
+  le = malloc(sizeof(channel_credentials_persistent_le_t));
+
+  le->default_root_certs = malloc(pem_roots_length+1);
+  memcpy(le->default_root_certs, pem_roots, pem_roots_length+1);
+
+  new_rsrc.ptr = le;
+  gpr_mu_lock(&cc_persistent_list_mu);
+  PHP_GRPC_PERSISTENT_LIST_UPDATE(&EG(persistent_list), persistent_list_key,
+                                  key_len, (void *)&new_rsrc);
+  gpr_mu_unlock(&cc_persistent_list_mu);
+}
+
 /**
  * Set default roots pem.
  * @param string $pem_roots PEM encoding of the server root certificates
@@ -116,8 +149,21 @@ PHP_METHOD(ChannelCredentials, setDefaultRootsPem) {
                          "setDefaultRootsPem expects 1 string", 1 TSRMLS_CC);
     return;
   }
-  default_pem_root_certs = gpr_malloc((pem_roots_length + 1) * sizeof(char));
-  memcpy(default_pem_root_certs, pem_roots, pem_roots_length + 1);
+  php_grpc_zend_resource *rsrc;
+  php_grpc_int key_len = strlen(persistent_list_key);
+  if (!(PHP_GRPC_PERSISTENT_LIST_FIND(&EG(persistent_list),
+                                      persistent_list_key,
+                                      key_len, rsrc))) {
+    update_root_certs_persistent_list(pem_roots,
+                                      pem_roots_length TSRMLS_CC);
+  } else {
+    channel_credentials_persistent_le_t *le =
+      (channel_credentials_persistent_le_t *)rsrc->ptr;
+    if (strcmp(pem_roots, le->default_root_certs) != 0) {
+      update_root_certs_persistent_list(pem_roots, pem_roots_length
+                                        TSRMLS_CC);
+    }
+  }
 }
 
 /**
@@ -226,6 +272,14 @@ PHP_METHOD(ChannelCredentials, createInsecure) {
   RETURN_NULL();
 }
 
+static void php_grpc_channel_credentials_plink_dtor(
+    php_grpc_zend_resource *rsrc TSRMLS_DC) {
+  channel_credentials_persistent_le_t *le =
+    (channel_credentials_persistent_le_t *)rsrc->ptr;
+  free(le->default_root_certs);
+  free(le);
+}
+
 ZEND_BEGIN_ARG_INFO_EX(arginfo_setDefaultRootsPem, 0, 0, 1)
   ZEND_ARG_INFO(0, pem_roots)
 ZEND_END_ARG_INFO()
@@ -261,12 +315,17 @@ static zend_function_entry channel_credentials_methods[] = {
   PHP_FE_END
 };
 
-void grpc_init_channel_credentials(TSRMLS_D) {
+GRPC_STARTUP_FUNCTION(channel_credentials) {
   zend_class_entry ce;
   INIT_CLASS_ENTRY(ce, "Grpc\\ChannelCredentials",
                    channel_credentials_methods);
   ce.create_object = create_wrapped_grpc_channel_credentials;
   grpc_ce_channel_credentials = zend_register_internal_class(&ce TSRMLS_CC);
+  gpr_mu_init(&cc_persistent_list_mu);
+  le_cc_plink = zend_register_list_destructors_ex(
+      NULL, php_grpc_channel_credentials_plink_dtor,
+      "Channel Credentials persistent default certs", module_number);
   PHP_GRPC_INIT_HANDLER(wrapped_grpc_channel_credentials,
                         channel_credentials_ce_handlers);
+  return SUCCESS;
 }

src/php/ext/grpc/channel_credentials.h

@@ -76,6 +76,10 @@ static inline wrapped_grpc_channel_credentials
 #endif /* PHP_MAJOR_VERSION */
 
 /* Initializes the ChannelCredentials PHP class */
-void grpc_init_channel_credentials(TSRMLS_D);
+GRPC_STARTUP_FUNCTION(channel_credentials);
+
+typedef struct _channel_credentials_persistent_le {
+  char *default_root_certs;
+} channel_credentials_persistent_le_t;
 
 #endif /* NET_GRPC_PHP_GRPC_CHANNEL_CREDENTIALS_H_ */

src/php/ext/grpc/php_grpc.c

@@ -243,7 +243,7 @@ PHP_MINIT_FUNCTION(grpc) {
   GRPC_STARTUP(channel);
   grpc_init_server(TSRMLS_C);
   grpc_init_timeval(TSRMLS_C);
-  grpc_init_channel_credentials(TSRMLS_C);
+  GRPC_STARTUP(channel_credentials);
   grpc_init_call_credentials(TSRMLS_C);
   grpc_init_server_credentials(TSRMLS_C);
   return SUCCESS;

src/php/tests/unit_tests/ChannelTest.php

@@ -532,8 +532,6 @@ class ChannelTest extends PHPUnit_Framework_TestCase
         $state = $this->channel2->getConnectivityState();
         $this->assertEquals(GRPC\CHANNEL_IDLE, $state);
 
-        // any dangling old connection to the same host must be
-        // manually closed
         $this->channel1->close();
         $this->channel2->close();
     }
@@ -544,6 +542,7 @@ class ChannelTest extends PHPUnit_Framework_TestCase
         $this->channel1 = new Grpc\Channel('localhost:1', []);
         $this->channel2 = new Grpc\Channel('localhost:1',
                                            ["force_new" => true]);
+        // channel3 shares with channel1
         $this->channel3 = new Grpc\Channel('localhost:1', []);
 
         // try to connect on channel2
@@ -555,29 +554,31 @@ class ChannelTest extends PHPUnit_Framework_TestCase
         $state = $this->channel2->getConnectivityState();
         $this->assertConnecting($state);
         $state = $this->channel3->getConnectivityState();
-        $this->assertConnecting($state);
+        $this->assertEquals(GRPC\CHANNEL_IDLE, $state);
 
         $this->channel1->close();
         $this->channel2->close();
     }
 
+    /**
+     * @expectedException RuntimeException
+     */
     public function testPersistentChannelForceNewOldChannelClose()
     {
 
         $this->channel1 = new Grpc\Channel('localhost:1', []);
         $this->channel2 = new Grpc\Channel('localhost:1',
                                            ["force_new" => true]);
+        // channel3 shares with channel1
         $this->channel3 = new Grpc\Channel('localhost:1', []);
 
         $this->channel1->close();
 
         $state = $this->channel2->getConnectivityState();
         $this->assertEquals(GRPC\CHANNEL_IDLE, $state);
-        $state = $this->channel3->getConnectivityState();
-        $this->assertEquals(GRPC\CHANNEL_IDLE, $state);
 
-        $this->channel2->close();
-        $this->channel3->close();
+        // channel3 already closed
+        $state = $this->channel3->getConnectivityState();
     }
 
     public function testPersistentChannelForceNewNewChannelClose()