From e6e2ec56ab77356160135f35645d38bd97915ed4 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Thu, 26 Dec 2024 13:40:39 +0100 Subject: [PATCH 1/6] Merge duplicate code blocks This makes the code less error-prone. --- Zend/zend_ini.c | 14 +------------- 1 file changed, 1 insertion(+), 13 deletions(-) diff --git a/Zend/zend_ini.c b/Zend/zend_ini.c index 9588574df6c..de37042047f 100644 --- a/Zend/zend_ini.c +++ b/Zend/zend_ini.c @@ -696,19 +696,7 @@ static zend_ulong zend_ini_parse_quantity_internal(zend_string *value, zend_ini_ return 0; } digits += 2; - if (UNEXPECTED(digits == str_end)) { - /* Escape the string to avoid null bytes and to make non-printable chars - * visible */ - smart_str_append_escaped(&invalid, ZSTR_VAL(value), ZSTR_LEN(value)); - smart_str_0(&invalid); - - *errstr = zend_strpprintf(0, "Invalid quantity \"%s\": no digits after base prefix, interpreting as \"0\" for backwards compatibility", - ZSTR_VAL(invalid.s)); - - smart_str_free(&invalid); - return 0; - } - if (UNEXPECTED(digits != zend_ini_consume_quantity_prefix(digits, str_end))) { + if (UNEXPECTED(digits == str_end || digits != zend_ini_consume_quantity_prefix(digits, str_end))) { /* Escape the string to avoid null bytes and to make non-printable chars * visible */ smart_str_append_escaped(&invalid, ZSTR_VAL(value), ZSTR_LEN(value)); From 2c267722b361fc32568f0b6d183a74d0f0c5f7a5 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Thu, 26 Dec 2024 13:49:26 +0100 Subject: [PATCH 2/6] Fix GH-16886: ini_parse_quantity() fails to emit warning for 0x+0 --- Zend/tests/zend_ini/gh16886.phpt | 41 ++++++++++++++++++++++++++++++++ Zend/zend_ini.c | 2 +- 2 files changed, 42 insertions(+), 1 deletion(-) create mode 100644 Zend/tests/zend_ini/gh16886.phpt diff --git a/Zend/tests/zend_ini/gh16886.phpt b/Zend/tests/zend_ini/gh16886.phpt new file mode 100644 index 00000000000..2e858945723 --- /dev/null +++ b/Zend/tests/zend_ini/gh16886.phpt @@ -0,0 +1,41 @@ +--TEST-- +GH-16886 (ini_parse_quantity() fails to emit warning for 0x+0) +--FILE-- + +--EXPECTF-- +Warning: Invalid quantity "0x 0": no digits after base prefix, interpreting as "0" for backwards compatibility in %s on line %d +0 + +Warning: Invalid quantity "0x+0": no digits after base prefix, interpreting as "0" for backwards compatibility in %s on line %d +0 + +Warning: Invalid quantity "0x-0": no digits after base prefix, interpreting as "0" for backwards compatibility in %s on line %d +0 + +Warning: Invalid quantity "0b 0": no digits after base prefix, interpreting as "0" for backwards compatibility in %s on line %d +0 + +Warning: Invalid quantity "0b+0": no digits after base prefix, interpreting as "0" for backwards compatibility in %s on line %d +0 + +Warning: Invalid quantity "0b-0": no digits after base prefix, interpreting as "0" for backwards compatibility in %s on line %d +0 + +Warning: Invalid quantity "0o 0": no digits after base prefix, interpreting as "0" for backwards compatibility in %s on line %d +0 + +Warning: Invalid quantity "0o+0": no digits after base prefix, interpreting as "0" for backwards compatibility in %s on line %d +0 + +Warning: Invalid quantity "0o-0": no digits after base prefix, interpreting as "0" for backwards compatibility in %s on line %d +0 diff --git a/Zend/zend_ini.c b/Zend/zend_ini.c index de37042047f..e521cf40d8e 100644 --- a/Zend/zend_ini.c +++ b/Zend/zend_ini.c @@ -598,7 +598,7 @@ static const char *zend_ini_consume_quantity_prefix(const char *const digits, co if (digits_consumed[0] == '0' && !isdigit(digits_consumed[1])) { /* Value is just 0 */ if ((digits_consumed+1) == str_end) { - return digits; + return digits_consumed; } switch (digits_consumed[1]) { From 7626e88de70fc0ed29873dd9d970b5688efc11f0 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Thu, 26 Dec 2024 14:16:35 +0100 Subject: [PATCH 3/6] Fix GH-16892: ini_parse_quantity() fails to parse inputs starting with 0x0b --- Zend/tests/zend_ini/gh16892.phpt | 22 ++++++++++++++++++++++ Zend/zend_ini.c | 11 ++++++++--- 2 files changed, 30 insertions(+), 3 deletions(-) create mode 100644 Zend/tests/zend_ini/gh16892.phpt diff --git a/Zend/tests/zend_ini/gh16892.phpt b/Zend/tests/zend_ini/gh16892.phpt new file mode 100644 index 00000000000..05cfe02192e --- /dev/null +++ b/Zend/tests/zend_ini/gh16892.phpt @@ -0,0 +1,22 @@ +--TEST-- +GH-16892 (ini_parse_quantity() fails to parse inputs starting with 0x0b) +--FILE-- + +--EXPECT-- +11 +11 +-11 +-11 +48879 +48879 +-48879 +-48879 diff --git a/Zend/zend_ini.c b/Zend/zend_ini.c index e521cf40d8e..f5d17807306 100644 --- a/Zend/zend_ini.c +++ b/Zend/zend_ini.c @@ -587,7 +587,7 @@ typedef enum { ZEND_INI_PARSE_QUANTITY_UNSIGNED, } zend_ini_parse_quantity_signed_result_t; -static const char *zend_ini_consume_quantity_prefix(const char *const digits, const char *const str_end) { +static const char *zend_ini_consume_quantity_prefix(const char *const digits, const char *const str_end, int base) { const char *digits_consumed = digits; /* Ignore leading whitespace. */ while (digits_consumed < str_end && zend_is_whitespace(*digits_consumed)) {++digits_consumed;} @@ -606,9 +606,14 @@ static const char *zend_ini_consume_quantity_prefix(const char *const digits, co case 'X': case 'o': case 'O': + digits_consumed += 2; + break; case 'b': case 'B': - digits_consumed += 2; + if (base != 16) { + /* 0b or 0B is valid in base 16, but not in the other supported bases. */ + digits_consumed += 2; + } break; } } @@ -696,7 +701,7 @@ static zend_ulong zend_ini_parse_quantity_internal(zend_string *value, zend_ini_ return 0; } digits += 2; - if (UNEXPECTED(digits == str_end || digits != zend_ini_consume_quantity_prefix(digits, str_end))) { + if (UNEXPECTED(digits == str_end || digits != zend_ini_consume_quantity_prefix(digits, str_end, base))) { /* Escape the string to avoid null bytes and to make non-printable chars * visible */ smart_str_append_escaped(&invalid, ZSTR_VAL(value), ZSTR_LEN(value)); From a2b820488015f37a03e139134538b10da12c3b5a Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Thu, 9 Jan 2025 19:52:11 +0100 Subject: [PATCH 4/6] Add comment Closes GH-17274. --- Zend/zend_ini.c | 1 + 1 file changed, 1 insertion(+) diff --git a/Zend/zend_ini.c b/Zend/zend_ini.c index f5d17807306..2ab476e25bd 100644 --- a/Zend/zend_ini.c +++ b/Zend/zend_ini.c @@ -701,6 +701,7 @@ static zend_ulong zend_ini_parse_quantity_internal(zend_string *value, zend_ini_ return 0; } digits += 2; + /* STRTOULL may silently ignore a prefix of whitespace, sign, and base prefix, which would be invalid at this position */ if (UNEXPECTED(digits == str_end || digits != zend_ini_consume_quantity_prefix(digits, str_end, base))) { /* Escape the string to avoid null bytes and to make non-printable chars * visible */ From bf4a776ee7a0f914ba0c87bc645ac77a01ce06fe Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Thu, 9 Jan 2025 19:53:23 +0100 Subject: [PATCH 5/6] NEWS --- NEWS | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/NEWS b/NEWS index 7c243ae9487..125c704b122 100644 --- a/NEWS +++ b/NEWS @@ -2,6 +2,12 @@ PHP NEWS ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||| ?? ??? ????, PHP 8.3.17 +- Core: + . Fixed bug GH-16892 (ini_parse_quantity() fails to parse inputs starting + with 0x0b). (nielsdos) + . Fixed bug GH-16886 (ini_parse_quantity() fails to emit warning for 0x+0). + (nielsdos) + - Enchant: . Fix crashes in enchant when passing null bytes. (nielsdos) From a2a7287b870cc21154905ff8a700cab2d7445a3f Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Thu, 9 Jan 2025 19:15:55 +0100 Subject: [PATCH 6/6] Fix GH-17409: Assertion failure Zend/zend_hash.c:1730 The array merging function may still hold the properties array while the object is already being destroyed. Therefore, we should take into account the refcount in simplexml's destruction code. It may be possible to trigger this in other ways too. Closes GH-17421. --- NEWS | 3 +++ ext/simplexml/simplexml.c | 4 ++-- ext/simplexml/tests/gh17409.phpt | 22 ++++++++++++++++++++++ 3 files changed, 27 insertions(+), 2 deletions(-) create mode 100644 ext/simplexml/tests/gh17409.phpt diff --git a/NEWS b/NEWS index 125c704b122..b1319961dc0 100644 --- a/NEWS +++ b/NEWS @@ -28,6 +28,9 @@ PHP NEWS - PHPDBG: . Fix crashes in function registration + test. (nielsdos, Girgias) +- SimpleXML: + . Fixed bug GH-17409 (Assertion failure Zend/zend_hash.c:1730). (nielsdos) + - SNMP: . Fixed bug GH-17330 (SNMP::setSecurity segfault on closed session). (David Carlier) diff --git a/ext/simplexml/simplexml.c b/ext/simplexml/simplexml.c index 00551267a45..18bfa31271e 100644 --- a/ext/simplexml/simplexml.c +++ b/ext/simplexml/simplexml.c @@ -2189,8 +2189,8 @@ static void sxe_object_free_storage(zend_object *object) sxe_object_free_iterxpath(sxe); if (sxe->properties) { - zend_hash_destroy(sxe->properties); - FREE_HASHTABLE(sxe->properties); + ZEND_ASSERT(!(GC_FLAGS(sxe->properties) & IS_ARRAY_IMMUTABLE)); + zend_hash_release(sxe->properties); } } /* }}} */ diff --git a/ext/simplexml/tests/gh17409.phpt b/ext/simplexml/tests/gh17409.phpt new file mode 100644 index 00000000000..f91ef38ba70 --- /dev/null +++ b/ext/simplexml/tests/gh17409.phpt @@ -0,0 +1,22 @@ +--TEST-- +GH-17409 (Assertion failure Zend/zend_hash.c) +--EXTENSIONS-- +simplexml +--CREDITS-- +YuanchengJiang +--FILE-- + + + + +'); +// Need to use $GLOBALS such that simplexml object is destroyed +var_dump(array_merge_recursive($GLOBALS, $GLOBALS)["root"]); +?> +--EXPECT-- +array(1) { + ["child"]=> + array(0) { + } +}