Skip to content

Commit 8f0ae2c

Browse files
zsistlazsistla
authored
fix(agent): Fix error reporting when EH_THROW is enabled (#876)
This PR 1)checks if EH_THROW(which signals to PHP to throw an exception from the error) is toggled and if so does not record an error because if uncaught it will be handled by the exception handler and if caught there's no need to record it. 2) add tests to verify fix More details: /* * For a the following error codes: * E_WARNING || E_CORE_WARNING || E_COMPILE_WARNING || E_USER_WARNING * PHP triggers an exception if EH_THROW is toggled on and then immediately * returns after throwing the exception. See for more info: * https://github.com/php/php-src/blob/master/main/main.c In that case, we * should not handle it, but we should exist and let the exception handler * deal with it; otherwise, we could record an error even if an exception is * caught. */ In the case where a php E_WARNING error was triggered.  We intercept the error from PHP and record it then pass execution back to PHP. But, for instance, in the particular case that deals with a filehandling which toggles EH_THROW on here: https://github.com/php/php-src/blob/master/ext/fileinfo/fileinfo.c#L176 PHP then handles the error but for a certain number of error codes, PHP triggers an exception if EH_THROW is toggled on: https://github.com/php/php-src/blob/master/main/main.c#L1254C1-L1259C24 and then immediately returns after throwing the exception. This leads to the case in the issue where the exception that was triggered is caught, but we recorded the error....
1 parent e0d2d12 commit 8f0ae2c

File tree

3 files changed

+206
-2
lines changed

3 files changed

+206
-2
lines changed

‎agent/php_error.c

+25-2
Original file line numberDiff line numberDiff line change
@@ -561,12 +561,36 @@ static int nr_php_should_record_error(int type, const char* format TSRMLS_DC) {
561561
return 0;
562562
}
563563

564+
/*
565+
* For a the following error types:
566+
* E_WARNING || E_CORE_WARNING || E_COMPILE_WARNING || E_USER_WARNING
567+
* PHP triggers an exception if EH_THROW is toggled on and then immediately
568+
* returns after throwing the exception. PHP 7 additionally triggers an exception
569+
* for E_RECOVERABLE_ERROR.
570+
* See for more info:
571+
* https://github.com/php/php-src/blob/master/main/main.c In that case, we
572+
* should not handle it, but we should exit and let the exception handler
573+
* deal with it; otherwise, we could record an error even if an exception is
574+
* caught.
575+
*/
576+
#if ZEND_MODULE_API_NO < ZEND_8_0_X_API_NO
577+
#define E_ERRORS_THROWING_EXCEPTIONS (E_WARNING | E_CORE_WARNING | E_COMPILE_WARNING | E_USER_WARNING | E_RECOVERABLE_ERROR)
578+
#else
579+
#define E_ERRORS_THROWING_EXCEPTIONS (E_WARNING | E_CORE_WARNING | E_COMPILE_WARNING | E_USER_WARNING)
580+
#endif
581+
582+
#define E_THROWS_EXCEPTION(e) ((E_ERRORS_THROWING_EXCEPTIONS & e) == e)
583+
584+
if (EG(error_handling) == EH_THROW && E_THROWS_EXCEPTION(type)) {
585+
// let the exception handler deal with this error
586+
return 0;
587+
}
588+
564589
errprio = nr_php_error_get_priority(type);
565590

