Skip to content

Commit 7236e5a

Browse files
committed
Delay #[Attribute] arg validation until runtime
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.
1 parent d670e13 commit 7236e5a

9 files changed

+100
-27
lines changed

Zend/tests/attributes/021_attribute_flags_type_is_validated.phpt

Lines changed: 11 additions & 2 deletions
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

Lines changed: 11 additions & 2 deletions
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

Lines changed: 11 additions & 2 deletions
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
Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,18 @@
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 | Attribute::TARGET_PARAMETER;
10+
}
11+
12+
var_dump((new ReflectionClass(x::class))->getAttributes()[0]->newInstance());
713
?>
8-
--EXPECTF--
9-
Fatal error: Cannot access "parent" when no class scope is active in %s on line %d
14+
--EXPECT--
15+
object(Attribute)#1 (1) {
16+
["flags"]=>
17+
int(33)
18+
}
Lines changed: 12 additions & 0 deletions
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

Lines changed: 16 additions & 8 deletions
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)
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. */
4647
if (FAILURE == zend_get_attribute_value(&flags, attr, 0, NULL)) {
47-
return;
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

Lines changed: 2 additions & 0 deletions
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);
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

Lines changed: 3 additions & 10 deletions
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);
6743+
if (EG(exception)) {
6744+
RETURN_THROWS();
67526745
}
67536746

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

ext/zend_test/tests/gh13970.phpt

Lines changed: 22 additions & 0 deletions
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)