Skip to content

Commit

Permalink
Fix use after free when rebinding __call closure
Browse files Browse the repository at this point in the history
We would end up freeing the function name twice here, once for
the original closure, and once for the rebound one.

Rather than further special casing the zend_closure_call_magic
case, always addref the function_name for internal functions,
the same we do for userland functions. To compensate, we need to
release the original function name when creating from callable
or call frame.

Fixes oss-fuzz #37695.
  • Loading branch information
nikic committed Aug 27, 2021
1 parent fdc6082 commit 4fcf0db
Show file tree
Hide file tree
Showing 2 changed files with 30 additions and 1 deletion.
20 changes: 20 additions & 0 deletions Zend/tests/closure_call_bind.phpt
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
--TEST--
Calling bindTo() on __call() closure
--FILE--
<?php

class Foo {
function __call($name, $args) {
echo "__call($name)\n";
}
}

$foo = new Foo;
$name = "foo";
Closure::fromCallable([$foo, $name . "bar"])->bindTo(new Foo)();
$foo->{$name . "bar"}(...)->bindTo(new Foo)();

?>
--EXPECT--
__call(foobar)
__call(foobar)
11 changes: 10 additions & 1 deletion Zend/zend_closures.c
Original file line number Diff line number Diff line change
Expand Up @@ -341,6 +341,10 @@ static zend_result zend_create_closure_from_callable(zval *return_value, zval *c
zend_create_fake_closure(return_value, mptr, mptr->common.scope, fcc.called_scope, NULL);
}

if (&mptr->internal_function == &call) {
zend_string_release(mptr->common.function_name);
}

return SUCCESS;
}
/* }}} */
Expand Down Expand Up @@ -482,7 +486,7 @@ static void zend_closure_free_storage(zend_object *object) /* {{{ */
zend_destroy_static_vars(&closure->func.op_array);
}
destroy_op_array(&closure->func.op_array);
} else if (closure->orig_internal_handler == zend_closure_call_magic) {
} else if (closure->func.type == ZEND_INTERNAL_FUNCTION) {
zend_string_release(closure->func.common.function_name);
}

Expand Down Expand Up @@ -739,6 +743,7 @@ static void zend_create_closure_ex(zval *res, zend_function *func, zend_class_en
closure->orig_internal_handler = closure->func.internal_function.handler;
}
closure->func.internal_function.handler = zend_closure_internal_handler;
zend_string_addref(closure->func.op_array.function_name);
if (!func->common.scope) {
/* if it's a free function, we won't set scope & this since they're meaningless */
this_ptr = NULL;
Expand Down Expand Up @@ -811,6 +816,10 @@ void zend_closure_from_frame(zval *return_value, zend_execute_data *call) { /* {
} else {
zend_create_fake_closure(return_value, mptr, mptr->common.scope, Z_CE(call->This), NULL);
}

if (&mptr->internal_function == &trampoline) {
zend_string_release(mptr->common.function_name);
}
} /* }}} */

void zend_closure_bind_var(zval *closure_zv, zend_string *var_name, zval *var) /* {{{ */
Expand Down

0 comments on commit 4fcf0db

Please sign in to comment.