Skip to content

InvalidTag::flattenExceptionBacktrace() corrupts reference arguments #249

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.

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

Closed
nikic opened this issue Jul 25, 2020 · 8 comments · Fixed by #250
Closed

InvalidTag::flattenExceptionBacktrace() corrupts reference arguments #249

nikic opened this issue Jul 25, 2020 · 8 comments · Fixed by #250

Comments

@nikic
Copy link

nikic commented Jul 25, 2020

If there are any arguments in the backtrace that are references, InvalidTag::flattenExceptionBacktrace() may corrupt them by converting them to strings. This came up in nikic/PHP-Parser#687.

The underlying PHP bug/feature https://bugs.php.net/bug.php?id=79108 that allows this has been fixed/removed in PHP 8.0, but this still needs a fix in this library in the meantime. It should be possible to remove the references using something like:

$val = $array[$x];
unset($array[$x]);
$array[$x] = doYourTransform($val);
@jaapio
Copy link
Member

jaapio commented Jul 26, 2020

Thanks for your report, we will have a look at this👍

@jaapio
Copy link
Member

jaapio commented Jul 26, 2020

@nikic if I understand the issue correctly, because we are changing the back trace of an exception we are changing references in the rest of the execution flow?

I think the best way to keep the behavior we want is to deep clone the exceptions? Or this not possible because it will always have a reference to the origin of the exception?

What we tried to do is make exception serialization possible, since a lot of using applications are serializing the output of this library to be able to cache the results.
I do fully understand that the hacking we are doing is very odd. And should not be done in all situations.

@williamdes
Copy link
Contributor

@jaapio
Copy link
Member

jaapio commented Jul 30, 2020

What I do understand is that from php8 this code will actually work...

However we still support php 7.2 and up. We need to address this in the right way. But I don't know yet how without breaking bc. The only thing I can come up with is a deep clone of the exceptions, but I don't know if that will work and keep bc.

We need to find out what our options are.

I'm not near a computer the upcoming week, so any pr or plan from any contributor is welcome.

@williamdes
Copy link
Contributor

Just some news: I can not find some time soon to fix this issue, feel free to fix it :)
I will post an update if I start working on this one.

@jaapio
Copy link
Member

jaapio commented Aug 12, 2020

I created a PR that should fix this issue. A review from you both would be very much appreciated :-D

@jaapio jaapio closed this as completed Aug 12, 2020
@jaapio jaapio reopened this Aug 12, 2020
@jaapio
Copy link
Member

jaapio commented Aug 15, 2020

Fixed in release 5.2.1

@williamdes
Copy link
Contributor

Fixed in release 5.2.1

Thank you so much !!
https://github.com/code-lts/doctum/releases/tag/v5.0.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants