Skip to content

Commit f8d1864

Browse files
iluuu1994iluuu1994
committed
Delay #[Attribute] arg validation until runtime
Fixes GH-13970 Closes GH-14105 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.
1 parent f035661 commit f8d1864

10 files changed

+106
-28
lines changed

‎NEWS

+4
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,10 @@ PHP NEWS
66
. Fixed buffer limit on Windows, replacing read call usage by _read.
77
(David Carlier)
88

9+
- Core:
10+
. Fixed bug GH-13970 (Incorrect validation of #[Attribute] flags type for
11+
non-compile-time expressions). (ilutov)
12+
913
- DOM:
1014
. Fix crashes when entity declaration is removed while still having entity
1115
references. (nielsdos)

‎Zend/tests/attributes/021_attribute_flags_type_is_validated.phpt

+11-2
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,15 @@ Attribute flags type is validated.
66
#[Attribute("foo")]
77
class A1 { }
88

9+
#[A1]
10+
class Foo {}
11+
12+
try {
13+
(new ReflectionClass(Foo::class))->getAttributes()[0]->newInstance();
14+
} catch (Error $e) {
15+
echo $e->getMessage(), "\n";
16+
}
17+
918
?>
10-
--EXPECTF--
11-
Fatal error: Attribute::__construct(): Argument #1 ($flags) must be of type int, string given in %s
19+
--EXPECT--
20+
Attribute::__construct(): Argument #1 ($flags) must be of type int, string given

‎Zend/tests/attributes/022_attribute_flags_value_is_validated.phpt

+11-2
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,15 @@ Attribute flags value is validated.
66
#[Attribute(-1)]
77
class A1 { }
88

9+
#[A1]
10+
class Foo { }
11+
12+
try {
13+
var_dump((new ReflectionClass(Foo::class))->getAttributes()[0]->newInstance());
14+
} catch (Error $e) {
15+
echo $e->getMessage(), "\n";
16+
}
17+
918
?>
10-
--EXPECTF--
11-
Fatal error: Invalid attribute flags specified in %s
19+
--EXPECT--
20+
Invalid attribute flags specified

‎Zend/tests/attributes/023_ast_node_in_validation.phpt

+11-2
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,15 @@ Attribute flags value is validated.
66
#[Attribute(Foo::BAR)]
77
class A1 { }
88

9+
#[A1]
10+
class Bar { }
11+
12+
try {
13+
var_dump((new ReflectionClass(Bar::class))->getAttributes()[0]->newInstance());
14+
} catch (Error $e) {
15+
echo $e->getMessage(), "\n";
16+
}
17+
918
?>
10-
--EXPECTF--
11-
Fatal error: Class "Foo" not found in %s on line %d
19+
--EXPECT--
20+
Class "Foo" not found

‎Zend/tests/attributes/032_attribute_validation_scope.phpt

+13-3
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,19 @@
11
--TEST--
2-
Validation for "Attribute" does not use a scope when evaluating constant ASTs
2+
Validation for "Attribute" uses the class scope when evaluating constant ASTs
33
--FILE--
44
<?php
55
#[Attribute(parent::x)]
66
class x extends y {}
7+
8+
class y {
9+
protected const x = Attribute::TARGET_CLASS;
10+
}
11+
12+
#[x]
13+
class z {}
14+
15+
var_dump((new ReflectionClass(z::class))->getAttributes()[0]->newInstance());
716
?>
8-
--EXPECTF--
9-
Fatal error: Cannot access "parent" when no class scope is active in %s on line %d
17+
--EXPECT--
18+
object(x)#1 (0) {
19+
}

‎Zend/tests/attributes/033_attribute_flags_type_is_not_validated_at_comp_time.phpt

+12
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
--TEST--
2+
Attribute flags type is not validated at compile time.
3+
--FILE--
4+
<?php
5+
6+
#[Attribute("foo")]
7+
class A1 { }
8+
9+
?>
10+
===DONE===
11+
--EXPECT--
12+
===DONE===

‎Zend/zend_attributes.c

+17-9
Original file line numberDiff line numberDiff line change
@@ -35,31 +35,39 @@ static zend_object_handlers attributes_object_handlers_sensitive_parameter_value
3535
static HashTable internal_attributes;
3636

