Skip to content

Commit

Permalink
Fix phpGH-16326: Memory management is broken for bad dictionaries
Browse files Browse the repository at this point in the history
We must not `efree()` `zend_string`s, since they may have a refcount
greater than one, and may even be interned.

We also must not confuse `zend_string *` with `zend_string **`.

And we should play it safe by using `safe_emalloc()` to avoid
theoretical integer overflows.
  • Loading branch information
cmb69 committed Oct 9, 2024
1 parent 1ee56bd commit c968f53
Show file tree
Hide file tree
Showing 2 changed files with 24 additions and 4 deletions.
20 changes: 20 additions & 0 deletions ext/zlib/tests/gh16326.phpt
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
--TEST--
GH-16326 (Memory management is broken for bad dictionaries)
--EXTENSIONS--
zlib
--FILE--
<?php
try {
deflate_init(ZLIB_ENCODING_DEFLATE, ["dictionary" => [" ", ""]]);
} catch (ValueError $ex) {
echo $ex->getMessage(), "\n";
}
try {
deflate_init(ZLIB_ENCODING_DEFLATE, ["dictionary" => ["hello", "wor\0ld"]]);
} catch (ValueError $ex) {
echo $ex->getMessage(), "\n";
}
?>
--EXPECT--
deflate_init(): Argument #2 ($options) must not contain empty strings
deflate_init(): Argument #2 ($options) must not contain strings with null bytes
8 changes: 4 additions & 4 deletions ext/zlib/zlib.c
Original file line number Diff line number Diff line change
Expand Up @@ -807,7 +807,7 @@ static bool zlib_create_dictionary_string(HashTable *options, char **dict, size_
if (zend_hash_num_elements(dictionary) > 0) {
char *dictptr;
zval *cur;
zend_string **strings = emalloc(sizeof(zend_string *) * zend_hash_num_elements(dictionary));
zend_string **strings = safe_emalloc(zend_hash_num_elements(dictionary), sizeof(zend_string *), 0);
zend_string **end, **ptr = strings - 1;

ZEND_HASH_FOREACH_VAL(dictionary, cur) {
Expand All @@ -816,10 +816,10 @@ static bool zlib_create_dictionary_string(HashTable *options, char **dict, size_
*++ptr = zval_get_string(cur);
if (!*ptr || ZSTR_LEN(*ptr) == 0 || EG(exception)) {
if (*ptr) {
efree(*ptr);
zend_string_release(*ptr);
}
while (--ptr >= strings) {
efree(ptr);
zend_string_release(*ptr);
}
efree(strings);
if (!EG(exception)) {
Expand All @@ -830,7 +830,7 @@ static bool zlib_create_dictionary_string(HashTable *options, char **dict, size_
for (i = 0; i < ZSTR_LEN(*ptr); i++) {
if (ZSTR_VAL(*ptr)[i] == 0) {
do {
efree(ptr);
zend_string_release(*ptr);
} while (--ptr >= strings);
efree(strings);
zend_argument_value_error(2, "must not contain strings with null bytes");
Expand Down

0 comments on commit c968f53

Please sign in to comment.