refactor: condense exception handling instrumentation (#825)

Move exception handler instrumentation from fcall_begin to fcall_end.
This requires a change of how we are obtaining the fields of the
exception, because calling `nr_php_call` during the handling of an
exception does not play nicely with PHP.

---------

Co-authored-by: Michal Nowacki <[email protected]>

agent/php_agent.c

@@ -4,13 +4,16 @@
  */
 #include "php_agent.h"
 #include "php_call.h"
+#include "php_error.h"
 #include "php_globals.h"
 #include "php_hash.h"
 #include "nr_rum.h"
 #include "util_logging.h"
 #include "util_memory.h"
 #include "util_strings.h"
 
+#include <Zend/zend_exceptions.h>
+
 #include "php_variables.h"
 
 static zval* nr_php_get_zval_object_property_with_class_internal(
@@ -63,28 +66,50 @@ static zval* nr_php_get_zval_object_property_with_class_internal(
 
 zval* nr_php_get_zval_object_property(zval* object,
                                       const char* cname TSRMLS_DC) {
-  char* name;
-
-  if ((0 == object) || (0 == cname) || (0 == cname[0])) {
-    return 0;
+  if ((NULL == object) || (NULL == cname) || (0 == cname[0])) {
+    return NULL;
   }
 
-  name = (char*)nr_alloca(nr_strlen(cname) + 1);
-  nr_strcpy(name, cname);
-
   if (nr_php_is_zval_valid_object(object)) {
     return nr_php_get_zval_object_property_with_class_internal(
-        object, Z_OBJCE_P(object), name TSRMLS_CC);
+        object, Z_OBJCE_P(object), cname TSRMLS_CC);
   } else if (IS_ARRAY == Z_TYPE_P(object)) {
     zval* data;
 
-    data = nr_php_zend_hash_find(Z_ARRVAL_P(object), name);
+    data = nr_php_zend_hash_find(Z_ARRVAL_P(object), cname);
     if (data) {
       return data;
     }
   }
 
-  return 0;
+  return NULL;
+}
+
+zval* nr_php_get_zval_base_exception_property(zval* exception,
+                                              const char* cname) {
+  zend_class_entry* ce;
+
+  if ((NULL == exception) || (NULL == cname) || (0 == cname[0])) {
+    return NULL;
+  }
+
+  if (nr_php_is_zval_valid_object(exception)) {
+    if (nr_php_error_zval_is_exception(exception)) {
+    /* 
+     * This is inline with what the php source code does to extract properties from
+     * errors and exceptions. Without getting the base class entry, certain values
+     * are incorrect for either errors/exceptions.
+     */
+#if ZEND_MODULE_API_NO >= ZEND_8_0_X_API_NO
+      ce = zend_get_exception_base(Z_OBJ_P(exception));
+#else
+      ce = zend_get_exception_base(exception);
+#endif
+      return nr_php_get_zval_object_property_with_class_internal(
+          exception, ce, cname TSRMLS_CC);
+    }
+  }
+  return NULL;
 }
 
 zval* nr_php_get_zval_object_property_with_class(zval* object,

agent/php_agent.h

@@ -133,13 +133,29 @@ typedef unsigned int nr_php_object_handle_t;
  * Purpose : Extract the named entity from a PHP object zval.
  *
  * Params  : 1. The object to extract the property from.
- *           2. The name of the object.
+ *           2. The name of the object property.
  *
  * Returns : The specified element or NULL if it was not found.
  */
 extern zval* nr_php_get_zval_object_property(zval* object,
                                              const char* cname TSRMLS_DC);
 
+/*
+ * Purpose : Extract the named entity from a PHP exception zval.
+ *           This determines if the passed exception is a PHP error
+ *           or a PHP exception, and extracts accordingly.
+ *
+ * Params  : 1. The exception to extract the property from.
+ *           2. The name of the exception property.
+ *
+ * Returns : The specified element or NULL if it was not found.
+ *           This returns the zval owned by the Zend engine, so
+ *           a refernce incremenet should take place if the return
+ *           value is to be kept around beyond the caller's scope.
+ */
+extern zval* nr_php_get_zval_base_exception_property(zval* exception,
+                                                     const char* cname);
+
 /*
  * Purpose : Extract the named property from a PHP object zval in a particular
  *           class context. (Useful for getting private properties from

agent/php_error.c

@@ -381,18 +381,22 @@ void nr_php_error_install_exception_handler(TSRMLS_D) {
  *              checked in any way, and is assumed to be a valid Exception
  *              object.
  *
- * Returns : A zval for the stack trace, which the caller will need to destroy,
- *           or NULL if no trace is available.
+ * Returns : A zval for the stack trace, which the caller will need to
+ *           delete reference to, or NULL if no trace is available.
  */
 static zval* nr_php_error_exception_stack_trace(zval* exception TSRMLS_DC) {
   zval* trace;
 
-  trace = nr_php_call(exception, "getTrace");
-  if (!nr_php_is_zval_valid_array(trace)) {
-    nr_php_zval_free(&trace);
+  trace = nr_php_get_zval_base_exception_property(exception, "trace");
+  if (NULL == trace || nr_php_is_zval_null(trace)) {
     return NULL;
   }
+  Z_ADDREF_P(trace);
 
+  if (!nr_php_is_zval_valid_array(trace)) {
+    Z_DELREF_P(trace);
+    return NULL;
+  }
   return trace;
 }
 
@@ -407,14 +411,13 @@ static zval* nr_php_error_exception_stack_trace(zval* exception TSRMLS_DC) {
  *           will need to free, or NULL if no file name is available.
  */
 static char* nr_php_error_exception_file(zval* exception TSRMLS_DC) {
-  zval* file_zv = nr_php_call(exception, "getFile");
+  zval* file_zv = nr_php_get_zval_object_property(exception, "file");
   char* file = NULL;
 
   if (nr_php_is_zval_valid_string(file_zv)) {
     file = nr_strndup(Z_STRVAL_P(file_zv), Z_STRLEN_P(file_zv));
   }
 
-  nr_php_zval_free(&file_zv);
   return file;
 }
 
@@ -429,7 +432,7 @@ static char* nr_php_error_exception_file(zval* exception TSRMLS_DC) {
  */
 static long nr_php_error_exception_line(zval* exception TSRMLS_DC) {
   long line = 0;
-  zval* line_zv = nr_php_call(exception, "getLine");
+  zval* line_zv = nr_php_get_zval_object_property(exception, "line");
 
   /*
    * All scalar types can be coerced to IS_LONG.
@@ -439,7 +442,6 @@ static long nr_php_error_exception_line(zval* exception TSRMLS_DC) {
     line = Z_LVAL_P(line_zv);
   }
 
-  nr_php_zval_free(&line_zv);
   return line;
 }
 
@@ -454,19 +456,13 @@ static long nr_php_error_exception_line(zval* exception TSRMLS_DC) {
  *           will need to free, or NULL if no message could be extracted.
  */
 static char* nr_php_error_exception_message(zval* exception TSRMLS_DC) {
-  /*
-   * This intentionally prefers getMessage(): __toString() can include stack
-   * dumps generated by PHP, which can include user data that we don't want to
-   * send up and for which it isn't obvious that it would be sent.
-   */
-  zval* message_zv = nr_php_call(exception, "getMessage");
+  zval* message_zv = nr_php_get_zval_base_exception_property(exception, "message");
   char* message = NULL;
 
   if (nr_php_is_zval_valid_string(message_zv)) {
     message = nr_strndup(Z_STRVAL_P(message_zv), Z_STRLEN_P(message_zv));
   }
 
-  nr_php_zval_free(&message_zv);
   return message;
 }
 
@@ -725,7 +721,7 @@ nr_status_t nr_php_error_record_exception(nrtxn_t* txn,
   nr_free(klass);
   nr_free(message);
   nr_free(stack_json);
-  nr_php_zval_free(&stack_trace);
+  Z_DELREF_P(stack_trace);
 
   return NR_SUCCESS;
 }

agent/php_execute.c

@@ -1873,29 +1873,6 @@ static void nr_php_instrument_func_begin(NR_EXECUTE_PROTO) {
   }
   wraprec = nr_php_get_wraprec(execute_data->func);
 
-  /*
-   * Check if it's a custom error handler. We don't need to wait for
-   * fcall_end to record the error to the transaction; it can be done earlier
-   * in fcall_begin. However, we must wait until fcall_end to add the error
-   * to any possible segment(s), as at this point we do not know when it
-   * will be caught.
-   */
-  if (NULL != wraprec && wraprec->is_exception_handler) {
-    /*
-     * Before starting the error handler segment, put the error it handled on
-     * the transaction. The choice of E_ERROR for the error level is
-     * basically arbitrary, but matches the error level PHP uses if there
-     * isn't an exception handler, so this should give more consistency for
-     * the user in terms of what they'll see with and without an exception
-     * handler installed.
-     */
-    zval* exception
-        = nr_php_get_user_func_arg(1, NR_EXECUTE_ORIG_ARGS TSRMLS_CC);
-    nr_php_error_record_exception(
-        NRPRG(txn), exception, nr_php_error_get_priority(E_ERROR), false,
-        "Uncaught exception ", &NRPRG(exception_filters) TSRMLS_CC);
-  }
-
   segment = nr_php_stacked_segment_init(segment);
   if (nrunlikely(NULL == segment)) {
     nrl_verbosedebug(NRL_AGENT, "Error initializing stacked segment.");
@@ -1984,11 +1961,18 @@ static void nr_php_instrument_func_end(NR_EXECUTE_PROTO) {
 
   if (wraprec && wraprec->is_exception_handler) {
     /*
-     * An exception handler sets the transaction exception in fcall_begin
-     * and does not have a return value, like an fcall_end during an
-     * uncaught exception. This code path is here to simplify and
-     * explicitly enumerate the possible cases.
+     * After running the exception handler segment, create an error from
+     * the exception it handled, and save the error in the transaction.
+     * The choice of E_ERROR for the error level is basically arbitrary,
+     * but matches the error level PHP uses if there isn't an exception
+     * handler, so this should give more consistency for the user in terms
+     * of what they'll see with and without an exception handler installed.
      */
+    zval* exception
+        = nr_php_get_user_func_arg(1, NR_EXECUTE_ORIG_ARGS TSRMLS_CC);
+    nr_php_error_record_exception(
+        NRPRG(txn), exception, nr_php_error_get_priority(E_ERROR), false,
+        "Uncaught exception ", &NRPRG(exception_filters) TSRMLS_CC);
   } else if (NULL == nr_php_get_return_value(NR_EXECUTE_ORIG_ARGS)) {
     /*
      * Having no return value (and not being an exception handler) indicates