Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: Error Backtraces v2 #17056

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
+157 −6
Open

RFC: Error Backtraces v2 #17056

Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
3b9bd2f
rfc: error_backtraces_v2
ericnorris Jan 7, 2025
aa9c4f4
address second round of PR comments
ericnorris Jan 8, 2025
47148c1
use `eval` instead of `new_oom.inc`
ericnorris Jan 8, 2025
a44ac31
fix tests that invoke PHP and thus don't inherit run-tests.php INI se…
ericnorris Jan 8, 2025
e843e06
clear backtrace when throwing an exception
ericnorris Jan 8, 2025
2cd6c3f
address third round of PR comments
ericnorris Jan 8, 2025
aaa2443
move initialization of error_backtrace to handle NTS
ericnorris Jan 8, 2025
5fd14ee
move `error_get_last` w/ fatal to separate test
ericnorris Jan 13, 2025
b1db022
rename `error_backtrace` global to `fatal_error_backtrace`
ericnorris Jan 13, 2025
9f29e80
rename `fatal_error_backtrace` -> `last_fatal_error_backtrace`
ericnorris Jan 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions Zend/zend_exceptions.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -904,6 +904,10 @@ ZEND_API ZEND_COLD zend_result zend_exception_error(zend_object *ex, int severit
ZVAL_OBJ(&exception, ex);
ce_exception = ex->ce;
EG(exception) = NULL;

zval_ptr_dtor(&EG(error_backtrace));
ZVAL_UNDEF(&EG(error_backtrace));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not 100% confident in this being the right fix, though it is similar to the EG(exception) = NULL on L906 above. Thoughts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems correct, this will clear the message for the next error. Do you have a test for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have a test for this?

In a sense, yes, since the existing ext/soap tests triggered the need for this, and once it was fixed the tests passed. On the other hand, it was only the ext/soap code that needs this. For example, throwing an exception that doesn't get caught in normal PHP won't trigger a double stack trace.

I'm concerned this (clearing the trace when throwing an exception) may not be enough and there may be other "weird" error handling things in extensions that may also end up having a double stack trace.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's hard to understand the problem exactly, it would be best to replicate in a new test.

Copy link
Contributor Author

@ericnorris ericnorris Jan 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not exactly sure how to replicate it in a test, since it has something to do with how ext/soap manipulates zend_error_cb in C. I linked the ext/soap test that demonstrates this behavior in the commit message for this change: e843e06

When I was debugging this, I noticed that ext/soap changes the zend_error_cb here:

php-src/ext/soap/soap.c

Line 548 in a091e52

zend_error_cb = soap_error_handler;

It's then kind of hard to follow, but roughly speaking it ends up in here:

php-src/ext/soap/soap.c

Lines 1868 to 1871 in a091e52

add_soap_fault_ex(&fault, &SOAP_GLOBAL(error_object), code, ZSTR_VAL(message), NULL, NULL);
Z_ADDREF(fault);
zend_throw_exception_object(&fault);
zend_bailout();

If I remember correctly, the backtrace exists at this point, and then calling zend_throw_exception_object(&fault); and zend_bailout(); end up triggering the original zend_error_cb which prints out the exception and the backtrace, thus producing a double "Stack trace:" output.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having dealt a tiny bit with ext/soap my guess is the following:

SOAP Uses the bailout mechanisms (i.e. fatal errors) as its "exception" mechanism (as it predates exceptions existing in the language), and it now catches those to throw them as proper exceptions (although it sometimes also throws normal exceptions) which is probably the cause of this.

Refactoring ext/soap so it stops doing this is a long term goal of mine, but the extension is quite complex.

Copy link
Contributor Author

@ericnorris ericnorris Jan 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, that makes sense. Do you think the above is an appropriate fix that might apply to other extensions doing stuff like this? I'm repeating myself, but I'm concerned that there might be other complex flows involving zend_error_cb that might run into related issues.

Would it make more sense to do backtrace fetching only in php_error_cb? Perhaps that would solve the issue since presumably EG(exception) would be set and could be checked before fetching? Edit: though that would mean other extensions couldn't rely on the backtrace existing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Frankly, the only other extensions I see overloading zend_error_cb are debuggers and profilers. ext/soap is very special in this instance.


if (ce_exception == zend_ce_parse_error || ce_exception == zend_ce_compile_error) {
zend_string *message = zval_get_string(GET_PROPERTY(&exception, ZEND_STR_MESSAGE));
zend_string *file = zval_get_string(GET_PROPERTY_SILENT(&exception, ZEND_STR_FILE));
Expand Down
Loading