Replace memcmp() with zend_string functions (#8216)

MaxKellermann · web-flow · commit b9e895bca0a4 · 2022-03-31T16:27:58.000+02:00
* 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.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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);

Original file line number	Diff line number	Diff line change
`@@ -112,10 +112,8 @@ static zend_attribute get_attribute_str(HashTable attributes, const char *str,`
`112`	`112`	`zend_attribute *attr;`
`113`	`113`
`114`	`114`	`ZEND_HASH_PACKED_FOREACH_PTR(attributes, attr) {`
`115`		`- if (attr->offset == offset && ZSTR_LEN(attr->lcname) == len) {`
`116`		`- if (0 == memcmp(ZSTR_VAL(attr->lcname), str, len)) {`
`117`		`- return attr;`
`118`		`- }`
	`115`	`+ if (attr->offset == offset && zend_string_equals_cstr(attr->lcname, str, len)) {`
	`116`	`+ return attr;`
`119`	`117`	`}`
`120`	`118`	`} ZEND_HASH_FOREACH_END();`
`121`	`119`	`}`
Original file line number	Diff line number	Diff line change
`@@ -266,9 +266,7 @@ static zend_always_inline bool zend_is_confusable_type(const zend_string *name,`
`266`	`266`	`/* Intentionally using case-sensitive comparison here, because "integer" is likely intended`
`267`	`267`	`* as a scalar type, while "Integer" is likely a class type. */`
`268`	`268`	`for (; info->name; ++info) {`
`269`		`- if (ZSTR_LEN(name) == info->name_len`
`270`		`- && memcmp(ZSTR_VAL(name), info->name, info->name_len) == 0`
`271`		`- ) {`
	`269`	`+ if (zend_string_equals_cstr(name, info->name, info->name_len)) {`
`272`	`270`	`*correct_name = info->correct_name;`
`273`	`271`	`return 1;`
`274`	`272`	`}`
`@@ -3379,7 +3377,7 @@ static uint32_t zend_get_arg_num(zend_function fn, zend_string arg_name) {`
`3379`	`3377`	`for (uint32_t i = 0; i < fn->common.num_args; i++) {`
`3380`	`3378`	`zend_internal_arg_info *arg_info = &fn->internal_function.arg_info[i];`
`3381`	`3379`	`size_t len = strlen(arg_info->name);`
`3382`		`- if (len == ZSTR_LEN(arg_name) && !memcmp(arg_info->name, ZSTR_VAL(arg_name), len)) {`
	`3380`	`+ if (zend_string_equals_cstr(arg_name, arg_info->name, len)) {`
`3383`	`3381`	`return i + 1;`
`3384`	`3382`	`}`
`3385`	`3383`	`}`
Original file line number	Diff line number	Diff line change
`@@ -135,10 +135,8 @@ static zend_always_inline zend_string *zend_interned_string_ht_lookup_ex(zend_ul`
`135`	`135`	`idx = HT_HASH(interned_strings, nIndex);`
`136`	`136`	`while (idx != HT_INVALID_IDX) {`
`137`	`137`	`p = HT_HASH_TO_BUCKET(interned_strings, idx);`
`138`		`- if ((p->h == h) && (ZSTR_LEN(p->key) == size)) {`
`139`		`- if (!memcmp(ZSTR_VAL(p->key), str, size)) {`
`140`		`- return p->key;`
`141`		`- }`
	`138`	`+ if ((p->h == h) && zend_string_equals_cstr(p->key, str, size)) {`
	`139`	`+ return p->key;`
`142`	`140`	`}`
`143`	`141`	`idx = Z_NEXT(p->val);`
`144`	`142`	`}`
Original file line number	Diff line number	Diff line change
`@@ -1096,9 +1096,7 @@ php_oci_connection php_oci_do_connect_ex(char username, int username_len, char`
`1096`	`1096`	`}`
`1097`	`1097`
`1098`	`1098`	`if ((tmp_val != NULL) && (tmp != NULL) &&`
`1099`		`- (ZSTR_LEN(tmp->hash_key) == ZSTR_LEN(hashed_details.s)) &&`
`1100`		`- (memcmp(ZSTR_VAL(tmp->hash_key), ZSTR_VAL(hashed_details.s),`
`1101`		`- ZSTR_LEN(tmp->hash_key)) == 0)) {`
	`1099`	`+ zend_string_equals(tmp->hash_key, hashed_details.s)) {`
`1102`	`1100`	`connection = tmp;`
`1103`	`1101`	`GC_ADDREF(connection->id);`
`1104`	`1102`	`}`
`@@ -2120,8 +2118,7 @@ static php_oci_spool php_oci_get_spool(char username, int username_len, char *`
`2120`	`2118`	`}`
`2121`	`2119`	`zend_register_persistent_resource_ex(session_pool->spool_hash_key, session_pool, le_psessionpool);`
`2122`	`2120`	`} else if (spool_out_le->type == le_psessionpool &&`
`2123`		`- ZSTR_LEN(((php_oci_spool *)(spool_out_le->ptr))->spool_hash_key) == ZSTR_LEN(spool_hashed_details.s) &&`
`2124`		`- 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) {`
	`2121`	`+ zend_string_equals(((php_oci_spool *)(spool_out_le->ptr))->spool_hash_key, spool_hashed_details.s)) {`
`2125`	`2122`	`/* retrieve the cached session pool */`
`2126`	`2123`	`session_pool = (php_oci_spool *)(spool_out_le->ptr);`
`2127`	`2124`	`}`