diff --git a/phpunit/src/SsaAnalysisTest.php b/phpunit/src/SsaAnalysisTest.php index b528155f..cec5703a 100644 --- a/phpunit/src/SsaAnalysisTest.php +++ b/phpunit/src/SsaAnalysisTest.php @@ -816,7 +816,20 @@ class SsaAnalysisTest extends TestCase )); $result = $this->invoke('collectDangerousPropOps', 'obj', [$stmt, $read]); - $this->assertSame([], $result, 'Passing the object to a dynamic call cannot unset the property, so it is not dangerous'); + $this->assertSame(['a' => true], $result, 'Passing the object to dynamic code may turn a property slot into a reference'); + } + + public function testCollectDangerousPropOpsObjectMethodReceiverWildcard(): void + { + $methodCall = new Expr\MethodCall(new Expr\Variable('obj'), 'mutate'); + $stmt = new Stmt\Expression($methodCall); + $read = new Stmt\Expression(new Expr\Assign( + new Expr\Variable('value'), + new Expr\PropertyFetch(new Expr\Variable('obj'), 'a') + )); + + $result = $this->invoke('collectDangerousPropOps', 'obj', [$stmt, $read]); + $this->assertSame(['a' => true], $result); } public function testCollectDangerousPropOpsInternalFunctionObjectArgumentIsSafe(): void diff --git a/src/Php/CompilerBase.php b/src/Php/CompilerBase.php index 9c6e6917..6a9ba502 100644 --- a/src/Php/CompilerBase.php +++ b/src/Php/CompilerBase.php @@ -5728,25 +5728,37 @@ class CompilerBase extends \PhpAot\Core\Translator { if ($stmt instanceof Node\Stmt\Class_) { $stmt = clone $stmt; + $shouldAddMixedReturn = fn (Node\Stmt\Class_ $class, Node\Stmt\ClassMethod $method): bool => + $this->shouldAddMixedReturnToEmbeddedClassMethod($class, $method); $traverser = new \PhpParser\NodeTraverser(); - $traverser->addVisitor(new class extends \PhpParser\NodeVisitorAbstract { + $traverser->addVisitor(new class($shouldAddMixedReturn) extends \PhpParser\NodeVisitorAbstract { + /** @var list */ + private array $classStack = []; + + public function __construct(private \Closure $shouldAddMixedReturn) + { + } + public function enterNode(Node $node) { - if (!$node instanceof Node\Stmt\ClassMethod || $node->returnType !== null) { + if ($node instanceof Node\Stmt\Class_) { + $this->classStack[] = $node; return null; } - $returnType = match (strtolower($node->name->toString())) { - '__construct', '__destruct' => null, - '__set', '__unserialize', '__unset', '__wakeup', '__clone' => 'void', - '__tostring' => 'string', - '__serialize', '__debuginfo', '__sleep' => 'array', - '__isset' => 'bool', - '__set_state' => 'object', - default => 'mixed', - }; - if ($returnType !== null) { - $node->returnType = new Node\Identifier($returnType); + if ($node instanceof Node\Stmt\ClassMethod && $node->returnType === null) { + $class = $this->classStack[count($this->classStack) - 1] ?? null; + if ($class !== null && ($this->shouldAddMixedReturn)($class, $node)) { + $node->returnType = new Node\Identifier('mixed'); + } + } + return null; + } + + public function leaveNode(Node $node) + { + if ($node instanceof Node\Stmt\Class_) { + array_pop($this->classStack); } return null; } @@ -5756,6 +5768,70 @@ class CompilerBase extends \PhpAot\Core\Translator return $this->printer->prettyPrint([$stmt]); } + protected function shouldAddMixedReturnToEmbeddedClassMethod(Node\Stmt\Class_ $class, Node\Stmt\ClassMethod $method): bool + { + $methodName = strtolower($method->name->toString()); + if ($this->isEmbeddedMagicMethodReturnSensitive($methodName)) { + return false; + } + + if (!empty($class->implements)) { + return true; + } + + return $class->extends !== null + && $this->ancestorMethodMayRequireMixedReturn($class->extends, $methodName); + } + + protected function isEmbeddedMagicMethodReturnSensitive(string $methodName): bool + { + return in_array($methodName, [ + '__construct', + '__destruct', + '__clone', + '__debuginfo', + '__isset', + '__serialize', + '__set', + '__set_state', + '__sleep', + '__tostring', + '__unserialize', + '__unset', + '__wakeup', + ], true); + } + + protected function ancestorMethodMayRequireMixedReturn(Node\Name $extends, string $methodName): bool + { + $className = ltrim($extends->toString(), '\\'); + + while ($className !== '') { + if ($this->hasClass($className)) { + $classDef = $this->getClass($className); + if ($classDef->hasMethod($methodName)) { + $functionDef = $classDef->getMethod($methodName)->functionDef; + return $functionDef !== null + && ($functionDef->returnTypeUndeclared || $functionDef->returnType === self::TYPE_VAR); + } + $className = $classDef->extends; + continue; + } + + if ($this->isInternalClass($className) || $this->isInternalInterface($className)) { + if (!Reflection::hasMethod($className, $methodName)) { + return false; + } + $returnType = Reflection::getMethodReturnType($className, $methodName); + return $returnType === null || strtolower($returnType) === 'mixed'; + } + + return true; + } + + return false; + } + protected function parseArrowFunction(Expr\ArrowFunction $expr): string { $nodeFinder = new NodeFinder(); diff --git a/src/Php/Optimizer/SsaPropOptimizer.php b/src/Php/Optimizer/SsaPropOptimizer.php index 7ad12c0d..25dc1fda 100644 --- a/src/Php/Optimizer/SsaPropOptimizer.php +++ b/src/Php/Optimizer/SsaPropOptimizer.php @@ -15,17 +15,18 @@ * 4. Property has a declared native type (int or float) * 5. No &$o->prop (reference capture of the property) * 6. No func(&$o->prop) (property passed by reference) - * 7. First access is not inside a loop or nested block scope + * 7. Object is not exposed to dynamic user code before later property access + * 8. First access is not inside a loop or nested block scope * - * unset($o->prop) and exposing the object to dynamic calls are NOT dangerous: - * the object handlers reject property unset, so a hoisted reference cannot be - * invalidated by either path. + * Direct unset($o->prop) is not dangerous: the object handlers reset/reject the + * unset path, so a hoisted reference is not invalidated by direct unset alone. */ namespace PhpAot\Php\Optimizer; use PhpAot\Php\Analysis\SsaBuilder; use PhpAot\Php\Analysis\SsaFlags; +use PhpAot\Php\Reflection; use PhpParser\Node; use PhpParser\Node\Expr; @@ -267,9 +268,11 @@ trait SsaPropOptimizer * Detects: * - $ref = &$o->prop — property becomes reference, zval type changes * - func(&$o->prop) or $obj->method(&$o->prop) — property passed by ref + * - mutate($o) or $o->method() — dynamic code may turn the property slot + * into a reference through the exposed object * - * unset($o->prop) and passing the object to dynamic calls are intentionally - * not treated as dangerous: the object handlers reject property unset. + * Direct unset($o->prop) is intentionally not treated as dangerous: the + * object handlers reset/reject property unset. */ protected function hasDangerousPropOps(string $objName, array $stmts): bool { @@ -377,12 +380,17 @@ trait SsaPropOptimizer $this->collectPropEvents($arg->value, $objName, $events); } } - // Exposing the object to a dynamic call (passing it as an argument or - // invoking a non-internal method on it) can no longer invalidate a - // hoisted property: the callee cannot unset the property, since the - // object handlers reject property unset. Only explicit by-reference - // captures (handled above) and direct &/refval on the property remain - // dangerous, so the receiver/argument exposure check is unnecessary. + if (!$this->isSafeObjectExposureCall($node)) { + if (($node instanceof Expr\MethodCall || $node instanceof Expr\NullsafeMethodCall) + && $this->isVarNamed($node->var, $objName)) { + $events[] = ['kind' => 'danger', 'prop' => '*']; + } + foreach ($node->args as $arg) { + if ($this->exprMayExposeObject($arg->value, $objName)) { + $events[] = ['kind' => 'danger', 'prop' => '*']; + } + } + } return; } @@ -438,6 +446,60 @@ trait SsaPropOptimizer } } + protected function isSafeObjectExposureCall(Expr\FuncCall|Expr\MethodCall|Expr\StaticCall|Expr\NullsafeMethodCall $node): bool + { + if ($node instanceof Expr\FuncCall) { + return $node->name instanceof Node\Name + && $this->isInternalFunctionName($node->name); + } + + if ($node instanceof Expr\StaticCall) { + return $node->class instanceof Node\Name + && $node->name instanceof Node\Identifier + && $this->isInternalClassCall($this->resolveStaticCallClassForSafety($node->class), $node->name->toString()); + } + + if (!$node->name instanceof Node\Identifier) { + return false; + } + + $className = $this->detectClassOfExpr($node->var); + return $this->isInternalClassCall($className, $node->name->toString()); + } + + protected function isInternalFunctionName(Node\Name $name): bool + { + $functionName = ltrim($name->toString(), '\\'); + + if (str_contains($functionName, '\\')) { + return false; + } + + return $this->isInternalFunction($functionName) || $this->isInternalFunction(strtolower($functionName)); + } + + protected function resolveStaticCallClassForSafety(Node\Name $classNode): string + { + $className = $classNode->toString(); + if ($className === 'self') { + return $this->classDef ? $this->getFullClassName() : ''; + } + if ($className === 'parent') { + return $this->classDef ? $this->classDef->extends : ''; + } + if ($className === 'static') { + return ''; + } + return $this->getNamespacedClassName($className); + } + + protected function isInternalClassCall(string $className, string $methodName): bool + { + return $className !== '' + && ($this->isInternalClass($className) || $this->isInternalInterface($className)) + && Reflection::hasMethod($className, $methodName); + } + /** * Dynamic property name expressions can contain normal property reads: * unset($o->{$other->name}) should still record $other->name if relevant. diff --git a/tests/aot/anon_class/003.phpt b/tests/aot/anon_class/003.phpt new file mode 100644 index 00000000..ecd56a5c --- /dev/null +++ b/tests/aot/anon_class/003.phpt @@ -0,0 +1,21 @@ +--TEST-- +Anonymous Classes - magic methods keep undeclared return semantics +--FILE-- +value = $value; + return 1; + } + }; + + $obj->dynamic = 2; + var_dump($obj->value); +} +?> +--EXPECT-- +int(2) diff --git a/tests/aot/class/unset-ref-prop.phpt b/tests/aot/class/unset-ref-prop.phpt new file mode 100644 index 00000000..6a52e61b --- /dev/null +++ b/tests/aot/class/unset-ref-prop.phpt @@ -0,0 +1,56 @@ +--TEST-- +unset typed property preserves existing property reference +--FILE-- +value); }'); + eval('function unset_name(UnsetRefProp $obj) { unset($obj->name); }'); + eval('function unset_items(UnsetRefProp $obj) { unset($obj->items); }'); + + $obj = new UnsetRefProp(); + + $valueRef =& $obj->value; + $valueRef = 7; + unset_value($obj); + var_dump($obj->value); + var_dump($valueRef); + $valueRef = 9; + var_dump($obj->value); + + $nameRef =& $obj->name; + unset_name($obj); + var_dump($obj->name); + var_dump($nameRef); + $nameRef = "changed"; + var_dump($obj->name); + + $itemsRef =& $obj->items; + unset_items($obj); + var_dump($obj->items); + var_dump($itemsRef); + $itemsRef[] = 3; + var_dump($obj->items); +} +?> +--EXPECT-- +int(0) +int(0) +int(9) +string(0) "" +string(0) "" +string(7) "changed" +array(0) { +} +array(0) { +} +array(1) { + [0]=> + int(3) +} diff --git a/tests/aot/optimizations/objprop-hoist-object-arg-reference-slot.phpt b/tests/aot/optimizations/objprop-hoist-object-arg-reference-slot.phpt new file mode 100644 index 00000000..e39e00bf --- /dev/null +++ b/tests/aot/optimizations/objprop-hoist-object-arg-reference-slot.phpt @@ -0,0 +1,29 @@ +--TEST-- +SSA object prop: object argument can turn property slot into reference +--FILE-- +a; + $ref = 99; +} + +function main(): void { + $o = new RefSlotFoo(); + $o->a = 1; + + bind_ref($o); + var_dump($o->a); + $o->a += 1; + + var_dump($o->a); +} +?> +--EXPECT-- +int(99) +int(100)