From 122d9bf2fd678f22626700c9e5c8c4ff65226939 Mon Sep 17 00:00:00 2001 From: Levi Morrison Date: Thu, 18 Jul 2024 11:10:57 -0600 Subject: [PATCH] fix: unsafe function casts This is a weird one. The actual error message talks about array bounds: In file included from /usr/local/src/php/Zend/zend.h:32, from /usr/local/src/php/Zend/zend_API.c:22: In function 'zend_check_magic_method_implementation', inlined from 'zend_register_functions' at /usr/local/src/php/Zend/zend_API.c:3182:4: /usr/local/src/php/Zend/zend_API.c:2759:34: error: array subscript 'zend_function {aka const union _zend_function}[0]' is partly outside array bounds of 'unsigned char[160]' [-Werror=array-bounds] 2759 | if (ZSTR_VAL(fptr->common.function_name)[0] != '_' /usr/local/src/php/Zend/zend_string.h:68:26: note: in definition of macro 'ZSTR_VAL' 68 | #define ZSTR_VAL(zstr) (zstr)->val | ^~~~ /usr/local/src/php/Zend/zend_API.c: In function 'zend_register_functions': /usr/local/src/php/Zend/zend_API.c:3052:32: note: object of size 160 allocated by 'malloc' 3052 | reg_function = malloc(sizeof(zend_internal_function)); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ The core issue is that you cannot make a `zend_internal_function *`, and then cast it to a wider `zend_function *`. There are two ways to fix this: 1. Malloc `zend_function` instead and initialize the zend_internal_function member. 2. Cast `zend_internal_function *` to just the narrower common struct, `zend_function_common *`, and work with that. The first option wastes memory. The second option seems better and conceptually helpful to reason about. Note that if we needed to cast from `zend_function_common *` back out to `zend_internal_function *`, that would be safe. --- Zend/zend_API.c | 76 ++++++++++++++++++++++----------------------- Zend/zend_API.h | 2 +- Zend/zend_compile.c | 2 +- Zend/zend_compile.h | 42 +++++++++++++++---------- 4 files changed, 65 insertions(+), 57 deletions(-) diff --git a/Zend/zend_API.c b/Zend/zend_API.c index 6a07e99202d0d..bc73daf00470f 100644 --- a/Zend/zend_API.c +++ b/Zend/zend_API.c @@ -2654,57 +2654,57 @@ ZEND_API zend_module_entry* zend_register_internal_module(zend_module_entry *mod /* }}} */ static void zend_check_magic_method_args( - uint32_t num_args, const zend_class_entry *ce, const zend_function *fptr, int error_type) + uint32_t num_args, const zend_class_entry *ce, const zend_function_common *fptr, int error_type) { - if (fptr->common.num_args != num_args) { + if (fptr->num_args != num_args) { if (num_args == 0) { zend_error(error_type, "Method %s::%s() cannot take arguments", - ZSTR_VAL(ce->name), ZSTR_VAL(fptr->common.function_name)); + ZSTR_VAL(ce->name), ZSTR_VAL(fptr->function_name)); } else if (num_args == 1) { zend_error(error_type, "Method %s::%s() must take exactly 1 argument", - ZSTR_VAL(ce->name), ZSTR_VAL(fptr->common.function_name)); + ZSTR_VAL(ce->name), ZSTR_VAL(fptr->function_name)); } else { zend_error(error_type, "Method %s::%s() must take exactly %" PRIu32 " arguments", - ZSTR_VAL(ce->name), ZSTR_VAL(fptr->common.function_name), num_args); + ZSTR_VAL(ce->name), ZSTR_VAL(fptr->function_name), num_args); } return; } for (uint32_t i = 0; i < num_args; i++) { if (QUICK_ARG_SHOULD_BE_SENT_BY_REF(fptr, i + 1)) { zend_error(error_type, "Method %s::%s() cannot take arguments by reference", - ZSTR_VAL(ce->name), ZSTR_VAL(fptr->common.function_name)); + ZSTR_VAL(ce->name), ZSTR_VAL(fptr->function_name)); return; } } } -static void zend_check_magic_method_arg_type(uint32_t arg_num, const zend_class_entry *ce, const zend_function *fptr, int error_type, int arg_type) +static void zend_check_magic_method_arg_type(uint32_t arg_num, const zend_class_entry *ce, const zend_function_common *fptr, int error_type, int arg_type) { - if ( - ZEND_TYPE_IS_SET(fptr->common.arg_info[arg_num].type) - && !(ZEND_TYPE_FULL_MASK(fptr->common.arg_info[arg_num].type) & arg_type) - ) { - zend_error(error_type, "%s::%s(): Parameter #%d ($%s) must be of type %s when declared", - ZSTR_VAL(ce->name), ZSTR_VAL(fptr->common.function_name), - arg_num + 1, ZSTR_VAL(fptr->common.arg_info[arg_num].name), - ZSTR_VAL(zend_type_to_string((zend_type) ZEND_TYPE_INIT_MASK(arg_type)))); - } + if ( + ZEND_TYPE_IS_SET(fptr->arg_info[arg_num].type) + && !(ZEND_TYPE_FULL_MASK(fptr->arg_info[arg_num].type) & arg_type) + ) { + zend_error(error_type, "%s::%s(): Parameter #%d ($%s) must be of type %s when declared", + ZSTR_VAL(ce->name), ZSTR_VAL(fptr->function_name), + arg_num + 1, ZSTR_VAL(fptr->arg_info[arg_num].name), + ZSTR_VAL(zend_type_to_string((zend_type) ZEND_TYPE_INIT_MASK(arg_type)))); + } } -static void zend_check_magic_method_return_type(const zend_class_entry *ce, const zend_function *fptr, int error_type, int return_type) +static void zend_check_magic_method_return_type(const zend_class_entry *ce, const zend_function_common *fptr, int error_type, int return_type) { - if (!(fptr->common.fn_flags & ZEND_ACC_HAS_RETURN_TYPE)) { + if (!(fptr->fn_flags & ZEND_ACC_HAS_RETURN_TYPE)) { /* For backwards compatibility reasons, do not enforce the return type if it is not set. */ return; } - if (ZEND_TYPE_PURE_MASK(fptr->common.arg_info[-1].type) & MAY_BE_NEVER) { + if (ZEND_TYPE_PURE_MASK(fptr->arg_info[-1].type) & MAY_BE_NEVER) { /* It is always legal to specify the never type. */ return; } - bool is_complex_type = ZEND_TYPE_IS_COMPLEX(fptr->common.arg_info[-1].type); - uint32_t extra_types = ZEND_TYPE_PURE_MASK(fptr->common.arg_info[-1].type) & ~return_type; + bool is_complex_type = ZEND_TYPE_IS_COMPLEX(fptr->arg_info[-1].type); + uint32_t extra_types = ZEND_TYPE_PURE_MASK(fptr->arg_info[-1].type) & ~return_type; if (extra_types & MAY_BE_STATIC) { extra_types &= ~MAY_BE_STATIC; is_complex_type = true; @@ -2712,52 +2712,52 @@ static void zend_check_magic_method_return_type(const zend_class_entry *ce, cons if (extra_types || (is_complex_type && return_type != MAY_BE_OBJECT)) { zend_error(error_type, "%s::%s(): Return type must be %s when declared", - ZSTR_VAL(ce->name), ZSTR_VAL(fptr->common.function_name), + ZSTR_VAL(ce->name), ZSTR_VAL(fptr->function_name), ZSTR_VAL(zend_type_to_string((zend_type) ZEND_TYPE_INIT_MASK(return_type)))); } } static void zend_check_magic_method_non_static( - const zend_class_entry *ce, const zend_function *fptr, int error_type) + const zend_class_entry *ce, const zend_function_common *fptr, int error_type) { - if (fptr->common.fn_flags & ZEND_ACC_STATIC) { + if (fptr->fn_flags & ZEND_ACC_STATIC) { zend_error(error_type, "Method %s::%s() cannot be static", - ZSTR_VAL(ce->name), ZSTR_VAL(fptr->common.function_name)); + ZSTR_VAL(ce->name), ZSTR_VAL(fptr->function_name)); } } static void zend_check_magic_method_static( - const zend_class_entry *ce, const zend_function *fptr, int error_type) + const zend_class_entry *ce, const zend_function_common *fptr, int error_type) { - if (!(fptr->common.fn_flags & ZEND_ACC_STATIC)) { + if (!(fptr->fn_flags & ZEND_ACC_STATIC)) { zend_error(error_type, "Method %s::%s() must be static", - ZSTR_VAL(ce->name), ZSTR_VAL(fptr->common.function_name)); + ZSTR_VAL(ce->name), ZSTR_VAL(fptr->function_name)); } } static void zend_check_magic_method_public( - const zend_class_entry *ce, const zend_function *fptr, int error_type) + const zend_class_entry *ce, const zend_function_common *fptr, int error_type) { // TODO: Remove this warning after adding proper visibility handling. - if (!(fptr->common.fn_flags & ZEND_ACC_PUBLIC)) { + if (!(fptr->fn_flags & ZEND_ACC_PUBLIC)) { zend_error(E_WARNING, "The magic method %s::%s() must have public visibility", - ZSTR_VAL(ce->name), ZSTR_VAL(fptr->common.function_name)); + ZSTR_VAL(ce->name), ZSTR_VAL(fptr->function_name)); } } static void zend_check_magic_method_no_return_type( - const zend_class_entry *ce, const zend_function *fptr, int error_type) + const zend_class_entry *ce, const zend_function_common *fptr, int error_type) { - if (fptr->common.fn_flags & ZEND_ACC_HAS_RETURN_TYPE) { + if (fptr->fn_flags & ZEND_ACC_HAS_RETURN_TYPE) { zend_error_noreturn(error_type, "Method %s::%s() cannot declare a return type", - ZSTR_VAL(ce->name), ZSTR_VAL(fptr->common.function_name)); + ZSTR_VAL(ce->name), ZSTR_VAL(fptr->function_name)); } } -ZEND_API void zend_check_magic_method_implementation(const zend_class_entry *ce, const zend_function *fptr, zend_string *lcname, int error_type) /* {{{ */ +ZEND_API void zend_check_magic_method_implementation(const zend_class_entry *ce, const zend_function_common *fptr, zend_string *lcname, int error_type) /* {{{ */ { - if (ZSTR_VAL(fptr->common.function_name)[0] != '_' - || ZSTR_VAL(fptr->common.function_name)[1] != '_') { + zend_string *function_name = fptr->function_name; + if (ZSTR_VAL(function_name)[0] != '_' || ZSTR_VAL(function_name)[1] != '_') { return; } @@ -3180,7 +3180,7 @@ ZEND_API zend_result zend_register_functions(zend_class_entry *scope, const zend if (scope) { zend_check_magic_method_implementation( - scope, (zend_function *)reg_function, lowercase_name, E_CORE_ERROR); + scope, (zend_function_common *)reg_function, lowercase_name, E_CORE_ERROR); zend_add_magic_method(scope, (zend_function *)reg_function, lowercase_name); } ptr++; diff --git a/Zend/zend_API.h b/Zend/zend_API.h index ab67dd5717e69..b4e54a4763a10 100644 --- a/Zend/zend_API.h +++ b/Zend/zend_API.h @@ -385,7 +385,7 @@ ZEND_API void zend_startup_modules(void); ZEND_API void zend_collect_module_handlers(void); ZEND_API void zend_destroy_modules(void); ZEND_API void zend_check_magic_method_implementation( - const zend_class_entry *ce, const zend_function *fptr, zend_string *lcname, int error_type); + const zend_class_entry *ce, const zend_function_common *fptr, zend_string *lcname, int error_type); ZEND_API void zend_add_magic_method(zend_class_entry *ce, zend_function *fptr, zend_string *lcname); ZEND_API zend_class_entry *zend_register_internal_class(zend_class_entry *class_entry); diff --git a/Zend/zend_compile.c b/Zend/zend_compile.c index 9253cb9708ce5..dd3224ad57269 100644 --- a/Zend/zend_compile.c +++ b/Zend/zend_compile.c @@ -8323,7 +8323,7 @@ static zend_op_array *zend_compile_func_decl_ex( if (is_method) { CG(zend_lineno) = decl->start_lineno; zend_check_magic_method_implementation( - CG(active_class_entry), (zend_function *) op_array, lcname, E_COMPILE_ERROR); + CG(active_class_entry), (zend_function_common *) op_array, lcname, E_COMPILE_ERROR); } else if (toplevel) { /* Only register the function after a successful compile */ if (UNEXPECTED(zend_hash_add_ptr(CG(function_table), lcname, op_array) == NULL)) { diff --git a/Zend/zend_compile.h b/Zend/zend_compile.h index c7e31877b5cd2..ed49c5bf6b6d9 100644 --- a/Zend/zend_compile.h +++ b/Zend/zend_compile.h @@ -559,27 +559,35 @@ typedef struct _zend_internal_function { #define ZEND_FN_SCOPE_NAME(function) ((function) && (function)->common.scope ? ZSTR_VAL((function)->common.scope->name) : "") +struct _zend_function_common { + // this unnamed union is so that QUICK_ARG_* macros work on this struct + union { + uint32_t quick_arg_flags; + struct { + uint8_t type; /* never used */ + uint8_t arg_flags[3]; /* bitset of arg_info.pass_by_reference */ + }; + }; + uint32_t fn_flags; + zend_string *function_name; + zend_class_entry *scope; + zend_function *prototype; + uint32_t num_args; + uint32_t required_num_args; + zend_arg_info *arg_info; /* index -1 represents the return value info, if any */ + HashTable *attributes; + ZEND_MAP_PTR_DEF(void **, run_time_cache); + zend_string *doc_comment; + uint32_t T; /* number of temporary variables */ + const zend_property_info *prop_info; /* The corresponding prop_info if this is a hook. */ +}; +typedef struct _zend_function_common zend_function_common; + union _zend_function { uint8_t type; /* MUST be the first element of this struct! */ uint32_t quick_arg_flags; - struct { - uint8_t type; /* never used */ - uint8_t arg_flags[3]; /* bitset of arg_info.pass_by_reference */ - uint32_t fn_flags; - zend_string *function_name; - zend_class_entry *scope; - zend_function *prototype; - uint32_t num_args; - uint32_t required_num_args; - zend_arg_info *arg_info; /* index -1 represents the return value info, if any */ - HashTable *attributes; - ZEND_MAP_PTR_DEF(void **, run_time_cache); - zend_string *doc_comment; - uint32_t T; /* number of temporary variables */ - const zend_property_info *prop_info; /* The corresponding prop_info if this is a hook. */ - } common; - + zend_function_common common; zend_op_array op_array; zend_internal_function internal_function; };