Just a simple use case showing how dangerous that is :
<?php
$p = function($p) { $this->$p = new Stdclass; };
$p->call($e = new Exception, 'trace');
throw $e;
That nearly crashes PHP7.
http://3v4l.org/fJj22
(The same trick using Reflection with PHP5 crashes the engine, this is a
known bug that we chose not to fix).
I am absolutely not confident at all having Closure::call() beeing able to
access private data.
Private is private.
For internal classes, this is even worse, and could lead to crash (look at
the example about Exception).
Changing a variable that is private, is absolutely NOT expected from the
class designer.
For User classes, this could lead to information leaks or bad behaviors,
and for internal classes this is even worse and could easilly segfault or
with a little bit of more magic and brain sauce, lead to memory disclosures.
I would like we re-open the debate about accessign private data with
external code (closure), especially in a write context.
Julien.P
Just a simple use case showing how dangerous that is :
<?php
$p = function($p) { $this->$p = new Stdclass; };
$p->call($e = new Exception, 'trace');
throw $e;That nearly crashes PHP7.
http://3v4l.org/fJj22(The same trick using Reflection with PHP5 crashes the engine, this is a
known bug that we chose not to fix).I am absolutely not confident at all having Closure::call() beeing able to
access private data.Private is private.
For internal classes, this is even worse, and could lead to crash (look at
the example about Exception).
Changing a variable that is private, is absolutely NOT expected from the
class designer.For User classes, this could lead to information leaks or bad behaviors,
and for internal classes this is even worse and could easilly segfault or
with a little bit of more magic and brain sauce, lead to memory
disclosures.I would like we re-open the debate about accessign private data with
external code (closure), especially in a write context.Julien.P
For the record this isn't exclusive to Closure::call, afaik the same thing
is possible with Closure::bind() since 5.4
http://3v4l.org/hlFS4
And there are/were a decent amount of discussion (and AFAIR even hydrate
libraries using this trick) about this, for example:
http://ocramius.github.io/blog/accessing-private-php-class-members-without-reflection/
So while I think that for 7.0 we could remove this behavior, the cat is
already out of the bag, as you can see from my paste you can use that code
to cause segfaults for php >=5.4.0, so I think it would be better to fix
the internal classes to properly handle/validate their properties instead
of blindly trusting their types.
--
Ferenc Kovács
@Tyr43l - http://tyrael.hu
Hi,
I'm no longer subscribed to internals so I don't know if this email will show up there. If not, feel free to forward it.
Just a simple use case showing how dangerous that is :
<?php
$p = function($p) { $this->$p = new Stdclass; };
$p->call($e = new Exception, 'trace');
throw $e;That nearly crashes PHP7.
http://3v4l.org/fJj22(The same trick using Reflection with PHP5 crashes the engine, this is a
known bug that we chose not to fix).I am absolutely not confident at all having Closure::call() beeing able to
access private data.Private is private.
For internal classes, this is even worse, and could lead to crash (look at
the example about Exception).
Changing a variable that is private, is absolutely NOT expected from the
class designer.For User classes, this could lead to information leaks or bad behaviors,
and for internal classes this is even worse and could easilly segfault or
with a little bit of more magic and brain sauce, lead to memory disclosures.I would like we re-open the debate about accessign private data with
external code (closure), especially in a write context.Julien.P
For the record this isn't exclusive to Closure::call, afaik the same thing is possible with Closure::bind() since 5.4
http://3v4l.org/hlFS4
And there are/were a decent amount of discussion (and AFAIR even hydrate libraries using this trick) about this, for example:
http://ocramius.github.io/blog/accessing-private-php-class-members-without-reflection/
So while I think that for 7.0 we could remove this behavior, the cat is already out of the bag, as you can see from my paste you can use that code to cause segfaults for php >=5.4.0, so I think it would be better to fix the internal classes to properly handle/validate their properties instead of blindly trusting their types.
Right, what Closure::call() does isn't particularly new. I'm not sure if it's a good thing that you can arbitrarily re-scope closures, but that's how it is.
Also, if information leaks are a worry, we've had things like ob_start() and var_dump(), and (array) for a while now.
A further thought: type hints for class properties would be helpful here.
Thanks.
--
Andrea Faulds
http://ajf.me/
For the record this isn't exclusive to Closure::call, afaik the same thing
is possible with Closure::bind() since 5.4
http://3v4l.org/hlFS4
And there are/were a decent amount of discussion (and AFAIR even hydrate
libraries using this trick) about this, for example:http://ocramius.github.io/blog/accessing-private-php-class-members-without-reflection/
Also note that this is the only way to access private properties by-ref
(yes, I do need that for my horrible use-cases. No, don't ask about it.)
Marco Pivetta
My opinion is, that we simply have to change PHP closures' scoping to be
more similar with JavaScript functions' scope. That would solve the current
challenge pointed on by @jpauli, thus closure would have access to private
object's members only if it was defined in that object's class / method
definition, so then engine can know that it was programmers intention. I
mean something like this:
<?php
$p = function() { $this->privateMember = new stdClass; };
class C {
private $privateMember;
private $closure;
function __construct() {
$this->closure = function() {
$this->privateMember = 32;
};
}
function callClosure() {
$this->closure->call($this);
}
}
$c = new C;
$c->callClosure(); // would succeed
$p->call($c); // Fatal error
?>
What do you think about this kind of behavior?
Best regards,
Kubo2
2015-04-17 22:03 GMT+02:00 Marco Pivetta ocramius@gmail.com:
For the record this isn't exclusive to Closure::call, afaik the same
thing
is possible with Closure::bind() since 5.4
http://3v4l.org/hlFS4
And there are/were a decent amount of discussion (and AFAIR even hydrate
libraries using this trick) about this, for example:http://ocramius.github.io/blog/accessing-private-php-class-members-without-reflection/
Also note that this is the only way to access private properties by-ref
(yes, I do need that for my horrible use-cases. No, don't ask about it.)Marco Pivetta
Hi!
Just a simple use case showing how dangerous that is :
<?php
$p = function($p) { $this->$p = new Stdclass; };
$p->call($e = new Exception, 'trace');
throw $e;
Yes, this is not good, and this is the consequence of allowing to rebind
closures. I'm not sure though how to fix it except for banning closures
from assuming scope of internal classes. If you assigned the scope of
Exception to it, it should have access to Exception - that's how the
scope works.
For User classes, this could lead to information leaks or bad behaviors,
I'm not sure what you mean by "information leaks", but the behavior is
on whoever wrote that code. People can write bad code, we can't disallow
this. We can restrict things that would really break (like segfault) but
I'm not sure what else we can do here. If you have the scope of the
class, that implies access to the private members of the class. So we
either have to not let the closure have the scope, or invent some
additional term of scope' that is not like real scope. I don't think
it'd be good.
--
Stas Malyshev
smalyshev@gmail.com
On Mon, Apr 20, 2015 at 12:21 AM, Stanislav Malyshev smalyshev@gmail.com
wrote:
Hi!
Just a simple use case showing how dangerous that is :
<?php
$p = function($p) { $this->$p = new Stdclass; };
$p->call($e = new Exception, 'trace');
throw $e;Yes, this is not good, and this is the consequence of allowing to rebind
closures. I'm not sure though how to fix it except for banning closures
from assuming scope of internal classes. If you assigned the scope of
Exception to it, it should have access to Exception - that's how the
scope works.For User classes, this could lead to information leaks or bad behaviors,
I'm not sure what you mean by "information leaks", but the behavior is
on whoever wrote that code. People can write bad code, we can't disallow
this. We can restrict things that would really break (like segfault) but
I'm not sure what else we can do here. If you have the scope of the
class, that implies access to the private members of the class. So we
either have to not let the closure have the scope, or invent some
additional term of scope' that is not like real scope. I don't think
it'd be good.
Yup Stas.
Perhaps the best thing to do is to forbid rebinding a Closure to an
internal class ?
I'm sure we could find segfaulting behaviors using such a trick on most of
our internals classes, aka mysqli, simplexmlelement, PDO or SPL classes.
Thoughts ?
Julien.P
Hey Julien,
Perhaps the best thing to do is to forbid rebinding a Closure to an internal class ?
I'm sure we could find segfaulting behaviors using such a trick on most of our internals classes, aka mysqli, simplexmlelement, PDO or SPL classes.
Thoughts ?
Sounds good to me! There’s already a precedent for having internal classes special with respect to Closure binding, as you can’t bind a method of an internal class to an object not of that class.
Thanks.
Andrea Faulds
http://ajf.me/
Hi!
Perhaps the best thing to do is to forbid rebinding a Closure to an
internal class ?
So I have checked into this further, and the good news is that we don't
have too many internal classes that declare private/protected
properties. Actually, the only one where you could mess stuff up was
Exception, and that seems to be recently fixed by adding typechecks for
(almost) unrelated unserialize issue. So for now, I do not see a lot of
ways to mess things up in core PHP. Of course, third-party extensions
may be different.
Still, I don't like the idea of rebinding to internal classes too much
(in fact, I'm not sure I like the whole rebinding thing as such in
general, but that's another story) so here's a pull that forbids this:
https://github.com/php/php-src/pull/1253
Please review and comment.
Stas Malyshev
smalyshev@gmail.com