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

Lack of infinite recursion detection for abstract factories #218

Open
boesing opened this issue Jan 21, 2024 · 5 comments
Open

Lack of infinite recursion detection for abstract factories #218

boesing opened this issue Jan 21, 2024 · 5 comments
Labels
Bug Something isn't working

Comments

@boesing
Copy link
Member

boesing commented Jan 21, 2024

Bug Report

Q A
Version(s) 3.22.1, 4.0.0*

Summary

There is a lack of infinite recursion detection for both get and has when it comes to abstract factories.

Ref: laminas/laminas-cache#284

Current behavior

When checking a service via has in factories (one abstract factory has to be involved) or receiving a service via get, an infinite recursion along with an application crash is the result.

  1. $container->has('foo');
  2. SM checks all services, aliases and factories for foo
  3. foo not found in services, aliases or factories
  4. SM checks all abstract factories for foo
  5. First abstract factory which checks for $container->has('foo'); will start over No. 1

How to reproduce

use Laminas\ServiceManager\Factory\AbstractFactoryInterface;
use Psr\Container\ContainerInterface;

final class GenericAbstractFactory implements AbstractFactoryInterface
{
    public function canCreate(ContainerInterface $container, $requestedName)
    {
         if (!$container->has('foo')) {
             return false;
         }
        
          $foo = $container->get('foo');
          // check foo for whatever
          return true; // or false, depending on what $foo represents
    }

    public function __invoke(ContainerInterface $container, $requestedName, ?array $options = null)
    {
         $foo = $container->get('foo'); // we can safely assume that `foo` exists since we checked that in `canCreate` alrady
         // do whatever with foo
         return stdClass(); // return service, maybe with data from foo
    }
}

final class GenericFactory
{
     public function __invoke(ContainerInterface $container): object
     {
          if (!$container->has('foo')) {
              return new stdClass();
          }

          $foo = $container->get('foo');
          // do whatever with foo
          return new stdClass();     
     }
}

$container = new ServiceManager([
     'abstract_factories' => [
         GenericAbstractFactory::class,
      ],
     'factories' => [
         stdClass::class => GenericFactory::class,
     ],
]);

$object = $container->get(stdClass::class); // infinite loop occurs
// unreachable line

Expected behavior

Infinite recursions are prevented from abstract factories.

@boesing boesing added the Bug Something isn't working label Jan 21, 2024
@Ocramius

This comment was marked as outdated.

@boesing

This comment was marked as outdated.

@boesing
Copy link
Member Author

boesing commented Jan 21, 2024

I would implement somthing like this:

private function abstractFactoryCanCreate(string $name): bool
{
foreach ($this->abstractFactories as $abstractFactory) {
if ($abstractFactory->canCreate($this->creationContext, $name)) {
return true;
}
}
$resolvedName = $this->aliases[$name] ?? $name;
if ($resolvedName !== $name) {
return $this->abstractFactoryCanCreate($resolvedName);
}
return false;

       if (isset($this->pendingAbstractFactoryChecks[$name])) { 
           return false; 
       }
       $this->pendingAbstractFactoryChecks[$name] = $name;
       foreach ($this->abstractFactories as $abstractFactory) {
           if ($abstractFactory->canCreate($this->creationContext, $name)) {
              unset($this->pendingAbstractFactoryChecks[$name]);
              return true;
           }
        }

        $resolvedName = $this->aliases[$name] ?? $name;
        if ($resolvedName !== $name) {
            if ($this->abstractFactoryCanCreate($resolvedName)) {
                unset($this->pendingAbstractFactoryChecks[$name]);
                return true;
            }
        }

        unset($this->pendingAbstractFactoryChecks[$name]);
        return false;

But I guess that the problem might be that we have to keep track of which abstract factory is currently being checked and just skip that abstract factory which was called lately so that all but the current one is being checked.
So the code above might be too "dumb".

@Ocramius

This comment was marked as outdated.

@boesing boesing changed the title Lack of infinite loop detection for abstract factories Lack of infinite recursion detection for abstract factories Jan 21, 2024
@boesing

This comment was marked as off-topic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants