Merge pull request #14149 from ZhouyihaiDing/v1.9.x_php_channel_leak

Backport PR #14125 - #14130 into 1.9.x

src/php/ext/grpc/channel.c

@@ -41,6 +41,7 @@
 
 #include <grpc/grpc.h>
 #include <grpc/grpc_security.h>
+#include <grpc/support/alloc.h>
 
 #include "completion_queue.h"
 #include "channel_credentials.h"
@@ -56,22 +57,63 @@ int le_plink;
 
 /* Frees and destroys an instance of wrapped_grpc_channel */
 PHP_GRPC_FREE_WRAPPED_FUNC_START(wrapped_grpc_channel)
+  bool is_last_wrapper = false;
+  // In_persistent_list is used when the user don't close the channel.
+  // In this case, le in the list won't be freed.
+  bool in_persistent_list = true;
   if (p->wrapper != NULL) {
     gpr_mu_lock(&p->wrapper->mu);
     if (p->wrapper->wrapped != NULL) {
-      php_grpc_zend_resource *rsrc;
-      php_grpc_int key_len = strlen(p->wrapper->key);
-      // only destroy the channel here if not found in the persistent list
-      gpr_mu_lock(&global_persistent_list_mu);
-      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);
+      if (p->wrapper->is_valid) {
+        php_grpc_zend_resource *rsrc;
+        php_grpc_int key_len = strlen(p->wrapper->key);
+        // only destroy the channel here if not found in the persistent list
+        gpr_mu_lock(&global_persistent_list_mu);
+        if (!(PHP_GRPC_PERSISTENT_LIST_FIND(&EG(persistent_list), p->wrapper->key,
+                                            key_len, rsrc))) {
+          in_persistent_list = false;
+          grpc_channel_destroy(p->wrapper->wrapped);
+          free(p->wrapper->target);
+          free(p->wrapper->args_hashstr);
+          if(p->wrapper->creds_hashstr != NULL){
+            free(p->wrapper->creds_hashstr);
+            p->wrapper->creds_hashstr = NULL;
+          }
+        }
+        gpr_mu_unlock(&global_persistent_list_mu);
       }
-      gpr_mu_unlock(&global_persistent_list_mu);
+    }
+    p->wrapper->ref_count -= 1;
+    if (p->wrapper->ref_count == 0) {
+      is_last_wrapper = true;
     }
     gpr_mu_unlock(&p->wrapper->mu);
+    if (is_last_wrapper) {
+      if (in_persistent_list) {
+        // If ref_count==0 and the key still in the list, it means the user
+        // don't call channel->close().persistent list should free the
+        // allocation in such case, as well as related wrapped channel.
+        if (p->wrapper->wrapped != NULL) {
+          gpr_mu_lock(&p->wrapper->mu);
+          grpc_channel_destroy(p->wrapper->wrapped);
+          free(p->wrapper->target);
+          free(p->wrapper->args_hashstr);
+          if(p->wrapper->creds_hashstr != NULL){
+            free(p->wrapper->creds_hashstr);
+            p->wrapper->creds_hashstr = NULL;
+          }
+          p->wrapper->wrapped = NULL;
+          php_grpc_delete_persistent_list_entry(p->wrapper->key,
+                                                strlen(p->wrapper->key)
+                                                TSRMLS_CC);
+          gpr_mu_unlock(&p->wrapper->mu);
+        }
+      }
+      gpr_mu_destroy(&p->wrapper->mu);
+      free(p->wrapper->key);
+      free(p->wrapper);
+    }
+    p->wrapper = NULL;
   }
 PHP_GRPC_FREE_WRAPPED_FUNC_END()
 
@@ -276,9 +318,16 @@ PHP_METHOD(Channel, __construct) {
   channel->wrapper->key = key;
   channel->wrapper->target = strdup(target);
   channel->wrapper->args_hashstr = strdup(sha1str);
+  channel->wrapper->creds_hashstr = NULL;
+  channel->wrapper->ref_count = 1;
+  channel->wrapper->is_valid = true;
   if (creds != NULL && creds->hashstr != NULL) {
-    channel->wrapper->creds_hashstr = creds->hashstr;
+    php_grpc_int creds_hashstr_len = strlen(creds->hashstr);
+    char *channel_creds_hashstr = malloc(creds_hashstr_len + 1);
+    strcpy(channel_creds_hashstr, creds->hashstr);
+    channel->wrapper->creds_hashstr = channel_creds_hashstr;
   }
+
   gpr_mu_init(&channel->wrapper->mu);
   smart_str_free(&buf);
 
@@ -303,7 +352,17 @@ PHP_METHOD(Channel, __construct) {
           channel, target, args, creds, key, key_len TSRMLS_CC);
     } else {
       efree(args.args);
+      if (channel->wrapper->creds_hashstr != NULL){
+        free(channel->wrapper->creds_hashstr);
+        channel->wrapper->creds_hashstr = NULL;
+      }
+      free(channel->wrapper->creds_hashstr);
+      free(channel->wrapper->key);
+      free(channel->wrapper->target);
+      free(channel->wrapper->args_hashstr);
+      free(channel->wrapper);
       channel->wrapper = le->channel;
+      channel->wrapper->ref_count += 1;
     }
   }
 }
@@ -323,7 +382,8 @@ PHP_METHOD(Channel, getTarget) {
   }
   char *target = grpc_channel_get_target(channel->wrapper->wrapped);
   gpr_mu_unlock(&channel->wrapper->mu);
-  PHP_GRPC_RETURN_STRING(target, 1);
+  PHP_GRPC_RETVAL_STRING(target, 1);
+  gpr_free(target);
 }
 
 /**
@@ -411,18 +471,46 @@ PHP_METHOD(Channel, watchConnectivityState) {
  */
 PHP_METHOD(Channel, close) {
   wrapped_grpc_channel *channel = Z_WRAPPED_GRPC_CHANNEL_P(getThis());
-  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);
+  bool is_last_wrapper = false;
+  if (channel->wrapper != NULL) {
+    // Channel_wrapper hasn't call close before.
+    gpr_mu_lock(&channel->wrapper->mu);
+    if (channel->wrapper->wrapped != NULL) {
+      if (channel->wrapper->is_valid) {
+        // Wrapped channel hasn't been destoryed by other wrapper.
+        grpc_channel_destroy(channel->wrapper->wrapped);
+        free(channel->wrapper->target);
+        free(channel->wrapper->args_hashstr);
+        if(channel->wrapper->creds_hashstr != NULL){
+          free(channel->wrapper->creds_hashstr);
+          channel->wrapper->creds_hashstr = NULL;
+        }
+        channel->wrapper->wrapped = NULL;
+        channel->wrapper->is_valid = false;
+
+        php_grpc_delete_persistent_list_entry(channel->wrapper->key,
+                                              strlen(channel->wrapper->key)
+                                              TSRMLS_CC);
+      }
+    }
+    channel->wrapper->ref_count -= 1;
+    if(channel->wrapper->ref_count == 0){
+      // Mark that the wrapper can be freed because mu should be
+      // destroyed outside the lock.
+      is_last_wrapper = true;
+    }
+    gpr_mu_unlock(&channel->wrapper->mu);
   }
-  gpr_mu_unlock(&channel->wrapper->mu);
+  gpr_mu_lock(&global_persistent_list_mu);
+  if (is_last_wrapper) {
+    gpr_mu_destroy(&channel->wrapper->mu);
+    free(channel->wrapper->key);
+    free(channel->wrapper);
+  }
+  // Set channel->wrapper to NULL to avoid call close twice for the same
+  // channel.
+  channel->wrapper = NULL;
+  gpr_mu_unlock(&global_persistent_list_mu);
 }
 
 // Delete an entry from the persistent list
@@ -437,6 +525,7 @@ void php_grpc_delete_persistent_list_entry(char *key, php_grpc_int key_len
     le = (channel_persistent_le_t *)rsrc->ptr;
     le->channel = NULL;
     php_grpc_zend_hash_del(&EG(persistent_list), key, key_len+1);
+    free(le);
   }
   gpr_mu_unlock(&global_persistent_list_mu);
 }

src/php/ext/grpc/channel.h

@@ -40,6 +40,11 @@ typedef struct _grpc_channel_wrapper {
   char *args_hashstr;
   char *creds_hashstr;
   gpr_mu mu;
+  // is_valid is used to check the wrapped channel has been freed
+  // before to avoid double free.
+  bool is_valid;
+  // ref_count is used to let the last wrapper free related channel and key.
+  size_t ref_count;
 } grpc_channel_wrapper;
 
 /* Wrapper struct for grpc_channel that can be associated with a PHP object */

src/php/ext/grpc/channel_credentials.c

@@ -59,6 +59,7 @@ static grpc_ssl_roots_override_result get_ssl_roots_override(
 PHP_GRPC_FREE_WRAPPED_FUNC_START(wrapped_grpc_channel_credentials)
   if (p->wrapped != NULL) {
     grpc_channel_credentials_release(p->wrapped);
+    p->wrapped = NULL;
   }
 PHP_GRPC_FREE_WRAPPED_FUNC_END()
 
@@ -199,8 +200,13 @@ PHP_METHOD(ChannelCredentials, createComposite) {
   grpc_channel_credentials *creds =
       grpc_composite_channel_credentials_create(cred1->wrapped, cred2->wrapped,
                                                 NULL);
+  // wrapped_grpc_channel_credentials object should keeps it's own
+  // allocation. Otherwise it conflicts free hashstr with call.c.
+  php_grpc_int cred1_len = strlen(cred1->hashstr);
+  char *cred1_hashstr = malloc(cred1_len+1);
+  strcpy(cred1_hashstr, cred1->hashstr);
   zval *creds_object =
-      grpc_php_wrap_channel_credentials(creds, cred1->hashstr, true
+      grpc_php_wrap_channel_credentials(creds, cred1_hashstr, true
                                         TSRMLS_CC);
   RETURN_DESTROY_ZVAL(creds_object);
 }

src/php/ext/grpc/php7_wrapper.h

@@ -33,6 +33,7 @@
 #define php_grpc_add_next_index_stringl(data, str, len, b) \
   add_next_index_stringl(data, str, len, b)
 
+#define PHP_GRPC_RETVAL_STRING(val, dup) RETVAL_STRING(val, dup)
 #define PHP_GRPC_RETURN_STRING(val, dup) RETURN_STRING(val, dup)
 #define PHP_GRPC_MAKE_STD_ZVAL(pzv) MAKE_STD_ZVAL(pzv)
 #define PHP_GRPC_FREE_STD_ZVAL(pzv)
@@ -145,6 +146,7 @@ static inline int php_grpc_zend_hash_find(HashTable *ht, char *key, int len,
 #define php_grpc_add_next_index_stringl(data, str, len, b) \
   add_next_index_stringl(data, str, len)
 
+#define PHP_GRPC_RETVAL_STRING(val, dup) RETVAL_STRING(val)
 #define PHP_GRPC_RETURN_STRING(val, dup) RETURN_STRING(val)
 #define PHP_GRPC_MAKE_STD_ZVAL(pzv) \
   pzv = (zval *)emalloc(sizeof(zval));