Zend: Fix reference counting for Closures in const-expr

[TimWolla]

Fixes #17851

[TimWolla]

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

[iluuu1994]

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.

[dstogov]

I didn't understand this why you release something in zend_persist_ast_calc().
Will this work if after zend_persist_calc() we will decide not persist the script (e.g. because of OOM)?

[iluuu1994]

The name is released in calc because it is also interned in calc. We cannot release it in persist because then the original reference is gone.

[dstogov]

This looks strange. You first destroy and then use. May be you should use the copy.

[iluuu1994]

The comment above should explain it. Persist assumes rc=1 and partially destroys the op_array. After that point, we can't call destroy_op_array() anymore. We do it before, but we know a reference will always be remaining.

[TimWolla]

Persist assumes rc=1

Should we add an assertion verifying that?

[iluuu1994]

Sure, that would make sense.

[TimWolla]

Looking at destroy_op_array and zend_persist_op_array_ex(), perhaps an alternative and possibly cleaner fix would be not handling the refcount of the function_name separately from the refcount of the op_array: No matter how many references there are to the op_array, there is only one op_array holding onto the function_name and thus there should only be 1 reference added for the use in that op_array, no?

[iluuu1994]

In this case, yes. But it's also used for traits, where we have different op_arrays pointing to the same opcodes arrays. You can probably use your own function that only increments refcount, but then you also can't use destroy_op_array() because it decrements the function name RC.