fix(agent): wrap zend_try in a func to avoid clobbering (#703)

Calls to `call_user_function` (in PHP 8.0 and 8.1) and
`zend_call_method_if_exists` (in PHP 8.2+) need to be wrapped by the
`zend_try`/`zend_catch`/`zend_end_try` block, which use
[`setjmp`](https://linux.die.net/man/3/setjmp) and
[`longjmp`](https://linux.die.net/man/3/longjmp), because according to
[`call_user_func()`](https://www.php.net/manual/en/function.call-user-func.php):
> Callbacks registered with functions such as `call_user_func()` and
[`call_user_func_array()`](https://www.php.net/manual/en/function.call-user-func-array.php)
will not be called if there is an uncaught exception thrown in a
previous callback.

So if we call something that causes an exception, it will block us from
future calls that use `call_user_func` or `call_user_func_array`.
Valgrind showed the agent and/or the Zend engine wasn’t properly
cleaning up after such cases and newer compilers had issues with this
when compiling with any optimization and generated the following error:
```
error: variable ‘retval’ might be clobbered by ‘longjmp’ or ‘vfork’) [-Werror=clobbered]
```
PHP developers solve this problem by using an automatic variable to
store user function call result - see
[here](https://github.com/php/php-src/blob/master/main/streams/userspace.c#L335-L340)
for an example in PHP source code how zend_call_method_if_exists is
called.

This solution wraps `zend_try`/`zend_catch`/`zend_end_try` constructs in
a function to localize the `retval` and avoid variable clobbering.

Author: ZNeumann

agent/php_call.c

@@ -10,6 +10,56 @@
 
 #include "Zend/zend_exceptions.h"
 
+/*
+ * zend_try family of macros entail the use of setjmp and longjmp, which can cause clobbering issues with
+ * non-primitive local variables. Abstracting these constructs into separate functions protects from this.
+ */
+#if ZEND_MODULE_API_NO >= ZEND_8_2_X_API_NO
+static int nr_php_call_try_catch(zend_object* object,
+                                 zend_string* method_name,
+                                 zval* retval,
+                                 zend_uint param_count,
+                                 zval* param_values) {
+  /*
+   * With PHP 8.2, functions that do not exist will cause a fatal error to
+   * be thrown. `zend_call_method_if_exists` will attempt to call a function and
+   * silently fail if it does not exist
+   */
+  int zend_result = FAILURE;
+  zend_try {
+    zend_result = zend_call_method_if_exists(object, method_name, retval,
+                                             param_count, param_values);
+  }
+  zend_catch { zend_result = FAILURE; }
+  zend_end_try();
+  return zend_result;
+}
+#elif ZEND_MODULE_API_NO < ZEND_8_2_X_API_NO \
+    && ZEND_MODULE_API_NO >= ZEND_8_0_X_API_NO
+static int nr_php_call_try_catch(zval* object_ptr,
+                                 zval* fname,
+                                 zval* retval,
+                                 zend_uint param_count,
+                                 zval* param_values) {
+  /*
+   * With PHP8, `call_user_function_ex` was removed and `call_user_function`
+   * became the recommended function.
+   * According to zend internals documentation:
+   * As of PHP 7.1.0, the function_table argument is not used and should
+   * always be NULL. See for more details:
+   * https://www.phpinternalsbook.com/php7/internal_types/functions/callables.html
+   */
+  int zend_result = FAILURE;
+  zend_try {
+    zend_result = call_user_function(EG(function_table), object_ptr, fname,
+                                     retval, param_count, param_values);
+  }
+  zend_catch { zend_result = FAILURE; }
+  zend_end_try();
+  return zend_result;
+}
+#endif
+
 zval* nr_php_call_user_func(zval* object_ptr,
                             const char* function_name,
                             zend_uint param_count,
@@ -53,11 +103,6 @@ zval* nr_php_call_user_func(zval* object_ptr,
   fname = nr_php_zval_alloc();
   nr_php_zval_str(fname, function_name);
 #if ZEND_MODULE_API_NO >= ZEND_8_2_X_API_NO /* PHP 8.2+ */
-  /*
-   * With PHP 8.2, functions that do not exist will cause a fatal error to
-   * be thrown. `zend_call_method_if_exists` will attempt to call a function and
-   * silently fail if it does not exist
-   */
   if (NULL != object_ptr) {
     object = Z_OBJ_P(object_ptr);
   } else {
@@ -69,26 +114,20 @@ zval* nr_php_call_user_func(zval* object_ptr,
   } else {
     return NULL;
   }
-  zend_try {
-  zend_result = zend_call_method_if_exists(object, method_name, retval,
-                                           param_count, param_values);
-  }
-  zend_catch { zend_result = FAILURE; }
-  zend_end_try();
-#elif ZEND_MODULE_API_NO < ZEND_8_2_X_API_NO \
-    && ZEND_MODULE_API_NO >= ZEND_8_0_X_API_NO
+
   /*
-   * With PHP8, `call_user_function_ex` was removed and `call_user_function`
-   * became the recommended function.  This does't return a FAILURE for
-   * exceptions and needs to be in a try/catch block in order to clean up
-   * properly.
+   * For PHP 8+, in the case of exceptions according to:
+   * https://www.php.net/manual/en/function.call-user-func.php
+   * Callbacks registered with functions such as call_user_func() and
+   * call_user_func_array() will not be called if there is an uncaught exception
+   * thrown in a previous callback. So if we call something that causes an
+   * exception, it will block us from future calls that use call_user_func or
+   * call_user_func_array and hence the need for a try/catch block.
    */
-  zend_try {
-    zend_result = call_user_function(EG(function_table), object_ptr, fname,
-                                     retval, param_count, param_values);
-  }
-  zend_catch { zend_result = FAILURE; }
-  zend_end_try();
+  zend_result =  nr_php_call_try_catch(object, method_name, retval, param_count, param_values);
+#elif ZEND_MODULE_API_NO < ZEND_8_2_X_API_NO \
+    && ZEND_MODULE_API_NO >= ZEND_8_0_X_API_NO
+  zend_result = nr_php_call_try_catch(object_ptr, fname, retval, param_count, param_values);
 
 #else
   zend_result = call_user_function_ex(EG(function_table), object_ptr, fname,

tests/integration/frameworks/wordpress/test_wordpress_apply_filters.php8.php

@@ -52,6 +52,10 @@ function apply_filters($tag, ...$args) {
     call_user_func_array($tag, $args);
 }
 
+//Simple mock of wordpress's get_theme_roots
+function get_theme_roots() {
+}
+
 function h($str) {
     echo "h: ";
     echo $str;

tests/integration/frameworks/wordpress/test_wordpress_do_action.php8.php

@@ -53,6 +53,10 @@ function do_action($tag, ...$args) {
     call_user_func_array($tag, $args);
 }
 
+//Simple mock of wordpress's get_theme_roots
+function get_theme_roots() {
+}
+
 function h() {
     echo "h\n";
     throw new Exception("Test Exception");