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.

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

[HttpKernel] Fix possible infinite loop of exceptions #30327

Merged
merged 1 commit into from Feb 22, 2019

Conversation

Projects
None yet
3 participants
@enumag
Copy link
Contributor

enumag commented Feb 21, 2019

Q A
Branch? 3.4
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT
Doc PR

I ran into an issue in the enqueue library which copied this part of code from Symfony. I'm now starting to understand what the problem is and it should most likely be fixed in Symfony as well.

I didn't actually run into it in Symfony itself but it seems at least hypothetically possible. Imagine if here $e is somehow the same (===) as $exception. The code below will then find the last exception in the getPrevious() chain and assigns $exception as the previous. However in the off chance that $exception is actually $e (the first exception in the chain) then it creates an infinite loop of exceptions which is not good for monolog and exception handlers.

What do you think?

@enumag

This comment has been minimized.

Copy link
Contributor Author

enumag commented Feb 21, 2019

Maybe do-while would be better?

$prev = $e;
do {
	if ($exception === $wrapper = $prev) {
		throw $e;
	}
} while ($prev = $wrapper->getPrevious());

@nicolas-grekas nicolas-grekas added this to the 3.4 milestone Feb 21, 2019

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Feb 21, 2019

LGTM

@enumag

This comment has been minimized.

Copy link
Contributor Author

enumag commented Feb 21, 2019

@nicolas-grekas What do you think about the do-while?

@enumag enumag marked this pull request as ready for review Feb 21, 2019

@enumag enumag force-pushed the enumag:patch-32 branch from 015da50 to 3447222 Feb 21, 2019

@enumag

This comment has been minimized.

Copy link
Contributor Author

enumag commented Feb 21, 2019

Squashed to one commit. Should be ready to merge in my opinion.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Feb 22, 2019

Good catch, thanks @enumag.

@nicolas-grekas nicolas-grekas merged commit 3447222 into symfony:3.4 Feb 22, 2019

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

nicolas-grekas added a commit that referenced this pull request Feb 22, 2019

bug #30327 [HttpKernel] Fix possible infinite loop of exceptions (enu…
…mag)

This PR was merged into the 3.4 branch.

Discussion
----------

[HttpKernel] Fix possible infinite loop of exceptions

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets |
| License       | MIT
| Doc PR        |

I ran into an [issue](php-enqueue/enqueue-dev#774) in the enqueue library which copied this part of code from Symfony. I'm now starting to understand what the problem is and it should most likely be fixed in Symfony as well.

I didn't actually run into it in Symfony itself but it seems at least hypothetically possible. Imagine if [here](https://github.com/symfony/symfony/blob/8c3dc8254a508593aa0637445659e93e39d31dca/src/Symfony/Component/HttpKernel/EventListener/ExceptionListener.php#L77) `$e` is somehow the same (===) as `$exception`. The code [below](https://github.com/symfony/symfony/blob/master/src/Symfony/Component/HttpKernel/EventListener/ExceptionListener.php#L82-L92) will then find the last exception in the `getPrevious()` chain and assigns `$exception` as the previous. However in the off chance that `$exception` is actually `$e` (the first exception in the chain) then it creates an infinite loop of exceptions which is not good for monolog and exception handlers.

What do you think?

Commits
-------

3447222 [HttpKernel] Fix possible infinite loop of exceptions

This was referenced Mar 3, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.