Skip to content

php8.6: Fix set visibility bypass through a primed ZEND_ASSIGN_OBJ cache slot - #24566

Open
brainexe wants to merge 1 commit into
php:PHP-8.6from
brainexe:fix/assign-obj-cache-slot-visibility
Open

brainexe wants to merge 1 commit into
php:PHP-8.6from
brainexe:fix/assign-obj-cache-slot-visibility

Conversation

@brainexe

Copy link
Copy Markdown
Contributor

Since GH-22709 (94136cf), the ZEND_ASSIGN_OBJ fast path passes check_writable = false to zend_assign_to_typed_prop() when the cache slot is primed. This relies on zend_std_write_property() leaving the slot primed only after the set visibility check has passed. zend_get_property_offset() primes the slot before that check, and the asymmetric visibility error path resets it again, but two other paths keep it primed without the check having passed:

  1. Readonly modification error. The first write to an initialized readonly property throws "Cannot modify readonly property" and returns with the slot primed. If the same opline later runs inside __clone(), where the property is reinitializable, the fast path skips the implicit protected(set) check, so code outside the class can reinitialize the property.
  2. __set() fallback. For an unset() property of a class with __set(), the visibility check is skipped on purpose and __set() is called, but the slot stays primed. A later run of the same opline on an object where the property is initialized then writes to a private(set) property from global scope.
class C {
    public private(set) int $x = 0;
    public function __set($name, $value) { echo "__set($name)\n"; }
    public function unsetX() { unset($this->x); }
}
function w(C $c, int $v) { $c->x = $v; }  // same opline every call

$a = new C(); $a->unsetX();
w($a, 1);            // __set(x), cache slot stays primed
$b = new C();
w($b, 2);            // expected: Error "Cannot modify private(set) property C::$x from global scope"
var_dump($b->x);     // PHP-8.6: int(2)
class P {
    public readonly int $x;
    public function __construct() { $this->x = 1; }
    public function __clone() { w($this, 3); }
}
function w(P $p, int $v) { try { $p->x = $v; } catch (Error $e) { echo $e->getMessage(), "\n"; } }

$p = new P();
w($p, 2);            // Cannot modify readonly property P::$x, cache slot stays primed
$q = clone $p;       // expected: Error "Cannot modify protected(set) readonly property P::$x from global scope"
var_dump($q->x);     // PHP-8.6: int(3)

The fix resets the cache slot on both paths, as the asymmetric visibility error path already does (moved to a shared exit_reset_cache_slot label). The __set() reset only happens inside the existing ZEND_ACC_READONLY|ZEND_ACC_PPP_SET_MASK branch, so plain public properties and the common unset-plus-__set() proxy pattern are unaffected.

The JIT helpers (zend_jit_assign_to_typed_prop() → verify_readonly_and_avis()) always check the set visibility, so only the VM fast path is affected; the repros fail the same way with opcache and with function/tracing JIT because these oplines run in the VM.

Affected: PHP-8.6 since 8.6.0beta1 (all betas and RCs so far) and master. Not affected: PHP-8.5 and older.

Tests

  • Zend/tests/asymmetric_visibility/cache_slot_magic_set.phpt
  • Zend/tests/readonly_props/cache_slot_clone_reinit.phpt

Validated on a debug build with AddressSanitizer of PHP-8.6 (730ef3d3683):

  • Both new tests fail without the fix and pass with it, with and without opcache.
  • Both reproducers behave correctly with the fix without opcache, with opcache, with function JIT and with tracing JIT, also in a 5000-iteration hot loop under tracing JIT.
  • Zend/tests, tests/classes, tests/lang and ext/opcache/tests under --asan without opcache, with opcache and with tracing JIT: 0 failures in each mode, both before (7010 passed) and after the fix (7012 passed, the 2 new tests).
  • The patch also applies cleanly to master for the merge-up.

Since phpGH-22709, the ZEND_ASSIGN_OBJ fast path doesn't check the set
visibility of a property when the cache slot is primed, assuming that
write_property() only leaves it primed after the check passed. But
write_property() also left it primed when it threw a readonly
modification error, or fell back to __set() for an unset property,
without checking the set visibility. A later execution of the same
opline could then modify a private(set) property, or reinitialize a
readonly property in __clone(), from a scope without set access.

Reset the cache slot on these paths as well.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant