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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DI] Fix false-positive circular ref leading to wrong exceptions or infinite loops at runtime #28060

Merged
merged 1 commit into from Aug 8, 2018

Conversation

Projects
None yet
5 participants
@nicolas-grekas
Member

nicolas-grekas commented Jul 25, 2018

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

When circular loops involve references in properties, method calls or configurators, it is possible to properly instantiate the related services.

The current logic is broken: ContainerBuilder considers some of these loops as self-referencing circular references, leading to a runtime exception, and in similar situations, PhpDumper generates code that turns to infinite loops at runtime 馃挜. These badly handled situations happen with inlined definitions.

This PR fixes both classes by making them track which references are really part of the constructors' chain, including inline definitions.

It also fixes dumping infinite loops when dumping circular loops involving lazy services while proxy-manager-bridge is not installed.

@@ -294,7 +294,7 @@ public function get($id, $invalidBehavior = /* self::EXCEPTION_ON_INVALID_REFERE
}
if (isset($this->loading[$id])) {
throw new ServiceCircularReferenceException($id, array_keys($this->loading));
throw new ServiceCircularReferenceException($id, array_merge(array_keys($this->loading), array($id)));

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jul 25, 2018

Member

Loosely-related improvement of the exception message: displays "foo -> bar -> foo" instead of just "foo -> bar" (already done this way in other places that throw ServiceCircularReferenceException.)

@nicolas-grekas

nicolas-grekas Jul 25, 2018

Member

Loosely-related improvement of the exception message: displays "foo -> bar -> foo" instead of just "foo -> bar" (already done this way in other places that throw ServiceCircularReferenceException.)

$dumper->dump();
$this->addToAssertionCount(1);
}
public function testCircularReferenceAllowanceForInlinedDefinitionsForLazyServices()

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jul 25, 2018

Member

this test case tests nothing anymore

@nicolas-grekas

nicolas-grekas Jul 25, 2018

Member

this test case tests nothing anymore

@nicolas-grekas

All green and tested.

return $service;
}
} catch (ServiceCircularReferenceException $e) {
if ($isConstructorArgument) {

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jul 29, 2018

Member

When a circular loop is detected but involves a property, method call or configurator, it's a loop we can work with, thus the new $isConstructorArgument state that is propagated in ContainerBuilder to keep track of these (the previous tracking based on two-states "loading" properties was naive and broken.)

@nicolas-grekas

nicolas-grekas Jul 29, 2018

Member

When a circular loop is detected but involves a property, method call or configurator, it's a loop we can work with, thus the new $isConstructorArgument state that is propagated in ContainerBuilder to keep track of these (the previous tracking based on two-states "loading" properties was naive and broken.)

}
$path[] = $parent.'". Try running "composer require symfony/proxy-manager-bridge';
throw new ServiceCircularReferenceException($parent, $path);

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jul 29, 2018

Member

Don't dump broken service factories when lazy services are not really lazy because proxy-manager is not here. Let's throw an exception suggesting to install it instead.

@nicolas-grekas

nicolas-grekas Jul 29, 2018

Member

Don't dump broken service factories when lazy services are not really lazy because proxy-manager is not here. Let's throw an exception suggesting to install it instead.

foreach ($inlinedDefinitions as $def) {
$this->getServiceCallsFromArguments(array($def->getArguments(), $def->getFactory()), $calls, $isPreInstance, $cId);
if ($preInstance && !$inlinedDefinitions[$def][1]) {

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jul 29, 2018

Member

Similar to the issue found in ContainerBuidler, PhpDumper also needs to keep track of service references that relate only to constructor arguments (of nested definitions). That's what $inlinedDefinitions[$def][1] does: if we're before the instantiation line of the service, and if the nested definition we're looking at is not involved in a constructor argument, it should be dumped after the instantiation line, thus continue here.

@nicolas-grekas

nicolas-grekas Jul 29, 2018

Member

Similar to the issue found in ContainerBuidler, PhpDumper also needs to keep track of service references that relate only to constructor arguments (of nested definitions). That's what $inlinedDefinitions[$def][1] does: if we're before the instantiation line of the service, and if the nested definition we're looking at is not involved in a constructor argument, it should be dumped after the instantiation line, thus continue here.

@@ -347,7 +367,7 @@ private function analyzeCircularReferences(array $edges, &$checkedNodes, &$curre
$node = $edge->getDestNode();
$id = $node->getId();
if ($node->getValue() && ($edge->isLazy() || $edge->isWeak())) {
if ($node->getValue() && (($edge->isLazy() && !$this->getProxyDumper() instanceof NullDumper) || $edge->isWeak())) {

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jul 29, 2018

Member

When no proxy dumper is set, lazy definitions should be involved into circular refs tracking.

@nicolas-grekas

nicolas-grekas Jul 29, 2018

Member

When no proxy dumper is set, lazy definitions should be involved into circular refs tracking.

foreach ($inlinedDefinitions as $def) {
if ($def === $definition || isset($constructorDefinitions[$def])) {
$constructorDefinitions[$def] = $inlinedDefinitions[$def];
} else {
$otherDefinitions[$def] = $inlinedDefinitions[$def];
}
$arguments = array($def->getArguments(), $def->getFactory(), $def->getProperties(), $def->getMethodCalls(), $def->getConfigurator());
$this->getServiceCallsFromArguments($arguments, $serviceCalls, false, $id);

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jul 29, 2018

Member

This factorizes computing $serviceCalls once instead of doing it in each method that requires the result.

@nicolas-grekas

nicolas-grekas Jul 29, 2018

Member

This factorizes computing $serviceCalls once instead of doing it in each method that requires the result.

@@ -1640,7 +1663,7 @@ private function hasReference($id, array $arguments, $deep = false, array &$visi
return true;
}
if (!$deep || isset($visited[$argumentId]) || !isset($this->circularReferences[$id][$argumentId])) {
if (!$deep || isset($visited[$argumentId]) || !isset($this->circularReferences[$argumentId])) {

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jul 29, 2018

Member

$this->circularReferences tells if an id is involved in a circular loop, but checking for both $id & $argumentId is broken with loops that involve more than two services.

@nicolas-grekas

nicolas-grekas Jul 29, 2018

Member

$this->circularReferences tells if an id is involved in a circular loop, but checking for both $id & $argumentId is broken with loops that involve more than two services.

@nicolas-grekas nicolas-grekas merged commit e843bb8 into symfony:3.4 Aug 8, 2018

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 Aug 8, 2018

bug #28060 [DI] Fix false-positive circular ref leading to wrong exce鈥
鈥tions or infinite loops at runtime (nicolas-grekas)

This PR was merged into the 3.4 branch.

Discussion
----------

[DI] Fix false-positive circular ref leading to wrong exceptions or infinite loops at runtime

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

When circular loops involve references in properties, method calls or configurators, it is possible to properly instantiate the related services.

The current logic is broken: `ContainerBuilder` considers some of these loops as self-referencing circular references, leading to a runtime exception, and in similar situations, `PhpDumper` generates code that turns to infinite loops at runtime 馃挜. These badly handled situations happen with inlined definitions.

This PR fixes both classes by making them track which references are really part of the constructors' chain, including inline definitions.

It also fixes dumping infinite loops when dumping circular loops involving lazy services while proxy-manager-bridge is not installed.

Commits
-------

e843bb8 [DI] Fix false-positive circular ref leading to wrong exceptions or infinite loops at runtime

@nicolas-grekas nicolas-grekas deleted the nicolas-grekas:di-fix-lazy-loops branch Aug 8, 2018

nicolas-grekas added a commit that referenced this pull request Aug 8, 2018

minor #28160 [DI] fix analyzing lazy refs involved in circular loops 鈥
鈥(nicolas-grekas)

This PR was merged into the 3.4 branch.

Discussion
----------

[DI] fix analyzing lazy refs involved in circular loops

| 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        | -

Follow up of #28060 to fix "deps=high" jobs.

Commits
-------

4e92d10 [DI] fix analyzing lazy refs involved in circular loops

This was referenced Aug 28, 2018

@hal869

This comment has been minimized.

Show comment
Hide comment
@hal869

hal869 Sep 2, 2018

This status == "Needs Review". Of what, specifically? More than the downgrade to DI 4.1.3, right?

hal869 commented Sep 2, 2018

This status == "Needs Review". Of what, specifically? More than the downgrade to DI 4.1.3, right?

@@ -66,7 +66,7 @@ private function getDefinitionId($id, ContainerBuilder $container)
$seen = array();
while ($container->hasAlias($id)) {
if (isset($seen[$id])) {
throw new ServiceCircularReferenceException($id, array_keys($seen));
throw new ServiceCircularReferenceException($id, array_merge(array_keys($seen), array($id)));

This comment has been minimized.

@crisanlucid

crisanlucid Sep 3, 2018

array_unique(array_merge($a, $b)) -> I think this will remove duplicates in case of the merge

@crisanlucid

crisanlucid Sep 3, 2018

array_unique(array_merge($a, $b)) -> I think this will remove duplicates in case of the merge

This comment has been minimized.

@stof

stof Sep 3, 2018

Member

but we precisely don't want to remove them, as a cycle will have a duplicate (the last item also happens earlier, which is precisely the root cause of the issue)

@stof

stof Sep 3, 2018

Member

but we precisely don't want to remove them, as a cycle will have a duplicate (the last item also happens earlier, which is precisely the root cause of the issue)

nicolas-grekas added a commit that referenced this pull request Sep 8, 2018

bug #28388 [DI] configure inlined services before injecting them when鈥
鈥 dumping the container (nicolas-grekas)

This PR was merged into the 3.4 branch.

Discussion
----------

[DI] configure inlined services before injecting them when dumping the container

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

#28060 introduced a change in the way inline services are dumped: these instances could end up being configured *after* being injected. This breaks e.g. using Doctrine's Configuration instances, which are expected to be fully defined before being injected into their consumers.

Fixing this required a significant refactorization because I was just unable to reason with the heavily scrambled logic in place right now. The new logic is still non-trivial, but at least it's manageable, thus easier to get correct.

(Replaces #28385 which is the same applied to 4.1 - should help with merges.)

Commits
-------

e5c5405 [DI] configure inlined services before injecting them when dumping the container
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment