Everything

php

Forbid binding methods to incompatible $this

php.internals

Nikita Popov

10 years ago

Hi internals!

Currently, inside instance methods the following invariant holds:

assert(is_null($this) || is_object($this))

This is a problem. I'd like to guarantee the following invariant instead:

assert(is_null($this) || $this instanceof self)

That is, ensure that if $this is not null, then it must be an object
"compatible" with the class, i.e. be in its inheritance hierarchy.

The absence of this guarantee, apart from being a major WTF in itself, also
prevents us from optimizing operations involving $this. In particular, the
following optimizations are not possible:

a) If typed properties land, we will not be able to use the type of
$this->typedProperty for optimization, because $this->typedProperty might
actually be referencing a property of a totally unrelated class.
b) We are not able to early-bind arguments to private or final method
calls, i.e. determine at compile-time whether an argument will use by-value
or by-reference argument passing and optimize accordingly.
c) We are not able to inline calls to private or final methods. (The
possibility of null is an issue in this instance as well, though a lesser
one.)

The good news is that, as of PHP 7.0, we are *very* close to guaranteeing
that "$this instanceof self" already. There exists only one little known
loophole: Obtaining a closure object wrapping the method using
ReflectionMethod::getClosure() and binding to an unrelated $this using
Closure::bindTo().

In PHP 7.0 we already heavily cut down [1] on what bindTo() is to allowed
to do on fake method closures. Namely, it was completely forbidden to
rebind the scope of methods, as this was already interacting badly with
optimizations in 7.0. We did not forbid binding to an incompatible $this
because it was not yet causing immediate issues at the time.

I'd like to remedy this oversight now and restrict $this binding of fake
method closures to compatible contexts only, i.e. for a method A::foo()
allow only instances of A or one of its descendant classes to be bound.
This limitation already exists for fake method closures targeting internal
methods.

Are there any objections to this change? If not, I'll merge the patch [2].
If yes, I'll write an RFC, as this change, while of no direct consequence
for PHP programmers, is important for the PHP implementation.

Regards,
Nikita

[1]: http://markmail.org/message/sjihplebuwsmdwex
[2]:
https://github.com/php/php-src/compare/master...nikic:forbigIncompatThis

Chris Riley

10 years ago

On 29 March 2016 at 23:21, Nikita Popov <nikita.ppv@gmail.com> wrote:

> Hi internals!
>
> Currently, inside instance methods the following invariant holds:
>
>     assert(is_null($this) || is_object($this))
>
> This is a problem. I'd like to guarantee the following invariant instead:
>
>     assert(is_null($this) || $this instanceof self)
>
> That is, ensure that if $this is not null, then it must be an object
> "compatible" with the class, i.e. be in its inheritance hierarchy.
>
> The absence of this guarantee, apart from being a major WTF in itself, also
> prevents us from optimizing operations involving $this. In particular, the
> following optimizations are not possible:
>
> a) If typed properties land, we will not be able to use the type of
> $this->typedProperty for optimization, because $this->typedProperty might
> actually be referencing a property of a totally unrelated class.
> b) We are not able to early-bind arguments to private or final method
> calls, i.e. determine at compile-time whether an argument will use by-value
> or by-reference argument passing and optimize accordingly.
> c) We are not able to inline calls to private or final methods. (The
> possibility of null is an issue in this instance as well, though a lesser
> one.)
>
> The good news is that, as of PHP 7.0, we are *very* close to guaranteeing
> that "$this instanceof self" already. There exists only one little known
> loophole: Obtaining a closure object wrapping the method using
> ReflectionMethod::getClosure() and binding to an unrelated $this using
> Closure::bindTo().
>
> In PHP 7.0 we already heavily cut down [1] on what bindTo() is to allowed
> to do on fake method closures. Namely, it was completely forbidden to
> rebind the scope of methods, as this was already interacting badly with
> optimizations in 7.0. We did not forbid binding to an incompatible $this
> because it was not yet causing immediate issues at the time.
>
> I'd like to remedy this oversight now and restrict $this binding of fake
> method closures to compatible contexts only, i.e. for a method A::foo()
> allow only instances of A or one of its descendant classes to be bound.
> This limitation already exists for fake method closures targeting internal
> methods.
>
> Are there any objections to this change? If not, I'll merge the patch [2].
> If yes, I'll write an RFC, as this change, while of no direct consequence
> for PHP programmers, is important for the PHP implementation.
>
> Regards,
> Nikita
>
> [1]: http://markmail.org/message/sjihplebuwsmdwex
> [2]:
> https://github.com/php/php-src/compare/master...nikic:forbigIncompatThis
>

Hi,

Will this patch break any use cases similar to:

class Bar {
private $a = 'hidden';
}

