Merge pull request #12183 from grpc/revert-12168-php-fix-segfault

Revert "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 (retval == NULL || Z_TYPE_P(retval) != IS_ARRAY) {
+  if (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,7 +81,6 @@ 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);
     }
@@ -203,8 +202,10 @@ 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 and separate underlying grpc_channel will be created
- * regardless. This will not affect existing channels.
+ * 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.
  *
  * @param string $target The hostname to associate with this channel
  * @param array $args_array The arguments to pass to the Channel
@@ -287,7 +288,7 @@ PHP_METHOD(Channel, __construct) {
   }
   channel->wrapper = malloc(sizeof(grpc_channel_wrapper));
   channel->wrapper->key = key;
-  channel->wrapper->target = strdup(target);
+  channel->wrapper->target = target;
   channel->wrapper->args_hashstr = sha1str;
   if (creds != NULL && creds->hashstr != NULL) {
     channel->wrapper->creds_hashstr = creds->hashstr;
@@ -295,7 +296,11 @@ PHP_METHOD(Channel, __construct) {
   gpr_mu_init(&channel->wrapper->mu);
   smart_str_free(&buf);
 
-  if (force_new || (creds != NULL && creds->has_call_creds)) {
+  if (force_new) {
+    php_grpc_delete_persistent_list_entry(key, key_len TSRMLS_CC);
+  }
+
+  if (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.
@@ -426,7 +431,6 @@ 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;
   }
 
@@ -460,7 +464,6 @@ 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,28 +57,15 @@ zend_class_entry *grpc_ce_channel_credentials;
 #if PHP_MAJOR_VERSION >= 7
 static zend_object_handlers channel_credentials_ce_handlers;
 #endif
-static gpr_mu cc_persistent_list_mu;
-int le_cc_plink;
-const char *persistent_list_key = "default_root_certs";
+static char *default_pem_root_certs = NULL;
 
 static grpc_ssl_roots_override_result get_ssl_roots_override(
     char **pem_root_certs) {
-  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);
+  *pem_root_certs = default_pem_root_certs;
+  if (default_pem_root_certs == NULL) {
     return GRPC_SSL_ROOTS_OVERRIDE_FAIL;
   }
+  return GRPC_SSL_ROOTS_OVERRIDE_OK;
 }
 
 /* Frees and destroys an instance of wrapped_grpc_channel_credentials */
@@ -113,26 +100,6 @@ 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
@@ -149,21 +116,8 @@ PHP_METHOD(ChannelCredentials, setDefaultRootsPem) {
                          "setDefaultRootsPem expects 1 string", 1 TSRMLS_CC);
     return;
   }
-  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);
-    }
-  }
+  default_pem_root_certs = gpr_malloc((pem_roots_length + 1) * sizeof(char));
+  memcpy(default_pem_root_certs, pem_roots, pem_roots_length + 1);
 }
 
 /**
@@ -272,14 +226,6 @@ 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()
@@ -315,17 +261,12 @@ static zend_function_entry channel_credentials_methods[] = {
   PHP_FE_END
 };
 
-GRPC_STARTUP_FUNCTION(channel_credentials) {
+void grpc_init_channel_credentials(TSRMLS_D) {
   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,10 +76,6 @@ static inline wrapped_grpc_channel_credentials
 #endif /* PHP_MAJOR_VERSION */
 
 /* Initializes the ChannelCredentials PHP class */
-GRPC_STARTUP_FUNCTION(channel_credentials);
-
-typedef struct _channel_credentials_persistent_le {
-  char *default_root_certs;
-} channel_credentials_persistent_le_t;
+void grpc_init_channel_credentials(TSRMLS_D);
 
 #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_STARTUP(channel_credentials);
+  grpc_init_channel_credentials(TSRMLS_C);
   grpc_init_call_credentials(TSRMLS_C);
   grpc_init_server_credentials(TSRMLS_C);
   return SUCCESS;

src/php/tests/unit_tests/ChannelTest.php

@@ -532,6 +532,8 @@ 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();
     }
@@ -542,7 +544,6 @@ 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
@@ -554,31 +555,29 @@ class ChannelTest extends PHPUnit_Framework_TestCase
         $state = $this->channel2->getConnectivityState();
         $this->assertConnecting($state);
         $state = $this->channel3->getConnectivityState();
-        $this->assertEquals(GRPC\CHANNEL_IDLE, $state);
+        $this->assertConnecting($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);
-
-        // channel3 already closed
         $state = $this->channel3->getConnectivityState();
+        $this->assertEquals(GRPC\CHANNEL_IDLE, $state);
+
+        $this->channel2->close();
+        $this->channel3->close();
     }
 
     public function testPersistentChannelForceNewNewChannelClose()