3737
void validate_attribute(zend_attribute *attr, uint32_t target, zend_class_entry *scope)
38+
{
39+
}
40+
41+
uint32_t zend_attribute_attribute_get_flags(zend_attribute *attr, zend_class_entry *scope)
3842
{
3943
// TODO: More proper signature validation: Too many args, incorrect arg names.
4044
if (attr->argc > 0) {
4145
zval flags;
4246

43-
/* As this is run in the middle of compilation, fetch the attribute value without
44-
* specifying a scope. The class is not fully linked yet, and we may seen an
45-
* inconsistent state. */
46-
if (FAILURE == zend_get_attribute_value(&flags, attr, 0, NULL)) {
47-
return;
47+
if (FAILURE == zend_get_attribute_value(&flags, attr, 0, scope)) {
48+
ZEND_ASSERT(EG(exception));
49+
return 0;
4850
}
4951

5052
if (Z_TYPE(flags) != IS_LONG) {
51-
zend_error_noreturn(E_ERROR,
53+
zend_throw_error(NULL,
5254
"Attribute::__construct(): Argument #1 ($flags) must be of type int, %s given",
5355
zend_zval_type_name(&flags)
5456
);
57+
zval_ptr_dtor(&flags);
58+
return 0;
5559
}
5660

57-
if (Z_LVAL(flags) & ~ZEND_ATTRIBUTE_FLAGS) {
58-
zend_error_noreturn(E_ERROR, "Invalid attribute flags specified");
61+
uint32_t flags_l = Z_LVAL(flags);
62+
if (flags_l & ~ZEND_ATTRIBUTE_FLAGS) {
63+
zend_throw_error(NULL, "Invalid attribute flags specified");
64+
return 0;
5965
}
6066

61-
zval_ptr_dtor(&flags);
67+
return flags_l;
6268
}
69+
70+
return ZEND_ATTRIBUTE_TARGET_ALL;
6371
}
6472

6573
static void validate_allow_dynamic_properties(

‎Zend/zend_attributes.h

+2
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,8 @@ ZEND_API zend_attribute *zend_add_attribute(
8585
HashTable **attributes, zend_string *name, uint32_t argc,
8686
uint32_t flags, uint32_t offset, uint32_t lineno);
8787

88+
uint32_t zend_attribute_attribute_get_flags(zend_attribute *attr, zend_class_entry *scope);
89+
8890
END_EXTERN_C()
8991

9092
static zend_always_inline zend_attribute *zend_add_class_attribute(zend_class_entry *ce, zend_string *name, uint32_t argc)

‎ext/reflection/php_reflection.c

+3-10
Original file line numberDiff line numberDiff line change
@@ -6739,16 +6739,9 @@ ZEND_METHOD(ReflectionAttribute, newInstance)
67396739
}
67406740

67416741
if (ce->type == ZEND_USER_CLASS) {
6742-
uint32_t flags = ZEND_ATTRIBUTE_TARGET_ALL;
6743-
6744-
if (marker->argc > 0) {
6745-
zval tmp;
6746-
6747-
if (FAILURE == zend_get_attribute_value(&tmp, marker, 0, ce)) {
6748-
RETURN_THROWS();
6749-
}
6750-
6751-
flags = (uint32_t) Z_LVAL(tmp);
6742+
uint32_t flags = zend_attribute_attribute_get_flags(marker, ce);
6743+
if (EG(exception)) {
6744+
RETURN_THROWS();
67526745
}
67536746

67546747
if (!(attr->target & flags)) {

‎ext/zend_test/tests/gh13970.phpt

+22
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
--TEST--
2+
GH-13970 (Incorrect validation of #[\Attribute]'s first parameter)
3+
--EXTENSIONS--
4+
zend_test
5+
--FILE--
6+
<?php
7+
#[Attribute(\ZendTestUnitEnum::Foo)]
8+
class Foo {}
9+
10+
#[Foo]
11+
function test() {}
12+
13+
$reflection = new ReflectionFunction('test');
14+
15+
try {
16+
var_dump($reflection->getAttributes()[0]->newInstance());
17+
} catch (Error $e) {
18+
echo $e->getMessage(), "\n";
19+
}
20+
?>
21+
--EXPECT--
22+
Attribute::__construct(): Argument #1 ($flags) must be of type int, ZendTestUnitEnum given

0 commit comments

Comments
 (0)