$x = new Bar();
$y = function() { return $this->a);
$z = $x->bindTo($x, $x);
echo $z(); //hidden

?

If not, I have no objections.

Nikita Popov

10 years ago

On Wed, Mar 30, 2016 at 11:29 AM, Chris Riley <t.carnage@gmail.com> wrote:

> On 29 March 2016 at 23:21, Nikita Popov <nikita.ppv@gmail.com> wrote:
>
>> Hi internals!
>>
>> Currently, inside instance methods the following invariant holds:
>>
>>     assert(is_null($this) || is_object($this))
>>
>> This is a problem. I'd like to guarantee the following invariant instead:
>>
>>     assert(is_null($this) || $this instanceof self)
>>
>> That is, ensure that if $this is not null, then it must be an object
>> "compatible" with the class, i.e. be in its inheritance hierarchy.
>>
>> The absence of this guarantee, apart from being a major WTF in itself,
>> also
>> prevents us from optimizing operations involving $this. In particular, the
>> following optimizations are not possible:
>>
>> a) If typed properties land, we will not be able to use the type of
>> $this->typedProperty for optimization, because $this->typedProperty might
>> actually be referencing a property of a totally unrelated class.
>> b) We are not able to early-bind arguments to private or final method
>> calls, i.e. determine at compile-time whether an argument will use
>> by-value
>> or by-reference argument passing and optimize accordingly.
>> c) We are not able to inline calls to private or final methods. (The
>> possibility of null is an issue in this instance as well, though a lesser
>> one.)
>>
>> The good news is that, as of PHP 7.0, we are *very* close to guaranteeing
>> that "$this instanceof self" already. There exists only one little known
>> loophole: Obtaining a closure object wrapping the method using
>> ReflectionMethod::getClosure() and binding to an unrelated $this using
>> Closure::bindTo().
>>
>> In PHP 7.0 we already heavily cut down [1] on what bindTo() is to allowed
>> to do on fake method closures. Namely, it was completely forbidden to
>> rebind the scope of methods, as this was already interacting badly with
>> optimizations in 7.0. We did not forbid binding to an incompatible $this
>> because it was not yet causing immediate issues at the time.
>>
>> I'd like to remedy this oversight now and restrict $this binding of fake
>> method closures to compatible contexts only, i.e. for a method A::foo()
>> allow only instances of A or one of its descendant classes to be bound.
>> This limitation already exists for fake method closures targeting internal
>> methods.
>>
>> Are there any objections to this change? If not, I'll merge the patch [2].
>> If yes, I'll write an RFC, as this change, while of no direct consequence
>> for PHP programmers, is important for the PHP implementation.
>>
>> Regards,
>> Nikita
>>
>> [1]: http://markmail.org/message/sjihplebuwsmdwex
>> [2]:
>> https://github.com/php/php-src/compare/master...nikic:forbigIncompatThis
>>
>
>
> Hi,
>
> Will this patch break any use cases similar to:
>
> class Bar {
> private $a = 'hidden';
> }
>
> $x = new Bar();
> $y = function() { return $this->a);
> $z = $x->bindTo($x, $x);
> echo $z(); //hidden
>
> ?
>
> If not, I have no objections.
>

Nope. Rebinding on "real" closures continues to work as usual, including
using it to access private properties of objects.

This change only affects closures created by
ReflectionMethod::getClosure(), i.e. closures that are really methods in
disguise.

Nikita

Dmitry Stogov

10 years ago

I agree, this ability is a dirty and annoying hack, but I'm sure, some people use it.

Tony, you don't use this in the new runkit replacement? :)

Thanks. Dmitry.

________________________________________
From: Nikita Popov <nikita.ppv@gmail.com>
Sent: Wednesday, March 30, 2016 01:21
To: PHP internals
Subject: [PHP-DEV] Forbid binding methods to incompatible $this

Hi internals!

Currently, inside instance methods the following invariant holds:

assert(is_null($this) || is_object($this))

This is a problem. I'd like to guarantee the following invariant instead:

assert(is_null($this) || $this instanceof self)

That is, ensure that if $this is not null, then it must be an object
"compatible" with the class, i.e. be in its inheritance hierarchy.

The absence of this guarantee, apart from being a major WTF in itself, also
prevents us from optimizing operations involving $this. In particular, the
following optimizations are not possible:

a) If typed properties land, we will not be able to use the type of
$this->typedProperty for optimization, because $this->typedProperty might
actually be referencing a property of a totally unrelated class.
b) We are not able to early-bind arguments to private or final method
calls, i.e. determine at compile-time whether an argument will use by-value
or by-reference argument passing and optimize accordingly.
c) We are not able to inline calls to private or final methods. (The
possibility of null is an issue in this instance as well, though a lesser
one.)

