Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:88736
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmail.com designates 209.85.212.177 as permitted sender)
MIME-Version: 1.0
Date: Sat, 10 Oct 2015 15:26:07 +0200
Message-ID: <CAF+90c_io7TG4P+=cZrMEwhv=+9PxLQDQa9fijSG7OPfoKg6hA@mail.gmail.com>
To: PHP internals <internals@lists.php.net>, Andrea Faulds <ajf@ajf.me>, 
	Stas Malyshev <smalyshev@sugarcrm.com>, Dmitry Stogov <dmitry@zend.com>, Bob Weinand <bwoebi@php.net>, 
	Anatol Belski <anatol.php@belski.net>
Content-Type: multipart/alternative; boundary=001a1135f0b2ef201e0521c009df
Subject: Forbid rebinding scope of closures created by ReflectionFunctionAbstract::getClosure()
From: nikita.ppv@gmail.com (Nikita Popov)

--001a1135f0b2ef201e0521c009df
Content-Type: text/plain; charset=UTF-8

Hi internals!

We have recently been reviewing the interaction between
ReflectionFunctionAbstract::getClosure(), a mechanism which converts an
ordinary function or method into a "fake" closure, and closure rebinding
using Closure::bindTo() and Closure::call().

It turns out that this combination has not yet received testing and
multiple crashes and leaks were found and fixed [1] [2] [3] [4].

We have one last outstanding changeset [5] waiting to land, which we want
to check back with internals first, as it constitutes a BC break late in
the PHP 7.0 release cycle.

This changeset forbids rebinding the *scope* of closures returned by
getClosure() completely. The background of this change is that PHP 7
includes optimizations that early-bind the scope of self:: during
compilation (and there are more optimizations of this nature pending to
land in PHP 7.1). This means that attempts to change the meaning of "self"
in an ordinary method at runtime will not always succeed and lead to
inconsistent results.

Based on feedback received on an earlier version of this patch [6] [7], we
have not added any additional restrictions on rebinding of $this.
getClosure()s of userland methods can still be bound to arbitrary $this
values and getClosure()s of internal methods can still be bound to
"compatible" $this values. (If someone sees a problem with allowing the
former, please speak up!)

Note that all this applies to ReflectionFunctionAbstract::getClosure()
only. Rebinding behavior of ordinary closures doesn't change.

It would be appreciated if some more people familiar with the closure
implementation could take a look at our rebinding rules and see if there's
any situations that aren't covered yet and could be problematic.

Thanks,
Nikita

[1] https://bugs.php.net/bug.php?id=70630
[2] https://bugs.php.net/bug.php?id=70674
[3] https://bugs.php.net/bug.php?id=70681
[4] https://bugs.php.net/bug.php?id=70685
[5] https://github.com/php/php-src/pull/1560
[6]
https://github.com/php/php-src/commit/517b5536259ecf7697f353f4bfbafde857fc1f81
[7]
https://github.com/php/php-src/commit/881c50252066132f83e190325e344f532be19033

--001a1135f0b2ef201e0521c009df--