From 23ffbd920b59a717d63a1626ed02bb9d0eb11629 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Sun, 4 Dec 2022 21:59:18 +0100 Subject: [PATCH 1/7] Fix #97836: Segfault in concat_function The following sequence of actions was happening which caused a null pointer dereference: 1. debug_backtrace() returns an array 2. The concatenation to $c will transform the array to a string via `zval_get_string_func` for op2 and output a warning. Note that zval op1 is of type string due to the first do-while sequence. 3. The warning of an implicit "array to string conversion" triggers the ob_start callback to run. This code transform $c (==op1) to a long. 4. The code below the 2 do-while sequences assume that both op1 and op2 are strings, but this is no longer the case. A dereference of the string will therefore result in a null pointer dereference. The solution used here is to work with the zend_string directly instead of with the ops. --- Zend/zend_operators.c | 123 ++++++++++++++++++++++++++---------------- 1 file changed, 78 insertions(+), 45 deletions(-) diff --git a/Zend/zend_operators.c b/Zend/zend_operators.c index a9932a6b592b6..3074bf7e49949 100644 --- a/Zend/zend_operators.c +++ b/Zend/zend_operators.c @@ -1939,109 +1939,142 @@ ZEND_API zend_result ZEND_FASTCALL shift_right_function(zval *result, zval *op1, ZEND_API zend_result ZEND_FASTCALL concat_function(zval *result, zval *op1, zval *op2) /* {{{ */ { - zval *orig_op1 = op1; - zval op1_copy, op2_copy; - - ZVAL_UNDEF(&op1_copy); - ZVAL_UNDEF(&op2_copy); + zval *orig_op1 = op1; + zend_string *op1_string, *op2_string = NULL; + bool free_op1_string = false; + bool free_op2_string = false; do { - if (UNEXPECTED(Z_TYPE_P(op1) != IS_STRING)) { + if (EXPECTED(Z_TYPE_P(op1) == IS_STRING)) { + op1_string = Z_STR_P(op1); + } else { if (Z_ISREF_P(op1)) { op1 = Z_REFVAL_P(op1); - if (Z_TYPE_P(op1) == IS_STRING) break; + if (Z_TYPE_P(op1) == IS_STRING) { + op1_string = Z_STR_P(op1); + break; + } } ZEND_TRY_BINARY_OBJECT_OPERATION(ZEND_CONCAT); - ZVAL_STR(&op1_copy, zval_get_string_func(op1)); + op1_string = zval_get_string_func(op1); if (UNEXPECTED(EG(exception))) { - zval_ptr_dtor_str(&op1_copy); + zend_string_release(op1_string); if (orig_op1 != result) { ZVAL_UNDEF(result); } return FAILURE; } + free_op1_string = true; if (result == op1) { if (UNEXPECTED(op1 == op2)) { - op2 = &op1_copy; + op2_string = op1_string; } } - op1 = &op1_copy; } } while (0); do { - if (UNEXPECTED(Z_TYPE_P(op2) != IS_STRING)) { - if (Z_ISREF_P(op2)) { - op2 = Z_REFVAL_P(op2); - if (Z_TYPE_P(op2) == IS_STRING) break; - } - ZEND_TRY_BINARY_OP2_OBJECT_OPERATION(ZEND_CONCAT); - ZVAL_STR(&op2_copy, zval_get_string_func(op2)); - if (UNEXPECTED(EG(exception))) { - zval_ptr_dtor_str(&op1_copy); - zval_ptr_dtor_str(&op2_copy); - if (orig_op1 != result) { - ZVAL_UNDEF(result); + if (!op2_string) { + if (EXPECTED(Z_TYPE_P(op2) == IS_STRING)) { + op2_string = Z_STR_P(op2); + } else { + if (Z_ISREF_P(op2)) { + op2 = Z_REFVAL_P(op2); + if (Z_TYPE_P(op2) == IS_STRING) { + op2_string = Z_STR_P(op2); + break; + } } - return FAILURE; + /* hold an additional reference because a userland function could free this */ + if (!free_op1_string) { + op1_string = zend_string_copy(op1_string); + free_op1_string = true; + } + ZEND_TRY_BINARY_OP2_OBJECT_OPERATION(ZEND_CONCAT); + op2_string = zval_get_string_func(op2); + if (UNEXPECTED(EG(exception))) { + zend_string_release(op1_string); + zend_string_release(op2_string); + if (orig_op1 != result) { + ZVAL_UNDEF(result); + } + return FAILURE; + } + free_op2_string = true; } - op2 = &op2_copy; } } while (0); - if (UNEXPECTED(Z_STRLEN_P(op1) == 0)) { - if (EXPECTED(result != op2)) { + if (UNEXPECTED(ZSTR_LEN(op1_string) == 0)) { + if (EXPECTED(free_op2_string || result != op2)) { if (result == orig_op1) { i_zval_ptr_dtor(result); } - ZVAL_COPY(result, op2); + if (free_op2_string) { + ZVAL_STR(result, op2_string); + free_op2_string = false; + } else { + ZVAL_STR_COPY(result, op2_string); + } } - } else if (UNEXPECTED(Z_STRLEN_P(op2) == 0)) { - if (EXPECTED(result != op1)) { + } else if (UNEXPECTED(ZSTR_LEN(op2_string) == 0)) { + if (EXPECTED(free_op1_string || result != op1)) { if (result == orig_op1) { i_zval_ptr_dtor(result); } - ZVAL_COPY(result, op1); + if (free_op1_string) { + ZVAL_STR(result, op1_string); + free_op1_string = false; + } else { + ZVAL_STR_COPY(result, op1_string); + } } } else { - size_t op1_len = Z_STRLEN_P(op1); - size_t op2_len = Z_STRLEN_P(op2); + size_t op1_len = ZSTR_LEN(op1_string); + size_t op2_len = ZSTR_LEN(op2_string); size_t result_len = op1_len + op2_len; zend_string *result_str; uint32_t flags = ZSTR_GET_COPYABLE_CONCAT_PROPERTIES_BOTH(Z_STR_P(op1), Z_STR_P(op2)); if (UNEXPECTED(op1_len > ZSTR_MAX_LEN - op2_len)) { + if (free_op1_string) zend_string_release(op1_string); + if (free_op2_string) zend_string_release(op2_string); zend_throw_error(NULL, "String size overflow"); - zval_ptr_dtor_str(&op1_copy); - zval_ptr_dtor_str(&op2_copy); if (orig_op1 != result) { ZVAL_UNDEF(result); } return FAILURE; } - if (result == op1 && Z_REFCOUNTED_P(result)) { + if (result == op1) { + if (free_op1_string) { + i_zval_ptr_dtor(result); + free_op1_string = false; + } /* special case, perform operations on result */ - result_str = zend_string_extend(Z_STR_P(result), result_len, 0); + result_str = zend_string_extend(op1_string, result_len, 0); + /* account for the case where result_str == op1_string == op2_string and the realloc is done */ + if (op1_string == op2_string) { + if (free_op2_string) zend_string_release(op2_string); + op2_string = result_str; + free_op2_string = false; + } } else { result_str = zend_string_alloc(result_len, 0); - memcpy(ZSTR_VAL(result_str), Z_STRVAL_P(op1), op1_len); + memcpy(ZSTR_VAL(result_str), ZSTR_VAL(op1_string), op1_len); if (result == orig_op1) { i_zval_ptr_dtor(result); } } GC_ADD_FLAGS(result_str, flags); - /* This has to happen first to account for the cases where result == op1 == op2 and - * the realloc is done. In this case this line will also update Z_STRVAL_P(op2) to - * point to the new string. The first op2_len bytes of result will still be the same. */ ZVAL_NEW_STR(result, result_str); - - memcpy(ZSTR_VAL(result_str) + op1_len, Z_STRVAL_P(op2), op2_len); + memcpy(ZSTR_VAL(result_str) + op1_len, ZSTR_VAL(op2_string), op2_len); ZSTR_VAL(result_str)[result_len] = '\0'; } - zval_ptr_dtor_str(&op1_copy); - zval_ptr_dtor_str(&op2_copy); + if (free_op1_string) zend_string_release(op1_string); + if (free_op2_string) zend_string_release(op2_string); + return SUCCESS; } /* }}} */ From 8ab19f75244d759022baaaf0755242ad161d853e Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Wed, 7 Dec 2022 11:03:52 +0100 Subject: [PATCH 2/7] Add regression tests for bug #79836 Co-authored-by: changochen1@gmail.com --- Zend/tests/bug79836.phpt | 18 ++++++++++++++++++ Zend/tests/bug79836_1.phpt | 18 ++++++++++++++++++ Zend/tests/bug79836_2.phpt | 25 +++++++++++++++++++++++++ 3 files changed, 61 insertions(+) create mode 100644 Zend/tests/bug79836.phpt create mode 100644 Zend/tests/bug79836_1.phpt create mode 100644 Zend/tests/bug79836_2.phpt diff --git a/Zend/tests/bug79836.phpt b/Zend/tests/bug79836.phpt new file mode 100644 index 0000000000000..5fb07396762f5 --- /dev/null +++ b/Zend/tests/bug79836.phpt @@ -0,0 +1,18 @@ +--TEST-- +Bug #79836 (Segfault in concat_function) +--INI-- +opcache.optimization_level = 0x7FFEBFFF & ~0x400 +--FILE-- + +--EXPECT-- +3 diff --git a/Zend/tests/bug79836_1.phpt b/Zend/tests/bug79836_1.phpt new file mode 100644 index 0000000000000..86e7f47671849 --- /dev/null +++ b/Zend/tests/bug79836_1.phpt @@ -0,0 +1,18 @@ +--TEST-- +Bug #79836 (Segfault in concat_function) +--INI-- +opcache.optimization_level = 0x7FFEBFFF & ~0x400 +--FILE-- + +--EXPECT-- +Done diff --git a/Zend/tests/bug79836_2.phpt b/Zend/tests/bug79836_2.phpt new file mode 100644 index 0000000000000..b02fcc13ea11b --- /dev/null +++ b/Zend/tests/bug79836_2.phpt @@ -0,0 +1,25 @@ +--TEST-- +Bug #79836 (Segfault in concat_function) +--FILE-- + +--EXPECT-- +abcdabcdabcdabcdabcdabcdabcdabcdabcdabcdabc From b9beac5f6a36d408eb4097ac5232a71e5da77515 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Thu, 8 Dec 2022 20:41:41 +0100 Subject: [PATCH 3/7] Add regression test for bug #81705 Co-authored-by: cmbecker69@gmx.de Co-authored-by: yukik@risec.co.jp --- Zend/tests/bug81705.phpt | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) create mode 100644 Zend/tests/bug81705.phpt diff --git a/Zend/tests/bug81705.phpt b/Zend/tests/bug81705.phpt new file mode 100644 index 0000000000000..1c00b1c77d4bb --- /dev/null +++ b/Zend/tests/bug81705.phpt @@ -0,0 +1,19 @@ +--TEST-- +Bug #81705 (type confusion/UAF on set_error_handler with concat operation) +--FILE-- + +--EXPECT-- +error +string(6) "aArray" \ No newline at end of file From 4434e87829ae090da47ff5309bfd121f0a2f64e4 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Wed, 7 Dec 2022 11:04:30 +0100 Subject: [PATCH 4/7] Add an additional test for concat_function for interned strings This tests the path op1_string_from_func == true, result == op1, op1 == op2 in the do-while, which wasn't tested before. --- .../tests/class_toString_concat_with_itself.phpt | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) create mode 100644 Zend/tests/class_toString_concat_with_itself.phpt diff --git a/Zend/tests/class_toString_concat_with_itself.phpt b/Zend/tests/class_toString_concat_with_itself.phpt new file mode 100644 index 0000000000000..96d28679b2f93 --- /dev/null +++ b/Zend/tests/class_toString_concat_with_itself.phpt @@ -0,0 +1,16 @@ +--TEST-- +Test concatenating a class instance that has __toString with itself +--FILE-- + +--EXPECT-- +abcabc From 73526db233bb2bda1b85a340c406d2e0a4f6962a Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Sat, 10 Dec 2022 00:44:37 +0100 Subject: [PATCH 5/7] Add an additional test for concat_function for non-interned strings This tests for the case where result == op1, op1 != op2 and op1_string and op2_string are non-interned strings that are equal. --- ...tring_concat_non_interned_with_itself.phpt | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+) create mode 100644 Zend/tests/class_toString_concat_non_interned_with_itself.phpt diff --git a/Zend/tests/class_toString_concat_non_interned_with_itself.phpt b/Zend/tests/class_toString_concat_non_interned_with_itself.phpt new file mode 100644 index 0000000000000..87b129ce9e796 --- /dev/null +++ b/Zend/tests/class_toString_concat_non_interned_with_itself.phpt @@ -0,0 +1,21 @@ +--TEST-- +Test concatenating a class instance that has __toString with itself that uses a non-interned string +--FILE-- + +--EXPECT-- +aaaaaa From 41d2524e98ff7fac4432a5160ea6b252ea37f8f0 Mon Sep 17 00:00:00 2001 From: nielsdos <7771979+nielsdos@users.noreply.github.com> Date: Fri, 12 May 2023 22:33:10 +0200 Subject: [PATCH 6/7] Fix after rebase --- Zend/zend_operators.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Zend/zend_operators.c b/Zend/zend_operators.c index 3074bf7e49949..a2dba169dd959 100644 --- a/Zend/zend_operators.c +++ b/Zend/zend_operators.c @@ -2033,7 +2033,7 @@ ZEND_API zend_result ZEND_FASTCALL concat_function(zval *result, zval *op1, zval size_t op2_len = ZSTR_LEN(op2_string); size_t result_len = op1_len + op2_len; zend_string *result_str; - uint32_t flags = ZSTR_GET_COPYABLE_CONCAT_PROPERTIES_BOTH(Z_STR_P(op1), Z_STR_P(op2)); + uint32_t flags = ZSTR_GET_COPYABLE_CONCAT_PROPERTIES_BOTH(op1_string, op2_string); if (UNEXPECTED(op1_len > ZSTR_MAX_LEN - op2_len)) { if (free_op1_string) zend_string_release(op1_string); From c6a6189f0956c2e329ad8f9d9061c7e10e1f2b48 Mon Sep 17 00:00:00 2001 From: nielsdos <7771979+nielsdos@users.noreply.github.com> Date: Sat, 13 May 2023 16:22:21 +0200 Subject: [PATCH 7/7] Simplify and comment --- Zend/zend_operators.c | 61 +++++++++++++++++++++++-------------------- 1 file changed, 33 insertions(+), 28 deletions(-) diff --git a/Zend/zend_operators.c b/Zend/zend_operators.c index a2dba169dd959..26f09495db8e8 100644 --- a/Zend/zend_operators.c +++ b/Zend/zend_operators.c @@ -1940,7 +1940,7 @@ ZEND_API zend_result ZEND_FASTCALL shift_right_function(zval *result, zval *op1, ZEND_API zend_result ZEND_FASTCALL concat_function(zval *result, zval *op1, zval *op2) /* {{{ */ { zval *orig_op1 = op1; - zend_string *op1_string, *op2_string = NULL; + zend_string *op1_string, *op2_string; bool free_op1_string = false; bool free_op2_string = false; @@ -1968,48 +1968,49 @@ ZEND_API zend_result ZEND_FASTCALL concat_function(zval *result, zval *op1, zval if (result == op1) { if (UNEXPECTED(op1 == op2)) { op2_string = op1_string; + goto has_op2_string; } } } } while (0); do { - if (!op2_string) { - if (EXPECTED(Z_TYPE_P(op2) == IS_STRING)) { - op2_string = Z_STR_P(op2); - } else { - if (Z_ISREF_P(op2)) { - op2 = Z_REFVAL_P(op2); - if (Z_TYPE_P(op2) == IS_STRING) { - op2_string = Z_STR_P(op2); - break; - } - } - /* hold an additional reference because a userland function could free this */ - if (!free_op1_string) { - op1_string = zend_string_copy(op1_string); - free_op1_string = true; + if (EXPECTED(Z_TYPE_P(op2) == IS_STRING)) { + op2_string = Z_STR_P(op2); + } else { + if (Z_ISREF_P(op2)) { + op2 = Z_REFVAL_P(op2); + if (Z_TYPE_P(op2) == IS_STRING) { + op2_string = Z_STR_P(op2); + break; } - ZEND_TRY_BINARY_OP2_OBJECT_OPERATION(ZEND_CONCAT); - op2_string = zval_get_string_func(op2); - if (UNEXPECTED(EG(exception))) { - zend_string_release(op1_string); - zend_string_release(op2_string); - if (orig_op1 != result) { - ZVAL_UNDEF(result); - } - return FAILURE; + } + /* hold an additional reference because a userland function could free this */ + if (!free_op1_string) { + op1_string = zend_string_copy(op1_string); + free_op1_string = true; + } + ZEND_TRY_BINARY_OP2_OBJECT_OPERATION(ZEND_CONCAT); + op2_string = zval_get_string_func(op2); + if (UNEXPECTED(EG(exception))) { + zend_string_release(op1_string); + zend_string_release(op2_string); + if (orig_op1 != result) { + ZVAL_UNDEF(result); } - free_op2_string = true; + return FAILURE; } + free_op2_string = true; } } while (0); +has_op2_string:; if (UNEXPECTED(ZSTR_LEN(op1_string) == 0)) { if (EXPECTED(free_op2_string || result != op2)) { if (result == orig_op1) { i_zval_ptr_dtor(result); } if (free_op2_string) { + /* transfer ownership of op2_string */ ZVAL_STR(result, op2_string); free_op2_string = false; } else { @@ -2022,6 +2023,7 @@ ZEND_API zend_result ZEND_FASTCALL concat_function(zval *result, zval *op1, zval i_zval_ptr_dtor(result); } if (free_op1_string) { + /* transfer ownership of op1_string */ ZVAL_STR(result, op1_string); free_op1_string = false; } else { @@ -2047,6 +2049,7 @@ ZEND_API zend_result ZEND_FASTCALL concat_function(zval *result, zval *op1, zval if (result == op1) { if (free_op1_string) { + /* op1_string will be used as the result, so we should not free it */ i_zval_ptr_dtor(result); free_op1_string = false; } @@ -2054,9 +2057,11 @@ ZEND_API zend_result ZEND_FASTCALL concat_function(zval *result, zval *op1, zval result_str = zend_string_extend(op1_string, result_len, 0); /* account for the case where result_str == op1_string == op2_string and the realloc is done */ if (op1_string == op2_string) { - if (free_op2_string) zend_string_release(op2_string); + if (free_op2_string) { + zend_string_release(op2_string); + free_op2_string = false; + } op2_string = result_str; - free_op2_string = false; } } else { result_str = zend_string_alloc(result_len, 0);