Fix circumvented added hooks in JIT #17870

iluuu1994 · 2025-02-20T21:00:47Z

The following code poses a problem in the JIT:

class A {
    public $prop = 1;
}

class B extends A {
    public $prop = 1 {
        get => parent::$prop::get() * 2;
    }
}

function test(A $a) {
    var_dump($a->prop);
}

test(new B);

The JIT would assume A::$prop in test() could be accessed directly through OBJ_PROP_NUM(). However, since child classes can add new hooks to existing properties, this assumption no longer holds.

To avoid introducing more JIT checks, a hooked property that overrides a unhooked property now results in a separate zval slot that is used instead of the parent slot. This causes the JIT to pick the slow path due to an IS_UNDEF value in the parent slot.

zend_class_entry.properties_info_table poses a problem in that zend_get_property_info_for_slot() and friends will be called using the child slot, which does not store its property info, since the parent slot already does. In this case, zend_get_property_info_for_slot() now provides a fallback that will iterate all property infos to find the correct one.

This also uncovered a bug (see Zend/tests/property_hooks/dump.phpt) where the default value of a parent property would accidentally be inherited by the child property.

Fixes GH-17376

ext/opcache/jit/zend_jit_ir.c

Zend/zend_API.c

Zend/zend_inheritance.c

The following code poses a problem in the JIT: ```php class A { public $prop = 1; } class B extends A { public $prop = 1 { get => parent::$prop::get() * 2; } } function test(A $a) { var_dump($a->prop); } test(new B); ``` The JIT would assume A::$prop in test() could be accessed directly through OBJ_PROP_NUM(). However, since child classes can add new hooks to existing properties, this assumption no longer holds. To avoid introducing more JIT checks, a hooked property that overrides a unhooked property now results in a separate zval slot that is used instead of the parent slot. This causes the JIT to pick the slow path due to an IS_UNDEF value in the parent slot. zend_class_entry.properties_info_table poses a problem in that zend_get_property_info_for_slot() and friends will be called using the child slot, which does not store its property info, since the parent slot already does. In this case, zend_get_property_info_for_slot() now provides a fallback that will iterate all property infos to find the correct one. This also uncovered a bug (see Zend/tests/property_hooks/dump.phpt) where the default value of a parent property would accidentally be inherited by the child property. Fixes phpGH-17376

iluuu1994 · 2025-02-21T18:36:50Z

I have no clue what this Windows failure is about. 😞 I can't reproduce on Linux with asan/msan/valgrind, w/wo opcache, nts/zts, etc. @cmb69 Can you help? I don't have a VM...

cmb69 · 2025-02-21T19:24:57Z

It's a JIT issue. Running ext\random\tests\02_engine\all_serialize_user.phpt under ASan/tracing JIT:

Random\Engine\Test\TestCountingEngine32
=================================================================
==13488==ERROR: AddressSanitizer: access-violation on unknown address 0x000000000014 (pc 0x7ffce98a8fa2 bp 0x0089ff7fc6b0 sp 0x0089ff7fadb0 T0)
==13488==The signal is caused by a READ memory access.
==13488==Hint: address points to the zero page.
    #0 0x7ffce98a8fa1 in verify_readonly_and_avis C:\php-sdk\phpdev\vs17\x64\php-src\ext\opcache\jit\zend_jit_helpers.c:2716
    #1 0x7ffce989b658 in zend_jit_post_inc_typed_prop C:\php-sdk\phpdev\vs17\x64\php-src\ext\opcache\jit\zend_jit_helpers.c:2976
    #2 0x200008000e61  (<unknown module>)
    #3 0x000000007f12  (<unknown module>)
    #4 0x000000000000  (<unknown module>)
    #5 0x000000007f13  (<unknown module>)
    #6 0x1205678a1cd7  (<unknown module>)
    #7 0x1205678a1c7f  (<unknown module>)
    #8 0x11f9678c0877  (<unknown module>)
    #9 0x11f9678c073f  (<unknown module>)
    #10 0x11f9678c0877  (<unknown module>)
    #11 0x002700000027  (<unknown module>)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: access-violation C:\php-sdk\phpdev\vs17\x64\php-src\ext\opcache\jit\zend_jit_helpers.c:2716 in verify_readonly_and_avis
