Skip to content
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.

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

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

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
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Corrupted zend_mm_heap when using a closure in attribute
2 participants