Everything

php

Home » Project » Newsgroups » php.internals » RFC: Error backtraces »

RFC: Error backtraces

php.internals

Max Semenik

6 years ago

Hi, I'd like to present a new RFC for your consideration:
https://wiki.php.net/rfc/error_backtraces

In a nutshell, I'm proposing to give errors equal rights with exceptions
and provide backtraces for them, too.

--
Best regards,
Max Semenik

Dan Ackroyd

6 years ago

On Fri, 29 May 2020 at 19:23, Max Semenik <maxsem.wiki@gmail.com> wrote:

>
> Hi, I'd like to present a new RFC for your consideration:
> https://wiki.php.net/rfc/error_backtraces
>

Hi Max,

I've also been thinking about how errors + warnings can be improved also.

For my own code, I convert all non-silenced errors and warnings into
an exception through the error handler*. That works pretty well, but
has one big downside - it only throws one type of exception.

I wrote some words about what we could do:
https://gist.github.com/Danack/6b5a86414ab9e9d9ac7e0a906216f1c5

The TL:DR of that is, we could add specific exceptions that should be
thrown if the error handler setup by the user decided to throw an
exception. I would be interested in hearing if that idea achieves the
benefits you're trying to get, in a less controversial way.

To give some feedback on your RFC.

"traces will be formatted exactly like for exceptions, and appended to
error messages,"

This has security implications as it would be possible for app secrets
to be included in the error message. That is less of a problem for
exceptions as you can write your own code for logging exceptions that
doesn't expose the variables if you don't want them exposed.

"..., traces are always generated for exceptions"

That depends on what callback function has been set for
'set_exception_handler'. Also....maybe use more formal language for
RFCs. Even minor swear words should be avoided imo.

"error_get_last() output will have another element added to it if a
trace is available:"

I strongly suspect that is a very bad idea. As the variables in the
stack trace are kept alive while that stack trace is kept alive, that
means the lifetime of those variables would depend on how long until
another error occurs.

That sounds like code that is hard to reason about.

"In the current proposed implementation, a new error flag
E_UNHANDLED_EXCEPTION is introduced,"

The RFC doesn't say what this is meant to do precisely.....but my gut
instinct doesn't like it.

cheers
Dan
Ack



* an error handler that converts all unsilenced errors that we care
about into an exception

function standardErrorHandler($errorNumber, $errorMessage, $errorFile,
$errorLine): bool
{
if (error_reporting() === 0) {
// Error reporting has been silenced
if ($errorNumber !== E_USER_DEPRECATED) {
return true;
}
}
if ($errorNumber === E_DEPRECATED) {
return false;
}
if ($errorNumber === E_CORE_ERROR || $errorNumber === E_ERROR) {
// For these two types, PHP is shutting down anyway. Return false
// to allow shutdown to continue
return false;
}
$message = "Error: [$errorNumber] $errorMessage in file $errorFile
on line $errorLine.";

throw new \Exception($message);
}

Max Semenik

6 years ago

Thanks for your input!

On Sat, May 30, 2020 at 8:36 PM Dan Ackroyd <Danack@basereality.com> wrote:

> For my own code, I convert all non-silenced errors and warnings into
> an exception through the error handler*. That works pretty well, but
> has one big downside - it only throws one type of exception.
>
> I wrote some words about what we could do:
> https://gist.github.com/Danack/6b5a86414ab9e9d9ac7e0a906216f1c5
>
> The TL:DR of that is, we could add specific exceptions that should be
> thrown if the error handler setup by the user decided to throw an
> exception. I would be interested in hearing if that idea achieves the
> benefits you're trying to get, in a less controversial way.
>

Your solution requires programmers to write code supporting this kind of
workflow, while I'm interested in providing useful information in all
cases, even for legacy apps.

> This has security implications as it would be possible for app secrets
> to be included in the error message. That is less of a problem for
> exceptions as you can write your own code for logging exceptions that
> doesn't expose the variables if you don't want them exposed.
>

That's why I propose an INI setting controlling this behavior.

> "..., traces are always generated for exceptions"

> That depends on what callback function has been set for
> 'set_exception_handler'.

From my reading of the code, backtraces are generated on object creation,
in zend_default_exception_new_ex().

> Also....maybe use more formal language for
> RFCs. Even minor swear words should be avoided imo.
>

Sorry, didn't realize the word is, uh, "informal".

> "error_get_last() output will have another element added to it if a
> trace is available:"
>
> I strongly suspect that is a very bad idea. As the variables in the
> stack trace are kept alive while that stack trace is kept alive, that
> means the lifetime of those variables would depend on how long until
> another error occurs.
>

As shown in the example output, traces are saved as string, so no
implications to lifetime.

> "In the current proposed implementation, a new error flag
> E_UNHANDLED_EXCEPTION is introduced,"
>
> The RFC doesn't say what this is meant to do precisely.....but my gut
> instinct doesn't like it.

Clarified this with a link to the code: I needed it to avoid outputting the
trace twice in some cases.

