Ocramius/ProxyManager

Notice: Only variable references should be returned by reference

greg0ire opened this issue · 27 comments

greg0ire commented

Description
I'm getting the notice in the title. The code that causes it looks like this:

$accessor = function & () use ($targetObject, $name, $value) {
    return $targetObject->$name = $value;
};

How to reproduce

I don't know how to reproduce the generation of this piece of code yet, but reproducing the notice is easy enough:

https://3v4l.org/dE26T

I suspect it is related to this service definition (which looks quite simple):

    #Config amqp_lib
    PhpAmqpLib\Connection\AMQPStreamConnection:
        lazy: true
        class: PhpAmqpLib\Connection\AMQPStreamConnection
        arguments:
            - "%swarrot.host%"
            - "%swarrot.port%"
            - "%swarrot.login%"
            - "%swarrot.password%"
            - "%swarrot.vhost%"

Possible solution

Do it in 2 steps: https://3v4l.org/j501X ?

Stack trace:


/var/www/var/cache/prod/ContainerMifQx3V/AMQPStreamConnection_99651dd.php:251,
--
/var/www/vendor/php-amqplib/php-amqplib/PhpAmqpLib/Connection/AbstractConnection.php:643,
/var/www/vendor/php-amqplib/php-amqplib/PhpAmqpLib/Channel/AbstractChannel.php:234,
/var/www/vendor/php-amqplib/php-amqplib/PhpAmqpLib/Channel/AbstractChannel.php:351,
/var/www/vendor/php-amqplib/php-amqplib/PhpAmqpLib/Channel/AMQPChannel.php:287,
/var/www/vendor/php-amqplib/php-amqplib/PhpAmqpLib/Channel/AMQPChannel.php:117,
/var/www/var/cache/prod/ContainerMifQx3V/getAMQPChannelService.php:35,
/var/www/var/cache/prod/ContainerMifQx3V/getAMQPChannelService.php:23,
/var/www/var/cache/prod/ContainerMifQx3V/AMQPChannel_f219a5d.php:116,
/var/www/var/cache/prod/ContainerMifQx3V/AMQPChannel_f219a5d.php:116,
/var/www/src/Index/Prepare/PrepareProductToIndex.php:173,
/var/www/src/Index/Prepare/PrepareProductToIndex.php:114,
/var/www/src/Index/Prepare/PrepareProductToIndex.php:70,
/var/www/src/Command/AlgoliaPrepareReindexFullCommand.php:107,
/var/www/vendor/symfony/console/Command/Command.php:258,
/var/www/vendor/symfony/console/Application.php:938,
/var/www/vendor/symfony/framework-bundle/Console/Application.php:96,
/var/www/vendor/symfony/console/Application.php:266,
/var/www/vendor/symfony/framework-bundle/Console/Application.php:82,
/var/www/vendor/symfony/console/Application.php:142,
/var/www/bin/console:49

And here is the contents of that container file: https://hastebin.net/wuzirolega.php

    public function __set($name, $value)
    {
        $this->initializer2d678 && ($this->initializer2d678->__invoke($valueHolder0a36b, $this, '__set', array('name' => $name, 'value' => $value), $this->initializer2d678) || 1) && $this->valueHolder0a36b = $valueHolder0a36b;

        if (isset(self::$publicProperties70f0e[$name])) {
            return ($this->valueHolder0a36b->$name = $value);
        }

        $realInstanceReflection = new \ReflectionClass(get_parent_class($this));

        if (! $realInstanceReflection->hasProperty($name)) {
            $targetObject = $this->valueHolder0a36b;

            return $targetObject->$name = $value;
            return;
        }

        $targetObject = $this->valueHolder0a36b;
        $accessor = function & () use ($targetObject, $name, $value) {
            return $targetObject->$name = $value;
        };
        $backtrace = debug_backtrace(true, 2);
        $scopeObject = isset($backtrace[1]['object']) ? $backtrace[1]['object'] : new \ProxyManager\Stub\EmptyClassStub();
        $accessor = $accessor->bindTo($scopeObject, get_class($scopeObject));
        $returnValue = & $accessor();

        return $returnValue;
    }

