Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:88751
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain zend.com designates 209.85.213.52 as permitted sender)
MIME-Version: 1.0
In-Reply-To: <CAF+90c_io7TG4P+=cZrMEwhv=+9PxLQDQa9fijSG7OPfoKg6hA@mail.gmail.com>
References: <CAF+90c_io7TG4P+=cZrMEwhv=+9PxLQDQa9fijSG7OPfoKg6hA@mail.gmail.com>
Date: Mon, 12 Oct 2015 21:24:51 +0300
Message-ID: <CA+9eiLviENrreQMHUguRYpjht=PCA4jepNWME72VA-ihZYeTTw@mail.gmail.com>
To: Nikita Popov <nikita.ppv@gmail.com>
Cc: PHP internals <internals@lists.php.net>, Andrea Faulds <ajf@ajf.me>, 
	Stas Malyshev <smalyshev@sugarcrm.com>, Bob Weinand <bwoebi@php.net>, 
	Anatol Belski <anatol.php@belski.net>
Content-Type: multipart/alternative; boundary=001a11415a8efac6190521ec712a
Subject: Re: Forbid rebinding scope of closures created by ReflectionFunctionAbstract::getClosure()
From: dmitry@zend.com (Dmitry Stogov)

--001a11415a8efac6190521ec712a
Content-Type: text/plain; charset=UTF-8

Hi Nikita,

I think the effect of the remaining patch is negligible.
It disables things, that shouldn't work by design, didn't work in fact and
leaded to crashes.
It might work only in some cases and only because of luck.

So, I think it's better to commit this before next RC.
Anatol is going to start rolling it on Tuesday.
He will also increase all API version numbers.

It would be great, if we stop any commits into PHP-7.0 except for critical
fixes now

Thanks. Dmitry.

On Sat, Oct 10, 2015 at 4:26 PM, Nikita Popov <nikita.ppv@gmail.com> wrote:

> 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
>

--001a11415a8efac6190521ec712a--