There are going to be some memory issues that CI will report, I couldn't figure those out, asked for help on the mailing list, https://news-web.php.net/php.internals/126065

So there are also opcache failures, not just the failures I had locally - I guess a data op isn't the right way to send a pointer to the attributes from the compilation to the runtime - I'll see if I can have it send the raw AST and delay the attribute compilation until runtime

So for the life of me, I can't figure out why I'm unable to get the cleanup in free_zend_constant() to run properly - I added debugging code

diff --git a/Zend/zend_constants.c b/Zend/zend_constants.c
index 8b92650816..ade2efb618 100644
--- a/Zend/zend_constants.c
+++ b/Zend/zend_constants.c
@@ -41,6 +41,8 @@ void free_zend_constant(zval *zv)
 {
        zend_constant *c = Z_PTR_P(zv);
 
+       fprintf(stderr, "Freeing constant %s\n", ZSTR_VAL(c->name));
+
        if (!(ZEND_CONSTANT_FLAGS(c) & CONST_PERSISTENT)) {
                zval_ptr_dtor_nogc(&c->value);
                if (c->name) {
@@ -50,7 +52,7 @@ void free_zend_constant(zval *zv)
                        zend_string_release_ex(c->filename, 0);
                }

and none of the compile-time constants in my test script are listed as being freed, though plenty of php-provided ones are. If the cleanup is never reached, then it makes sense that the attributes are not properly freed, but why isn't the cleanup reached?

@DanielEScherzer

From f411ddf4c984ed8d614d003e9eb209387b904f9b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Tim=20D=C3=BCsterhus?= <tim@tideways-gmbh.com>
Date: Wed, 27 Nov 2024 17:32:48 +0100
Subject: [PATCH] Fix constant attribute leak

---
 Zend/zend_execute_API.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Zend/zend_execute_API.c b/Zend/zend_execute_API.c
index 9ebc15f3a4..f4adeb855f 100644
--- a/Zend/zend_execute_API.c
+++ b/Zend/zend_execute_API.c
@@ -302,6 +302,9 @@ ZEND_API void zend_shutdown_executor_values(bool fast_shutdown)
                                if (c->filename) {
                                        zend_string_release_ex(c->filename, 0);
                                }
+                               if (c->attributes) {
+                                       zend_hash_release(c->attributes);
+                               }
                                efree(c);
                                zend_string_release_ex(key, 0);
                        } ZEND_HASH_MAP_FOREACH_END_DEL();
-- 
2.43.0

This appears to fix the leak for me. I did not verify if it is possible to call free_zend_constant in that location. If it is, that should probably be a separate PR.

@DanielEScherzer

[patch omitted for brevity]

This appears to fix the leak for me. I did not verify if it is possible to call free_zend_constant in that location. If it is, that should probably be a separate PR.

Thanks - I forgot there was a second place that constants are freed, though I should have remembered it from adding ReflectionConstant::getFileName()

So on circleci the deprecation messages don't seem to be properly found, and on windows there are failures with exit code 1073741819, but only for the Reflection tests

So on circleci the deprecation messages don't seem to be properly found

@DanielEScherzer This is not related to CircleCI / ARM, but to JIT. You should be able to reproduce the issue locally with:

sapi/cli/php run-tests.php --asan --show-diff -P -q -d zend_extension=$(pwd)/modules/opcache.so -d opcache.enable_cli=1 -d opcache.jit=tracing -d opcache.protect_memory=1 --repeat 2 Zend/tests/attributes/deprecated/constants/const_messages.phpt

see: #11293 (comment)

I have taken the liberty to push a fix into your PR.

I can reproduce issues for the 3 tests that fail on Windows by using a non-debug ZTS build and running with Valgrind.

Specifically when I configure as follows:

./configure --enable-zend-test --enable-option-checking=fatal --enable-phpdbg --enable-fpm --enable-werror --enable-zts

And run the tests as follows:

sapi/cli/php run-tests.php -m --show-diff Zend/tests/attributes/001_placement.phpt ext/reflection/tests/ReflectionConstant_getAttributes.phpt ext/reflection/tests/ReflectionConstant_isDeprecated_userland.phpt

Running bash Zend/tests/attributes/001_placement.sh valgrind after the failed test then reveals:

==810251== Conditional jump or move depends on uninitialised value(s)
==810251==    at 0x6A1A9A: zend_resolve_class_name (zend_compile.c:1175)
==810251==    by 0x6A5A44: zend_resolve_class_name_ast (zend_compile.c:1209)
==810251==    by 0x6A5A44: zend_compile_attributes (zend_compile.c:7374)
==810251==    by 0x6B8D31: zend_constant_add_attributes (zend_constants.c:551)
==810251==    by 0x6DA516: ZEND_DECLARE_ATTRIBUTED_CONST_SPEC_CONST_CONST_HANDLER (zend_vm_execute.h:8031)
==810251==    by 0x713424: execute_ex (zend_vm_execute.h:59663)
==810251==    by 0x71DDD8: zend_execute (zend_vm_execute.h:64291)
==810251==    by 0x77FC0B: zend_execute_script (zend.c:1934)
==810251==    by 0x611626: php_execute_script_ex (main.c:2577)
==810251==    by 0x7819E2: do_cli (php_cli.c:938)
==810251==    by 0x3538B2: main (php_cli.c:1313)
==810251== 
==810251== Conditional jump or move depends on uninitialised value(s)
==810251==    at 0x69DF30: zend_prefix_with_ns (zend_compile.c:1062)
==810251==    by 0x6A5A44: zend_resolve_class_name_ast (zend_compile.c:1209)
==810251==    by 0x6A5A44: zend_compile_attributes (zend_compile.c:7374)
==810251==    by 0x6B8D31: zend_constant_add_attributes (zend_constants.c:551)
==810251==    by 0x6DA516: ZEND_DECLARE_ATTRIBUTED_CONST_SPEC_CONST_CONST_HANDLER (zend_vm_execute.h:8031)
==810251==    by 0x713424: execute_ex (zend_vm_execute.h:59663)
==810251==    by 0x71DDD8: zend_execute (zend_vm_execute.h:64291)
==810251==    by 0x77FC0B: zend_execute_script (zend.c:1934)
==810251==    by 0x611626: php_execute_script_ex (main.c:2577)
==810251==    by 0x7819E2: do_cli (php_cli.c:938)
==810251==    by 0x3538B2: main (php_cli.c:1313)

RFC filed: https://wiki.php.net/rfc/attributes-on-constants
Discussion thread: https://news-web.php.net/php.internals/126084

Rebased to update for #17101 fix, then squashed and added UPGRADING

Fixed conflict from #17306

And in GDB:

Hint: You can easily spawn a single (failed) test in gdb by doing: bash Zend/tests/attributes/constants/target_all_targets_const-explicit.sh gdb (i.e. replace phpt by sh and append the gdb argument).

You're missing the equivalent in zend_file_cache_unserialize_zval(). It also doesn't seem correct, you would actually need to call SERIALIZE_ATTRIBUTES on that pointer. The code should look similar to ext/opcache/zend_persist.c (or ext/opcache/zend_persist_calc.c if there's no existing loop over the opcodes). Another execution of the tests with --file-cache-use should reveal those issues.

You're missing the equivalent in zend_file_cache_unserialize_zval(). It also doesn't seem correct, you would actually need to call SERIALIZE_ATTRIBUTES on that pointer. The code should look similar to ext/opcache/zend_persist.c (or ext/opcache/zend_persist_calc.c if there's no existing loop over the opcodes). Another execution of the tests with --file-cache-use should reveal those issues.

Tested with --file-cache-use now too, thanks for bearing with me

No worries, this part of the code is not easy to understand. I would prefer if not all IS_PTR zvals would assumed to be attributes, like we do in zend_persist_op_array_ex(). If that's more difficult for some reason, this is fine too.

No worries, this part of the code is not easy to understand. I would prefer if not all IS_PTR zvals would assumed to be attributes, like we do in zend_persist_op_array_ex(). If that's more difficult for some reason, this is fine too.

I had poked around in GDB and didn't see any way to tell that it was an attribute (or not) but given that so far there was nothing that would be IS_PTR it seems that at least for now the only thing that can be IS_PTR is attributes.

--
FREEBSD failure seems unrelated, and I don't understand the CircleCI since the tests are passing on the other checks and that code shouldn't have changed

I had poked around in GDB and didn't see any way to tell that it was an attribute (or not) but given that so far there was nothing that would be IS_PTR it seems that at least for now the only thing that can be IS_PTR is attributes.

We can't tell from the zval alone, see how zend_persist_op_array_ex() checks for the given opcode and handles it specially. It looks like we're already looping through opcodes in zend_file_cache_serialize_op_array(), so adding a similar check there shouldn't cost too much.

I had poked around in GDB and didn't see any way to tell that it was an attribute (or not) but given that so far there was nothing that would be IS_PTR it seems that at least for now the only thing that can be IS_PTR is attributes.

We can't tell from the zval alone, see how zend_persist_op_array_ex() checks for the given opcode and handles it specially. It looks like we're already looping through opcodes in zend_file_cache_serialize_op_array(), so adding a similar check there shouldn't cost too much.

Except that here the values are stored in op_array->literals and that stores all of the literals, not just the attributes. For the target_all_targets_const-explicit.phpt test, we have op_array->last_literal being 12, with the literals (op_array->literals[0]):

• [0] - the string EXAMPLE
• [1] - the string FOO
• [2] - the attributes pointer
• [3] - the string ReflectionConstant
• [4] - lowercase string reflectionconstant
• [5] - the string getAttributes
• [6] - lowercase string getattributes
• [7] - the string var_dump
• [8] - the lval 0
• [9] - the string newInstance
• [10] - lowercase string newinstance
• [11] - the lval 1

So we would need to somehow add that information to the parsing of ->literals

Any objections to merging this as is? (I'll update UPGRADING, rebase, and squash)

Except that here the values are stored in op_array->literals and that stores all of the literals, not just the attributes.

The same goes for persist. See how it's done there. The zval persisting skips the IS_PTR, the pointer is then handled when looping over the oparray, because only then we know what the content of the pointer actually is.

Except that here the values are stored in op_array->literals and that stores all of the literals, not just the attributes.

The same goes for persist. See how it's done there. The zval persisting skips the IS_PTR, the pointer is then handled when looping over the oparray, because only then we know what the content of the pointer actually is.

I assumed that the point of this was to ensure that we didn't just assume that all IS_PTR zvals were pointing to attributes, but even if we come back after looping through the oparray, how do we know that any given literal is associated with the declaration of attributes? It seems we would just move the assumption

Until the assumption is incorrect, is there any harm in leaving this as-is?

From ext/opcache/zend_persist.c:

if (opline->opcode == ZEND_OP_DATA && (opline-1)->opcode == ZEND_DECLARE_ATTRIBUTED_CONST) {
	zval *literal = RT_CONSTANT(opline, opline->op1);
	HashTable *attributes = Z_PTR_P(literal);
	attributes = zend_persist_attributes(attributes);
	ZVAL_PTR(literal, attributes);
}

This changes the assumption from "all IS_PTR literals are attributes" to "DECLARE_ATTRIBUTED_CONST OP_DATA literals are attributes". That's quite a different assumption.

Until the assumption is incorrect, is there any harm in leaving this as-is?

Then, the question is, why do we make the assumption only half the time (i.e. not persist and persist_calc)? If that's really the preferred way, then it should at least be the same everywhere.

if (opline->opcode == ZEND_OP_DATA && (opline-1)->opcode == ZEND_DECLARE_ATTRIBUTED_CONST) {
	zval *literal = RT_CONSTANT(opline, opline->op1);
	HashTable *attributes = Z_PTR_P(literal);
	attributes = zend_persist_attributes(attributes);
	ZVAL_PTR(literal, attributes);
}

I didn't realize I could also access the literal from there, that makes more sense, will do

Oh, just as i approved I saw the arm failure. This look related, no?

I spent way too long to figure out that the issue was that in ext/opcache/jit/zend_jit_vm_helpers.c instead of calling CONST_UNPROTECT_RECURSION() to release the recursion I was just calling CONST_UNPROTECT_RECURSION() a second time - facepalm

Tests now all pass
If there is no further review I'll merge this tomorrow

This introduced a memory leak because now the parser accepts invalid code: it's now possible to define constants inside functions (out of the global scope). See also: https://issues.oss-fuzz.com/issues/416302790
Or cleaned up reproducer:

<?php
function x(){
    #[Attr] const X = 1;
}

Previously you would've errored with Parse error: syntax error, unexpected token "const" in /in/8uNTK on line 4 but now it silently accepts this code (and therefore there is a memory leak). This should be fixed by disallowing this at the grammar level.

I think it can be fixed by making a rule attributed_top_statement in the parser, I'll take a quick look. (#18537)