> * an error handler that converts all unsilenced errors that we care
> about into an exception
>
> function standardErrorHandler($errorNumber, $errorMessage, $errorFile,
> $errorLine): bool
> [snip]

The problem with this is that it doesn't get called on fatal errors, i.e.
you can't get traces for the most serious problems.

On Sun, May 31, 2020 at 4:47 PM Nikita Popov <nikita.ppv@gmail.com> wrote:

> I'm concerned about the performance implications of this change. Backtrace
> gathering is quite expensive and doing this for every diagnostic will have
> a large performance impact if there are many of them. This is okay if your
> code is intended to be diagnostics clean, but unfortunately this is not a
> given. If you generate 10k deprecation warnings during a request, you're
> definitely going to feel those backtraces.
>

INI setting should take care of this. If your code is crap, don't flip it
on.

> It is already possible to get a backtrace for all non-fatal diagnostics
> through a custom error handler, which allows you to control whether you
> want to collect a backtrace for a particular error. As such, I would
> recommend to limit this functionality to fatal errors only, where such a
> possibility does not currently exist. Additionally fatal errors also
> naturally limit the performance impact, because there can (approximately)
> be only one fatal error per request.
>

I guess having this only for fatals is better than nothing, however since
this functionality is optional, I don't see leaks via traces as a serious
problem. Backtraces are disabled by default in code and php.ini-production,
so it's safe by default.

On Tue, Jun 2, 2020 at 1:29 PM Peter Stalman <sarkedev@gmail.com> wrote:

> Isn't this something that is handled fairly well by xdebug already?

Yes, but most people don't run xdebug in production. Also, it's not part of
PHP core and thus some people might not have it in their hosting
environment. Having basic diagnostics seems to me like a good thing to have
in PHP proper.

-------
I've updated the RFC: clarified some parts and removed the voting question
whether the configuration setting is needed, discussion clearly shows so.

--
Best regards,
Max Semenik

Nikita Popov

6 years ago

On Tue, Jun 2, 2020 at 10:57 PM Max Semenik <maxsem.wiki@gmail.com> wrote:

> On Sun, May 31, 2020 at 4:47 PM Nikita Popov <nikita.ppv@gmail.com> wrote:
>
> > I'm concerned about the performance implications of this change.
> Backtrace
> > gathering is quite expensive and doing this for every diagnostic will
> have
> > a large performance impact if there are many of them. This is okay if
> your
> > code is intended to be diagnostics clean, but unfortunately this is not a
> > given. If you generate 10k deprecation warnings during a request, you're
> > definitely going to feel those backtraces.
> >
>
> INI setting should take care of this. If your code is crap, don't flip it
> on.
>

There's a couple of problems with this line of argumentation:

First, even if the code is crap, wouldn't it still want to have stack
traces for fatal errors? Why should crap code be denied stack traces? Isn't
that the code that needs them most? If the code weren't crap it wouldn't be
causing fatal errors anyway ;)

Second, PHP has plenty of functions (especially I/O functions) where
ignoring notices and warnings is a requirement. You are not interested in
back traces for errors your ignore. Keep in mind that error_get_last() data
is populated independently of whether errors are suppressed or not.

Similarly, if your code promoted all warnings to exceptions using a custom
error handler, then you'll perform a duplicate backtrace calculation, first
when the warning is originally through, and then again if you construct the
exception. (This is less bad than other cases, because it's "just" twice as
slow.)

Finally, even if we completely disregard the question of performance, back
traces are very noisy. You get one fatal error per request, but you might
easily get 1k warnings if something happens to trigger in a loop. With back
traces, those 1k warnings will be 100k lines.

I'm not sure if limiting it to just fatals is the best choice, but I do
think that this needs a bit more control. And limited to just backtraces
for fatals, I think this functionality should be enabled by default.

This might also want to integrate with the zend.exception_ignore_args
option (or, because that one is exception-specific, a renamed option that
does the same thing). We will want to disable argument gathering in a
default production configuration, because the arguments may contain
sensitive data that must not appear inside log files.

Regards,
Nikita

Nikita Popov

5 years ago

On Wed, Jun 3, 2020 at 9:04 AM Nikita Popov <nikita.ppv@gmail.com> wrote:

