Merge pull request #12184 from stanley-cheung/test-grpc-146rc2

PHP: fix segfault

package.xml

@@ -22,9 +22,7 @@
  </stability>
  <license>BSD</license>
  <notes>
-- Fixed a Windows installation issue #12108
-- Fixed a MacOS mutex segfault #12109
-- Fixed a ZTS compilation issue #12109
+- Fixed segfault when tests were run under Travis #12123
  </notes>
  <contents>
   <dir baseinstalldir="/" name="/">

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,8 @@ 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);
+        free(p->wrapper->args_hashstr);
       }
       gpr_mu_unlock(&global_persistent_list_mu);
     }
@@ -202,10 +204,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
+ * and returned. 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,19 +288,15 @@ PHP_METHOD(Channel, __construct) {
   }
   channel->wrapper = malloc(sizeof(grpc_channel_wrapper));
   channel->wrapper->key = key;
-  channel->wrapper->target = target;
-  channel->wrapper->args_hashstr = sha1str;
+  channel->wrapper->target = strdup(target);
+  channel->wrapper->args_hashstr = strdup(sha1str);
   if (creds != NULL && creds->hashstr != NULL) {
     channel->wrapper->creds_hashstr = creds->hashstr;
   }
   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,12 +427,14 @@ 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);
+    free(channel->wrapper->args_hashstr);
     channel->wrapper->wrapped = NULL;
-  }
 
-  php_grpc_delete_persistent_list_entry(channel->wrapper->key,
-                                        strlen(channel->wrapper->key)
-                                        TSRMLS_CC);
+    php_grpc_delete_persistent_list_entry(channel->wrapper->key,
+                                          strlen(channel->wrapper->key)
+                                          TSRMLS_CC);
+  }
   gpr_mu_unlock(&channel->wrapper->mu);
 }
 
@@ -464,12 +462,11 @@ 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->key);
-      free(le->channel);
+      free(le->channel->target);
+      free(le->channel->args_hashstr);
     }
     gpr_mu_unlock(&le->channel->mu);
   }
-  free(le);
 }
 
 ZEND_BEGIN_ARG_INFO_EX(arginfo_construct, 0, 0, 2)

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()

templates/package.xml.template

@@ -24,9 +24,7 @@
    </stability>
    <license>BSD</license>
    <notes>
-  - Fixed a Windows installation issue #12108
-  - Fixed a MacOS mutex segfault #12109
-  - Fixed a ZTS compilation issue #12109
+  - Fixed segfault when tests were run under Travis #12123
    </notes>
    <contents>
     <dir baseinstalldir="/" name="/">