From 49a6c1a4fdc5031d1b631738a60a0c566c45396d Mon Sep 17 00:00:00 2001 From: ZNeumann Date: Mon, 5 Feb 2024 13:25:35 -0700 Subject: [PATCH] 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 --- agent/php_agent.c | 45 +++++++++++++++++++++++++++++++++++---------- agent/php_agent.h | 18 +++++++++++++++++- agent/php_error.c | 30 +++++++++++++----------------- agent/php_execute.c | 38 +++++++++++--------------------------- 4 files changed, 76 insertions(+), 55 deletions(-) diff --git a/agent/php_agent.c b/agent/php_agent.c index 2502000c7..e064afc76 100644 --- a/agent/php_agent.c +++ b/agent/php_agent.c @@ -4,6 +4,7 @@ */ #include "php_agent.h" #include "php_call.h" +#include "php_error.h" #include "php_globals.h" #include "php_hash.h" #include "nr_rum.h" @@ -11,6 +12,8 @@ #include "util_memory.h" #include "util_strings.h" +#include + #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, diff --git a/agent/php_agent.h b/agent/php_agent.h index deeabac79..2d061ea00 100644 --- a/agent/php_agent.h +++ b/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 diff --git a/agent/php_error.c b/agent/php_error.c index d59858147..3702870a3 100644 --- a/agent/php_error.c +++ b/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; } diff --git a/agent/php_execute.c b/agent/php_execute.c index c02a033f2..628f5244a 100644 --- a/agent/php_execute.c +++ b/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