Skip to content

Commit

Permalink
bug #28060 [DI] Fix false-positive circular ref leading to wrong exce…
Browse files Browse the repository at this point in the history 
…ptions 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
  • Loading branch information
@nicolas-grekas
nicolas-grekas committed Aug 8, 2018
2 parents 2bae183 + e843bb8 commit ba31bab
Show file tree
Hide file tree
Showing 9 changed files with 474 additions and 146 deletions.
2 changes: 1 addition & 1 deletion src/Symfony/Component/DependencyInjection/Compiler/ResolveReferencesToAliasesPass.php
View file
Expand Up @@ -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)));
}
$seen[$id] = true;
$id = $container->normalizeId($container->getAlias($id));
Expand Down
2 changes: 1 addition & 1 deletion src/Symfony/Component/DependencyInjection/Container.php
View file
Expand Up @@ -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->loading[$id] = true;
Expand Down
94 changes: 45 additions & 49 deletions src/Symfony/Component/DependencyInjection/ContainerBuilder.php
View file
Expand Up @@ -122,7 +122,6 @@ class ContainerBuilder extends Container implements TaggedContainerInterface
private $autoconfiguredInstanceof = array();

private $removedIds = array();
private $alreadyLoading = array();

private static $internalTypes = array(
'int' => true,
Expand Down Expand Up @@ -588,22 +587,32 @@ public function get($id, $invalidBehavior = ContainerInterface::EXCEPTION_ON_INV
return $this->doGet($id, $invalidBehavior);
}

private function doGet($id, $invalidBehavior = ContainerInterface::EXCEPTION_ON_INVALID_REFERENCE, array &$inlineServices = array())
private function doGet($id, $invalidBehavior = ContainerInterface::EXCEPTION_ON_INVALID_REFERENCE, array &$inlineServices = null, $isConstructorArgument = false)
{
$id = $this->normalizeId($id);

if (isset($inlineServices[$id])) {
return $inlineServices[$id];
}
if (ContainerInterface::IGNORE_ON_UNINITIALIZED_REFERENCE === $invalidBehavior) {
return parent::get($id, $invalidBehavior);
if (null === $inlineServices) {
$isConstructorArgument = true;
$inlineServices = array();
}
if ($service = parent::get($id, ContainerInterface::NULL_ON_INVALID_REFERENCE)) {
return $service;
try {
if (ContainerInterface::IGNORE_ON_UNINITIALIZED_REFERENCE === $invalidBehavior) {
return parent::get($id, $invalidBehavior);
}
if ($service = parent::get($id, ContainerInterface::NULL_ON_INVALID_REFERENCE)) {
return $service;
}
} catch (ServiceCircularReferenceException $e) {
if ($isConstructorArgument) {
throw $e;
}
}

if (!isset($this->definitions[$id]) && isset($this->aliasDefinitions[$id])) {
return $this->doGet((string) $this->aliasDefinitions[$id], $invalidBehavior, $inlineServices);
return $this->doGet((string) $this->aliasDefinitions[$id], $invalidBehavior, $inlineServices, $isConstructorArgument);
}

try {
Expand All @@ -616,16 +625,17 @@ private function doGet($id, $invalidBehavior = ContainerInterface::EXCEPTION_ON_
throw $e;
}

$loading = isset($this->alreadyLoading[$id]) ? 'loading' : 'alreadyLoading';
$this->{$loading}[$id] = true;
if ($isConstructorArgument) {
$this->loading[$id] = true;
}

try {
$service = $this->createService($definition, $inlineServices, $id);
return $this->createService($definition, $inlineServices, $isConstructorArgument, $id);
} finally {
unset($this->{$loading}[$id]);
if ($isConstructorArgument) {
unset($this->loading[$id]);
}
}

return $service;
}

/**
Expand Down Expand Up @@ -1092,7 +1102,7 @@ public function findDefinition($id)
* @throws RuntimeException When the service is a synthetic service
* @throws InvalidArgumentException When configure callable is not callable
*/
private function createService(Definition $definition, array &$inlineServices, $id = null, $tryProxy = true)
private function createService(Definition $definition, array &$inlineServices, $isConstructorArgument = false, $id = null, $tryProxy = true)
{
if (null === $id && isset($inlineServices[$h = spl_object_hash($definition)])) {
return $inlineServices[$h];
Expand All @@ -1110,16 +1120,14 @@ private function createService(Definition $definition, array &$inlineServices, $
@trigger_error($definition->getDeprecationMessage($id), E_USER_DEPRECATED);
}

if ($tryProxy && $definition->isLazy()) {
$proxy = $this
->getProxyInstantiator()
->instantiateProxy(
$this,
$definition,
$id, function () use ($definition, &$inlineServices, $id) {
return $this->createService($definition, $inlineServices, $id, false);
}
);
if ($tryProxy && $definition->isLazy() && !$tryProxy = !($proxy = $this->proxyInstantiator) || $proxy instanceof RealServiceInstantiator) {
$proxy = $proxy->instantiateProxy(
$this,
$definition,
$id, function () use ($definition, &$inlineServices, $id) {
return $this->createService($definition, $inlineServices, true, $id, false);
}
);
$this->shareService($definition, $proxy, $id, $inlineServices);

return $proxy;
Expand All @@ -1131,19 +1139,21 @@ private function createService(Definition $definition, array &$inlineServices, $
require_once $parameterBag->resolveValue($definition->getFile());
}

$arguments = $this->doResolveServices($parameterBag->unescapeValue($parameterBag->resolveValue($definition->getArguments())), $inlineServices);

if (null !== $id && $definition->isShared() && isset($this->services[$id]) && ($tryProxy || !$definition->isLazy())) {
return $this->services[$id];
}
$arguments = $this->doResolveServices($parameterBag->unescapeValue($parameterBag->resolveValue($definition->getArguments())), $inlineServices, $isConstructorArgument);

if (null !== $factory = $definition->getFactory()) {
if (\is_array($factory)) {
$factory = array($this->doResolveServices($parameterBag->resolveValue($factory[0]), $inlineServices), $factory[1]);
$factory = array($this->doResolveServices($parameterBag->resolveValue($factory[0]), $inlineServices, $isConstructorArgument), $factory[1]);
} elseif (!\is_string($factory)) {
throw new RuntimeException(sprintf('Cannot create service "%s" because of invalid factory', $id));
}
}

if (null !== $id && $definition->isShared() && isset($this->services[$id]) && ($tryProxy || !$definition->isLazy())) {
return $this->services[$id];
}

if (null !== $factory) {
$service = \call_user_func_array($factory, $arguments);

if (!$definition->isDeprecated() && \is_array($factory) && \is_string($factory[0])) {
Expand Down Expand Up @@ -1214,11 +1224,11 @@ public function resolveServices($value)
return $this->doResolveServices($value);
}

private function doResolveServices($value, array &$inlineServices = array())
private function doResolveServices($value, array &$inlineServices = array(), $isConstructorArgument = false)
{
if (\is_array($value)) {
foreach ($value as $k => $v) {
$value[$k] = $this->doResolveServices($v, $inlineServices);
$value[$k] = $this->doResolveServices($v, $inlineServices, $isConstructorArgument);
}
} elseif ($value instanceof ServiceClosureArgument) {
$reference = $value->getValues()[0];
Expand Down Expand Up @@ -1261,9 +1271,9 @@ private function doResolveServices($value, array &$inlineServices = array())
return $count;
});
} elseif ($value instanceof Reference) {
$value = $this->doGet((string) $value, $value->getInvalidBehavior(), $inlineServices);
$value = $this->doGet((string) $value, $value->getInvalidBehavior(), $inlineServices, $isConstructorArgument);
} elseif ($value instanceof Definition) {
$value = $this->createService($value, $inlineServices);
$value = $this->createService($value, $inlineServices, $isConstructorArgument);
} elseif ($value instanceof Parameter) {
$value = $this->getParameter((string) $value);
} elseif ($value instanceof Expression) {
Expand Down Expand Up @@ -1584,20 +1594,6 @@ protected function getEnv($name)
}
}

/**
* Retrieves the currently set proxy instantiator or instantiates one.
*
* @return InstantiatorInterface
*/
private function getProxyInstantiator()
{
if (!$this->proxyInstantiator) {
$this->proxyInstantiator = new RealServiceInstantiator();
}

return $this->proxyInstantiator;
}

private function callMethod($service, $call, array &$inlineServices)
{
foreach (self::getServiceConditionals($call[1]) as $s) {
Expand Down Expand Up @@ -1627,7 +1623,7 @@ private function shareService(Definition $definition, $service, $id, array &$inl

if (null !== $id && $definition->isShared()) {
$this->services[$id] = $service;
unset($this->loading[$id], $this->alreadyLoading[$id]);
unset($this->loading[$id]);
}
}

Expand Down

0 comments on commit ba31bab

Please sign in to comment.