The good news is that, as of PHP 7.0, we are *very* close to guaranteeing
that "$this instanceof self" already. There exists only one little known
loophole: Obtaining a closure object wrapping the method using
ReflectionMethod::getClosure() and binding to an unrelated $this using
Closure::bindTo().

In PHP 7.0 we already heavily cut down [1] on what bindTo() is to allowed
to do on fake method closures. Namely, it was completely forbidden to
rebind the scope of methods, as this was already interacting badly with
optimizations in 7.0. We did not forbid binding to an incompatible $this
because it was not yet causing immediate issues at the time.

I'd like to remedy this oversight now and restrict $this binding of fake
method closures to compatible contexts only, i.e. for a method A::foo()
allow only instances of A or one of its descendant classes to be bound.
This limitation already exists for fake method closures targeting internal
methods.

Are there any objections to this change? If not, I'll merge the patch [2].
If yes, I'll write an RFC, as this change, while of no direct consequence
for PHP programmers, is important for the PHP implementation.

Regards,
Nikita

[1]: http://markmail.org/message/sjihplebuwsmdwex
[2]:
https://github.com/php/php-src/compare/master...nikic:forbigIncompatThis

Nikita Popov

10 years ago

On Wed, Mar 30, 2016 at 11:34 AM, Dmitry Stogov <dmitry@zend.com> wrote:

> I agree, this ability is a dirty and annoying hack, but I'm sure, some
> people use it.
>
> Tony, you don't use this in the new runkit replacement? :)
>
> Thanks. Dmitry.
>

Is this referring to https://github.com/badoo/soft-mocks? If so, searching
the codebase for getClosure gives no results, so it shouldn't be affected.

I know that the Go! AOP framework used to do some odd things with closure
binding on methods. However their use case was *already* completely broken
by the limitations we added in 7.0 and the feature was removed.

Nikita

Alexander Lissachenko

10 years ago

Hello, internals!

Go! AOP was used closure rebinding to an incompatible context only for one
minor specific feature, called privileged advices, where method was
executing in the context of target class. But all main closure binding in
the framework core work only with compatible contexts, so everything is ok
with PHP7 now.

So, this new patch is ok for me. Anyway, if I need this functionality
again, I can convert methods to the closures via AST transformation of
classes.

2016-03-30 14:02 GMT+03:00 Nikita Popov <nikita.ppv@gmail.com>:

Dmitry Stogov

10 years ago

Great it's not used anymore [&#X1f60a]

I personally agree to remove this.

Nikita, can you also investigate the need for double pointer to object in

typedef union _zend_function *(*zend_object_get_method_t)(zend_object **object, zend_string *method, const zval *key);

As I remember, it's necessary only for some SPL hack, and looks like a terrible design decision.

Thanks. Dmitry.

________________________________
From: Alexander Lisachenko <lisachenko.it@gmail.com>
Sent: Wednesday, March 30, 2016 15:07
To: Nikita Popov
Cc: Dmitry Stogov; Antony Dovgal; PHP internals
Subject: Re: [PHP-DEV] Forbid binding methods to incompatible $this

Hello, internals!

Go! AOP was used closure rebinding to an incompatible context only for one minor specific feature, called privileged advices, where method was executing in the context of target class. But all main closure binding in the framework core work only with compatible contexts, so everything is ok with PHP7 now.

So, this new patch is ok for me. Anyway, if I need this functionality again, I can convert methods to the closures via AST transformation of classes.

2016-03-30 14:02 GMT+03:00 Nikita Popov <nikita.ppv@gmail.com<mailto:nikita.ppv@gmail.com>>:
On Wed, Mar 30, 2016 at 11:34 AM, Dmitry Stogov <dmitry@zend.com<mailto:dmitry@zend.com>> wrote:

> I agree, this ability is a dirty and annoying hack, but I'm sure, some
> people use it.
>
> Tony, you don't use this in the new runkit replacement? :)
>
> Thanks. Dmitry.
>

Is this referring to https://github.com/badoo/soft-mocks? If so, searching
the codebase for getClosure gives no results, so it shouldn't be affected.

I know that the Go! AOP framework used to do some odd things with closure
binding on methods. However their use case was *already* completely broken
by the limitations we added in 7.0 and the feature was removed.

Nikita

Nikita Popov

10 years ago

On Wed, Mar 30, 2016 at 6:18 PM, Dmitry Stogov <dmitry@zend.com> wrote:

> Great it's not used anymore [image: &#X1f60a]
>
> I personally agree to remove this.
>
>
> Nikita, can you also investigate the need for double pointer to object in
>
>
> typedef union _zend_function *(*zend_object_get_method_t)(zend_object
> **object, zend_string *method, const zval *key);
>
>
> As I remember, it's necessary only for some SPL hack, and looks like a
> terrible design decision.
>

This is used for forwarding method calls to the the "inner" object in SPL
iterators. It's basically the internal equivalent of doing:

function __call($method, $args) {
return $this->inner->$method(...$args);
}

I don't think we can drop this feature at this point (deprecate?). We can
of course change the implementation to use a trampoline function to avoid
the zend_object** hack, but I'm not sure that would be worthwhile.

Nikita

Dmitry Stogov

10 years ago

On 03/30/2016 10:46 PM, Nikita Popov wrote:

> On Wed, Mar 30, 2016 at 6:18 PM, Dmitry Stogov <dmitry@zend.com
> <mailto:dmitry@zend.com>> wrote:
>
>     Great it's not used anymore &#X1f60a
>
>     I personally agree to remove this.
>
>
>     Nikita, can you also investigate the need for double pointer to
>     object in
>
>
>     typedef union _zend_function
>     *(*zend_object_get_method_t)(zend_object **object, zend_string
>     *method, const zval *key);
>
>
>     As I remember, it's necessary only for some SPL hack, and looks
>     like a terrible design decision.
>
> This is used for forwarding method calls to the the "inner" object in
> SPL iterators. It's basically the internal equivalent of doing:
>
> function __call($method, $args) {
>     return $this->inner->$method(...$args);
> }
>
> I don't think we can drop this feature at this point (deprecate?). We
> can of course change the implementation to use a trampoline function
> to avoid the zend_object** hack, but I'm not sure that would be
> worthwhile.

thank you for discovering this.
Of course we can't drop or change SPL behavior.
Trampoline looks like a possible but too complex solution.
anyway, it would be great to remove this hack.

Thanks. Dmitry.

Nikita Popov

10 years ago

On Wed, Mar 30, 2016 at 7:21 AM, Nikita Popov <nikita.ppv@gmail.com> wrote:

> Hi internals!
>
> Currently, inside instance methods the following invariant holds:
>
>     assert(is_null($this) || is_object($this))
>
> This is a problem. I'd like to guarantee the following invariant instead:
>
>     assert(is_null($this) || $this instanceof self)
>
> That is, ensure that if $this is not null, then it must be an object
> "compatible" with the class, i.e. be in its inheritance hierarchy.
>
> The absence of this guarantee, apart from being a major WTF in itself,
> also prevents us from optimizing operations involving $this. In particular,
> the following optimizations are not possible:
>
> a) If typed properties land, we will not be able to use the type of
> $this->typedProperty for optimization, because $this->typedProperty might
> actually be referencing a property of a totally unrelated class.
> b) We are not able to early-bind arguments to private or final method
> calls, i.e. determine at compile-time whether an argument will use by-value
> or by-reference argument passing and optimize accordingly.
> c) We are not able to inline calls to private or final methods. (The
> possibility of null is an issue in this instance as well, though a lesser
> one.)
>
> The good news is that, as of PHP 7.0, we are *very* close to guaranteeing
> that "$this instanceof self" already. There exists only one little known
> loophole: Obtaining a closure object wrapping the method using
> ReflectionMethod::getClosure() and binding to an unrelated $this using
> Closure::bindTo().
>
> In PHP 7.0 we already heavily cut down [1] on what bindTo() is to allowed
> to do on fake method closures. Namely, it was completely forbidden to
> rebind the scope of methods, as this was already interacting badly with
> optimizations in 7.0. We did not forbid binding to an incompatible $this
> because it was not yet causing immediate issues at the time.
>
> I'd like to remedy this oversight now and restrict $this binding of fake
> method closures to compatible contexts only, i.e. for a method A::foo()
> allow only instances of A or one of its descendant classes to be bound.
> This limitation already exists for fake method closures targeting internal
> methods.
>
> Are there any objections to this change? If not, I'll merge the patch [2].
> If yes, I'll write an RFC, as this change, while of no direct consequence
> for PHP programmers, is important for the PHP implementation.
>
> Regards,
> Nikita
>
> [1]: http://markmail.org/message/sjihplebuwsmdwex
> [2]:
> https://github.com/php/php-src/compare/master...nikic:forbigIncompatThis
>

As the discussion was positive, this is now merged as
https://github.com/php/php-src/commit/75af8150f58fb55637ac12b33d469b27adef9d76
.

Nikita

Yasuo Ohgaki

10 years ago

Hi Nikita,

On Fri, Apr 8, 2016 at 7:42 AM, Nikita Popov <nikita.ppv@gmail.com> wrote:

> As the discussion was positive, this is now merged as
> https://github.com/php/php-src/commit/75af8150f58fb55637ac12b33d469b27adef9d76

Could you write UPGRADING for this?

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net