> On Tue, Jun 2, 2020 at 10:57 PM Max Semenik <maxsem.wiki@gmail.com> wrote:
>
>> On Sun, May 31, 2020 at 4:47 PM Nikita Popov <nikita.ppv@gmail.com>
>> wrote:
>>
>> > I'm concerned about the performance implications of this change.
>> Backtrace
>> > gathering is quite expensive and doing this for every diagnostic will
>> have
>> > a large performance impact if there are many of them. This is okay if
>> your
>> > code is intended to be diagnostics clean, but unfortunately this is not
>> a
>> > given. If you generate 10k deprecation warnings during a request, you're
>> > definitely going to feel those backtraces.
>> >
>>
>> INI setting should take care of this. If your code is crap, don't flip it
>> on.
>>
>
> There's a couple of problems with this line of argumentation:
>
> First, even if the code is crap, wouldn't it still want to have stack
> traces for fatal errors? Why should crap code be denied stack traces? Isn't
> that the code that needs them most? If the code weren't crap it wouldn't be
> causing fatal errors anyway ;)
>
> Second, PHP has plenty of functions (especially I/O functions) where
> ignoring notices and warnings is a requirement. You are not interested in
> back traces for errors your ignore. Keep in mind that error_get_last() data
> is populated independently of whether errors are suppressed or not.
>
> Similarly, if your code promoted all warnings to exceptions using a custom
> error handler, then you'll perform a duplicate backtrace calculation, first
> when the warning is originally through, and then again if you construct the
> exception. (This is less bad than other cases, because it's "just" twice as
> slow.)
>
> Finally, even if we completely disregard the question of performance, back
> traces are very noisy. You get one fatal error per request, but you might
> easily get 1k warnings if something happens to trigger in a loop. With back
> traces, those 1k warnings will be 100k lines.
>
> I'm not sure if limiting it to just fatals is the best choice, but I do
> think that this needs a bit more control. And limited to just backtraces
> for fatals, I think this functionality should be enabled by default.
>
> This might also want to integrate with the zend.exception_ignore_args
> option (or, because that one is exception-specific, a renamed option that
> does the same thing). We will want to disable argument gathering in a
> default production configuration, because the arguments may contain
> sensitive data that must not appear inside log files.
>
> Regards,
> Nikita
>

To put my feedback into more actionable form: Rather than adding an ini
setting that enables error backtraces for all diagnostics, I'd make it a
mask of error types for which a backtrace should be captured, and default
it to fatal errors only. So error_reporting=E_ALL,
error_backtrace=E_ERROR|E_CORE_ERROR|E_COMPILE_ERROR|E_USER_ERROR|E_RECOVERABLE_ERROR|E_PARSE.
It might be handy to expose the internal E_FATAL_ERRORS constant we have
for that.

I think this should give a very sensible default behavior, and people who
want to capture backtraces for all errors still have an option to do so.

Finally, I'm wondering if the backtrace in error_get_last() shouldn't be in
array form, rather than string form.

Regards,
Nikita

Dan Ackroyd

6 years ago

On Tue, 2 Jun 2020 at 21:57, Max Semenik <maxsem.wiki@gmail.com> wrote:

>
> Thanks for your input!
>

> The problem with this is that it doesn't get called on fatal errors,

That would be an easier and smaller thing to fix, rather than also
changing error/warning handling.

A stack trace shown on fatal errors that doesn't include any variables
would be a useful thing imo.

> I don't see leaks via traces as a serious problem.

That is an opinion not universally shared.

I know one company that failed a security audit, and so failed to win
a contract, due to variables being exposed in stacktraces that reached
their log files.

> Your solution requires programmers to write code
> supporting this kind of workflow,

Sorry, not sure what you mean.

The changes would be internal, and then would naturally reach the
users outer exception handler, where they should already have logging
of uncaught exceptions. So can't see why it would be a problem.

> Sorry, didn't realize the word is, uh, "informal".

Informal? _Infernal_ more like. *ba-dum-tish*

Danack wrote:

>> That depends on what callback function has been set for
>> 'set_exception_handler'.

Max wrote:

> From my reading of the code, backtraces are generated on object creation,

I meant that whether the trace is converted to a string and logged
anywhere is dependent on what the exception handler chooses to do.

> INI setting should take care of this.

People really, really really, _really_ don't  like ini settings.

Although application level ini settings can be appropriate in some
cases, a new ini setting here doesn't feel like a great choice.

Combine that with "As shown in the example output, traces are saved as
string" and I really don't like it.

We want to make it possible to decide what to do with errors programmatically.

Converting stack traces to strings before a user handler can decide
what to do with them is not a good approach.

To summarise, adding stack trace to fatal error output (without
variables) sounds good to me, and the idea I said before of adding
exceptions to errors/warning would allow better user control over what
happens also sounds good, but more ini settings and traces as strings
sounds not good.

cheers
Dan
Ack

Nikita Popov

6 years ago

On Fri, May 29, 2020 at 8:24 PM Max Semenik <maxsem.wiki@gmail.com> wrote:

> Hi, I'd like to present a new RFC for your consideration:
> https://wiki.php.net/rfc/error_backtraces
>
> In a nutshell, I'm proposing to give errors equal rights with exceptions
> and provide backtraces for them, too.
>

I'm concerned about the performance implications of this change. Backtrace
gathering is quite expensive and doing this for every diagnostic will have
a large performance impact if there are many of them. This is okay if your
code is intended to be diagnostics clean, but unfortunately this is not a
given. If you generate 10k deprecation warnings during a request, you're
definitely going to feel those backtraces.

It is already possible to get a backtrace for all non-fatal diagnostics
through a custom error handler, which allows you to control whether you
want to collect a backtrace for a particular error. As such, I would
recommend to limit this functionality to fatal errors only, where such a
possibility does not currently exist. Additionally fatal errors also
naturally limit the performance impact, because there can (approximately)
be only one fatal error per request.

Nikita