From b9e895bca0a479cfebc63c782f556b016ea07f19 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Thu, 31 Mar 2022 16:27:58 +0200 Subject: [PATCH] Replace memcmp() with zend_string functions (#8216) * ext/oci8: use zend_string_equals() Eliminate duplicate code. * main/php_variables: use zend_string_equals_literal() Eliminate duplicate code. * Zend/zend_string: add zend_string_equals_cstr() Allows eliminating duplicate code. * Zend, ext/{opcache,standard}, main/output: use zend_string_equals_cstr() Eliminate duplicate code. * Zend/zend_string: add zend_string_starts_with() * ext/{opcache,phar,spl,standard}: use zend_string_starts_with() This adds missing length checks to several callers, e.g. in cache_script_in_shared_memory(). This is important when the zend_string is shorter than the string parameter, when memcmp() happens to check backwards; this can result in an out-of-bounds memory access. --- Zend/zend_attributes.c | 6 ++---- Zend/zend_compile.c | 6 ++---- Zend/zend_execute.c | 2 +- Zend/zend_execute_API.c | 3 +-- Zend/zend_hash.c | 9 +++------ Zend/zend_string.c | 6 ++---- Zend/zend_string.h | 20 +++++++++++++++++++- ext/oci8/oci8.c | 7 ++----- ext/opcache/ZendAccelerator.c | 8 +++----- ext/phar/stream.c | 6 ++---- ext/spl/spl_directory.c | 2 +- ext/standard/proc_open.c | 2 +- ext/standard/string.c | 10 +++------- main/output.c | 2 +- main/php_variables.c | 3 +-- 15 files changed, 44 insertions(+), 48 deletions(-) diff --git a/Zend/zend_attributes.c b/Zend/zend_attributes.c index 8a373752472..0d27d1bcaa2 100644 --- a/Zend/zend_attributes.c +++ b/Zend/zend_attributes.c @@ -112,10 +112,8 @@ static zend_attribute *get_attribute_str(HashTable *attributes, const char *str, zend_attribute *attr; ZEND_HASH_PACKED_FOREACH_PTR(attributes, attr) { - if (attr->offset == offset && ZSTR_LEN(attr->lcname) == len) { - if (0 == memcmp(ZSTR_VAL(attr->lcname), str, len)) { - return attr; - } + if (attr->offset == offset && zend_string_equals_cstr(attr->lcname, str, len)) { + return attr; } } ZEND_HASH_FOREACH_END(); } diff --git a/Zend/zend_compile.c b/Zend/zend_compile.c index 039e95913f6..cfe44d3f376 100644 --- a/Zend/zend_compile.c +++ b/Zend/zend_compile.c @@ -266,9 +266,7 @@ static zend_always_inline bool zend_is_confusable_type(const zend_string *name, /* Intentionally using case-sensitive comparison here, because "integer" is likely intended * as a scalar type, while "Integer" is likely a class type. */ for (; info->name; ++info) { - if (ZSTR_LEN(name) == info->name_len - && memcmp(ZSTR_VAL(name), info->name, info->name_len) == 0 - ) { + if (zend_string_equals_cstr(name, info->name, info->name_len)) { *correct_name = info->correct_name; return 1; } @@ -3379,7 +3377,7 @@ static uint32_t zend_get_arg_num(zend_function *fn, zend_string *arg_name) { for (uint32_t i = 0; i < fn->common.num_args; i++) { zend_internal_arg_info *arg_info = &fn->internal_function.arg_info[i]; size_t len = strlen(arg_info->name); - if (len == ZSTR_LEN(arg_name) && !memcmp(arg_info->name, ZSTR_VAL(arg_name), len)) { + if (zend_string_equals_cstr(arg_name, arg_info->name, len)) { return i + 1; } } diff --git a/Zend/zend_execute.c b/Zend/zend_execute.c index 5b2ce68ffb3..c176696f3f2 100644 --- a/Zend/zend_execute.c +++ b/Zend/zend_execute.c @@ -4675,7 +4675,7 @@ static zend_always_inline uint32_t zend_get_arg_offset_by_name( for (uint32_t i = 0; i < num_args; i++) { zend_internal_arg_info *arg_info = &fbc->internal_function.arg_info[i]; size_t len = strlen(arg_info->name); - if (len == ZSTR_LEN(arg_name) && !memcmp(arg_info->name, ZSTR_VAL(arg_name), len)) { + if (zend_string_equals_cstr(arg_name, arg_info->name, len)) { *cache_slot = fbc; *(uintptr_t *)(cache_slot + 1) = i; return i; diff --git a/Zend/zend_execute_API.c b/Zend/zend_execute_API.c index e791f986f8e..e9ecc5d6697 100644 --- a/Zend/zend_execute_API.c +++ b/Zend/zend_execute_API.c @@ -1817,8 +1817,7 @@ ZEND_API zend_result zend_set_local_var_str(const char *name, size_t len, zval * do { if (ZSTR_H(*str) == h && - ZSTR_LEN(*str) == len && - memcmp(ZSTR_VAL(*str), name, len) == 0) { + zend_string_equals_cstr(*str, name, len)) { zval *var = EX_VAR_NUM(str - op_array->vars); zval_ptr_dtor(var); ZVAL_COPY_VALUE(var, value); diff --git a/Zend/zend_hash.c b/Zend/zend_hash.c index 6e809c87341..0bfe1271ea3 100644 --- a/Zend/zend_hash.c +++ b/Zend/zend_hash.c @@ -711,8 +711,7 @@ static zend_always_inline Bucket *zend_hash_str_find_bucket(const HashTable *ht, p = HT_HASH_TO_BUCKET_EX(arData, idx); if ((p->h == h) && p->key - && (ZSTR_LEN(p->key) == len) - && !memcmp(ZSTR_VAL(p->key), str, len)) { + && zend_string_equals_cstr(p->key, str, len)) { return p; } idx = Z_NEXT(p->val); @@ -1556,8 +1555,7 @@ ZEND_API zend_result ZEND_FASTCALL zend_hash_str_del_ind(HashTable *ht, const ch p = HT_HASH_TO_BUCKET(ht, idx); if ((p->h == h) && p->key - && (ZSTR_LEN(p->key) == len) - && !memcmp(ZSTR_VAL(p->key), str, len)) { + && zend_string_equals_cstr(p->key, str, len)) { if (Z_TYPE(p->val) == IS_INDIRECT) { zval *data = Z_INDIRECT(p->val); @@ -1602,8 +1600,7 @@ ZEND_API zend_result ZEND_FASTCALL zend_hash_str_del(HashTable *ht, const char * p = HT_HASH_TO_BUCKET(ht, idx); if ((p->h == h) && p->key - && (ZSTR_LEN(p->key) == len) - && !memcmp(ZSTR_VAL(p->key), str, len)) { + && zend_string_equals_cstr(p->key, str, len)) { zend_string_release(p->key); p->key = NULL; _zend_hash_del_el_ex(ht, idx, p, prev); diff --git a/Zend/zend_string.c b/Zend/zend_string.c index c24d5dfcdb4..1284e908a55 100644 --- a/Zend/zend_string.c +++ b/Zend/zend_string.c @@ -135,10 +135,8 @@ static zend_always_inline zend_string *zend_interned_string_ht_lookup_ex(zend_ul idx = HT_HASH(interned_strings, nIndex); while (idx != HT_INVALID_IDX) { p = HT_HASH_TO_BUCKET(interned_strings, idx); - if ((p->h == h) && (ZSTR_LEN(p->key) == size)) { - if (!memcmp(ZSTR_VAL(p->key), str, size)) { - return p->key; - } + if ((p->h == h) && zend_string_equals_cstr(p->key, str, size)) { + return p->key; } idx = Z_NEXT(p->val); } diff --git a/Zend/zend_string.h b/Zend/zend_string.h index 19cd926eff7..06a4edc01ae 100644 --- a/Zend/zend_string.h +++ b/Zend/zend_string.h @@ -339,6 +339,11 @@ static zend_always_inline void zend_string_release_ex(zend_string *s, bool persi } } +static zend_always_inline bool zend_string_equals_cstr(const zend_string *s1, const char *s2, size_t s2_length) +{ + return ZSTR_LEN(s1) == s2_length && !memcmp(ZSTR_VAL(s1), s2, s2_length); +} + #if defined(__GNUC__) && (defined(__i386__) || (defined(__x86_64__) && !defined(__ILP32__))) BEGIN_EXTERN_C() ZEND_API bool ZEND_FASTCALL zend_string_equal_val(zend_string *s1, zend_string *s2); @@ -367,7 +372,20 @@ static zend_always_inline bool zend_string_equals(zend_string *s1, zend_string * (ZSTR_LEN(str) == sizeof(c) - 1 && !zend_binary_strcasecmp(ZSTR_VAL(str), ZSTR_LEN(str), (c), sizeof(c) - 1)) #define zend_string_equals_literal(str, literal) \ - (ZSTR_LEN(str) == sizeof(literal)-1 && !memcmp(ZSTR_VAL(str), literal, sizeof(literal) - 1)) + zend_string_equals_cstr(str, literal, strlen(literal)) + +static zend_always_inline bool zend_string_starts_with_cstr(const zend_string *str, const char *prefix, size_t prefix_length) +{ + return ZSTR_LEN(str) >= prefix_length && !memcmp(ZSTR_VAL(str), prefix, prefix_length); +} + +static zend_always_inline bool zend_string_starts_with(const zend_string *str, const zend_string *prefix) +{ + return zend_string_starts_with_cstr(str, ZSTR_VAL(prefix), ZSTR_LEN(prefix)); +} + +#define zend_string_starts_with_literal(str, prefix) \ + zend_string_starts_with_cstr(str, prefix, strlen(prefix)) /* * DJBX33A (Daniel J. Bernstein, Times 33 with Addition) diff --git a/ext/oci8/oci8.c b/ext/oci8/oci8.c index ac79c57f2b2..f75a3a89187 100644 --- a/ext/oci8/oci8.c +++ b/ext/oci8/oci8.c @@ -1096,9 +1096,7 @@ php_oci_connection *php_oci_do_connect_ex(char *username, int username_len, char } if ((tmp_val != NULL) && (tmp != NULL) && - (ZSTR_LEN(tmp->hash_key) == ZSTR_LEN(hashed_details.s)) && - (memcmp(ZSTR_VAL(tmp->hash_key), ZSTR_VAL(hashed_details.s), - ZSTR_LEN(tmp->hash_key)) == 0)) { + zend_string_equals(tmp->hash_key, hashed_details.s)) { connection = tmp; GC_ADDREF(connection->id); } @@ -2120,8 +2118,7 @@ static php_oci_spool *php_oci_get_spool(char *username, int username_len, char * } zend_register_persistent_resource_ex(session_pool->spool_hash_key, session_pool, le_psessionpool); } else if (spool_out_le->type == le_psessionpool && - ZSTR_LEN(((php_oci_spool *)(spool_out_le->ptr))->spool_hash_key) == ZSTR_LEN(spool_hashed_details.s) && - memcmp(ZSTR_VAL(((php_oci_spool *)(spool_out_le->ptr))->spool_hash_key), ZSTR_VAL(spool_hashed_details.s), ZSTR_LEN(spool_hashed_details.s)) == 0) { + zend_string_equals(((php_oci_spool *)(spool_out_le->ptr))->spool_hash_key, spool_hashed_details.s)) { /* retrieve the cached session pool */ session_pool = (php_oci_spool *)(spool_out_le->ptr); } diff --git a/ext/opcache/ZendAccelerator.c b/ext/opcache/ZendAccelerator.c index e17fc16a947..362126db23d 100644 --- a/ext/opcache/ZendAccelerator.c +++ b/ext/opcache/ZendAccelerator.c @@ -590,10 +590,8 @@ static zend_always_inline zend_string *accel_find_interned_string_ex(zend_ulong if (EXPECTED(pos != STRTAB_INVALID_POS)) { do { s = STRTAB_POS_TO_STR(&ZCSG(interned_strings), pos); - if (EXPECTED(ZSTR_H(s) == h) && EXPECTED(ZSTR_LEN(s) == size)) { - if (!memcmp(ZSTR_VAL(s), str, size)) { - return s; - } + if (EXPECTED(ZSTR_H(s) == h) && zend_string_equals_cstr(s, str, size)) { + return s; } pos = STRTAB_COLLISION(s); } while (pos != STRTAB_INVALID_POS); @@ -1637,7 +1635,7 @@ static zend_persistent_script *cache_script_in_shared_memory(zend_persistent_scr zend_accel_error(ACCEL_LOG_INFO, "Cached script '%s'", ZSTR_VAL(new_persistent_script->script.filename)); if (key && /* key may contain non-persistent PHAR aliases (see issues #115 and #149) */ - memcmp(ZSTR_VAL(key), "phar://", sizeof("phar://") - 1) != 0 && + !zend_string_starts_with_literal(key, "phar://") && !zend_string_equals(new_persistent_script->script.filename, key)) { /* link key to the same persistent script in hash table */ zend_string *new_key = accel_new_interned_key(key); diff --git a/ext/phar/stream.c b/ext/phar/stream.c index 9bba67c6fc9..b45b662398c 100644 --- a/ext/phar/stream.c +++ b/ext/phar/stream.c @@ -912,8 +912,7 @@ static int phar_wrapper_rename(php_stream_wrapper *wrapper, const char *url_from ZEND_HASH_MAP_FOREACH_BUCKET(&phar->virtual_dirs, b) { str_key = b->key; - if (ZSTR_LEN(str_key) >= from_len && - memcmp(ZSTR_VAL(str_key), ZSTR_VAL(resource_from->path)+1, from_len) == 0 && + if (zend_string_starts_with_cstr(str_key, ZSTR_VAL(resource_from->path)+1, from_len) && (ZSTR_LEN(str_key) == from_len || IS_SLASH(ZSTR_VAL(str_key)[from_len]))) { new_str_key = zend_string_alloc(ZSTR_LEN(str_key) + to_len - from_len, 0); @@ -930,8 +929,7 @@ static int phar_wrapper_rename(php_stream_wrapper *wrapper, const char *url_from ZEND_HASH_MAP_FOREACH_BUCKET(&phar->mounted_dirs, b) { str_key = b->key; - if (ZSTR_LEN(str_key) >= from_len && - memcmp(ZSTR_VAL(str_key), ZSTR_VAL(resource_from->path)+1, from_len) == 0 && + if (zend_string_starts_with_cstr(str_key, ZSTR_VAL(resource_from->path)+1, from_len) && (ZSTR_LEN(str_key) == from_len || IS_SLASH(ZSTR_VAL(str_key)[from_len]))) { new_str_key = zend_string_alloc(ZSTR_LEN(str_key) + to_len - from_len, 0); diff --git a/ext/spl/spl_directory.c b/ext/spl/spl_directory.c index b6e53c2921b..f752593c923 100644 --- a/ext/spl/spl_directory.c +++ b/ext/spl/spl_directory.c @@ -740,7 +740,7 @@ void spl_filesystem_object_construct(INTERNAL_FUNCTION_PARAMETERS, zend_long cto /* spl_filesystem_dir_open() may emit an E_WARNING */ zend_replace_error_handling(EH_THROW, spl_ce_UnexpectedValueException, &error_handling); #ifdef HAVE_GLOB - if (SPL_HAS_FLAG(ctor_flags, DIT_CTOR_GLOB) && memcmp(ZSTR_VAL(path), "glob://", sizeof("glob://")-1) != 0) { + if (SPL_HAS_FLAG(ctor_flags, DIT_CTOR_GLOB) && !zend_string_starts_with_literal(path, "glob://")) { path = zend_strpprintf(0, "glob://%s", ZSTR_VAL(path)); spl_filesystem_dir_open(intern, path); zend_string_release(path); diff --git a/ext/standard/proc_open.c b/ext/standard/proc_open.c index a57e66bd979..28420b4f803 100644 --- a/ext/standard/proc_open.c +++ b/ext/standard/proc_open.c @@ -728,7 +728,7 @@ static zend_result set_proc_descriptor_to_pipe(descriptorspec_item *desc, zend_s desc->type = DESCRIPTOR_TYPE_PIPE; - if (strncmp(ZSTR_VAL(zmode), "w", 1) != 0) { + if (!zend_string_starts_with_literal(zmode, "w")) { desc->parentend = newpipe[1]; desc->childend = newpipe[0]; desc->mode_flags = O_WRONLY; diff --git a/ext/standard/string.c b/ext/standard/string.c index 593c67cb322..00d422fb185 100644 --- a/ext/standard/string.c +++ b/ext/standard/string.c @@ -1810,11 +1810,7 @@ PHP_FUNCTION(str_starts_with) Z_PARAM_STR(needle) ZEND_PARSE_PARAMETERS_END(); - if (ZSTR_LEN(needle) > ZSTR_LEN(haystack)) { - RETURN_FALSE; - } - - RETURN_BOOL(memcmp(ZSTR_VAL(haystack), ZSTR_VAL(needle), ZSTR_LEN(needle)) == 0); + RETURN_BOOL(zend_string_starts_with(haystack, needle)); } /* }}} */ @@ -4738,14 +4734,14 @@ static zend_string *try_setlocale_str(zend_long cat, zend_string *loc) { /* C locale is represented as NULL. */ BG(ctype_string) = NULL; return ZSTR_CHAR('C'); - } else if (len == ZSTR_LEN(loc) && !memcmp(ZSTR_VAL(loc), retval, len)) { + } else if (zend_string_equals_cstr(loc, retval, len)) { BG(ctype_string) = zend_string_copy(loc); return zend_string_copy(BG(ctype_string)); } else { BG(ctype_string) = zend_string_init(retval, len, 0); return zend_string_copy(BG(ctype_string)); } - } else if (len == ZSTR_LEN(loc) && !memcmp(ZSTR_VAL(loc), retval, len)) { + } else if (zend_string_equals_cstr(loc, retval, len)) { return zend_string_copy(loc); } } diff --git a/main/output.c b/main/output.c index 3e29f8f1526..7b5bc49dab8 100644 --- a/main/output.c +++ b/main/output.c @@ -584,7 +584,7 @@ PHPAPI int php_output_handler_started(const char *name, size_t name_len) handlers = (php_output_handler **) zend_stack_base(&OG(handlers)); for (i = 0; i < count; ++i) { - if (name_len == ZSTR_LEN(handlers[i]->name) && !memcmp(ZSTR_VAL(handlers[i]->name), name, name_len)) { + if (zend_string_equals_cstr(handlers[i]->name, name, name_len)) { return 1; } } diff --git a/main/php_variables.c b/main/php_variables.c index 0261cf0098e..812b46bc9c7 100644 --- a/main/php_variables.c +++ b/main/php_variables.c @@ -702,8 +702,7 @@ static void php_autoglobal_merge(HashTable *dest, HashTable *src) || Z_TYPE_P(dest_entry) != IS_ARRAY) { Z_TRY_ADDREF_P(src_entry); if (string_key) { - if (!globals_check || ZSTR_LEN(string_key) != sizeof("GLOBALS") - 1 - || memcmp(ZSTR_VAL(string_key), "GLOBALS", sizeof("GLOBALS") - 1)) { + if (!globals_check || !zend_string_equals_literal(string_key, "GLOBALS")) { zend_hash_update(dest, string_key, src_entry); } else { Z_TRY_DELREF_P(src_entry);