Skip to content

Commit

Permalink
Unify magic method visibility check
Browse files Browse the repository at this point in the history 
This was missing entirely for the internal function case.
  • Loading branch information
@nikic
nikic committed Jul 20, 2020
1 parent 9f0213f commit 149029b
Show file tree
Hide file tree
Showing 4 changed files with 25 additions and 38 deletions.
2 changes: 0 additions & 2 deletions Zend/tests/magic_methods_007.phpt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,4 @@ abstract class b {

?>
--EXPECTF--
Warning: The magic method b::__set() must have public visibility in %s on line %d

Fatal error: Method b::__set() must take exactly 2 arguments in %s on line %d
2 changes: 0 additions & 2 deletions Zend/tests/magic_methods_010.phpt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,4 @@ class a {

?>
--EXPECTF--
Warning: The magic method a::__toString() must have public visibility in %s on line %d

Fatal error: Method a::__toString() cannot take arguments in %s on line %d
29 changes: 23 additions & 6 deletions Zend/zend_API.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2049,6 +2049,16 @@ static void zend_check_magic_method_static(
}
}

static void zend_check_magic_method_public(
const char *name, const zend_class_entry *ce, const zend_function *fptr, int error_type)
{
// TODO: Remove this warning after adding proper visibility handling.
if (!(fptr->common.fn_flags & ZEND_ACC_PUBLIC)) {
zend_error(E_WARNING, "The magic method %s::%s() must have public visibility",
ZSTR_VAL(ce->name), name);
}
}

static void zend_check_magic_method_no_return_type(
const char *name, const zend_class_entry *ce, const zend_function *fptr, int error_type)
{
Expand Down Expand Up @@ -2079,38 +2089,50 @@ ZEND_API void zend_check_magic_method_implementation(const zend_class_entry *ce,
} else if (zend_string_equals_literal(lcname, ZEND_GET_FUNC_NAME)) {
zend_check_magic_method_args(1, "__get", ce, fptr, error_type);
zend_check_magic_method_non_static("__get", ce, fptr, error_type);
zend_check_magic_method_public("__get", ce, fptr, error_type);
} else if (zend_string_equals_literal(lcname, ZEND_SET_FUNC_NAME)) {
zend_check_magic_method_args(2, "__set", ce, fptr, error_type);
zend_check_magic_method_non_static("__set", ce, fptr, error_type);
zend_check_magic_method_public("__set", ce, fptr, error_type);
} else if (zend_string_equals_literal(lcname, ZEND_UNSET_FUNC_NAME)) {
zend_check_magic_method_args(1, "__unset", ce, fptr, error_type);
zend_check_magic_method_non_static("__unset", ce, fptr, error_type);
zend_check_magic_method_public("__unset", ce, fptr, error_type);
} else if (zend_string_equals_literal(lcname, ZEND_ISSET_FUNC_NAME)) {
zend_check_magic_method_args(1, "__isset", ce, fptr, error_type);
zend_check_magic_method_non_static("__isset", ce, fptr, error_type);
zend_check_magic_method_public("__isset", ce, fptr, error_type);
} else if (zend_string_equals_literal(lcname, ZEND_CALL_FUNC_NAME)) {
zend_check_magic_method_args(2, "__call", ce, fptr, error_type);
zend_check_magic_method_non_static("__call", ce, fptr, error_type);
zend_check_magic_method_public("__call", ce, fptr, error_type);
} else if (zend_string_equals_literal(lcname, ZEND_CALLSTATIC_FUNC_NAME)) {
zend_check_magic_method_args(2, "__callStatic", ce, fptr, error_type);
zend_check_magic_method_static("__callStatic", ce, fptr, error_type);
zend_check_magic_method_public("__callStatic", ce, fptr, error_type);
} else if (zend_string_equals_literal(lcname, ZEND_TOSTRING_FUNC_NAME)) {
zend_check_magic_method_args(0, "__toString", ce, fptr, error_type);
zend_check_magic_method_non_static("__toString", ce, fptr, error_type);
zend_check_magic_method_public("__toString", ce, fptr, error_type);
} else if (zend_string_equals_literal(lcname, ZEND_DEBUGINFO_FUNC_NAME)) {
zend_check_magic_method_args(0, "__debugInfo", ce, fptr, error_type);
zend_check_magic_method_non_static("__debugInfo", ce, fptr, error_type);
zend_check_magic_method_public("__debugInfo", ce, fptr, error_type);
} else if (zend_string_equals_literal(lcname, "__serialize")) {
zend_check_magic_method_args(0, "__serialize", ce, fptr, error_type);
zend_check_magic_method_non_static("__serialize", ce, fptr, error_type);
zend_check_magic_method_public("__serialize", ce, fptr, error_type);
} else if (zend_string_equals_literal(lcname, "__unserialize")) {
zend_check_magic_method_args(1, "__unserialize", ce, fptr, error_type);
zend_check_magic_method_non_static("__unserialize", ce, fptr, error_type);
zend_check_magic_method_public("__unserialize", ce, fptr, error_type);
} else if (zend_string_equals_literal(lcname, "__set_state")) {
zend_check_magic_method_args(1, "__set_state", ce, fptr, error_type);
zend_check_magic_method_static("__set_state", ce, fptr, error_type);
zend_check_magic_method_public("__set_state", ce, fptr, error_type);
} else if (zend_string_equals_literal(lcname, "__invoke")) {
zend_check_magic_method_non_static("__invoke", ce, fptr, error_type);
zend_check_magic_method_public("__invoke", ce, fptr, error_type);
}
}
/* }}} */
Expand Down Expand Up @@ -2294,11 +2316,9 @@ ZEND_API int zend_register_functions(zend_class_entry *scope, const zend_functio
reg_function = NULL;
} else if (zend_string_equals_literal(lowercase_name, ZEND_CONSTRUCTOR_FUNC_NAME)) {
ctor = reg_function;
ctor->common.fn_flags |= ZEND_ACC_CTOR;
} else if (zend_string_equals_literal(lowercase_name, ZEND_DESTRUCTOR_FUNC_NAME)) {
dtor = reg_function;
if (internal_function->num_args) {
zend_error(error_type, "Destructor %s::%s() cannot take arguments", ZSTR_VAL(scope->name), ptr->fname);

This comment has been minimized.

Sign in to view

Copy link
@carusogabriel

carusogabriel Jul 20, 2020

Contributor

@nikic was this intentionally?

This comment has been minimized.

Sign in to view

Copy link
@nikic

nikic Jul 20, 2020

Author Member

Yeah, this is already checked in zend_check_magic_method_implementation now.
}
} else if (zend_string_equals_literal(lowercase_name, ZEND_CLONE_FUNC_NAME)) {
clone = reg_function;
} else if (zend_string_equals_literal(lowercase_name, ZEND_CALL_FUNC_NAME)) {
Expand Down Expand Up @@ -2368,9 +2388,6 @@ ZEND_API int zend_register_functions(zend_class_entry *scope, const zend_functio
scope->__debugInfo = __debugInfo;
scope->__serialize = __serialize;
scope->__unserialize = __unserialize;
if (ctor) {
ctor->common.fn_flags |= ZEND_ACC_CTOR;
}
efree((char*)lc_class_name);
}
return SUCCESS;
Expand Down
30 changes: 2 additions & 28 deletions Zend/zend_compile.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6443,16 +6443,6 @@ static void zend_compile_implicit_closure_uses(closure_info *info)
ZEND_HASH_FOREACH_END();
}

static void zend_check_magic_method_attr(uint32_t attr, zend_class_entry *ce, const char* method) /* {{{ */
{
if (!(attr & ZEND_ACC_PUBLIC)) {
zend_error(E_WARNING,
"The magic method %s::%s() must have public visibility",
ZSTR_VAL(ce->name), method);
}
}
/* }}} */

static void add_stringable_interface(zend_class_entry *ce) {
for (uint32_t i = 0; i < ce->num_interfaces; i++) {
if (zend_string_equals_literal(ce->interface_names[i].lc_name, "stringable")) {
Expand Down Expand Up @@ -6523,49 +6513,36 @@ zend_string *zend_begin_method_decl(zend_op_array *op_array, zend_string *name,
/* pass */
} else if (zend_string_equals_literal(lcname, ZEND_CONSTRUCTOR_FUNC_NAME)) {
ce->constructor = (zend_function *) op_array;
ce->constructor->common.fn_flags |= ZEND_ACC_CTOR;
} else if (zend_string_equals_literal(lcname, ZEND_DESTRUCTOR_FUNC_NAME)) {
ce->destructor = (zend_function *) op_array;
} else if (zend_string_equals_literal(lcname, ZEND_CLONE_FUNC_NAME)) {
ce->clone = (zend_function *) op_array;
} else if (zend_string_equals_literal(lcname, ZEND_CALL_FUNC_NAME)) {
zend_check_magic_method_attr(fn_flags, ce, "__call");
ce->__call = (zend_function *) op_array;
} else if (zend_string_equals_literal(lcname, ZEND_CALLSTATIC_FUNC_NAME)) {
zend_check_magic_method_attr(fn_flags, ce, "__callStatic");
ce->__callstatic = (zend_function *) op_array;
} else if (zend_string_equals_literal(lcname, ZEND_GET_FUNC_NAME)) {
zend_check_magic_method_attr(fn_flags, ce, "__get");
ce->__get = (zend_function *) op_array;
ce->ce_flags |= ZEND_ACC_USE_GUARDS;
} else if (zend_string_equals_literal(lcname, ZEND_SET_FUNC_NAME)) {
zend_check_magic_method_attr(fn_flags, ce, "__set");
ce->__set = (zend_function *) op_array;
ce->ce_flags |= ZEND_ACC_USE_GUARDS;
} else if (zend_string_equals_literal(lcname, ZEND_UNSET_FUNC_NAME)) {
zend_check_magic_method_attr(fn_flags, ce, "__unset");
ce->__unset = (zend_function *) op_array;
ce->ce_flags |= ZEND_ACC_USE_GUARDS;
} else if (zend_string_equals_literal(lcname, ZEND_ISSET_FUNC_NAME)) {
zend_check_magic_method_attr(fn_flags, ce, "__isset");
ce->__isset = (zend_function *) op_array;
ce->ce_flags |= ZEND_ACC_USE_GUARDS;
} else if (zend_string_equals_literal(lcname, ZEND_TOSTRING_FUNC_NAME)) {
zend_check_magic_method_attr(fn_flags, ce, "__toString");
ce->__tostring = (zend_function *) op_array;
add_stringable_interface(ce);
} else if (zend_string_equals_literal(lcname, ZEND_INVOKE_FUNC_NAME)) {
zend_check_magic_method_attr(fn_flags, ce, "__invoke");
} else if (zend_string_equals_literal(lcname, ZEND_DEBUGINFO_FUNC_NAME)) {
zend_check_magic_method_attr(fn_flags, ce, "__debugInfo");
ce->__debugInfo = (zend_function *) op_array;
} else if (zend_string_equals_literal(lcname, "__serialize")) {
zend_check_magic_method_attr(fn_flags, ce, "__serialize");
ce->__serialize = (zend_function *) op_array;
} else if (zend_string_equals_literal(lcname, "__unserialize")) {
zend_check_magic_method_attr(fn_flags, ce, "__unserialize");
ce->__unserialize = (zend_function *) op_array;
} else if (zend_string_equals_literal(lcname, "__set_state")) {
zend_check_magic_method_attr(fn_flags, ce, "__set_state");
}

return lcname;
Expand Down Expand Up @@ -6740,6 +6717,7 @@ void zend_compile_func_decl(znode *result, zend_ast *ast, zend_bool toplevel) /*
zend_compile_stmt(stmt_ast);

if (is_method) {
CG(zend_lineno) = decl->start_lineno;
zend_check_magic_method_implementation(
CG(active_class_entry), (zend_function *) op_array, method_lcname, E_COMPILE_ERROR);
zend_string_release_ex(method_lcname, 0);
Expand Down Expand Up @@ -7159,10 +7137,6 @@ void zend_compile_class_decl(znode *result, zend_ast *ast, zend_bool toplevel) /
/* Reset lineno for final opcodes and errors */
CG(zend_lineno) = ast->lineno;

if (ce->constructor) {
ce->constructor->common.fn_flags |= ZEND_ACC_CTOR;
}

if ((ce->ce_flags & (ZEND_ACC_IMPLICIT_ABSTRACT_CLASS|ZEND_ACC_INTERFACE|ZEND_ACC_TRAIT|ZEND_ACC_EXPLICIT_ABSTRACT_CLASS)) == ZEND_ACC_IMPLICIT_ABSTRACT_CLASS) {
zend_verify_abstract_class(ce);
}
Expand Down

0 comments on commit 149029b

Please sign in to comment.