566591
if (0 == errprio) {
567592
return 0;
568593
}
569-
570594
if (NR_SUCCESS != nr_txn_record_error_worthy(NRPRG(txn), errprio)) {
571595
return 0;
572596
}
@@ -625,7 +649,6 @@ void nr_php_error_cb(int type,
625649

626650
stack_json = nr_php_backtrace_to_json(0 TSRMLS_CC);
627651
errclass = nr_php_error_get_type_string(type);
628-
629652
nr_txn_record_error(NRPRG(txn), nr_php_error_get_priority(type), true,
630653
msg, errclass, stack_json);
631654

‎tests/integration/errors/test_EH_THROW_errors_caught_exception.php

+69
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,69 @@
1+
<?php
2+
/*
3+
* Copyright 2020 New Relic Corporation. All rights reserved.
4+
* SPDX-License-Identifier: Apache-2.0
5+
*/
6+
7+
/*DESCRIPTION
8+
For a certain number of error types, PHP triggers an exception if EH_THROW is toggled on.
9+
Verify we don't record the error if the exception is thrown and caught.
10+
There should be no traced errors, error events, or errors attached to span events.
11+
*/
12+
13+
/*INI
14+
display_errors=1
15+
log_errors=0
16+
error_reporting=E_ALL
17+
*/
18+
19+
/*EXPECT_SPAN_EVENTS
20+
[
21+
"?? agent run id",
22+
{
23+
"reservoir_size": 10000,
24+
"events_seen": 1
25+
},
26+
[
27+
[
28+
{
29+
"traceId": "??",
30+
"duration": "??",
31+
"transactionId": "??",
32+
"name": "OtherTransaction\/php__FILE__",
33+
"guid": "??",
34+
"type": "Span",
35+
"category": "generic",
36+
"priority": "??",
37+
"sampled": true,
38+
"nr.entryPoint": true,
39+
"timestamp": "??",
40+
"transaction.name": "OtherTransaction\/php__FILE__"
41+
},
42+
{},
43+
{}
44+
]
45+
]
46+
]
47+
*/
48+
49+
50+
/*EXPECT_REGEX
51+
Exception caught*
52+
*/
53+
54+
/*EXPECT_TRACED_ERRORS
55+
null
56+
*/
57+
58+
/*EXPECT_ERROR_EVENTS
59+
null
60+
*/
61+
62+
$base_directory = __DIR__ . '/nonexist';;
63+
64+
try {
65+
$n = new RecursiveDirectoryIterator( $base_directory ) ;
66+
}
67+
catch (\Exception $e) {
68+
echo ("Exception caught: " . $e->getMessage());
69+
}

‎tests/integration/errors/test_EH_THROW_errors_uncaught_exception.php

+112
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,112 @@
1+
<?php
2+
/*
3+
* Copyright 2020 New Relic Corporation. All rights reserved.
4+
* SPDX-License-Identifier: Apache-2.0
5+
*/
6+
7+
/*DESCRIPTION
8+
For a certain number of error types, PHP triggers an exception if EH_THROW is toggled on.
9+
Verify we record the error if the exception is thrown and NOT caught.
10+
There should be traced errors, error events, and an error attached to span events.
11+
*/
12+
13+
/*INI
14+
display_errors=1
15+
log_errors=0
16+
error_reporting=E_ALL
17+
*/
18+
19+
/*EXPECT_SPAN_EVENTS
20+
[
21+
"?? agent run id",
22+
{
23+
"reservoir_size": 10000,
24+
"events_seen": 1
25+
},
26+
[
27+
[
28+
{
29+
"traceId": "??",
30+
"duration": "??",
31+
"transactionId": "??",
32+
"name": "OtherTransaction\/php__FILE__",
33+
"guid": "??",
34+
"type": "Span",
35+
"category": "generic",
36+
"priority": "??",
37+
"sampled": true,
38+
"nr.entryPoint": true,
39+
"timestamp": "??",
40+
"transaction.name": "OtherTransaction\/php__FILE__"
41+
},
42+
{},
43+
{
44+
"error.message": "?? Uncaught exception ??",
45+
"error.class": "UnexpectedValueException"
46+
}
47+
]
48+
]
49+
]
50+
*/
51+
52+
53+
54+
/*EXPECT_REGEX
55+
Fatal error*
56+
*/
57+
58+
/*EXPECT_TRACED_ERRORS
59+
[
60+
"?? agent run id",
61+
[
62+
[
63+
"?? when",
64+
"OtherTransaction/php__FILE__",
65+
"?? Uncaught exception ??",
66+
"UnexpectedValueException",
67+
{
68+
"stack_trace": "??",
69+
"agentAttributes": "??",
70+
"intrinsics": "??"
71+
},
72+
"?? transaction ID"
73+
]
74+
]
75+
]
76+
*/
77+
78+
/*EXPECT_ERROR_EVENTS
79+
[
80+
"?? agent run id",
81+
{
82+
"reservoir_size": "??",
83+
"events_seen": 1
84+
},
85+
[
86+
[
87+
{
88+
"type": "TransactionError",
89+
"timestamp": "??",
90+
"error.class": "UnexpectedValueException",
91+
"error.message": "?? Uncaught exception ??",
92+
"transactionName": "OtherTransaction\/php__FILE__",
93+
"duration": "??",
94+
"nr.transactionGuid": "??",
95+
"guid": "??",
96+
"sampled": true,
97+
"priority": "??",
98+
"traceId": "??",
99+
"spanId": "??"
100+
},
101+
{},
102+
{}
103+
]
104+
]
105+
]
106+
*/
107+
108+
$base_directory = __DIR__ . '/nonexist';;
109+
110+
$n = new RecursiveDirectoryIterator( $base_directory );
111+
112+
echo("Should not get here");

0 commit comments

Comments
 (0)