From c497191299c8837f9bdc7f883b99d18c952071ce Mon Sep 17 00:00:00 2001 From: Amber Sistla Date: Thu, 14 Mar 2024 10:31:29 -0700 Subject: [PATCH] fix(agent): Add missing errors decode and prioritization (#850) Add remaining error codes listed in https://github.com/php/php-src/blob/master/Zend/zend_errors.h for completeness in decoding and prioritizing errors. Added/modified tests accordingly. --- agent/Makefile.frag | 1 + agent/php_error.c | 58 +++++++-- agent/php_error.h | 9 ++ agent/tests/test_php_error.c | 115 ++++++++++++++++++ .../errors/test_E_DEPRECATED_2.php7.php | 4 +- .../errors/test_E_DEPRECATED_2.php8.php | 4 +- .../errors/test_E_DEPRECATED_2.php81.php | 4 +- .../errors/test_E_DEPRECATED_payload1.php | 4 +- .../errors/test_E_DEPRECATED_payload2.php | 4 +- .../errors/test_E_DEPRECATED_payload3.php | 4 +- .../errors/test_E_ERROR.php7.4.php | 3 + .../errors/test_E_RECOVERABLE.php7.php | 4 +- .../integration/errors/test_E_STRICT.php7.php | 10 +- .../errors/test_E_USER_DEPRECATED.php | 4 +- .../integration/errors/test_E_USER_ERROR.php | 15 +-- 15 files changed, 202 insertions(+), 41 deletions(-) create mode 100644 agent/tests/test_php_error.c diff --git a/agent/Makefile.frag b/agent/Makefile.frag index 3633379cb..f11dfbf75 100644 --- a/agent/Makefile.frag +++ b/agent/Makefile.frag @@ -97,6 +97,7 @@ TEST_BINARIES = \ tests/test_pdo_mysql \ tests/test_pdo_pgsql \ tests/test_pgsql \ + tests/test_php_error \ tests/test_php_execute \ tests/test_php_minit \ tests/test_php_stack \ diff --git a/agent/php_error.c b/agent/php_error.c index 3702870a3..5f0015575 100644 --- a/agent/php_error.c +++ b/agent/php_error.c @@ -287,6 +287,9 @@ PHP_FUNCTION(newrelic_exception_handler) { #endif /* PHP7 */ } +/* PHP Fatal errors: E_ERROR | E_USER_ERROR | E_PARSE | E_CORE_ERROR | + * E_COMPILE_ERROR | E_RECOVERABLE_ERROR */ + int nr_php_error_get_priority(int type) { switch (type) { case E_PARSE: @@ -299,6 +302,8 @@ int nr_php_error_get_priority(int type) { return 50; case E_ERROR: return 50; + case E_RECOVERABLE_ERROR: + return 50; case E_COMPILE_WARNING: return 40; case E_CORE_WARNING: @@ -307,6 +312,12 @@ int nr_php_error_get_priority(int type) { return 40; case E_WARNING: return 40; + case E_DEPRECATED: + return 30; + case E_USER_DEPRECATED: + return 30; + case E_STRICT: /* Included for backward compatibility */ + return 10; case E_USER_NOTICE: return 0; case E_NOTICE: @@ -466,30 +477,51 @@ static char* nr_php_error_exception_message(zval* exception TSRMLS_DC) { return message; } -static const char* get_error_type_string(int type) { +const char* nr_php_error_get_type_string(int type) { + /* NOTE: PHP 7 makes E_STRICT irrelevant, reclassifying most of the errors as + * proper warnings, notices or E_DEPRECATED: + * https://wiki.php.net/rfc/reclassify_e_strict The E_STRICT constant will be + * retained for better compatibility, it will simply no longer have meaning in + * PHP 7. While PHP 7 was backward compatible, PHP 8 does not use E_STRICT. + */ + + /* Note: + * With PHP7.4+ we should never actually be getting E_ERROR, or + * E_RECOVERABLE_ERROR here. Both are fatal errors and are handled by the + * agent's uncaught exceptions logic (unless E_RECOVERABLE_ERROR in which case + * there is no error to see) + */ switch (type) { - case E_ERROR: + case E_ERROR: /* 1 */ return "E_ERROR"; - case E_WARNING: + case E_WARNING: /* 2 */ return "E_WARNING"; - case E_PARSE: + case E_PARSE: /* 4 */ return "E_PARSE"; - case E_NOTICE: + case E_NOTICE: /* 8 */ return "E_NOTICE"; - case E_CORE_ERROR: + case E_CORE_ERROR: /* 16 */ return "E_CORE_ERROR"; - case E_CORE_WARNING: + case E_CORE_WARNING: /* 32 */ return "E_CORE_WARNING"; - case E_COMPILE_ERROR: + case E_COMPILE_ERROR: /* 64 */ return "E_COMPILE_ERROR"; - case E_COMPILE_WARNING: + case E_COMPILE_WARNING: /* 128 */ return "E_COMPILE_WARNING"; - case E_USER_ERROR: + case E_USER_ERROR: /* 256 */ return "E_USER_ERROR"; - case E_USER_WARNING: + case E_USER_WARNING: /* 512 */ return "E_USER_WARNING"; - case E_USER_NOTICE: + case E_USER_NOTICE: /* 1024 */ return "E_USER_NOTICE"; + case E_STRICT: /* 2048 */ + return "E_STRICT"; + case E_RECOVERABLE_ERROR: /* 4096 */ + return "E_RECOVERABLE_ERROR"; + case E_DEPRECATED: /* 8192 */ + return "E_DEPRECATED"; + case E_USER_DEPRECATED: /* 16348 */ + return "E_USER_DEPRECATED"; default: return "Error"; } @@ -597,7 +629,7 @@ void nr_php_error_cb(int type, #endif /* PHP < 8.0 */ stack_json = nr_php_backtrace_to_json(0 TSRMLS_CC); - errclass = get_error_type_string(type); + errclass = nr_php_error_get_type_string(type); nr_txn_record_error(NRPRG(txn), nr_php_error_get_priority(type), true, msg, errclass, stack_json); diff --git a/agent/php_error.h b/agent/php_error.h index 98c06708a..decf80877 100644 --- a/agent/php_error.h +++ b/agent/php_error.h @@ -112,6 +112,15 @@ extern PHP_FUNCTION(newrelic_exception_handler); */ extern int nr_php_error_get_priority(int type); +/* + * Purpose : Converts a PHP error type into a readable string. + * + * Params : 1. The error type. + * + * Returns : The PHP error type as constant, static string (must not be freed). + */ +extern const char* nr_php_error_get_type_string(int type); + /* * Purpose : Install newrelic_exception_handler as the user exception handler * in PHP. diff --git a/agent/tests/test_php_error.c b/agent/tests/test_php_error.c new file mode 100644 index 000000000..6e62a94ae --- /dev/null +++ b/agent/tests/test_php_error.c @@ -0,0 +1,115 @@ +/* + * Copyright 2020 New Relic Corporation. All rights reserved. + * SPDX-License-Identifier: Apache-2.0 + */ +#include "tlib_php.h" +#include "tlib_main.h" + +#include "php_agent.h" +#include "php_call.h" +#include "php_globals.h" +#include "php_error.h" + +tlib_parallel_info_t parallel_info + = {.suggested_nthreads = -1, .state_size = 0}; + +static void test_error_get_priority() { + /* + * Test : Unknown type. + */ + tlib_pass_if_int_equal("Unknown error type", 20, + nr_php_error_get_priority(-1)); + tlib_pass_if_int_equal("Unknown error type", 20, + nr_php_error_get_priority(3)); + + /* + * Test : Normal operation. + */ + + tlib_pass_if_int_equal("Known error type", 0, + nr_php_error_get_priority(E_NOTICE)); + tlib_pass_if_int_equal("Known error type", 0, + nr_php_error_get_priority(E_USER_NOTICE)); + tlib_pass_if_int_equal("Known error type", 10, + nr_php_error_get_priority(E_STRICT)); + tlib_pass_if_int_equal("Known error type", 30, + nr_php_error_get_priority(E_USER_DEPRECATED)); + tlib_pass_if_int_equal("Known error type", 30, + nr_php_error_get_priority(E_DEPRECATED)); + tlib_pass_if_int_equal("Known error type", 40, + nr_php_error_get_priority(E_USER_WARNING)); + tlib_pass_if_int_equal("Known error type", 40, + nr_php_error_get_priority(E_WARNING)); + tlib_pass_if_int_equal("Known error type", 40, + nr_php_error_get_priority(E_CORE_WARNING)); + tlib_pass_if_int_equal("Known error type", 40, + nr_php_error_get_priority(E_COMPILE_WARNING)); + tlib_pass_if_int_equal("Known error type", 50, + nr_php_error_get_priority(E_RECOVERABLE_ERROR)); + tlib_pass_if_int_equal("Known error type", 50, + nr_php_error_get_priority(E_ERROR)); + tlib_pass_if_int_equal("Known error type", 50, + nr_php_error_get_priority(E_USER_ERROR)); + tlib_pass_if_int_equal("Known error type", 50, + nr_php_error_get_priority(E_CORE_ERROR)); + tlib_pass_if_int_equal("Known error type", 50, + nr_php_error_get_priority(E_COMPILE_ERROR)); + tlib_pass_if_int_equal("Known error type", 50, + nr_php_error_get_priority(E_PARSE)); +} + +static void test_get_error_type_string() { + /* + * Test : Unknown type. + */ + tlib_pass_if_str_equal("Unknown error type", "Error", + nr_php_error_get_type_string(-1)); + tlib_pass_if_str_equal("Unknown error type", "Error", + nr_php_error_get_type_string(3)); + + /* + * Test : Normal operation. + */ + + tlib_pass_if_str_equal("Known error type", "E_NOTICE", + nr_php_error_get_type_string(E_NOTICE)); + tlib_pass_if_str_equal("Known error type", "E_USER_NOTICE", + nr_php_error_get_type_string(E_USER_NOTICE)); + tlib_pass_if_str_equal("Known error type", "E_STRICT", + nr_php_error_get_type_string(E_STRICT)); + tlib_pass_if_str_equal("Known error type", "E_USER_NOTICE", + nr_php_error_get_type_string(E_USER_NOTICE)); + tlib_pass_if_str_equal("Known error type", "E_USER_DEPRECATED", + nr_php_error_get_type_string(E_USER_DEPRECATED)); + tlib_pass_if_str_equal("Known error type", "E_DEPRECATED", + nr_php_error_get_type_string(E_DEPRECATED)); + tlib_pass_if_str_equal("Known error type", "E_USER_WARNING", + nr_php_error_get_type_string(E_USER_WARNING)); + tlib_pass_if_str_equal("Known error type", "E_WARNING", + nr_php_error_get_type_string(E_WARNING)); + tlib_pass_if_str_equal("Known error type", "E_CORE_WARNING", + nr_php_error_get_type_string(E_CORE_WARNING)); + tlib_pass_if_str_equal("Known error type", "E_COMPILE_WARNING", + nr_php_error_get_type_string(E_COMPILE_WARNING)); + tlib_pass_if_str_equal("Known error type", "E_RECOVERABLE_ERROR", + nr_php_error_get_type_string(E_RECOVERABLE_ERROR)); + tlib_pass_if_str_equal("Known error type", "E_ERROR", + nr_php_error_get_type_string(E_ERROR)); + tlib_pass_if_str_equal("Known error type", "E_USER_ERROR", + nr_php_error_get_type_string(E_USER_ERROR)); + tlib_pass_if_str_equal("Known error type", "E_CORE_ERROR", + nr_php_error_get_type_string(E_CORE_ERROR)); + tlib_pass_if_str_equal("Known error type", "E_COMPILE_ERROR", + nr_php_error_get_type_string(E_COMPILE_ERROR)); + tlib_pass_if_str_equal("Known error type", "E_PARSE", + nr_php_error_get_type_string(E_PARSE)); +} + +void test_main(void* p NRUNUSED) { + tlib_php_engine_create("" PTSRMLS_CC); + + test_error_get_priority(); + test_get_error_type_string(); + + tlib_php_engine_destroy(TSRMLS_C); +} diff --git a/tests/integration/errors/test_E_DEPRECATED_2.php7.php b/tests/integration/errors/test_E_DEPRECATED_2.php7.php index 994ecbd57..b82fd3e26 100644 --- a/tests/integration/errors/test_E_DEPRECATED_2.php7.php +++ b/tests/integration/errors/test_E_DEPRECATED_2.php7.php @@ -36,7 +36,7 @@ "?? when", "OtherTransaction/php__FILE__", "mktime(): You should be using the time() function instead", - "Error", + "E_DEPRECATED", { "stack_trace": [ " in mktime called at __FILE__ (??)" @@ -62,7 +62,7 @@ { "type": "TransactionError", "timestamp": "??", - "error.class": "Error", + "error.class": "E_DEPRECATED", "error.message": "mktime(): You should be using the time() function instead", "transactionName": "OtherTransaction\/php__FILE__", "duration": "??", diff --git a/tests/integration/errors/test_E_DEPRECATED_2.php8.php b/tests/integration/errors/test_E_DEPRECATED_2.php8.php index 047df65d2..f9b037756 100644 --- a/tests/integration/errors/test_E_DEPRECATED_2.php8.php +++ b/tests/integration/errors/test_E_DEPRECATED_2.php8.php @@ -36,7 +36,7 @@ "?? when", "OtherTransaction/php__FILE__", "Required parameter $b follows optional parameter $a", - "Error", + "E_DEPRECATED", { "stack_trace": "??", "agentAttributes": "??", @@ -60,7 +60,7 @@ { "type": "TransactionError", "timestamp": "??", - "error.class": "Error", + "error.class": "E_DEPRECATED", "error.message": "Required parameter $b follows optional parameter $a", "transactionName": "OtherTransaction\/php__FILE__", "duration": "??", diff --git a/tests/integration/errors/test_E_DEPRECATED_2.php81.php b/tests/integration/errors/test_E_DEPRECATED_2.php81.php index f90f2f92b..15d1e0b3a 100644 --- a/tests/integration/errors/test_E_DEPRECATED_2.php81.php +++ b/tests/integration/errors/test_E_DEPRECATED_2.php81.php @@ -33,7 +33,7 @@ "?? when", "OtherTransaction/php__FILE__", "Optional parameter $a declared before required parameter $b is implicitly treated as a required parameter", - "Error", + "E_DEPRECATED", { "stack_trace": "??", "agentAttributes": "??", @@ -57,7 +57,7 @@ { "type": "TransactionError", "timestamp": "??", - "error.class": "Error", + "error.class": "E_DEPRECATED", "error.message": "Optional parameter $a declared before required parameter $b is implicitly treated as a required parameter", "transactionName": "OtherTransaction\/php__FILE__", "duration": "??", diff --git a/tests/integration/errors/test_E_DEPRECATED_payload1.php b/tests/integration/errors/test_E_DEPRECATED_payload1.php index 5626dcb02..7060c5a3b 100644 --- a/tests/integration/errors/test_E_DEPRECATED_payload1.php +++ b/tests/integration/errors/test_E_DEPRECATED_payload1.php @@ -27,7 +27,7 @@ "??", "OtherTransaction/php__FILE__", "Function newrelic_accept_distributed_trace_payload() is deprecated. Please see https://docs.newrelic.com/docs/agents/php-agent/features/distributed-tracing-php-agent#manual for more details.", - "Error", + "E_DEPRECATED", { "stack_trace": [ " in newrelic_accept_distributed_trace_payload called at __FILE__ (??)" @@ -53,7 +53,7 @@ { "type": "TransactionError", "timestamp": "??", - "error.class": "Error", + "error.class": "E_DEPRECATED", "error.message": "Function newrelic_accept_distributed_trace_payload() is deprecated. Please see https://docs.newrelic.com/docs/agents/php-agent/features/distributed-tracing-php-agent#manual for more details.", "transactionName": "OtherTransaction\/php__FILE__", "duration": "??", diff --git a/tests/integration/errors/test_E_DEPRECATED_payload2.php b/tests/integration/errors/test_E_DEPRECATED_payload2.php index 57efa3bd5..4e6c5ee4e 100644 --- a/tests/integration/errors/test_E_DEPRECATED_payload2.php +++ b/tests/integration/errors/test_E_DEPRECATED_payload2.php @@ -28,7 +28,7 @@ "??", "OtherTransaction/php__FILE__", "Function newrelic_accept_distributed_trace_payload_httpsafe() is deprecated. Please see https://docs.newrelic.com/docs/agents/php-agent/features/distributed-tracing-php-agent#manual for more details.", - "Error", + "E_DEPRECATED", { "stack_trace": [ " in newrelic_accept_distributed_trace_payload_httpsafe called at __FILE__ (??)" @@ -54,7 +54,7 @@ { "type": "TransactionError", "timestamp": "??", - "error.class": "Error", + "error.class": "E_DEPRECATED", "error.message": "Function newrelic_accept_distributed_trace_payload_httpsafe() is deprecated. Please see https://docs.newrelic.com/docs/agents/php-agent/features/distributed-tracing-php-agent#manual for more details.", "transactionName": "OtherTransaction\/php__FILE__", "duration": "??", diff --git a/tests/integration/errors/test_E_DEPRECATED_payload3.php b/tests/integration/errors/test_E_DEPRECATED_payload3.php index dbad21a38..a83c862f2 100644 --- a/tests/integration/errors/test_E_DEPRECATED_payload3.php +++ b/tests/integration/errors/test_E_DEPRECATED_payload3.php @@ -28,7 +28,7 @@ "??", "OtherTransaction/php__FILE__", "Function newrelic_create_distributed_trace_payload() is deprecated. Please see https://docs.newrelic.com/docs/agents/php-agent/features/distributed-tracing-php-agent#manual for more details.", - "Error", + "E_DEPRECATED", { "stack_trace": [ " in newrelic_create_distributed_trace_payload called at __FILE__ (??)" @@ -54,7 +54,7 @@ { "type": "TransactionError", "timestamp": "??", - "error.class": "Error", + "error.class": "E_DEPRECATED", "error.message": "Function newrelic_create_distributed_trace_payload() is deprecated. Please see https://docs.newrelic.com/docs/agents/php-agent/features/distributed-tracing-php-agent#manual for more details.", "transactionName": "OtherTransaction\/php__FILE__", "duration": "??", diff --git a/tests/integration/errors/test_E_ERROR.php7.4.php b/tests/integration/errors/test_E_ERROR.php7.4.php index fe6e67abd..aff68c9d5 100644 --- a/tests/integration/errors/test_E_ERROR.php7.4.php +++ b/tests/integration/errors/test_E_ERROR.php7.4.php @@ -6,6 +6,9 @@ /*DESCRIPTION The agent should capture and report fatal errors. +With PHP 7.4+, E_ERROR are fatal errors triggered by exceptions and are no longer handled +using nr_php_error_cb with uses the error type as the error.class. Instead, they are handled +by newrelic_exception_handler which uses the exception name as the error.class. */ /*SKIPIF diff --git a/tests/integration/errors/test_E_RECOVERABLE.php7.php b/tests/integration/errors/test_E_RECOVERABLE.php7.php index 2eb2581a6..18db40280 100644 --- a/tests/integration/errors/test_E_RECOVERABLE.php7.php +++ b/tests/integration/errors/test_E_RECOVERABLE.php7.php @@ -30,7 +30,7 @@ "?? when", "OtherTransaction/php__FILE__", "Object of class stdClass could not be converted to string", - "Error", + "E_RECOVERABLE_ERROR", { "stack_trace": [ " in run_test_in_a_function called at __FILE__ (??)" @@ -56,7 +56,7 @@ { "type": "TransactionError", "timestamp": "??", - "error.class": "Error", + "error.class": "E_RECOVERABLE_ERROR", "error.message": "Object of class stdClass could not be converted to string", "transactionName": "OtherTransaction\/php__FILE__", "duration": "??", diff --git a/tests/integration/errors/test_E_STRICT.php7.php b/tests/integration/errors/test_E_STRICT.php7.php index fc30bc8cd..545bd7e0c 100644 --- a/tests/integration/errors/test_E_STRICT.php7.php +++ b/tests/integration/errors/test_E_STRICT.php7.php @@ -6,6 +6,9 @@ /*DESCRIPTION The agent should capture and report strict standards warnings. +PHP 7 makes E_STRICT irrelevant, reclassifying most of the errors as proper warnings, +notices or E_DEPRECATED: https://wiki.php.net/rfc/reclassify_e_strict +The E_STRICT constant will be retained for better compatibility, it will simply no longer have meaning in PHP 7. */ /*SKIPIF @@ -14,8 +17,9 @@ die("skip: PHP 5 not supported\n"); } if (version_compare(PHP_VERSION, "7.4", ">=")) { - die("skip: PHP >= 7.4.0 not supported\n"); + die("skip: PHP 7.4+ not supported\n"); } + */ /*INI @@ -34,7 +38,7 @@ "?? when", "OtherTransaction/php__FILE__", "htmlentities(): Only basic entities substitution is supported for multi-byte encodings other than UTF-8; functionality is equivalent to htmlspecialchars", - "Error", + "E_STRICT", { "stack_trace": [ " in htmlentities called at __FILE__ (??)" @@ -60,7 +64,7 @@ { "type": "TransactionError", "timestamp": "??", - "error.class": "Error", + "error.class": "E_STRICT", "error.message": "htmlentities(): Only basic entities substitution is supported for multi-byte encodings other than UTF-8; functionality is equivalent to htmlspecialchars", "transactionName": "OtherTransaction\/php__FILE__", "duration": "??", diff --git a/tests/integration/errors/test_E_USER_DEPRECATED.php b/tests/integration/errors/test_E_USER_DEPRECATED.php index 24c457108..12577c722 100644 --- a/tests/integration/errors/test_E_USER_DEPRECATED.php +++ b/tests/integration/errors/test_E_USER_DEPRECATED.php @@ -26,7 +26,7 @@ "?? when", "OtherTransaction/php__FILE__", "Sample E_USER_DEPRECATED", - "Error", + "E_USER_DEPRECATED", { "stack_trace": [ " in trigger_error called at __FILE__ (??)" @@ -52,7 +52,7 @@ { "type": "TransactionError", "timestamp": "??", - "error.class": "Error", + "error.class": "E_USER_DEPRECATED", "error.message": "Sample E_USER_DEPRECATED", "transactionName": "OtherTransaction\/php__FILE__", "duration": "??", diff --git a/tests/integration/errors/test_E_USER_ERROR.php b/tests/integration/errors/test_E_USER_ERROR.php index 7faf20d03..845f471a1 100644 --- a/tests/integration/errors/test_E_USER_ERROR.php +++ b/tests/integration/errors/test_E_USER_ERROR.php @@ -5,8 +5,7 @@ */ /*DESCRIPTION -The agent should capture and report user-generated fatal errors along with a -stack trace. +The agent should capture and report E_USER_ERROR events. */ /*INI @@ -15,7 +14,7 @@ */ /*EXPECT_REGEX -^\s*(PHP )?Fatal error:\s*Sample E_USER_ERROR in .*? on line [0-9]+\s*$ +^\s*(PHP )?Fatal error:\s*Incorrect parameters, correct ones expected .*? on line [0-9]+\s*$ */ /*EXPECT_TRACED_ERRORS @@ -25,12 +24,10 @@ [ "?? when", "OtherTransaction/php__FILE__", - "Sample E_USER_ERROR", + "Incorrect parameters, correct ones expected", "E_USER_ERROR", { - "stack_trace": [ - " in trigger_error called at __FILE__ (??)" - ], + "stack_trace": "??", "agentAttributes": "??", "intrinsics": "??" }, @@ -53,7 +50,7 @@ "type": "TransactionError", "timestamp": "??", "error.class": "E_USER_ERROR", - "error.message": "Sample E_USER_ERROR", + "error.message": "Incorrect parameters, correct ones expected", "transactionName": "OtherTransaction\/php__FILE__", "duration": "??", "nr.transactionGuid": "??", @@ -70,4 +67,4 @@ ] */ -trigger_error("Sample E_USER_ERROR", E_USER_ERROR); +trigger_error("Incorrect parameters, correct ones expected", E_USER_ERROR);