Delay #[Attribute] arg validation until runtime #14105
Conversation
|
I want to try this out myself (that's why I requested my review), but a quick note already: Don't forget to mention the issue in the commit message or PR for proper linking. |
I believe additions are ok in patches, but I will remove |
There was a problem hiding this comment.
What I wanted to test: Whether the placement of internal attributes is still validated, as that could have a memory-safety impact depending on what exactly they do.
Consider adding these test cases:
<?php
#[\Attribute("test")]
function test() {
}
and
<?php
#[\SensitiveParameter]
function test() {
}
There was a problem hiding this comment.
What I wanted to test: Whether the placement of internal attributes is still validated, as that could have a memory-safety impact depending on what exactly they do.
Placement of internal attributes are still checked at compile time. See:
Lines 6767 to 6774 in b6b9eab
That is, if you're not using a class alias. At that point, this guarantee goes out the window. However, from what I see, validator doesn't get a reference to the target anyway (which is very odd). validate_allow_dynamic_properties only works because it happens to only work for classes, and validator does get the scope.
Yes, I could confirm this myself with the two test cases I mentioned. |
e382ab9 to
7236e5a
Compare
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.
Fixes GH-13970
We cannot validate at compile-time for multiple reasons:
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.