Repository navigation
Conversation
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.
|
Quoted below: written with Claude, an LLM, as CONTRIBUTING.md asks us to mark. The builds and test runs behind it are on my machine; the commands to repeat them are under the fold.
|
| return variable_ptr; | ||
|
|
||
| exit_reset_cache_slot: | ||
| if (cache_slot) { |
There was a problem hiding this comment.
The slot reset is now written out twice (the exit_reset_cache_slot label and the new else branch). A small static helper would avoid the duplication and keep the invariant in one place. It can then be called from the readonly error, the asymmetric visibility error and the __set() branch, and the new label is no longer needed.
|
Fix looks correct, left some minor comments. One general concern: the ASSIGN_OBJ fast path from GH-22709 relies on an implicit invariant ("primed slot = write access was checked"), but the slot is primed before the checks, so every exit that skips them must remember to reset it. We've already have 3 such exits, and a future one will silently bring the bypass back. It would be safer to make this hold by construction, e.g. prime the slot only after the checks pass, or separate the two things: the property lookup cache and a "write access verified" marker that only the fast path consults. That doesn't need to block this fix, but it deserves a follow-up for master. |
Extract the cache slot reset into a static helper so the invariant (the ZEND_ASSIGN_OBJ fast path skips the set visibility check on a primed slot) is documented in one place, and call it from the readonly error, the asymmetric visibility error and the __set() fallback. The exit_reset_cache_slot label is no longer needed.
|
Quoted below: written with Claude, an LLM, as CONTRIBUTING.md asks us to mark. The builds and test runs behind it are on my machine; the commands to repeat them are under the fold.
|
Since GH-22709 (94136cf), the
ZEND_ASSIGN_OBJfast path passescheck_writable = falsetozend_assign_to_typed_prop()when the cache slot is primed. This relies onzend_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:__clone(), where the property is reinitializable, the fast path skips the implicitprotected(set)check, so code outside the class can reinitialize the property.__set()fallback. For anunset()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 aprivate(set)property from global scope.The fix resets the cache slot on both paths, as the asymmetric visibility error path already does (moved to a shared
exit_reset_cache_slotlabel). The__set()reset only happens inside the existingZEND_ACC_READONLY|ZEND_ACC_PPP_SET_MASKbranch, 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.phptZend/tests/readonly_props/cache_slot_clone_reinit.phptValidated on a debug build with AddressSanitizer of PHP-8.6 (
730ef3d3683):Zend/tests,tests/classes,tests/langandext/opcache/testsunder--asanwithout 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).