Skip to content

Incorrect validation of #[\Attribute]’s first parameter #13970

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
TimWolla opened this issue Apr 15, 2024 · 2 comments
Closed

Incorrect validation of #[\Attribute]’s first parameter #13970

TimWolla opened this issue Apr 15, 2024 · 2 comments
Labels
Bug Category: Engine Extension: reflection Status: Verified

Comments

@TimWolla
Copy link
Member

Description

The following code:

<?php
#[Attribute(Random\IntervalBoundary::ClosedOpen)]
class Foo {

}

#[Foo]
function test1() {

}

$reflection = new ReflectionFunction('test1');
var_dump($reflection->getAttributes()[0]->newInstance());
?>

Resulted in this output:

Fatal error: Uncaught Error: Attribute "Foo" cannot target function (allowed targets: ) in php-src/test.php:13
Stack trace:
#0 php-src/test.php(13): ReflectionAttribute->newInstance()
#1 {main}
  thrown in php-src/test.php on line 13
[Mon Apr 15 15:49:24 2024]  Script:  'php-src/test.php'
php-src/Zend/zend_objects.c(189) :  Freeing 0x000073dc9105c780 (56 bytes), script=php-src/test.php
=== Total 1 memory leaks detected ===

But I expected this output instead:

  1. No memory leak.
  2. Something related to the argument not being a correct integer.

PHP Version

git master

Operating System

Ubuntu 23.10
@TimWolla
Copy link
Member Author

Found while working on #11293.

@nielsdos
Copy link
Member

nielsdos commented Apr 15, 2024 •
edited
Loading

This turns out to be a regression, regressed in #8373.
In particular, we'll fail in zend_get_attribute_value that was called from validate_attribute to read the target.
That's because of this check:

php-src/Zend/zend_ast.c

Lines 807 to 811 in d407266

// 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)) {
return FAILURE;
}

Note that we are in compilation, so the check will be true and we'll fail to resolve the enum.
If I change that check to if (CG(compiler_options) & ZEND_COMPILE_PRELOAD) { it works, but I guess that'll break in preloading... I'll check.
EDIT: of course it does
EDIT 2: checking both compiler_options and in_compilation, like so if (CG(in_compilation) && CG(compiler_options) & ZEND_COMPILE_PRELOAD) { makes all ext/opcache tests pass, now checking preloading...
EDIT 3: as expected, doesn't work, probably the check should happen at a difference place?

nielsdos added a commit to nielsdos/php-src that referenced this issue Apr 15, 2024
@nielsdos
Fix phpGH-13970: Incorrect validation of #[\Attribute]’s first parameter
e734d76
nielsdos added a commit to nielsdos/php-src that referenced this issue Apr 15, 2024
@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 a pull request Apr 15, 2024 that will close this issue
Fix GH-13970: Incorrect validation of #[\Attribute]’s first parameter #13976
Closed
@TimWolla TimWolla mentioned this issue Apr 26, 2024
Merged
2 tasks
@iluuu1994 iluuu1994 mentioned this issue May 1, 2024
Closed
iluuu1994 added a commit to iluuu1994/php-src that referenced this issue May 4, 2024
@iluuu1994
Delay #[Attribute] arg validation until runtime
e382ab9 
Fixes phpGH-13970

We cannot validate at compile-time for multiple reasons:

* Evaluating the argument naively with zend_get_attribute_value can lead to code
  execution at compile time through the new expression, leading to possible
  reentrance of the compiler.
* Even if the evaluation was possible, it would need to be restricted to the
  current file, because constant values coming from other files can change
  without affecting the current compilation unit. For this reason, validation
  would need to be repeated at runtime anyway.
* Enums cannot be instantiated at compile-time (the actual bug report). This
  could be allowed here, because the value is immediately destroyed. But given
  the other issues, this won't be needed.

Instead, we just move it to runtime entirely. It's only needed for
ReflectionAttribute::newInstance(), which is not particularly a hot path. The
checks are also simple.
iluuu1994 added a commit to iluuu1994/php-src that referenced this issue May 4, 2024
@iluuu1994
Delay #[Attribute] arg validation until runtime
7236e5a 
Fixes phpGH-13970

We cannot validate at compile-time for multiple reasons:

* Evaluating the argument naively with zend_get_attribute_value can lead to code
  execution at compile time through the new expression, leading to possible
  reentrance of the compiler.
* Even if the evaluation was possible, it would need to be restricted to the
  current file, because constant values coming from other files can change
  without affecting the current compilation unit. For this reason, validation
  would need to be repeated at runtime anyway.
* Enums cannot be instantiated at compile-time (the actual bug report). This
  could be allowed here, because the value is immediately destroyed. But given
  the other issues, this won't be needed.

Instead, we just move it to runtime entirely. It's only needed for
ReflectionAttribute::newInstance(), which is not particularly a hot path. The
checks are also simple.
iluuu1994 added a commit to iluuu1994/php-src that referenced this issue May 4, 2024
@iluuu1994
Delay #[Attribute] arg validation until runtime
192a6ae 
Fixes phpGH-13970

We cannot validate at compile-time for multiple reasons:

* Evaluating the argument naively with zend_get_attribute_value can lead to code
  execution at compile time through the new expression, leading to possible
  reentrance of the compiler.
* Even if the evaluation was possible, it would need to be restricted to the
  current file, because constant values coming from other files can change
  without affecting the current compilation unit. For this reason, validation
  would need to be repeated at runtime anyway.
* Enums cannot be instantiated at compile-time (the actual bug report). This
  could be allowed here, because the value is immediately destroyed. But given
  the other issues, this won't be needed.

Instead, we just move it to runtime entirely. It's only needed for
ReflectionAttribute::newInstance(), which is not particularly a hot path. The
checks are also simple.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug Category: Engine Extension: reflection Status: Verified
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix GH-13970: Incorrect validation of #[\Attribute]’s first parameter nielsdos/php-src
2 participants
@TimWolla @nielsdos