From c968f53667241009bcf66bf8ef1693fb4fe12514 Mon Sep 17 00:00:00 2001 From: "Christoph M. Becker" Date: Thu, 10 Oct 2024 00:34:21 +0200 Subject: [PATCH 1/3] Fix GH-16326: Memory management is broken for bad dictionaries 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. --- ext/zlib/tests/gh16326.phpt | 20 ++++++++++++++++++++ ext/zlib/zlib.c | 8 ++++---- 2 files changed, 24 insertions(+), 4 deletions(-) create mode 100644 ext/zlib/tests/gh16326.phpt diff --git a/ext/zlib/tests/gh16326.phpt b/ext/zlib/tests/gh16326.phpt new file mode 100644 index 0000000000000..3b547f33d16aa --- /dev/null +++ b/ext/zlib/tests/gh16326.phpt @@ -0,0 +1,20 @@ +--TEST-- +GH-16326 (Memory management is broken for bad dictionaries) +--EXTENSIONS-- +zlib +--FILE-- + [" ", ""]]); +} 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 diff --git a/ext/zlib/zlib.c b/ext/zlib/zlib.c index 8f1df51c96d8d..43e9491e24249 100644 --- a/ext/zlib/zlib.c +++ b/ext/zlib/zlib.c @@ -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) { @@ -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)) { @@ -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"); From 395b1ee19f35f882449c0433c8f98f076906017b Mon Sep 17 00:00:00 2001 From: "Christoph M. Becker" Date: Thu, 10 Oct 2024 13:09:02 +0200 Subject: [PATCH 2/3] Apply suggested improvements by @TimWolla --- ext/zlib/tests/gh16326.phpt | 6 ++++++ ext/zlib/zlib.c | 26 +++++++++++--------------- 2 files changed, 17 insertions(+), 15 deletions(-) diff --git a/ext/zlib/tests/gh16326.phpt b/ext/zlib/tests/gh16326.phpt index 3b547f33d16aa..b4997368549f7 100644 --- a/ext/zlib/tests/gh16326.phpt +++ b/ext/zlib/tests/gh16326.phpt @@ -14,7 +14,13 @@ try { } catch (ValueError $ex) { echo $ex->getMessage(), "\n"; } +try { + deflate_init(ZLIB_ENCODING_DEFLATE, ["dictionary" => [" ", new stdClass]]); +} catch (Error $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 +Object of class stdClass could not be converted to string diff --git a/ext/zlib/zlib.c b/ext/zlib/zlib.c index 43e9491e24249..49c91ac2d5a2e 100644 --- a/ext/zlib/zlib.c +++ b/ext/zlib/zlib.c @@ -814,28 +814,24 @@ static bool zlib_create_dictionary_string(HashTable *options, char **dict, size_ size_t i; *++ptr = zval_get_string(cur); - if (!*ptr || ZSTR_LEN(*ptr) == 0 || EG(exception)) { - if (*ptr) { + ZEND_ASSERT(*ptr); + if (ZSTR_LEN(*ptr) == 0 || EG(exception)) { + do { zend_string_release(*ptr); - } - while (--ptr >= strings) { - zend_string_release(*ptr); - } + } while (--ptr >= strings); efree(strings); if (!EG(exception)) { zend_argument_value_error(2, "must not contain empty strings"); } return 0; } - for (i = 0; i < ZSTR_LEN(*ptr); i++) { - if (ZSTR_VAL(*ptr)[i] == 0) { - do { - zend_string_release(*ptr); - } while (--ptr >= strings); - efree(strings); - zend_argument_value_error(2, "must not contain strings with null bytes"); - return 0; - } + if (zend_str_has_nul_byte(*ptr)) { + do { + zend_string_release(*ptr); + } while (--ptr >= strings); + efree(strings); + zend_argument_value_error(2, "must not contain strings with null bytes"); + return 0; } *dictlen += ZSTR_LEN(*ptr) + 1; From 3ca9c831dfd8f146c9e0867893c353321c66b3cd Mon Sep 17 00:00:00 2001 From: "Christoph M. Becker" Date: Thu, 10 Oct 2024 13:21:44 +0200 Subject: [PATCH 3/3] Remove unused variable --- ext/zlib/zlib.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/ext/zlib/zlib.c b/ext/zlib/zlib.c index 49c91ac2d5a2e..e85db94801af0 100644 --- a/ext/zlib/zlib.c +++ b/ext/zlib/zlib.c @@ -811,8 +811,6 @@ static bool zlib_create_dictionary_string(HashTable *options, char **dict, size_ zend_string **end, **ptr = strings - 1; ZEND_HASH_FOREACH_VAL(dictionary, cur) { - size_t i; - *++ptr = zval_get_string(cur); ZEND_ASSERT(*ptr); if (ZSTR_LEN(*ptr) == 0 || EG(exception)) {