Skip to content

Commit b9e895b

Browse files
MaxKellermannMaxKellermann
authored
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.
1 parent 54440fa commit b9e895b

15 files changed

+44
-48
lines changed

‎Zend/zend_attributes.c

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -112,10 +112,8 @@ static zend_attribute *get_attribute_str(HashTable *attributes, const char *str,
112112
zend_attribute *attr;
113113

114114
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;
119117
}
120118
} ZEND_HASH_FOREACH_END();
121119
}

‎Zend/zend_compile.c

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -266,9 +266,7 @@ static zend_always_inline bool zend_is_confusable_type(const zend_string *name,
266266
/* Intentionally using case-sensitive comparison here, because "integer" is likely intended
267267
* as a scalar type, while "Integer" is likely a class type. */
268268
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)) {
272270
*correct_name = info->correct_name;
273271
return 1;
274272
}
@@ -3379,7 +3377,7 @@ static uint32_t zend_get_arg_num(zend_function *fn, zend_string *arg_name) {
33793377
for (uint32_t i = 0; i < fn->common.num_args; i++) {
33803378
zend_internal_arg_info *arg_info = &fn->internal_function.arg_info[i];
33813379
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)) {
33833381
return i + 1;
33843382
}
33853383
}

‎Zend/zend_execute.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4675,7 +4675,7 @@ static zend_always_inline uint32_t zend_get_arg_offset_by_name(
46754675
for (uint32_t i = 0; i < num_args; i++) {
46764676
zend_internal_arg_info *arg_info = &fbc->internal_function.arg_info[i];
46774677
size_t len = strlen(arg_info->name);
4678-
if (len == ZSTR_LEN(arg_name) && !memcmp(arg_info->name, ZSTR_VAL(arg_name), len)) {
4678+
if (zend_string_equals_cstr(arg_name, arg_info->name, len)) {
46794679
*cache_slot = fbc;
46804680
*(uintptr_t *)(cache_slot + 1) = i;
46814681
return i;

‎Zend/zend_execute_API.c

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1817,8 +1817,7 @@ ZEND_API zend_result zend_set_local_var_str(const char *name, size_t len, zval *
18171817

18181818
do {
18191819
if (ZSTR_H(*str) == h &&
1820-
ZSTR_LEN(*str) == len &&
1821-
memcmp(ZSTR_VAL(*str), name, len) == 0) {
1820+
zend_string_equals_cstr(*str, name, len)) {
18221821
zval *var = EX_VAR_NUM(str - op_array->vars);
18231822
zval_ptr_dtor(var);
18241823
ZVAL_COPY_VALUE(var, value);

‎Zend/zend_hash.c

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -711,8 +711,7 @@ static zend_always_inline Bucket *zend_hash_str_find_bucket(const HashTable *ht,
711711
p = HT_HASH_TO_BUCKET_EX(arData, idx);
712712
if ((p->h == h)
713713
&& p->key
714-
&& (ZSTR_LEN(p->key) == len)
715-
&& !memcmp(ZSTR_VAL(p->key), str, len)) {
714+
&& zend_string_equals_cstr(p->key, str, len)) {
716715
return p;
717716
}
718717
idx = Z_NEXT(p->val);
@@ -1556,8 +1555,7 @@ ZEND_API zend_result ZEND_FASTCALL zend_hash_str_del_ind(HashTable *ht, const ch
15561555
p = HT_HASH_TO_BUCKET(ht, idx);
15571556
if ((p->h == h)
15581557
&& p->key
1559-
&& (ZSTR_LEN(p->key) == len)
1560-
&& !memcmp(ZSTR_VAL(p->key), str, len)) {
1558+
&& zend_string_equals_cstr(p->key, str, len)) {
15611559
if (Z_TYPE(p->val) == IS_INDIRECT) {
15621560
zval *data = Z_INDIRECT(p->val);
15631561

@@ -1602,8 +1600,7 @@ ZEND_API zend_result ZEND_FASTCALL zend_hash_str_del(HashTable *ht, const char *
16021600
p = HT_HASH_TO_BUCKET(ht, idx);
16031601
if ((p->h == h)
16041602
&& p->key
1605-
&& (ZSTR_LEN(p->key) == len)
1606-
&& !memcmp(ZSTR_VAL(p->key), str, len)) {
1603+
&& zend_string_equals_cstr(p->key, str, len)) {
16071604
zend_string_release(p->key);
16081605
p->key = NULL;
16091606
_zend_hash_del_el_ex(ht, idx, p, prev);

‎Zend/zend_string.c

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -135,10 +135,8 @@ static zend_always_inline zend_string *zend_interned_string_ht_lookup_ex(zend_ul
135135
idx = HT_HASH(interned_strings, nIndex);
136136
while (idx != HT_INVALID_IDX) {
137137
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;
142140
}
143141
idx = Z_NEXT(p->val);
144142
}

‎Zend/zend_string.h

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -339,6 +339,11 @@ static zend_always_inline void zend_string_release_ex(zend_string *s, bool persi
339339
}
340340
}
341341

342+
static zend_always_inline bool zend_string_equals_cstr(const zend_string *s1, const char *s2, size_t s2_length)
343+
{
344+
return ZSTR_LEN(s1) == s2_length && !memcmp(ZSTR_VAL(s1), s2, s2_length);
345+
}
346+
342347
#if defined(__GNUC__) && (defined(__i386__) || (defined(__x86_64__) && !defined(__ILP32__)))
343348
BEGIN_EXTERN_C()
344349
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 *
367372
(ZSTR_LEN(str) == sizeof(c) - 1 && !zend_binary_strcasecmp(ZSTR_VAL(str), ZSTR_LEN(str), (c), sizeof(c) - 1))
368373

369374
#define zend_string_equals_literal(str, literal) \
370-
(ZSTR_LEN(str) == sizeof(literal)-1 && !memcmp(ZSTR_VAL(str), literal, sizeof(literal) - 1))
375+
zend_string_equals_cstr(str, literal, strlen(literal))
376+
377+
static zend_always_inline bool zend_string_starts_with_cstr(const zend_string *str, const char *prefix, size_t prefix_length)
378+
{
379+
return ZSTR_LEN(str) >= prefix_length && !memcmp(ZSTR_VAL(str), prefix, prefix_length);
380+
}
381+
382+
static zend_always_inline bool zend_string_starts_with(const zend_string *str, const zend_string *prefix)
383+
{
384+
return zend_string_starts_with_cstr(str, ZSTR_VAL(prefix), ZSTR_LEN(prefix));
385+
}
386+
387+
#define zend_string_starts_with_literal(str, prefix) \
388+
zend_string_starts_with_cstr(str, prefix, strlen(prefix))
371389

372390
/*
373391
* DJBX33A (Daniel J. Bernstein, Times 33 with Addition)

‎ext/oci8/oci8.c

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1096,9 +1096,7 @@ php_oci_connection *php_oci_do_connect_ex(char *username, int username_len, char
10961096
}
10971097

10981098
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)) {
11021100
connection = tmp;
11031101
GC_ADDREF(connection->id);
11041102
}
@@ -2120,8 +2118,7 @@ static php_oci_spool *php_oci_get_spool(char *username, int username_len, char *
21202118
}
21212119
zend_register_persistent_resource_ex(session_pool->spool_hash_key, session_pool, le_psessionpool);
21222120
} 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)) {
21252122
/* retrieve the cached session pool */
21262123
session_pool = (php_oci_spool *)(spool_out_le->ptr);
21272124
}

‎ext/opcache/ZendAccelerator.c

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -590,10 +590,8 @@ static zend_always_inline zend_string *accel_find_interned_string_ex(zend_ulong
590590
if (EXPECTED(pos != STRTAB_INVALID_POS)) {
591591
do {
592592
s = STRTAB_POS_TO_STR(&ZCSG(interned_strings), pos);
593-
if (EXPECTED(ZSTR_H(s) == h) && EXPECTED(ZSTR_LEN(s) == size)) {
594-
if (!memcmp(ZSTR_VAL(s), str, size)) {
595-
return s;
596-
}
593+
if (EXPECTED(ZSTR_H(s) == h) && zend_string_equals_cstr(s, str, size)) {
594+
return s;
597595
}
598596
pos = STRTAB_COLLISION(s);
599597
} while (pos != STRTAB_INVALID_POS);
@@ -1637,7 +1635,7 @@ static zend_persistent_script *cache_script_in_shared_memory(zend_persistent_scr
16371635
zend_accel_error(ACCEL_LOG_INFO, "Cached script '%s'", ZSTR_VAL(new_persistent_script->script.filename));
16381636
if (key &&
16391637
/* key may contain non-persistent PHAR aliases (see issues #115 and #149) */
1640-
memcmp(ZSTR_VAL(key), "phar://", sizeof("phar://") - 1) != 0 &&
1638+
!zend_string_starts_with_literal(key, "phar://") &&
16411639
!zend_string_equals(new_persistent_script->script.filename, key)) {
16421640
/* link key to the same persistent script in hash table */
16431641
zend_string *new_key = accel_new_interned_key(key);

‎ext/phar/stream.c

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -912,8 +912,7 @@ static int phar_wrapper_rename(php_stream_wrapper *wrapper, const char *url_from
912912

913913
ZEND_HASH_MAP_FOREACH_BUCKET(&phar->virtual_dirs, b) {
914914
str_key = b->key;
915-
if (ZSTR_LEN(str_key) >= from_len &&
916-
memcmp(ZSTR_VAL(str_key), ZSTR_VAL(resource_from->path)+1, from_len) == 0 &&
915+
if (zend_string_starts_with_cstr(str_key, ZSTR_VAL(resource_from->path)+1, from_len) &&
917916
(ZSTR_LEN(str_key) == from_len || IS_SLASH(ZSTR_VAL(str_key)[from_len]))) {
918917

919918
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
930929

931930
ZEND_HASH_MAP_FOREACH_BUCKET(&phar->mounted_dirs, b) {
932931
str_key = b->key;
933-
if (ZSTR_LEN(str_key) >= from_len &&
934-
memcmp(ZSTR_VAL(str_key), ZSTR_VAL(resource_from->path)+1, from_len) == 0 &&
932+
if (zend_string_starts_with_cstr(str_key, ZSTR_VAL(resource_from->path)+1, from_len) &&
935933
(ZSTR_LEN(str_key) == from_len || IS_SLASH(ZSTR_VAL(str_key)[from_len]))) {
936934

937935
new_str_key = zend_string_alloc(ZSTR_LEN(str_key) + to_len - from_len, 0);

‎ext/spl/spl_directory.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -740,7 +740,7 @@ void spl_filesystem_object_construct(INTERNAL_FUNCTION_PARAMETERS, zend_long cto
740740
/* spl_filesystem_dir_open() may emit an E_WARNING */
741741
zend_replace_error_handling(EH_THROW, spl_ce_UnexpectedValueException, &error_handling);
742742
#ifdef HAVE_GLOB
743-
if (SPL_HAS_FLAG(ctor_flags, DIT_CTOR_GLOB) && memcmp(ZSTR_VAL(path), "glob://", sizeof("glob://")-1) != 0) {
743+
if (SPL_HAS_FLAG(ctor_flags, DIT_CTOR_GLOB) && !zend_string_starts_with_literal(path, "glob://")) {
744744
path = zend_strpprintf(0, "glob://%s", ZSTR_VAL(path));
745745
spl_filesystem_dir_open(intern, path);
746746
zend_string_release(path);

‎ext/standard/proc_open.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -728,7 +728,7 @@ static zend_result set_proc_descriptor_to_pipe(descriptorspec_item *desc, zend_s
728728

729729
desc->type = DESCRIPTOR_TYPE_PIPE;
730730

731-
if (strncmp(ZSTR_VAL(zmode), "w", 1) != 0) {
731+
if (!zend_string_starts_with_literal(zmode, "w")) {
732732
desc->parentend = newpipe[1];
733733
desc->childend = newpipe[0];
734734
desc->mode_flags = O_WRONLY;

‎ext/standard/string.c

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1810,11 +1810,7 @@ PHP_FUNCTION(str_starts_with)
18101810
Z_PARAM_STR(needle)
18111811
ZEND_PARSE_PARAMETERS_END();
18121812

1813-
if (ZSTR_LEN(needle) > ZSTR_LEN(haystack)) {
1814-
RETURN_FALSE;
1815-
}
1816-
1817-
RETURN_BOOL(memcmp(ZSTR_VAL(haystack), ZSTR_VAL(needle), ZSTR_LEN(needle)) == 0);
1813+
RETURN_BOOL(zend_string_starts_with(haystack, needle));
18181814
}
18191815
/* }}} */
18201816

@@ -4738,14 +4734,14 @@ static zend_string *try_setlocale_str(zend_long cat, zend_string *loc) {
47384734
/* C locale is represented as NULL. */
47394735
BG(ctype_string) = NULL;
47404736
return ZSTR_CHAR('C');
4741-
} else if (len == ZSTR_LEN(loc) && !memcmp(ZSTR_VAL(loc), retval, len)) {
4737+
} else if (zend_string_equals_cstr(loc, retval, len)) {
47424738
BG(ctype_string) = zend_string_copy(loc);
47434739
return zend_string_copy(BG(ctype_string));
47444740
} else {
47454741
BG(ctype_string) = zend_string_init(retval, len, 0);
47464742
return zend_string_copy(BG(ctype_string));
47474743
}
4748-
} else if (len == ZSTR_LEN(loc) && !memcmp(ZSTR_VAL(loc), retval, len)) {
4744+
} else if (zend_string_equals_cstr(loc, retval, len)) {
47494745
return zend_string_copy(loc);
47504746
}
47514747
}

‎main/output.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -584,7 +584,7 @@ PHPAPI int php_output_handler_started(const char *name, size_t name_len)
584584
handlers = (php_output_handler **) zend_stack_base(&OG(handlers));
585585

586586
for (i = 0; i < count; ++i) {
587-
if (name_len == ZSTR_LEN(handlers[i]->name) && !memcmp(ZSTR_VAL(handlers[i]->name), name, name_len)) {
587+
if (zend_string_equals_cstr(handlers[i]->name, name, name_len)) {
588588
return 1;
589589
}
590590
}

‎main/php_variables.c

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -702,8 +702,7 @@ static void php_autoglobal_merge(HashTable *dest, HashTable *src)
702702
|| Z_TYPE_P(dest_entry) != IS_ARRAY) {
703703
Z_TRY_ADDREF_P(src_entry);
704704
if (string_key) {
705-
if (!globals_check || ZSTR_LEN(string_key) != sizeof("GLOBALS") - 1
706-
|| memcmp(ZSTR_VAL(string_key), "GLOBALS", sizeof("GLOBALS") - 1)) {
705+
if (!globals_check || !zend_string_equals_literal(string_key, "GLOBALS")) {
707706
zend_hash_update(dest, string_key, src_entry);
708707
} else {
709708
Z_TRY_DELREF_P(src_entry);

0 commit comments

Comments
 (0)