From 35d0405c4790f0ce668c9e1b8b05197e55d29a05 Mon Sep 17 00:00:00 2001 From: Bob Weinand Date: Mon, 5 Oct 2015 17:49:32 +0200 Subject: [PATCH] Allow random $this on non-internal Closures again As it turns out, there is actually no reason to prevent this, it even was a bigger BC break than expected... Also fixes a memory leak (the Closure leaks) when calling internal functions via Closure by moving it out of leave helper onto caller side for TOP_CODE: $z = new SplStack; $z->push(20); $x = (new ReflectionMethod("SplStack", "pop"))->getClosure($z); var_dump($x()); --- Zend/tests/bug70630.phpt | 11 ++++---- Zend/tests/closure_041.phpt | 11 ++++---- Zend/tests/closure_043.phpt | 17 ++++--------- Zend/tests/closure_061.phpt | 11 ++------ Zend/tests/closure_062.phpt | 19 ++++++++------ Zend/tests/closure_063.phpt | 27 +++++++++++++------- Zend/tests/closure_call.phpt | 5 +--- Zend/zend_closures.c | 49 ++++++++++++++++-------------------- Zend/zend_execute_API.c | 3 +++ Zend/zend_vm_def.h | 6 ++--- Zend/zend_vm_execute.h | 6 ++--- 11 files changed, 78 insertions(+), 87 deletions(-) diff --git a/Zend/tests/bug70630.phpt b/Zend/tests/bug70630.phpt index a46c460df0..8a42a1f5a4 100644 --- a/Zend/tests/bug70630.phpt +++ b/Zend/tests/bug70630.phpt @@ -4,7 +4,7 @@ Bug #70630 (Closure::call/bind() crash with ReflectionFunction->getClosure()) getClosure(); @@ -15,10 +15,9 @@ foreach (["substr", "foo"] as $fn) { ?> --EXPECTF-- -Warning: Cannot bind function substr to an object in %s on line %d +Warning: substr() expects at least 2 parameters, 0 given in %s on line %d -Warning: Cannot bind function substr to an object or class in %s on line %d +Warning: Cannot bind function substr to a class scope in %s on line %d +ok -Warning: Cannot bind function foo to an object in %s on line %d - -Warning: Cannot bind function foo to an object or class in %s on line %d +Warning: Cannot bind function foo to a class scope in %s on line %d diff --git a/Zend/tests/closure_041.phpt b/Zend/tests/closure_041.phpt index 947517b4ed..6c368f6e17 100644 --- a/Zend/tests/closure_041.phpt +++ b/Zend/tests/closure_041.phpt @@ -53,9 +53,9 @@ $d = $nonstaticScoped->bindTo(null); $d(); echo "\n"; $d->bindTo($d); echo "After binding, with same-class instance for the bound ones", "\n"; -$d = $staticUnscoped->bindTo(new A); $d(); echo "\n"; +$d = $staticUnscoped->bindTo(new A); /* $d(); */ echo "\n"; $d = $nonstaticUnscoped->bindTo(new A); $d(); echo " (should be scoped to dummy class)\n"; -$d = $staticScoped->bindTo(new A); $d(); echo "\n"; +$d = $staticScoped->bindTo(new A); /* $d(); */ echo "\n"; $d = $nonstaticScoped->bindTo(new A); $d(); echo "\n"; echo "After binding, with different instance for the bound ones", "\n"; @@ -64,6 +64,7 @@ $d = $nonstaticScoped->bindTo(new B); $d(); echo "\n"; echo "Done.\n"; +?> --EXPECTF-- Before binding scoped to A: bool(false) @@ -86,14 +87,12 @@ bound: no After binding, with same-class instance for the bound ones Warning: Cannot bind an instance to a static closure in %s on line %d -scoped to A: bool(false) -bound: no + scoped to A: bool(false) bound: A (should be scoped to dummy class) Warning: Cannot bind an instance to a static closure in %s on line %d -scoped to A: bool(true) -bound: no + scoped to A: bool(true) bound: A After binding, with different instance for the bound ones diff --git a/Zend/tests/closure_043.phpt b/Zend/tests/closure_043.phpt index 98c88fda39..7763b41d5e 100644 --- a/Zend/tests/closure_043.phpt +++ b/Zend/tests/closure_043.phpt @@ -26,19 +26,20 @@ $d = $staticUnscoped->bindTo(null, null); $d(); echo "\n"; $d = $staticScoped->bindTo(null, null); $d(); echo "\n"; echo "After binding, null scope, with instance", "\n"; -$d = $staticUnscoped->bindTo(new A, null); $d(); echo "\n"; -$d = $staticScoped->bindTo(new A, null); $d(); echo "\n"; +$d = $staticUnscoped->bindTo(new A, null); /* $d(); */ echo "\n"; +$d = $staticScoped->bindTo(new A, null); /* $d();n*/ echo "\n"; echo "After binding, with scope, no instance", "\n"; $d = $staticUnscoped->bindTo(null, 'A'); $d(); echo "\n"; $d = $staticScoped->bindTo(null, 'A'); $d(); echo "\n"; echo "After binding, with scope, with instance", "\n"; -$d = $staticUnscoped->bindTo(new A, 'A'); $d(); echo "\n"; -$d = $staticScoped->bindTo(new A, 'A'); $d(); echo "\n"; +$d = $staticUnscoped->bindTo(new A, 'A'); /* $d(); */ echo "\n"; +$d = $staticScoped->bindTo(new A, 'A'); /* $d(); */ echo "\n"; echo "Done.\n"; +?> --EXPECTF-- Before binding bool(false) @@ -57,13 +58,9 @@ bool(false) After binding, null scope, with instance Warning: Cannot bind an instance to a static closure in %s on line %d -bool(false) -bool(false) Warning: Cannot bind an instance to a static closure in %s on line %d -bool(false) -bool(false) After binding, with scope, no instance bool(true) @@ -75,12 +72,8 @@ bool(false) After binding, with scope, with instance Warning: Cannot bind an instance to a static closure in %s on line %d -bool(true) -bool(false) Warning: Cannot bind an instance to a static closure in %s on line %d -bool(true) -bool(false) Done. diff --git a/Zend/tests/closure_061.phpt b/Zend/tests/closure_061.phpt index 342b05ca5d..344b9438be 100644 --- a/Zend/tests/closure_061.phpt +++ b/Zend/tests/closure_061.phpt @@ -1,5 +1,5 @@ --TEST-- -Closure::call() or Closure::bind() to independent class must fail +Closure::call() or Closure::bind() to independent class --FILE-- 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: Function name must be a string in %s:%d -Stack trace: -#0 %s(%d): callMethodOn('foo', 'initClass', Object(baz)) -#1 {main} - thrown in %s on line %d +string(3) "foo" diff --git a/Zend/tests/closure_062.phpt b/Zend/tests/closure_062.phpt index 574b43a3c2..3c147e89f1 100644 --- a/Zend/tests/closure_062.phpt +++ b/Zend/tests/closure_062.phpt @@ -1,9 +1,9 @@ --TEST-- -Force failure with Closure binding to unknown scopes/$this with Closure::call() +Test Closure binding to unknown scopes with Closure::call() --FILE-- getClosure(); $x->call(new stdClass); @@ -26,14 +26,17 @@ class d { function foo() { print "Success\n"; yield; } } $x = (new ReflectionMethod("d", "foo"))->getClosure(new d); $x->call(new d)->current(); +// internal functions with unknown scope must fail +$x = (new ReflectionMethod("Closure", "bindTo"))->getClosure(function() {}); +$x->call(new a); + ?> --EXPECTF-- - -Warning: Cannot bind function foo to an object in %s on line %d - -Warning: Cannot bind function a::foo to object of class stdClass in %s on line %d +stdClass +stdClass b - -Warning: Cannot bind function c::foo to object of class stdClass in %s on line %d +stdClass a Success + +Warning: Cannot bind closure of internal method Closure::bindTo to unrelated object of class a in %s on line %d diff --git a/Zend/tests/closure_063.phpt b/Zend/tests/closure_063.phpt index e854b578e2..d826a5efd0 100644 --- a/Zend/tests/closure_063.phpt +++ b/Zend/tests/closure_063.phpt @@ -3,17 +3,18 @@ Force failure with Closure binding to unknown scopes/$this with Closure::bind() --FILE-- getClosure(); -$x->bindTo(new stdClass); +$x->bindTo(new stdClass)(); +$x->bindTo(new stdClass, "stdClass"); class a { function foo() { echo get_class($this), "\n"; } } $x = (new ReflectionMethod("a", "foo"))->getClosure(new a); -$x->bindTo(new stdClass); +$x->bindTo(new stdClass)(); class c extends stdClass { function foo() { echo get_class($this), "\n"; } } $x = (new ReflectionMethod("c", "foo"))->getClosure(new c); -$x->bindTo(new stdClass); +$x->bindTo(new stdClass)(); class b extends a {} $x = (new ReflectionMethod("a", "foo"))->getClosure(new a); @@ -24,6 +25,12 @@ $x->bindTo(new b, "c"); $x = (new ReflectionMethod("a", "foo"))->getClosure(new a); $x->bindTo(new a)(); +class z extends SplStack {} +$x = (new ReflectionMethod("SplStack", "pop"))->getClosure(new SplStack); +$z = new z; $z->push(20); +var_dump($x->bindTo($z)()); +$x->bindTo($z, "z"); + class d { function foo() { print "Success\n"; yield; } } class e extends d {} $x = (new ReflectionMethod("d", "foo"))->getClosure(new d); @@ -34,17 +41,19 @@ $x->bindTo(new e, "e"); ?> --EXPECTF-- +stdClass -Warning: Cannot bind function foo to an object or class in %s on line %d - -Warning: Cannot bind function a::foo to object of class stdClass in %s on line %d - -Warning: Cannot bind function c::foo to object of class stdClass in %s on line %d +Warning: Cannot bind closure to scope of internal class stdClass in %s on line %d +stdClass +stdClass b b Warning: Cannot bind function a::foo to scope class c in %s on line %d a +int(20) + +Warning: Cannot bind function SplDoublyLinkedList::pop to scope class z in %s on line %d Success Success Success diff --git a/Zend/tests/closure_call.phpt b/Zend/tests/closure_call.phpt index e8bed36aec..ecaa12eb40 100644 --- a/Zend/tests/closure_call.phpt +++ b/Zend/tests/closure_call.phpt @@ -36,7 +36,6 @@ $elePHPant->x = 7; // Try on a StdClass var_dump($bar->call($elePHPant)); - $beta = function ($z) { return $this->x * $z; }; @@ -60,8 +59,6 @@ $foo->call(new FooBar); int(0) int(0) int(3) - -Warning: Cannot bind closure to object of internal class stdClass in %s line %d -NULL +int(7) int(21) int(3) diff --git a/Zend/zend_closures.c b/Zend/zend_closures.c index c06aac609d..03f7153bb3 100644 --- a/Zend/zend_closures.c +++ b/Zend/zend_closures.c @@ -94,22 +94,11 @@ ZEND_METHOD(Closure, call) return; } - 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; - } - } - newobj = Z_OBJ_P(newthis); - if (newobj->ce != closure->func.common.scope && newobj->ce->type == ZEND_INTERNAL_CLASS) { + if (closure->func.type == ZEND_INTERNAL_FUNCTION && closure->func.common.scope != NULL && !instanceof_function(newobj->ce, closure->func.common.scope)) { /* rebinding to internal class is not allowed */ - zend_error(E_WARNING, "Cannot bind closure to object of internal class %s", ZSTR_VAL(newobj->ce->name)); + zend_error(E_WARNING, "Cannot bind closure of internal method %s::%s to unrelated object of class %s", ZSTR_VAL(closure->func.common.scope->name), ZSTR_VAL(closure->func.common.function_name), ZSTR_VAL(newobj->ce->name)); return; } @@ -138,7 +127,7 @@ ZEND_METHOD(Closure, call) } if (closure->func.type == ZEND_USER_FUNCTION && (closure->func.common.fn_flags & ZEND_ACC_REAL_CLOSURE)) { - /* use scope of passed object */ + /* use scope of passed object; we must not change scope of functions and methods, only true Closures */ fci_cache.function_handler->common.scope = Z_OBJCE_P(newthis); /* Runtime cache relies on bound scope to be immutable, hence we need a separate rt cache in case scope changed */ @@ -177,6 +166,7 @@ ZEND_METHOD(Closure, bind) if ((newthis != NULL) && (closure->func.common.fn_flags & ZEND_ACC_STATIC)) { zend_error(E_WARNING, "Cannot bind an instance to a static closure"); + RETURN_NULL(); } if (scope_arg != NULL) { /* scope argument was given */ @@ -198,7 +188,7 @@ ZEND_METHOD(Closure, bind) 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; + RETURN_NULL(); } } else { /* scope argument not given; do not change the scope by default */ ce = closure->func.common.scope; @@ -530,32 +520,37 @@ ZEND_API void zend_create_closure(zval *res, zend_function *func, zend_class_ent { zend_closure *closure; - if ((scope == NULL) && this_ptr && (Z_TYPE_P(this_ptr) != IS_UNDEF)) { - /* use dummy scope if we're binding an object without specifying a scope */ - /* maybe it would be better to create one for this purpose */ - scope = zend_ce_closure; - } - if (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 */ + /* verify that we aren't binding a internal function to a wrong scope */ if (func->common.scope != NULL) { - if (scope && scope != func->common.scope) { - zend_error(E_WARNING, "Cannot bind function %s::%s to scope class %s", ZSTR_VAL(func->common.scope->name), ZSTR_VAL(func->common.function_name), ZSTR_VAL(scope->name)); + if (scope != func->common.scope) { + if (scope) { + zend_error(E_WARNING, "Cannot bind function %s::%s to scope class %s", ZSTR_VAL(func->common.scope->name), ZSTR_VAL(func->common.function_name), ZSTR_VAL(scope->name)); + } else { + zend_error(E_WARNING, "Cannot unbind function %s::%s from its scope", ZSTR_VAL(func->common.scope->name), ZSTR_VAL(func->common.function_name)); + } ZVAL_NULL(res); return; } - if (scope && this_ptr && (func->common.fn_flags & ZEND_ACC_STATIC) == 0 && !instanceof_function(Z_OBJCE_P(this_ptr), 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)); + if (func->type == ZEND_INTERNAL_FUNCTION && this_ptr && (Z_TYPE_P(this_ptr) != IS_UNDEF) && !instanceof_function(Z_OBJCE_P(this_ptr), func->common.scope)) { + /* rebinding to internal class is not allowed */ + zend_error(E_WARNING, "Cannot bind closure of internal method %s::%s to unrelated object of class %s", ZSTR_VAL(func->common.scope->name), ZSTR_VAL(func->common.function_name), ZSTR_VAL(Z_OBJCE_P(this_ptr)->name)); ZVAL_NULL(res); return; } } else if (scope) { - zend_error(E_WARNING, "Cannot bind function %s to an object or class", ZSTR_VAL(func->common.function_name)); + zend_error(E_WARNING, "Cannot bind function %s to a class scope", ZSTR_VAL(func->common.function_name)); ZVAL_NULL(res); return; } } + if ((scope == NULL) && this_ptr && (Z_TYPE_P(this_ptr) != IS_UNDEF)) { + /* use dummy scope if we're binding an object without specifying a scope */ + /* maybe it would be better to create one for this purpose */ + scope = zend_ce_closure; + } + object_init_ex(res, zend_ce_closure); closure = (zend_closure *) Z_OBJ_P(res); diff --git a/Zend/zend_execute_API.c b/Zend/zend_execute_API.c index 965b08eb38..2030e0265a 100644 --- a/Zend/zend_execute_API.c +++ b/Zend/zend_execute_API.c @@ -851,6 +851,9 @@ int zend_call_function(zend_fcall_info *fci, zend_fcall_info_cache *fci_cache) / if (EXPECTED((func->op_array.fn_flags & ZEND_ACC_GENERATOR) == 0)) { zend_init_execute_data(call, &func->op_array, fci->retval); zend_execute_ex(call); + if (UNEXPECTED(ZEND_CALL_INFO(call) & ZEND_CALL_CLOSURE)) { + OBJ_RELEASE((zend_object*)func->op_array.prototype); + } } else { zend_generator_create_zval(call, &func->op_array, fci->retval); } diff --git a/Zend/zend_vm_def.h b/Zend/zend_vm_def.h index fd0e35458f..a4b082b7b5 100644 --- a/Zend/zend_vm_def.h +++ b/Zend/zend_vm_def.h @@ -2418,9 +2418,6 @@ ZEND_VM_HELPER(zend_leave_helper, ANY, ANY) } zend_vm_stack_free_extra_args_ex(call_info, execute_data); EG(current_execute_data) = EX(prev_execute_data); - if (UNEXPECTED(call_info & ZEND_CALL_CLOSURE)) { - OBJ_RELEASE((zend_object*)EX(func)->op_array.prototype); - } } else /* if (call_kind == ZEND_CALL_TOP_CODE) */ { zend_array *symbol_table = EX(symbol_table); @@ -3861,6 +3858,9 @@ ZEND_VM_C_LABEL(fcall_end_change_scope): } OBJ_RELEASE(object); } + if (UNEXPECTED((ZEND_CALL_INFO(call) & ZEND_CALL_CLOSURE) && (fbc->common.fn_flags & ZEND_ACC_GENERATOR) == 0)) { + OBJ_RELEASE((zend_object*)fbc->op_array.prototype); + } EG(scope) = EX(func)->op_array.scope; ZEND_VM_C_LABEL(fcall_end): diff --git a/Zend/zend_vm_execute.h b/Zend/zend_vm_execute.h index b8da79921d..e2a4fa94b0 100644 --- a/Zend/zend_vm_execute.h +++ b/Zend/zend_vm_execute.h @@ -534,9 +534,6 @@ static ZEND_OPCODE_HANDLER_RET ZEND_FASTCALL zend_leave_helper_SPEC(ZEND_OPCODE_ } zend_vm_stack_free_extra_args_ex(call_info, execute_data); EG(current_execute_data) = EX(prev_execute_data); - if (UNEXPECTED(call_info & ZEND_CALL_CLOSURE)) { - OBJ_RELEASE((zend_object*)EX(func)->op_array.prototype); - } } else /* if (call_kind == ZEND_CALL_TOP_CODE) */ { zend_array *symbol_table = EX(symbol_table); @@ -919,6 +916,9 @@ fcall_end_change_scope: } OBJ_RELEASE(object); } + if (UNEXPECTED((ZEND_CALL_INFO(call) & ZEND_CALL_CLOSURE) && (fbc->common.fn_flags & ZEND_ACC_GENERATOR) == 0)) { + OBJ_RELEASE((zend_object*)fbc->op_array.prototype); + } EG(scope) = EX(func)->op_array.scope; fcall_end: -- 2.40.0