Skip to content

Commit

Permalink
Fixed bug #70630 (Closure::call/bind() crash with ReflectionFunction-…
Browse files Browse the repository at this point in the history 
…>getClosure())

This additionally removes support for binding to an unknown (not in parent hierarchy) scope.
Removing support for cross-scope is necessary for certain compile-time assumptions (like class constants) to prevent unexpected results
  • Loading branch information
@bwoebi
bwoebi committed Oct 3, 2015
1 parent 4cb6342 commit 517b553
Show file tree
Hide file tree
Showing 6 changed files with 124 additions and 16 deletions.
4 changes: 4 additions & 0 deletions NEWS
View file
Expand Up @@ -2,6 +2,10 @@ PHP NEWS
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
15 Oct 2015, PHP 7.0.0 RC 5

- Core
. Fixed bug #70630 (Closure::call/bind() crash with
ReflectionFunction->getClosure()). (Bob)

- Mcrypt:
. Fixed bug #70625 (mcrypt_encrypt() won't return data when no IV was
specified under RC4). (Nikita)
Expand Down
24 changes: 24 additions & 0 deletions Zend/tests/bug70630.phpt
View file
@@ -0,0 +1,24 @@
--TEST--
Bug #70630 (Closure::call/bind() crash with ReflectionFunction->getClosure())
--FILE--
<?php

class a {}
function foo() {}

foreach (["substr", "foo"] as $fn) {
$x = (new ReflectionFunction($fn))->getClosure();
$x->call(new a);
Closure::bind($x, new a, "a");
}

?>
--EXPECTF--

Warning: Cannot bind function substr to an object in %s on line %d

Warning: Cannot bind function substr to an object in %s on line %d

Warning: Cannot bind function foo to an object in %s on line %d

Warning: Cannot bind function foo to an object in %s on line %d
64 changes: 64 additions & 0 deletions Zend/tests/closure_061.phpt
View file
@@ -0,0 +1,64 @@
--TEST--
Closure::call() or Closure::bind() to independent class must fail
--FILE--
<?php

class foo {
public $var;

function initClass() {
$this->var = __CLASS__;
}
}

class bar {
public $var;

function initClass() {
$this->var = __CLASS__;
}

function getVar() {
assert($this->var !== null); // ensure initClass was called
return $this->var;
}
}

class baz extends bar {
public $var;

function initClass() {
$this->var = __CLASS__;
}
}

function callMethodOn($class, $method, $object) {
$closure = (new ReflectionMethod($class, $method))->getClosure((new ReflectionClass($class))->newInstanceWithoutConstructor());
$closure = $closure->bindTo($object, $class);
return $closure();
}

$baz = new baz;

callMethodOn("baz", "initClass", $baz);
var_dump($baz->getVar());

callMethodOn("bar", "initClass", $baz);
var_dump($baz->getVar());

callMethodOn("foo", "initClass", $baz);
var_dump($baz->getVar());

?>
--EXPECTF--
string(3) "baz"
string(3) "bar"

Warning: Cannot bind function foo::initClass to object of class baz in %s on line %d

Fatal error: Uncaught Error: Using $this when not in object context in %s:%d
Stack trace:
#0 %s(%d): initClass()
#1 %s(%d): callMethodOn('foo', 'initClass', Object(baz))
#2 {main}
thrown in %s on line %d
44 changes: 30 additions & 14 deletions Zend/zend_closures.c
View file
Expand Up @@ -94,10 +94,12 @@ ZEND_METHOD(Closure, call)
return;
}

if (closure->func.type == ZEND_INTERNAL_FUNCTION) {
/* verify that we aren't binding internal function to a wrong object */
if ((closure->func.common.fn_flags & ZEND_ACC_STATIC) == 0 &&
!instanceof_function(Z_OBJCE_P(newthis), closure->func.common.scope)) {
if (closure->func.type != ZEND_USER_FUNCTION || (closure->func.common.fn_flags & ZEND_ACC_REAL_CLOSURE) == 0) {
/* verify that we aren't binding methods to a wrong object */
if (closure->func.common.scope == NULL) {
zend_error(E_WARNING, "Cannot bind function %s to an object", ZSTR_VAL(closure->func.common.function_name));
return;
} else if (!instanceof_function(Z_OBJCE_P(newthis), closure->func.common.scope)) {
zend_error(E_WARNING, "Cannot bind function %s::%s to object of class %s", ZSTR_VAL(closure->func.common.scope->name), ZSTR_VAL(closure->func.common.function_name), ZSTR_VAL(Z_OBJCE_P(newthis)->name));
return;
}
Expand Down Expand Up @@ -165,7 +167,7 @@ ZEND_METHOD(Closure, bind)
RETURN_NULL();
}

closure = (zend_closure *)Z_OBJ_P(zclosure);
closure = (zend_closure *) Z_OBJ_P(zclosure);

if ((newthis != NULL) && (closure->func.common.fn_flags & ZEND_ACC_STATIC)) {
zend_error(E_WARNING, "Cannot bind an instance to a static closure");
Expand All @@ -187,7 +189,7 @@ ZEND_METHOD(Closure, bind)
}
zend_string_release(class_name);
}
if(ce && ce != closure->func.common.scope && ce->type == ZEND_INTERNAL_CLASS) {
if (ce && ce != closure->func.common.scope && ce->type == ZEND_INTERNAL_CLASS) {
/* rebinding to internal class is not allowed */
zend_error(E_WARNING, "Cannot bind closure to scope of internal class %s", ZSTR_VAL(ce->name));
return;
Expand All @@ -202,6 +204,22 @@ ZEND_METHOD(Closure, bind)
called_scope = ce;
}

/* verify that we aren't binding methods to a wrong object */
if (closure->func.type != ZEND_USER_FUNCTION || (closure->func.common.fn_flags & ZEND_ACC_REAL_CLOSURE) == 0) {
if (!closure->func.common.scope) {
if (ce) {
zend_error(E_WARNING, "Cannot bind function %s to an object", ZSTR_VAL(closure->func.common.function_name));
return;
}
} else if (!ce) {
zend_error(E_WARNING, "Cannot bind function %s::%s to no class", ZSTR_VAL(closure->func.common.scope->name), ZSTR_VAL(closure->func.common.function_name));
return;
} else if (!instanceof_function(ce, closure->func.common.scope)) {
zend_error(E_WARNING, "Cannot bind function %s::%s to class %s", ZSTR_VAL(closure->func.common.scope->name), ZSTR_VAL(closure->func.common.function_name), ZSTR_VAL(ce->name));
return;
}
}

zend_create_closure(return_value, &closure->func, ce, called_scope, newthis);
new_closure = (zend_closure *) Z_OBJ_P(return_value);

Expand Down Expand Up @@ -242,11 +260,9 @@ ZEND_API zend_function *zend_get_closure_invoke_method(zend_object *object) /* {
* and we won't check arguments on internal function. We also set
* ZEND_ACC_USER_ARG_INFO flag to prevent invalid usage by Reflection */
invoke->type = ZEND_INTERNAL_FUNCTION;
invoke->internal_function.fn_flags =
ZEND_ACC_PUBLIC | ZEND_ACC_CALL_VIA_HANDLER | (closure->func.common.fn_flags & keep_flags);
invoke->internal_function.fn_flags = ZEND_ACC_PUBLIC | ZEND_ACC_CALL_VIA_HANDLER | (closure->func.common.fn_flags & keep_flags);
if (closure->func.type != ZEND_INTERNAL_FUNCTION || (closure->func.common.fn_flags & ZEND_ACC_USER_ARG_INFO)) {
invoke->internal_function.fn_flags |=
ZEND_ACC_USER_ARG_INFO;
invoke->internal_function.fn_flags |= ZEND_ACC_USER_ARG_INFO;
}
invoke->internal_function.handler = ZEND_MN(Closure___invoke);
invoke->internal_function.module = 0;
Expand Down Expand Up @@ -523,10 +539,10 @@ ZEND_API void zend_create_closure(zval *res, zend_function *func, zend_class_ent

object_init_ex(res, zend_ce_closure);

closure = (zend_closure *)Z_OBJ_P(res);
closure = (zend_closure *) Z_OBJ_P(res);

memcpy(&closure->func, func, func->type == ZEND_USER_FUNCTION ? sizeof(zend_op_array) : sizeof(zend_internal_function));
closure->func.common.prototype = (zend_function*)closure;
closure->func.common.prototype = (zend_function *) closure;
closure->func.common.fn_flags |= ZEND_ACC_CLOSURE;

if ((scope == NULL) && this_ptr && (Z_TYPE_P(this_ptr) != IS_UNDEF)) {
Expand All @@ -550,7 +566,8 @@ ZEND_API void zend_create_closure(zval *res, zend_function *func, zend_class_ent
if (closure->func.op_array.refcount) {
(*closure->func.op_array.refcount)++;
}
} else {
}
if (closure->func.type != ZEND_USER_FUNCTION || (func->common.fn_flags & ZEND_ACC_REAL_CLOSURE) == 0) {
/* verify that we aren't binding internal function to a wrong scope */
if(func->common.scope != NULL) {
if(scope && !instanceof_function(scope, func->common.scope)) {
Expand All @@ -561,7 +578,6 @@ ZEND_API void zend_create_closure(zval *res, zend_function *func, zend_class_ent
!instanceof_function(Z_OBJCE_P(this_ptr), closure->func.common.scope)) {
zend_error(E_WARNING, "Cannot bind function %s::%s to object of class %s", ZSTR_VAL(func->common.scope->name), ZSTR_VAL(func->common.function_name), ZSTR_VAL(Z_OBJCE_P(this_ptr)->name));
scope = NULL;
this_ptr = NULL;
}
} else {
/* if it's a free function, we won't set scope & this since they're meaningless */
Expand Down
2 changes: 1 addition & 1 deletion Zend/zend_compile.c
View file
Expand Up @@ -4854,7 +4854,7 @@ void zend_compile_func_decl(znode *result, zend_ast *ast) /* {{{ */
op_array->doc_comment = zend_string_copy(decl->doc_comment);
}
if (decl->kind == ZEND_AST_CLOSURE) {
op_array->fn_flags |= ZEND_ACC_CLOSURE;
op_array->fn_flags |= ZEND_ACC_CLOSURE | ZEND_ACC_REAL_CLOSURE;
}

if (is_method) {
Expand Down
2 changes: 1 addition & 1 deletion Zend/zend_compile.h
View file
Expand Up @@ -238,7 +238,7 @@ typedef struct _zend_try_catch_element {
/* user class has methods with static variables */
#define ZEND_HAS_STATIC_IN_METHODS 0x800000


#define ZEND_ACC_REAL_CLOSURE 0x40
#define ZEND_ACC_CLOSURE 0x100000
#define ZEND_ACC_GENERATOR 0x800000

Expand Down

25 comments on commit 517b553

@Ocramius
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: this is exactly why nobody uses ReflectionFunction#getClosure(), since it introduces silly rules and does dangerous assumptions.

Following code is much better than ReflectionMethod#getClosure() (for instance methods), for example:

namespace ReflectionWorkarounds;

class NotAnInstanceMethodException extends InvalidArgumentException {}

function getInstanceMethodClosure(\ReflectionMethod $method, $object) {
    if ($method->isStatic()) {
        throw new NotAnInstanceMethodException();
    }

    $methodName = $method->getName();

    return Closure::bind(
        function (...$params) use ($methodName) {
            return $this->{$methodName}(...$params);
        },
        $object,
        $method->getDeclaringClass()->getName()
    );
}

@laruence
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing something means bc break. it definitely needs a discuss. I am going to revert this, only fix the segfault.. please drop a mail to internal about the BC break.

@bwoebi
Copy link
Member Author

@bwoebi bwoebi commented on 517b553 Oct 4, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@laruence please don't revert that… It actually is only about rebinding Closures of actual methods and functions (like these you get via Reflection, not actual Closures), where it is already buggy due to some compile-time assumptions.
It strictly seen is a minor BC break, of something already having buggy behavior.

@laruence
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bwoebi hmm, I just feel the REAL_CLOSURE your introduced smells bad.. maybe you should fix that in reflection side? ACC_CLOSURE and ACC_REAL_CLOUSRE... :<

@bwoebi
Copy link
Member Author

@bwoebi bwoebi commented on 517b553 Oct 4, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@laruence There is no reference to the parent op_array or similar, hence it's not trivial to see what the origins are. The ZEND_ACC_CLOSURE is necessary in every case for proper refcounting of the Closure (and thus of the op_array). Hence the only solution I found is adding a new constant. Feel free ameliorate this patch; it's just the best and simplest I've found.

@laruence
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bwoebi okey, thanks for explaining , I need some time to understand it.

@Danack
Copy link
Contributor

@Danack Danack commented on 517b553 Oct 4, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if it's only breaking userland code that is stupid, slipping in an API change in a minor point release is pretty bogus.

@nikic
Copy link
Member

@nikic nikic commented on 517b553 Oct 4, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Danack This is not breaking userland code that is stupid, this is explicitly forbidding something that does not work in the first place.

Though I suspect this patch is not conservative enough and we have to forbid all rebinding for "not real" closures.

@Danack
Copy link
Contributor

@Danack Danack commented on 517b553 Oct 4, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nikic The code below works in the RC candidates for 7.0. The code does not work after Bob's commit.

Warning: Cannot bind function Foo::someMethod to object of class Bar

I realise the code is really stupid, and would only be used by insane people, but it still seems to be a BC break that was slipped in along with a valid bug fix.

class Foo {
    public function someMethod() {
        return "Foo::someMethod ".get_class($this);
    }
}

class Bar {
    public function someMethod() {
        return "Bar::someMethod ".get_class($this);
    }
}

$foo = new Foo();
$bar = new Bar();

$reflClass = new ReflectionClass('Foo');
$reflMethod = $reflClass->getMethod('someMethod');
$fooClosure = $reflMethod->getClosure($foo);
$barClosure = $fooClosure->bindTo($bar);

echo $fooClosure()."\n";
echo $barClosure()."\n";

@Ocramius
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and would only be used by insane people

I take offense to that :P

But yes, this patch is broken and should be reverted anyway, with a reminder that ReflectionFunction->getClosure() is a terrible API anyway.

@bwoebi
Copy link
Member Author

@bwoebi bwoebi commented on 517b553 Oct 4, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, that's what the commit attempts to do… though, I'm wondering whether we really need to remove that completely… maybe we could only just change called scope and leave scope intact (for function and method Closures).

That way the above (@Danack example) would still work, but assumptions about self-binding are not violated.

@nikic
Copy link
Member

@nikic nikic commented on 517b553 Oct 4, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Danack The fact that it works is only incidental of that particular code. If you use something like self::class or self::CONST or a bunch of other things I don't remember right now, you're going to get incorrect behavior. The engine assumes that things like self actually do mean self inside a class method.

@frenchcomp
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,
Like spotted by Danack, this patch produces a BC Break... \ReflectionMethod::getClosure() is a usefull method in some cases. (like allow to implement easily some patterns for final devs). My lib (github.com/UniAlteri/states) does not work with 7.1-dev with this patch :/

Thanks.
Richard

@bwoebi
Copy link
Member Author

@bwoebi bwoebi commented on 517b553 Oct 4, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@frenchcomp yeah, definitely, but this is just about rebinding these (Closure::call() and Closure::bindTo() being restricted in what they accept as scopes/$this)

@frenchcomp
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But currently there are not indications in the documentation about Closure::bindTo() et Closure::call() to restrict $newthis to the same initial scope. Your path introduces a BC break and not provides an alternative !

@Ocramius
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whisper just revert it whisper

@frenchcomp
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, please do it ;)

If there are an issue when a rebinded closure requires a non defined constant in the new scope : it is the developer's responsibility, not a php, PHP should only throw an error like other inexistant contant :) This patch should only fix the segfault.

@nikic
Copy link
Member

@nikic nikic commented on 517b553 Oct 4, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Ocramius Sure, we could revert this on the grounds of unexpectedly having an actual BC impact, but that is no more than a temporary measure to allow further discussion. The issue that we don't properly support rebinds for real methods still exists. We can either forbid it (or at least the unsafe parts -- we can allow rebinds with objects of the same class) or we need to correctly support it. And, at least in my opinion, allowing doing something odd like this is not worth removing valuable preconditions (both for the compiler and the developer) like "self refers to the class the method is defined on" and "$this is an instance of the class the method is defined on".

@Ocramius
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but that is no more than a temporary measure to allow further discussion.

No, that's what happens when a bad patch lands (in any project, AFAIK).

@Ocramius
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: not having a patch for a bug is better than having a broken patch for a bug (except for security issues, ofc)

@bwoebi
Copy link
Member Author

@bwoebi bwoebi commented on 517b553 Oct 4, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The patch isn't broken, it's just too conservative. I'll push a second commit with more tests and fixed behavior a bit later today.

@Ocramius
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sigh

too conservative

This means "bc compliant", ya know? ;-)

@nikic
Copy link
Member

@nikic nikic commented on 517b553 Oct 4, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to add that while right now the only misbehaviors I'm aware of are just that, this will likely not stay that way in the future. E.g. if we start optimizing calls of the form self::method() in the future by specializing them to the method signature (what we do for calls to fully qualified known functions), it likely wouldn't simply misbehave, but lead to segmentation faults.

(And I do highly doubt we would want to stay away from doing things like that to support this "feature".)

@Tyrael
Copy link
Member

@Tyrael Tyrael commented on 517b553 Oct 4, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer changes with BC breaks to be discussed first as they tend to be a sensitive topic, even more when we are so close to the final 7.0 release.

@frenchcomp
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For information, the new commit is here : 881c502 but there are always a BC break ...

Please sign in to comment.