I originally reported that at symfony/symfony#39089

This comes from these lines

ProxyManager/tests/ProxyManagerTest/ProxyGenerator/Util/PublicScopeSimulatorTest.php

Lines 78 to 80 in f65ae0f

$accessor = function & () use ($targetObject, $name, $value) {
return $targetObject->$foo = $baz;
};

Ocramius commented

@greg0ire I suggest dumping the DIC and looking at subclasses of AMQPStreamConnection there: the generated code will be there.

greg0ire commented

There is a link with the generated code that Github didn't highlight because I failed to prefix it with https://, fixed now.

UPD: I also added a link to the relevant lines in this lib. This is 4 year old code apparently, I'm not quite sure why I didn't experience that issue before 😅

Ocramius commented

Hmm, this happens when trying to access a property that is not defined/accessible on the class.

The code being executed is supposed to simulate what happens in such scenario, for example by raising notices/warnings/errors like PHP core would.

What is happening here is probably that some property access in /var/www/vendor/php-amqplib/php-amqplib/PhpAmqpLib/Connection/AbstractConnection.php:643, is reading (or writing) to a property that is not supposed to be accessible, and therefore tripping this "warning simulation code".

The notice about by-ref being used incorrectly is still valid, but the reason why that code is reached seems to be related to an incorrect property access.

greg0ire commented

The property being accessed here seems to be last_frame: https://github.com/php-amqplib/php-amqplib/blob/a18b083726a8311222e19d71aa0ec12acff7bfec/PhpAmqpLib/Connection/AbstractConnection.php#L250

It's protected, but in the same class so it should be accessible 🤔

greg0ire commented

Also, in https://3v4l.org/dE26T , bar is accessible, yet we get a notice.

Ocramius commented

I wonder if this is not caught because of phpt runner defaults: we have tests for this stuff, for example in https://github.com/Ocramius/ProxyManager/blob/f65ae0f9dcbdd9d6ad3abb721a9e09c3d7d868a4/tests/language-feature-scripts/lazy-loading-value-holder-denies-protected-property-read.phpt

Perhaps we need to set an ini_set('error_reporting', E_ALL) missing there, hence why we didn't notice it?

ProxyManager/tests/language-feature-scripts/init.php

Lines 1 to 12 in f65ae0f

<?php
declare(strict_types=1);
use ProxyManager\Configuration;
use ProxyManager\GeneratorStrategy\EvaluatingGeneratorStrategy;
require_once __DIR__ . '/../../vendor/autoload.php';
$configuration = new Configuration();
$configuration->setGeneratorStrategy(new EvaluatingGeneratorStrategy());

greg0ire commented

I don't get a notice when running vendor/bin/phpunit tests/language-feature-scripts/lazy-loading-value-holder-denies-protected-property-read.phpt and I'm using -1 as a value for error_reporting… is this the right test though? It seems to be about denying a read, when in our case, it should be granted.

Ocramius commented

There are tests for public property access in the integration test suite:

ProxyManager/tests/ProxyManagerTest/Functional/LazyLoadingValueHolderFunctionalTest.php

Lines 151 to 165 in f65ae0f

/**
* @param mixed $propertyValue
*
* @dataProvider getPropertyAccessProxies
*/
public function testPropertyReadAccess(
object $instance,
VirtualProxyInterface $proxy,
string $publicProperty,
$propertyValue
): void {
self::assertSame($propertyValue, $proxy->$publicProperty);
self::assertTrue($proxy->isProxyInitialized());
self::assertEquals($instance, $proxy->getWrappedValueHolderValue());
}

greg0ire commented

I have no notice for that one either:

