Skip to content
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.

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

JIT+private array property access inside closure accesses private property in child class #12380

Closed
danog opened this issue Oct 7, 2023 · 2 comments

Comments

@danog
Copy link
Contributor

danog commented Oct 7, 2023

Description

The following code:

php -d opcache.enable_cli=1 -d opcache.jit=tracing -d opcache.jit_buffer_size=1G -d opcache.jit_max_root_traces=1000000 -d opcache.jit_max_side_traces=1000000 -d opcache.jit_max_exit_counters=1000000 -d opcache.jit_hot_loop=1 -d opcache.jit_hot_func=1 -d opcache.jit_hot_return=1 -d opcache.jit_hot_side_exit=1 -d memory_limit=-1 a.php

<?php

abstract class a
{
    private array $v = ['test'];

    public function test(): void
    {
        if ($this->v === []) {
            throw new AssertionError("Impossible (1)");
        }
        (function (): void {
            if ($this->v === []) {
                throw new AssertionError("Impossible (2)");
            }
        })();
    }
}

final class b extends a {
    private array $v = [];
}
$a = new b;

for ($i = 0; $i < 30; $i++) {
    $a->test();
}

Resulted in this output:

PHP Fatal error:  Uncaught AssertionError: Impossible (2) in /home/daniil/repos/amp/a.php:16
Stack trace:
#0 [internal function]: a->{closure}()
#1 /home/daniil/repos/amp/a.php(19): Fiber->start()
#2 /home/daniil/repos/amp/a.php(29): a->test()
#3 {main}
  thrown in /home/daniil/repos/amp/a.php on line 16

But I expected this output instead:

This is the root cause of #12249

Ping @dstogov

PHP Version

8.2.11

Operating System

Arch linux

@danog danog changed the title JIT+Isset on a private property inside fiber accesses private property in child class JIT+Isset on a private property inside closure accesses private property in child class Oct 7, 2023
@danog danog changed the title JIT+Isset on a private property inside closure accesses private property in child class JIT+private array property access inside closure accesses private property in child class Oct 7, 2023
@nielsdos
Copy link
Member

nielsdos commented Oct 7, 2023

Thanks for the nice reproducer, I can reproduce this with the tracing jit only (not function jit etc).
var_dump($this->v) inside the closure shows that we're getting the array from a, but var_dump($this); shows both as expected.

EDIT: I think the problem is that zend_get_known_property_info doesn't take into account that the scope matters for private properties, I'll try something and report back.

@iluuu1994
Copy link
Member

@danog Fantastic, thank you for the reproducer! 🙂

nielsdos added a commit to nielsdos/php-src that referenced this issue Oct 7, 2023
…esses private property in child class

For private fields, the scope has to be taken into account, otherwise
the property info may come from the wrong ce.
nielsdos added a commit to nielsdos/php-src that referenced this issue Oct 7, 2023
…esses private property in child class

For private fields, the scope has to be taken into account, otherwise
the property info may come from the wrong ce.
nielsdos added a commit that referenced this issue Oct 9, 2023
* PHP-8.1:
  Fix GH-8996: DOMNode serialization on PHP ^8.1
  Fix GH-12380: JIT+private array property access inside closure accesses private property in child class
nielsdos added a commit that referenced this issue Oct 9, 2023
* PHP-8.2:
  Fix GH-8996: DOMNode serialization on PHP ^8.1
  Fix GH-12380: JIT+private array property access inside closure accesses private property in child class
nielsdos added a commit that referenced this issue Oct 9, 2023
* PHP-8.3:
  Fix GH-8996: DOMNode serialization on PHP ^8.1
  Fix GH-12380: JIT+private array property access inside closure accesses private property in child class
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants