Fixed PHP memory leaks and arginfo errors (protocolbuffers#8614)

* Fixed a bunch of incorrect arginfo and a few incorrect error messages.

* Passes mem check test with no leaks!

* WIP.

* Fix build warning that was causing Bazel build to fail.

* Added compatibility code for PHP <8.0.

* Added test_valgrind target and made tests Valgrind-clean.

* Updated Valgrind test to fail if memory leaks are detected.

* Removed intermediate shell script so commands are easier to cut, paste, and modify.

* Passing all Valgrind tests!

* Hoist addref into ObjCache_Get().

* Removed special case of map descriptors by keying object map on upb_msgdef.

* Removed all remaining RETURN_ZVAL() macros.

* Removed all explicit reference add/del operations.

* Added REFCOUNTING.md to Makefile.am.

Makefile.am

@@ -801,6 +801,7 @@ objectivec_EXTRA_DIST=                                                       \
 php_EXTRA_DIST=                                                       \
   composer.json                                                       \
   php/README.md                                                       \
+  php/REFCOUNTING.md                                                  \
   php/composer.json                                                   \
   php/ext/google/protobuf/arena.c                                     \
   php/ext/google/protobuf/arena.h                                     \

kokoro/linux/php_all/build.sh

@@ -5,13 +5,16 @@
 
 set -ex
 
-cd $(dirname $0)
+# Change to repo base.
+cd $(dirname $0)/../../..
+
+docker run $(test -t 0 && echo "-it") -v$PWD:/workspace gcr.io/protobuf-build/php/linux:8.0.5-dbg-14a06550010c0649bf69b6c9b803c1ca609bbb6d "composer test_valgrind"
+
+docker run $(test -t 0 && echo "-it") -v$PWD:/workspace gcr.io/protobuf-build/php/linux:7.0.33-dbg-14a06550010c0649bf69b6c9b803c1ca609bbb6d "composer test && composer test_c"
+docker run $(test -t 0 && echo "-it") -v$PWD:/workspace gcr.io/protobuf-build/php/linux:7.3.28-dbg-14a06550010c0649bf69b6c9b803c1ca609bbb6d "composer test && composer test_c"
+docker run $(test -t 0 && echo "-it") -v$PWD:/workspace gcr.io/protobuf-build/php/linux:7.4.18-dbg-14a06550010c0649bf69b6c9b803c1ca609bbb6d "composer test && composer test_c"
+docker run $(test -t 0 && echo "-it") -v$PWD:/workspace gcr.io/protobuf-build/php/linux:8.0.5-dbg-14a06550010c0649bf69b6c9b803c1ca609bbb6d "composer test && composer test_c"
 
 # Most of our tests use a debug build of PHP, but we do one build against an opt
 # php just in case that surfaces anything unexpected.
-../test_php.sh gcr.io/protobuf-build/php/linux:8.0.5-14a06550010c0649bf69b6c9b803c1ca609bbb6d
-
-../test_php.sh gcr.io/protobuf-build/php/linux:7.0.33-dbg-14a06550010c0649bf69b6c9b803c1ca609bbb6d
-../test_php.sh gcr.io/protobuf-build/php/linux:7.3.28-dbg-14a06550010c0649bf69b6c9b803c1ca609bbb6d
-../test_php.sh gcr.io/protobuf-build/php/linux:7.4.18-dbg-14a06550010c0649bf69b6c9b803c1ca609bbb6d
-../test_php.sh gcr.io/protobuf-build/php/linux:8.0.5-dbg-14a06550010c0649bf69b6c9b803c1ca609bbb6d
+docker run $(test -t 0 && echo "-it") -v$PWD:/workspace gcr.io/protobuf-build/php/linux:8.0.5-14a06550010c0649bf69b6c9b803c1ca609bbb6d "composer test && composer test_c"

php/REFCOUNTING.md

@@ -0,0 +1,112 @@
+
+# Refcounting Tips
+
+One of the trickiest parts of the C extension for PHP is getting the refcounting
+right.  These are some notes about the basics of what you should know,
+especially if you're not super familiar with PHP's C API.
+
+These notes cover the same general material as [the Memory Management chapter of
+the PHP internal's
+book](https://www.phpinternalsbook.com/php7/zvals/memory_management.html), but
+calls out some points that were not immediately clear to me.
+
+##  Zvals
+
+In the PHP C API, the `zval` type is roughly analogous to a variable in PHP, eg:
+
+```php
+    // Think of $a as a "zval".
+    $a = [];
+```
+
+The equivalent PHP C code would be:
+
+```c
+    zval a;
+    ZVAL_NEW_ARR(&a);  // Allocates and assigns a new array.
+```
+
+PHP is reference counted, so each variable -- and thus each zval -- will have a
+reference on whatever it points to (unless its holding a data type that isn't
+refcounted at all, like numbers). Since the zval owns a reference, it must be
+explicitly destroyed in order to release this reference.
+
+```c
+    zval a;
+    ZVAL_NEW_ARR(&a);
+
+    // The destructor for a zval, this must be called or the ref will be leaked.
+    zval_ptr_dtor(&a);
+```
+
+Whenever you see a `zval`, you can assume it owns a ref (or is storing a
+non-refcounted type). If you see a `zval*`, which is also quite common, then
+this is *pointing to* something that owns a ref, but it does not own a ref
+itself.
+
+The [`ZVAL_*` family of
+macros](https://github.com/php/php-src/blob/4030a00e8b6453aff929362bf9b25c193f72c94a/Zend/zend_types.h#L883-L1109)
+initializes a `zval` from a specific value type.  A few examples:
+
+* `ZVAL_NULL(&zv)`: initializes the value to `null`
+* `ZVAL_LONG(&zv, 5)`: initializes a `zend_long` (integer) value
+* `ZVAL_ARR(&zv, arr)`: initializes a `zend_array*` value (refcounted)
+* `ZVAL_OBJ(&zv, obj)`: initializes a `zend_object*` value (refcounted)
+
+Note that all of our custom objects (messages, repeated fields, descriptors,
+etc) are `zend_object*`.
+
+The variants that initialize from a refcounted type do *not* increase the
+refcount. This makes them suitable for initializing from a newly-created object:
+
+```c
+    zval zv;
+    ZVAL_OBJ(&zv, CreateObject());
+```
+
+Once in a while, we want to initialize a `zval` while also increasing the
+reference count. For this we can use `ZVAL_OBJ_COPY()`:
+
+```c
+zend_object *some_global;
+
+void GetGlobal(zval *zv) {
+    // We want to create a new ref to an existing object.
+    ZVAL_OBJ_COPY(zv, some_global);
+}
+```
+
+## Transferring references
+
+A `zval`'s ref must be released at some point. While `zval_ptr_dtor()` is the
+simplest way of releasing a ref, it is not the most common (at least in our code
+base). More often, we are returning the `zval` back to PHP from C.
+
+```c
+    zval zv;
+    InitializeOurZval(&zv);
+    // Returns the value of zv to the caller and donates our ref.
+    RETURN_COPY_VALUE(&zv);
+```
+
+The `RETURN_COPY_VALUE()` macro (standard in PHP 8.x, and polyfilled in earlier
+versions) is the most common way we return a value back to PHP, because it
+donates our `zval`'s refcount to the caller, and thus saves us from needing to
+destroy our `zval` explicitly. This is ideal when we have a full `zval` to
+return.
+
+Once in a while we have a `zval*` to return instead. For example when we parse
+parameters to our function and ask for a `zval`, PHP will give us pointers to
+the existing `zval` structures instead of creating new ones.
+
+```c
+    zval *val;
+    if (zend_parse_parameters(ZEND_NUM_ARGS(), "z", &val) == FAILURE) {
+      return;
+    }
+    // Returns a copy of this zval, adding a ref in the process.
+    RETURN_COPY(val);
+```
+
+When we use `RETURN_COPY`, the refcount is increased; this is perfect for
+returning a `zval*` when we do not own a ref on it.

php/composer.json

@@ -24,6 +24,7 @@
   },
   "scripts": {
     "test_c": "./generate_test_protos.sh && ./tests/compile_extension.sh && php -dextension=ext/google/protobuf/modules/protobuf.so vendor/bin/phpunit --bootstrap tests/force_c_ext.php tests",
+    "test_valgrind": "./generate_test_protos.sh && ./tests/compile_extension.sh && ZEND_DONT_UNLOAD_MODULES=1 USE_ZEND_ALLOC=0 valgrind --leak-check=full --error-exitcode=1 php -dextension=ext/google/protobuf/modules/protobuf.so vendor/bin/phpunit --bootstrap tests/force_c_ext.php tests",
     "test": "./generate_test_protos.sh && vendor/bin/phpunit tests",
     "aggregate_metadata_test": "./generate_test_protos.sh --aggregate_metadata && vendor/bin/phpunit tests"
   }

php/ext/google/protobuf/array.c

@@ -337,7 +337,7 @@ PHP_METHOD(RepeatedField, offsetGet) {
 
   msgval = upb_array_get(intern->array, index);
   Convert_UpbToPhp(msgval, &ret, intern->type, &intern->arena);
-  RETURN_ZVAL(&ret, 0, 1);
+  RETURN_COPY_VALUE(&ret);
 }
 
 /**
@@ -447,7 +447,7 @@ PHP_METHOD(RepeatedField, count) {
 PHP_METHOD(RepeatedField, getIterator) {
   zval ret;
   RepeatedFieldIter_make(&ret, getThis());
-  RETURN_ZVAL(&ret, 0, 1);
+  RETURN_COPY_VALUE(&ret);
 }
 
 ZEND_BEGIN_ARG_INFO_EX(arginfo_construct, 0, 0, 1)
@@ -579,7 +579,7 @@ PHP_METHOD(RepeatedFieldIter, current) {
   msgval = upb_array_get(array, index);
 
   Convert_UpbToPhp(msgval, &ret, field->type, &field->arena);
-  RETURN_ZVAL(&ret, 0, 1);
+  RETURN_COPY_VALUE(&ret);
 }
 
 /**

php/ext/google/protobuf/convert.c

@@ -76,7 +76,7 @@ PHP_METHOD(Util, checkMapField) {
                             &val_type, &klass) == FAILURE) {
     return;
   }
-  RETURN_ZVAL(val, 1, 0);
+  RETURN_COPY(val);
 }
 
 // The result of checkRepeatedField() is assigned, so we need to return the
@@ -89,13 +89,18 @@ PHP_METHOD(Util, checkRepeatedField) {
       FAILURE) {
     return;
   }
-  RETURN_ZVAL(val, 1, 0);
+  RETURN_COPY(val);
 }
 
 ZEND_BEGIN_ARG_INFO_EX(arginfo_checkPrimitive, 0, 0, 1)
   ZEND_ARG_INFO(0, value)
 ZEND_END_ARG_INFO()
 
+ZEND_BEGIN_ARG_INFO_EX(arginfo_checkString, 0, 0, 1)
+  ZEND_ARG_INFO(0, value)
+  ZEND_ARG_INFO(0, check_utf8)
+ZEND_END_ARG_INFO()
+
 ZEND_BEGIN_ARG_INFO_EX(arginfo_checkMessage, 0, 0, 2)
   ZEND_ARG_INFO(0, value)
   ZEND_ARG_INFO(0, class)
@@ -123,15 +128,15 @@ static zend_function_entry util_methods[] = {
          ZEND_ACC_PUBLIC|ZEND_ACC_STATIC)
   PHP_ME(Util, checkUint64, arginfo_checkPrimitive,
          ZEND_ACC_PUBLIC|ZEND_ACC_STATIC)
-  PHP_ME(Util, checkEnum,   arginfo_checkPrimitive,
+  PHP_ME(Util, checkEnum,   arginfo_checkMessage,
          ZEND_ACC_PUBLIC|ZEND_ACC_STATIC)
   PHP_ME(Util, checkFloat,  arginfo_checkPrimitive,
          ZEND_ACC_PUBLIC|ZEND_ACC_STATIC)
   PHP_ME(Util, checkDouble, arginfo_checkPrimitive,
          ZEND_ACC_PUBLIC|ZEND_ACC_STATIC)
   PHP_ME(Util, checkBool,   arginfo_checkPrimitive,
          ZEND_ACC_PUBLIC|ZEND_ACC_STATIC)
-  PHP_ME(Util, checkString, arginfo_checkPrimitive,
+  PHP_ME(Util, checkString, arginfo_checkString,
          ZEND_ACC_PUBLIC|ZEND_ACC_STATIC)
   PHP_ME(Util, checkBytes,  arginfo_checkPrimitive,
          ZEND_ACC_PUBLIC|ZEND_ACC_STATIC)

php/ext/google/protobuf/convert.h

@@ -60,9 +60,10 @@ bool Convert_PhpToUpbAutoWrap(zval *val, upb_msgval *upb_val, TypeInfo type,
                               upb_arena *arena);
 
 // Converts |upb_val| to a PHP zval according to |type|. This may involve
-// creating a PHP wrapper object. If type == UPB_TYPE_MESSAGE, then |desc| must
-// be the Descriptor for this message type. Any newly created wrapper object
+// creating a PHP wrapper object. Any newly created wrapper object
 // will reference |arena|.
+//
+// The caller owns a reference to the returned value.
 void Convert_UpbToPhp(upb_msgval upb_val, zval *php_val, TypeInfo type,
                       zval *arena);