Skip to content

Fix GH-13970: Incorrect validation of #[\Attribute]’s first parameter #13976

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
Closed
wants to merge 1 commit into from
Closed

Fix GH-13970: Incorrect validation of #[\Attribute]’s first parameter #13976

wants to merge 1 commit into from

Conversation

nielsdos
Copy link
Member

In c1959e6 code was added to deal with preloading of enums, which had to be special cased because they are object instances. In 4397811 this code was changed because array instances can contain object instances due to enums. In that commit, the code started using zval_update_constant_ex() and stopped ZEND_AST_CONST_ENUM_INIT resolution when in compilation.

Since the attribute in the testcase is resolved during compilation, the resolution fails as well, even though in this case we can resolve it fine as the problem is only with persisting constants during preloading.

This patch restores the original code but changes the IS_OBJECT condition to Z_COLLECTABLE such that both arrays and objects that are resolved from IS_CONSTANT_AST are not persisted to shm.

I don't know for sure if this is the best approach. An alternative I thought of is looping through the array to see if it contains objects but that seems more complicated and I'm not sure that's better.

@nielsdos
Fix phpGH-13970: Incorrect validation of #[\Attribute]’s first parameter
988bbe7 
In c1959e6 code was added to deal with preloading of enums, which had
to be special cased because they are object instances.
In 4397811 this code was changed because array instances can contain
object instances due to enums. In that commit, the code started using
zval_update_constant_ex() and stopped ZEND_AST_CONST_ENUM_INIT
resolution when in compilation.

Since the attribute in the testcase is resolved during compilation, the
resolution fails as well, even though in this case we can resolve it
fine as the problem is only with constants during preloading.

This patch restores the original code but changes the IS_OBJECT
condition to Z_COLLECTABLE such that both arrays and objects that are
resolved from IS_CONSTANT_AST are not persisted to shm.
@nielsdos nielsdos linked an issue Apr 15, 2024 that may be closed by this pull request
Incorrect validation of #[\Attribute]’s first parameter #13970
Closed
dstogov
dstogov reviewed Apr 15, 2024
View reviewed changes
Copy link
Member

@dstogov dstogov left a comment

Choose a reason for hiding this comment

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

The patch looks wrong to me (but I may be wrong as well).
@iluuu1994 please take a look

Zend/zend_ast.c
@@ -769,12 +769,6 @@ static zend_result ZEND_FASTCALL zend_ast_evaluate_ex(zval *result, zend_ast *as
break;
case ZEND_AST_CONST_ENUM_INIT:
{
// Preloading will attempt to resolve constants but objects can't be stored in shm
// Aborting here to store the const AST instead
if (CG(in_compilation)) {
Copy link
Member

Choose a reason for hiding this comment

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

May this should be replaced by if (CG(compiler_options) & ZEND_COMPILE_PRELOAD) {?

Copy link
Member Author

Choose a reason for hiding this comment

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

Replacing this with CG(in_compilation) && (CG(compiler_options) & ZEND_COMPILE_PRELOAD) fixes the issue when not in preloading, but the issue still remains when in preloading.

ext/opcache/ZendAccelerator.c
{
zval tmp;
ZVAL_COPY(&tmp, val);
if (zval_update_constant_ex(&tmp, scope) == FAILURE || Z_COLLECTABLE(tmp)) {
Copy link
Member

Choose a reason for hiding this comment

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

I don't see why all COLLECTABLE values can't be resolved.
This disables resolution of all constants which values are arrays.

Copy link
Member Author

Choose a reason for hiding this comment

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

Too conservative maybe yes, an alternative is looping (recursively) over the array and checking if they contain objects.

@iluuu1994
Copy link
Member

I believe the root cause is that attributes call validator only during compilation:

php-src/Zend/zend_compile.c

Line 6762 in c3acfb1

config->validator(attr, target, CG(active_class_entry));

But also allow skipping validation when arguments cannot be resolved:

php-src/Zend/zend_attributes.c

Lines 43 to 48 in c3acfb1

/* As this is run in the middle of compilation, fetch the attribute value without
* specifying a scope. The class is not fully linked yet, and we may seen an
* inconsistent state. */
if (FAILURE == zend_get_attribute_value(&flags, attr, 0, NULL)) {
return;
}

To solve this properly, we either need to:

  1. Repeat validation at runtime, when all dependencies have been resolved.
  2. Throw during compilation, when some dependencies cannot be resolved.

Otherwise, we may get unexpected non-int values in ReflectionAttribute::newInstance:

php-src/ext/reflection/php_reflection.c

Lines 6747 to 6751 in c3acfb1

if (FAILURE == zend_get_attribute_value(&tmp, marker, 0, ce)) {
RETURN_THROWS();
}
flags = (uint32_t) Z_LVAL(tmp);

@nielsdos
Copy link
Member Author

Your analysis is right, but I don't fully agree on the solution.
We can perfectly fine resolve the ast constant dependency to an enum object during compile time. The failure is caused by the patch for fixing preloading constants, which might've been too conservative.
Of course, as you say we can do with better error handling than just returning failure.

@iluuu1994
Copy link
Member

iluuu1994 commented Apr 16, 2024 •
edited
Loading

We can perfectly fine resolve the ast constant dependency to an enum object during compile time.

In this case that's true. But we can't always fully resolve the constant AST (e.g. when preloading, or compiling with opcache). In that case, we'll still run into the problem of either having to delay resolution of the constant AST until runtime, or erroring early. It seems the Attribute parameters already have some similar limitations. E.g. they will not actually trigger the autoloader (because the compiler is not re-entrant) which makes it impossible to use unloaded classes, or even global constants declared in the same file.

So, I agree that we may allow resolution of the AST immediately when opcache is not enabled, but that would just be an enhancement, rather than a standalone solution for this issue.

Maybe I'm missing something.

@nielsdos
Copy link
Member Author

nielsdos commented Apr 16, 2024 •
edited
Loading

I don't see why we can't resolve it in opcache or during preloading. It's not like we're storing the result. Could be just me.
Edit: I suppose maybe with some indirections via class constants this can happen.
Anyway, closing this then.

@nielsdos nielsdos closed this Apr 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dstogov dstogov dstogov left review comments

@iluuu1994 iluuu1994 Awaiting requested review from iluuu1994

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

Successfully merging this pull request may close these issues.

Incorrect validation of #[\Attribute]’s first parameter
3 participants
@nielsdos @iluuu1994 @dstogov