vendor/bin/phpunit tests/ProxyManagerTest/Functional/LazyLoadingValueHolderFunctionalTest.php  --filter testPropertyRead
PHPUnit 9.4.3 by Sebastian Bergmann and contributors.

Runtime:       PHP 7.4.12
Configuration: /tmp/ProxyManager/phpunit.xml.dist
Warning:       Your XML configuration validates against a deprecated schema.
Suggestion:    Migrate your XML configuration using "--migrate-configuration"!

....                                                                                                                                                                                                                              4 / 4 (100%)

Time: 00:00.004, Memory: 12.00 MB

OK (4 tests, 12 assertions)

I do have one when executing the code from 3v4l locally.

Ocramius commented

Mean that the code path is not hit at all: will need to run tests with code coverage on top of generated proxies 🤔

Ocramius commented

I wonder if this is due to AMQPStreamConnection not having correct reflection information: is that an extension, or a library class?

greg0ire commented

I believe it's from https://github.com/php-amqplib/php-amqplib/blob/master/PhpAmqpLib/Connection/AMQPStreamConnection.php , but I see we also have the amqp extension enabled 😬 but given the stack trace I think the issue would be with the lib?

greg0ire commented

The extension has no stub for this class: https://github.com/php-amqp/php-amqp/tree/master/stubs so I think that settles it.

Ocramius commented

@greg0ire so if I see this correctly:

  • there is no reflection information for these AMQPStreamConnection (in the upstream extension)
  • due to that, we observe no property map generation in proxies that inherit from AMQPStreamConnection
  • once a property access on a proxy of type AMQPStreamConnection is performed, since no property definitions exist, the executed code path reaches the property access emulation code.

There are therefore 2 interrelated issues:

  1. missing reflection information leading to imprecise proxy
  2. imprecise proxy having some invalid by-ref closure definition (fixable, just hard to test - possibly worth testing a proxy that extends from stdClass)
greg0ire commented

I'm not sure the extension has anything to do with this. From the README of https://github.com/php-amqplib/php-amqplib

This library is a pure PHP implementation of the AMQP 0-9-1 protocol.

I'm not even sure why we have the amqp extension enabled on that server, it might be a remnant from a time where we did not use that library.

I think the library and the extension have different APIs that do not interact at all with each other.

greg0ire commented

I reproduced the issue locally; if I disable the amqp extension, I keep getting that issue, so it's probably unrelated to that extension.

Ocramius commented

Is it somehow reproducible in isolation?

greg0ire commented

I will work on a reproducer 👍

greg0ire commented

Reproduced 🎉

git clone https://github.com/greg0ire/ocramius-proxy-manager-reproducer-632
cd ocramius-proxy-manager-reproducer-632
composer install
cat var/cache/dev/Container*/AMQPStreamConnection_*.php # that file should have the faulty code

I started a project from scratch and didn't have to do much to reproduce the issue (see the second commit in my repository), so I think it's a bit weird I'm the only one reporting this…

Ocramius commented

@greg0ire could you commit var/cache from your environment? You know exactly what I'm gonna say about cloning a project... :-P

greg0ire commented

Indeed, my bad (but that code shouldn't be generated in __get either I suppose).

Also, the 2 return statements here look a bit weird too: https://github.com/greg0ire/ocramius-proxy-manager-reproducer-632/blob/48da78a815dbe105734f5d85faa82e1dcf36274d/var/cache/dev/Container3vGn1b7/AMQPStreamConnection_99651dd.php#L344-L345 😅

Ocramius commented

(but that code shouldn't be generated in __get either I suppose).

That code is supposed to be in __get to simulate a notice/fatal on property access

Also, the 2 return statements here look a bit weird too:

yeah, the codegen is really just string concatenation, so I never really bothered to clean those up (generated code is full of them)

  • 👍1
Ocramius commented

Handled via #652

  • 🎉1
greg0ire commented

Thanks! You guys rock!