8000 Zend: Fix reference counting for Closures in const-expr by TimWolla · Pull Request #17853 · php/php-src · GitHub
[go: up one dir, main page]
Skip to content

Zend: Fix reference counting for Closures in const-expr #17853

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged 8000
merged 2 commits into from
Mar 27, 2025
Merged

Zend: Fix reference counting for Closures in const-expr #17853

merged 2 commits into from
Mar 27, 2025

Conversation

TimWolla
Copy link
Member

Fixes #17851

@TimWolla TimWolla requested a review from iluuu1994 February 18, 2025 15:43
@TimWolla TimWolla marked this pull request as draft February 18, 2025 15:46
@TimWolla
Copy link
Member Author

Ah, this doesn't actually fix the issue: It's only reproducible without OPcache.

@TimWolla TimWolla force-pushed the attribute-autoload-uaf branch from 04f5a7c to 3c8493a Compare February 18, 2025 15:55
TimWolla
TimWolla commented Feb 18, 2025
View reviewed changes
iluuu1994
iluuu1994 reviewed Feb 19, 2025
View reviewed changes
Copy link
Member
@iluuu1994 iluuu1994 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

diff --git a/ext/opcache/zend_persist.c b/ext/opcache/zend_persist.c
index bf0b3d19881..87b55454b84 100644
--- a/ext/opcache/zend_persist.c
+++ b/ext/opcache/zend_persist.c
@@ -190,6 +190,9 @@ static zend_ast *zend_persist_ast(zend_ast *ast)
 		node = (zend_ast *) copy;
 	} else if (ast->kind == ZEND_AST_OP_ARRAY) {
 		zend_ast_op_array *copy = zend_shared_memdup(ast, sizeof(zend_ast_op_array));
+		/* We're holding a separate reference to the op_array in the AST. Release it
+		 * early because zend_persist_op_array() is destructive. */
+		destroy_op_array(copy->op_array);
 		zval z;
 		ZVAL_PTR(&z, copy->op_array);
 		zend_persist_op_array(&z);
diff --git a/ext/opcache/zend_persist_calc.c b/ext/opcache/zend_persist_calc.c
index 0c53923354c..5381f8f6e1c 100644
--- a/ext/opcache/zend_persist_calc.c
+++ b/ext/opcache/zend_persist_calc.c
@@ -91,6 +91,14 @@ static void zend_persist_ast_calc(zend_ast *ast)
 		zval z;
 		ZVAL_PTR(&z, zend_ast_get_op_array(ast)->op_array);
 		zend_persist_op_array_calc(&z);
+
+		/* If op_array is shared, the function name refcount is still incremented for each use,
+		 * so we need to release it here. We remembered the original function name in xlat. */
+		zend_string *old_function_name =
+			zend_shared_alloc_get_xlat_entry(&zend_ast_get_op_array(ast)->op_array->function_name);
+		if (old_function_name) {
+			zend_string_release_ex(old_function_name, 0);
+		}
 	} else if (zend_ast_is_decl(ast)) {
 		/* Not implemented. */
 		ZEND_UNREACHABLE();

This should fix the issue. It's not pretty but it works.

@TimWolla TimWolla force-pushed the attribute-autoload-uaf branch from ffd97c7 to 9a7cd28 Compare February 20, 2025 07:45
@TimWolla TimWolla marked this pull request as ready for review February 20, 2025 07:46
@TimWolla TimWolla requested a review from dstogov as a code owner February 20, 2025 07:46
dstogov
dstogov reviewed Feb 21, 2025
View reviewed changes
dstogov
dstogov reviewed Feb 21, 2025
View reviewed changes
withinboredom
withinboredom reviewed Feb 24, 2025
View reviewed changes
@TimWolla TimWolla force-pushed the attribute-autoload-uaf branch from 48c418e to bb252e4 Compare February 27, 2025 15:59
@iluuu1994
Copy link
Member
iluuu1994 commented Feb 27, 2025 •
edited
Loading

@TimWolla This example seems to leak:

const Closure = static function () {
    static $x = str_repeat('a', 10);
    echo "called", PHP_EOL;
};

var_dump(Closure);
(Closure)();

It might be because destroy_op_array destroys static_variables for dynamic_func_defs:

php-src/Zend/zend_opcode.c

Lines 641 to 645 in da1e254

if (op_array->dynamic_func_defs[i]->static_variables
&& (op_array->dynamic_func_defs[i]->fn_flags & ZEND_ACC_CLOSURE)) {
zend_array_destroy(op_array->dynamic_func_defs[i]->static_variables);
op_array->dynamic_func_defs[i]->static_variables = NULL;
}

However, for zend_closure_free_storage(), we have cloned static_variables and thus the original array leaks...

@TimWolla TimWolla force-pushed the attribute-autoload-uaf branch 2 times, most recently from 712fb01 to f54c7ce Compare March 7, 2025 11:41
@TimWolla TimWolla force-pushed the attribute-autoload-uaf branch from 323e899 to 2365ff7 Compare March 27, 2025 08:07
@TimWolla TimWolla merged commit 45d1acf into php:master Mar 27, 2025
9 checks passed
@TimWolla TimWolla deleted the attribute-autoload-uaf branch March 27, 2025 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@withinboredom withinboredom withinboredom left review comments

@dstogov dstogov Awaiting requested review from dstogov dstogov is a code owner

@iluuu1994 iluuu1994 Awaiting requested review from iluuu1994

Assignees
No one assigned
Labels
Category: Engine Extension: opcache
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Corrupted zend_mm_heap when using a closure in attribute
4 participants
@TimWolla @iluuu1994 @withinboredom @dstogov
0