==13488==ABORTING

Does that help?

iluuu1994 · 2025-02-21T21:02:30Z

Thank you very much! It might, I'll have a look very soon.

Z_PROP_TABLE_OFFSET already gives us an index into a void* array. Thus, we only need to multiply by sizeof(void*).

cmb69 · 2025-02-22T00:14:40Z

Looks good. :)

iluuu1994 · 2025-02-22T00:15:52Z

@cmb69 Indeed, it did help 😊 Thanks again!

ext/opcache/jit/zend_jit_ir.c

arnaud-lb

This looks good to me!

dstogov

I didn't understand the implementation in all details yet, but the idea is clear and seems should work.

In case child hook updates the real property, only the child real property is changed, and the parent property is kept UNDEF forever. Right?

Please, answer my questions. I'll review this once again tomorrow.
@nielsdos please also take a look.

dstogov · 2025-02-25T11:16:20Z

Zend/tests/property_hooks/parent_get_plain.phpt

-    public $prop {
+    public $prop = 42 {


Why do you need these tests changes?
Does the patch break some previous behavior?

Yes, this patch also fixes a bug. Properties do not inherit default values, but we accidentally did for properties with hooks.

If the fix may be done separately, it's better to do this separately (add bug report and so on).
If the fix relays on new inheritance logic, and can't be simple separated, don't do this.

dstogov · 2025-02-25T11:27:46Z

Zend/zend_lazy_objects.c

-		return table[prop_num];
+		if (table[prop_num]) {


When table[prop_num] may be NULL?
Only for regular properties overridden with hooked ones?

Only for regular properties overridden with hooked ones?

They will only ever be NULL for properties with hooks that override properties without hooks. This could add an EXPECTED() guard, if you like.

OK. I don't care.

iluuu1994 · 2025-02-25T11:42:23Z

In case child hook updates the real property, only the child real property is changed, and the parent property is kept UNDEF forever. Right?

Yes, exactly.

dstogov · 2025-02-25T11:55:27Z

Zend/zend_objects_API.c

+	ZEND_HASH_MAP_FOREACH_PTR(&obj->ce->properties_info, prop_info) {
+		if (prop_info->offset == offset) {
+			return prop_info;
+		}
+	} ZEND_HASH_FOREACH_END();


Think, if it's possible to improve this linear search.
This is not a big problem or stopper, but it would be good o avoid this.

May be we may store the prop_info somehow differently...
Or may be check only the child properties (skipping ce->parrent->default_properties_count)...

May be we may store the prop_info somehow differently...

A straight-forward solution in that regard would be to store a clone of properties_info_table that sets both the parent and child offset to the child property. This was not done in properties_info_table to avoid breaking iteration over the table. That would not be ABI compatible though (modification of zend_class_entry), so should likely be done in master only.

Checking only child properties will already be a welcome improvement

nielsdos

The concept seems right, but this may break some assumptions elsewhere in code about slots/inheritance.
For example, this code:

<?php

class A {
    public $prop = 1;
}

class B extends A {
    public $prop = 1 { get => parent::$prop::get() * 2; }
}

class C extends B {
    public $prop = 3;
}

function test(A $a) {
    var_dump((array)$a);
}

test(new C);

Now outputs:

array(2) {
  ["prop"]=>
  int(3)
  ["prop"]=>
  int(3)
}

Which is wrong, this worked prior to this patch.

I also tried to play a bit with multiple parent classes, to test how that interacts. But I got some weird behaviour that I cannot understand. This may be another bug unrelated to this but I'll bring it up here to ask and show what I tried to do. This breaks in PHP with or without JIT, it causes an infinite recursion:

<?php

class A {
    public $prop = 1;
}

class B extends A {
    public $prop = 1 { get => parent::$prop::get() * 2; }
}

class C extends B {
    public $prop = 3;
}

function test(A $a) {
    var_dump($a->prop);
}

test(new C);

Something seems off here or I just don't understand how this is supposed to work.

nielsdos · 2025-02-25T20:10:31Z

Zend/zend_inheritance.c

+				if (use_child_prop && child_info->offset == ZEND_VIRTUAL_PROPERTY_OFFSET) {
+					child_info->offset = OBJ_PROP_TO_OFFSET(ce->default_properties_count);
+					ce->default_properties_count++;
+					ce->default_properties_table = perealloc(ce->default_properties_table, sizeof(zval) * ce->default_properties_count, ce->type == ZEND_INTERNAL_CLASS);


Doing reallocs with linear size increases is not very efficient, but okay this should be rare and happens during inheritance so it shouldn't be a big problem / showstopper.

I fully agree ofc. That said, this excerpt comes from zend_declare_typed_property() which will trigger much more commonly than this code path.

nielsdos · 2025-02-25T20:16:10Z

Zend/zend_objects_API.c

+	ZEND_HASH_MAP_FOREACH_PTR(&obj->ce->properties_info, prop_info) {
+		if (prop_info->offset == offset) {
+			return prop_info;
+		}
+	} ZEND_HASH_FOREACH_END();


Checking only child properties will already be a welcome improvement

iluuu1994 · 2025-02-25T21:41:28Z

Nice find, thank you! I'll have a look.

The property info prototype would point to the parent instead of the first ancestor.

dstogov

I found more related problems...

<?php

class A {
    public $prop = 1;
}

class B extends A {
    public $prop = 2 { get => parent::$prop::get() * 2; }
}

class C extends B {
    public $prop = 3;
}

function test(A $a) {
    var_dump($a);
    var_dump((array)$a);
    var_dump(get_object_vars($a));
    var_dump(unserialize(serialize($a)));
    var_dump(json_decode(json_encode($a)));
}

test(new C);

now output

object(C)#1 (1) {
  ["prop"]=>
  int(3)
}
array(1) {
  ["prop"]=>
  int(3)
}
array(1) {
  ["prop"]=>
  int(6)
}
object(C)#2 (1) {
  ["prop"]=>
  int(3)
}
object(stdClass)#2 (1) {
  ["prop"]=>
  int(6)
}

Interesting this breaks ext/json and not serialize(). This may also affect other third-party extensions.

Also try to pass new B instead of new C. I don't know what should it return, but the output is definitely inconsistent (sometimes 2 sometimes 4). Way be I'm wrong here.

Please also check for consistency of indirect calls to set hook from unserialize() and other similar places

iluuu1994 · 2025-02-26T14:14:30Z

@dstogov This looks correct at first glance. Note that hooks are inherited in child properties, so while Cdoesn't declare them explicitly, it is still there. JSON and get_object_vars call hooks, while the other functions do not. You can find a rationale for each here: https://wiki.php.net/rfc/property-hooks#interaction_with_serialization

dstogov · 2025-02-26T14:45:02Z

@dstogov This looks correct at first glance. Note that hooks are inherited in child properties, so while Cdoesn't declare them explicitly, it is still there. JSON and get_object_vars call hooks, while the other functions do not. You can find a rationale for each here: https://wiki.php.net/rfc/property-hooks#interaction_with_serialization

OK. If this is on purpose it's hard to understand the purpose.
Anyway I don't object any more.

nielsdos

I wanted to play a bit with this and lazy objects, but was blocked by another issue (unrelated to this). (#17941)
I think this is fine now.

iluuu1994 · 2025-02-26T20:27:32Z

Thank you all for the reviews and testing!

iluuu1994 requested a review from arnaud-lb February 20, 2025 21:00

iluuu1994 commented Feb 20, 2025

View reviewed changes

ext/opcache/jit/zend_jit_ir.c Outdated Show resolved Hide resolved

iluuu1994 changed the base branch from master to PHP-8.4 February 20, 2025 21:05

nielsdos reviewed Feb 20, 2025

View reviewed changes

Zend/zend_API.c Outdated Show resolved Hide resolved

github-actions bot added Category: Engine Extension: opcache ABI break labels Feb 20, 2025

iluuu1994 mentioned this pull request Feb 20, 2025

Fix JIT for overridden properties with added hooks #17395

Closed

iluuu1994 commented Feb 21, 2025

View reviewed changes

Zend/zend_inheritance.c Outdated Show resolved Hide resolved

iluuu1994 force-pushed the gh-17376-alt-2 branch from 4cf0f0e to add3e2b Compare February 21, 2025 17:48

Fix prop_info_offset calculation

7042ec6

Z_PROP_TABLE_OFFSET already gives us an index into a void* array. Thus, we only need to multiply by sizeof(void*).

[skip ci] Adjust comment

a48f879

iluuu1994 marked this pull request as ready for review February 22, 2025 00:21

iluuu1994 requested a review from dstogov as a code owner February 22, 2025 00:21

iluuu1994 commented Feb 24, 2025

View reviewed changes

ext/opcache/jit/zend_jit_ir.c Outdated Show resolved Hide resolved

Revert unnecessary changes

060a409

iluuu1994 force-pushed the gh-17376-alt-2 branch from 21ec227 to 060a409 Compare February 24, 2025 21:55

arnaud-lb approved these changes Feb 25, 2025

View reviewed changes

dstogov reviewed Feb 25, 2025

View reviewed changes

nielsdos requested changes Feb 25, 2025

View reviewed changes

Fix multi-level inheritance with hooks

8ca7bc4

The property info prototype would point to the parent instead of the first ancestor.

dstogov requested changes Feb 26, 2025

View reviewed changes

dstogov approved these changes Feb 26, 2025

View reviewed changes

nielsdos approved these changes Feb 26, 2025

View reviewed changes

iluuu1994 closed this in 376e90f Feb 26, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix circumvented added hooks in JIT #17870

Fix circumvented added hooks in JIT #17870

iluuu1994 commented Feb 20, 2025

iluuu1994 commented Feb 21, 2025

cmb69 commented Feb 21, 2025

iluuu1994 commented Feb 21, 2025

cmb69 commented Feb 22, 2025

iluuu1994 commented Feb 22, 2025

arnaud-lb left a comment

dstogov left a comment

dstogov Feb 25, 2025

iluuu1994 Feb 25, 2025

dstogov Feb 25, 2025

dstogov Feb 25, 2025

iluuu1994 Feb 25, 2025

dstogov Feb 25, 2025

iluuu1994 commented Feb 25, 2025

dstogov Feb 25, 2025

iluuu1994 Feb 25, 2025

nielsdos Feb 25, 2025

nielsdos left a comment

nielsdos Feb 25, 2025

iluuu1994 Feb 25, 2025

nielsdos Feb 25, 2025

iluuu1994 commented Feb 25, 2025

dstogov left a comment •

edited

Loading

iluuu1994 commented Feb 26, 2025

dstogov commented Feb 26, 2025

nielsdos left a comment •

edited

Loading

iluuu1994 commented Feb 26, 2025

Fix circumvented added hooks in JIT #17870

Fix circumvented added hooks in JIT #17870

Conversation

iluuu1994 commented Feb 20, 2025

iluuu1994 commented Feb 21, 2025

cmb69 commented Feb 21, 2025

iluuu1994 commented Feb 21, 2025

cmb69 commented Feb 22, 2025

iluuu1994 commented Feb 22, 2025

arnaud-lb left a comment

Choose a reason for hiding this comment

dstogov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iluuu1994 commented Feb 25, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nielsdos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iluuu1994 commented Feb 25, 2025

dstogov left a comment • edited Loading

Choose a reason for hiding this comment

iluuu1994 commented Feb 26, 2025

dstogov commented Feb 26, 2025

nielsdos left a comment • edited Loading

Choose a reason for hiding this comment

iluuu1994 commented Feb 26, 2025

dstogov left a comment •

edited

Loading

nielsdos left a comment •

edited

Loading