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.
newrelic · Mar 14, 2024 · c497191 · c497191
1 parent 86f7753
commit c497191
Show file tree

Hide file tree

Showing 15 changed files with 202 additions and 41 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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": "??",