Welcome! Log In Create A New Profile

Advanced

[PHP-DEV] Stop Exceptions capturing object references for trace arguments

Posted by Rowan Collins 
Hi All,

This topic has been discussed a couple of times before:
- http://marc.info/?t=138118341600002&r=1&w=2
- http://marc.info/?t=142708828500001&r=1&w=2
- http://marc.info/?t=143798102800002&r=1&w=2
I was inspired to revive the topic by this bug report:
https://bugs.php.net/bug.php?id=75056&edit=3

Currently, the backtrace of an Exception stores the following:

- function: string
- line: integer
- file: string
- class: string
- type: string
- args: array

This is effectively the same as debug_backtrace() with neither
DEBUG_BACKTRACE_PROVIDE_OBJECT nor DEBUG_BACKTRACE_IGNORE_ARGS set.

The "args" part of this contains full object references to anything that
happens to have been a function argument in the stack, and causes two
problems:

- Serializing an exception can fail, because some of these referenced
objects may be non-serializable (e.g. SimpleXMLElement, PDO).
- Destructors for these objects do not fire until the Exception is
destructed, causing resources to be held open unexpectedly after the
stack has been unwound. (The only other place I know of destructors
being unreliable in PHP is if there are circular references, in which
case the destructor will only fire when the GC detects the cycle.)

Both effects are almost entirely unpredictable, because it depends if
the object happens to have been a parameter in the current stack, rather
than a local variable or $this reference.

The simplest solution would be to simply remove this 'args' key from the
backtraces, as when DEBUG_BACKTRACE_IGNORE_ARGS is set on
debug_backtrace(). This would mean that any normal exception would hold
no object references, so would not extend any lifetimes, and would not
trigger any serialization errors.

Obviously, this has the downside of breaking any code that makes use of
this array; if anyone has examples of practical uses for it I would be
grateful to see them, and whether a compromise such as storing the class
name but not the object would be useful.

Regards,

--
Rowan Collins
[IMSoP]


--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Hi!

> The "args" part of this contains full object references to anything that
> happens to have been a function argument in the stack, and causes two
> problems:

I think it makes sense to make exception not to collect args. In fact, I
think this may also be one of rare cases where new ini setting would be
appropriate, where the default could be the old way (at least for now,
for BC), and recommended production setting would be off.

--
Stas Malyshev
smalyshev@gmail.com

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
On 12/08/2017 04:25, Stanislav Malyshev wrote:
> Hi!
>
>> The "args" part of this contains full object references to anything that
>> happens to have been a function argument in the stack, and causes two
>> problems:
> I think it makes sense to make exception not to collect args. In fact, I
> think this may also be one of rare cases where new ini setting would be
> appropriate, where the default could be the old way (at least for now,
> for BC), and recommended production setting would be off.
>

My only concern is if anyone is using this data for anything other than
debugging - e.g. if they were somehow extracting extra context about the
exception by traversing the backtrace to a particular point and grabbing
the arguments. That feels like a horrible hack to me, but that doesn't
mean someone isn't relying on it.

An interesting side effect of an ini setting, though, is that it could
in turn be made to raise deprecation warnings at some point, whereas
just accessing the 'args' element of an array is pretty hard to detect.
So for instance, we could theoretically have: 7.3, add ini setting
defaulting to on; 7.4, change default to off and raise deprecation
notice if it's turned on; 8.0, remove feature, error if ini setting is
not set to on (ignore if set to off for smoother upgrades).

Regards,

--
Rowan Collins
[IMSoP]


--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Hi!

> My only concern is if anyone is using this data for anything other than
> debugging - e.g. if they were somehow extracting extra context about the
> exception by traversing the backtrace to a particular point and grabbing
> the arguments. That feels like a horrible hack to me, but that doesn't
> mean someone isn't relying on it.

Sure, that's why I propose to keep the option. If you use the horrible
hack, I think it wouldn't be too much to ask to set an INI variable.
Since it's rather local, it can be INI_ALL so you could do it in the
script even.

> An interesting side effect of an ini setting, though, is that it could
> in turn be made to raise deprecation warnings at some point, whereas
> just accessing the 'args' element of an array is pretty hard to detect.
> So for instance, we could theoretically have: 7.3, add ini setting
> defaulting to on; 7.4, change default to off and raise deprecation
> notice if it's turned on; 8.0, remove feature, error if ini setting is
> not set to on (ignore if set to off for smoother upgrades).

Yep, sounds like a plan. Not sure if we need to drop it for 8.0 even,
but we can vote on that. Anybody to make an RFC for this? :)

--
Stas Malyshev
smalyshev@gmail.com

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
2017-08-12 17:54 GMT+02:00 Stanislav Malyshev <smalyshev@gmail.com>:

> Hi!
>
> > My only concern is if anyone is using this data for anything other than
> > debugging - e.g. if they were somehow extracting extra context about the
> > exception by traversing the backtrace to a particular point and grabbing
> > the arguments. That feels like a horrible hack to me, but that doesn't
> > mean someone isn't relying on it.
>
> Sure, that's why I propose to keep the option. If you use the horrible
> hack, I think it wouldn't be too much to ask to set an INI variable.
> Since it's rather local, it can be INI_ALL so you could do it in the
> script even.
>
> > An interesting side effect of an ini setting, though, is that it could
> > in turn be made to raise deprecation warnings at some point, whereas
> > just accessing the 'args' element of an array is pretty hard to detect.
> > So for instance, we could theoretically have: 7.3, add ini setting
> > defaulting to on; 7.4, change default to off and raise deprecation
> > notice if it's turned on; 8.0, remove feature, error if ini setting is
> > not set to on (ignore if set to off for smoother upgrades).
>
> Yep, sounds like a plan. Not sure if we need to drop it for 8.0 even,
> but we can vote on that. Anybody to make an RFC for this? :)
>

If it's explicitly needed, someone could still just call
`debug_backtrace()` and manually store the args in the exception
constructor, no?

Would the args still be available in the stringified version? Because
they're quite useful for debugging there.

Regards, Niklas
On 13/08/2017 15:31, Niklas Keller wrote:
>
> If it's explicitly needed, someone could still just call
> `debug_backtrace()` and manually store the args in the exception
> constructor, no?

That depends exactly how it's being used, but yes if you control the
throw site or Exception constructor you could grab the backtrace manually.


>
> Would the args still be available in the stringified version? Because
> they're quite useful for debugging there.

Hm, I guess we'd need to store the string representation (the class
name, basically) somewhere so we could keep that. Doesn't seem like it
ought to be too hard, unless there's some hidden complexity or
performance impact there.

Regards,

--
Rowan Collins
[IMSoP]


--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Sorry, only registered users may post in this forum.

Click here to login