Fix GH-21776: use-after-free in zend_std_read_property magic __isset#21786
Fix GH-21776: use-after-free in zend_std_read_property magic __isset#21786iliaal wants to merge 1 commit intophp:masterfrom
Conversation
iluuu1994
left a comment
iluuu1994
left a comment
There was a problem hiding this comment.
Looks correct, but given this is non-trivial I'd prefer to target master. This should not target 8.5 anyway, as it applies to 8.4 also.
| exit: | ||
| if (release_zobj) { | ||
| OBJ_RELEASE(zobj); | ||
| } |
There was a problem hiding this comment.
You could move this above exit with a new label and adjust the gotos in if (UNEXPECTED(zend_lazy_object_must_init(zobj))) { to go there instead. This check is not necessary the majority of the time.
There was a problem hiding this comment.
Fair enough, let me adjust, I just looked at bug report and it mention 8.5 so started from there
There was a problem hiding this comment.
Moved the release into a release_zobj_exit: label above exit: and routed the three lazy-init-block jumps there. The isset-false, getter, and wrong-offset paths still goto exit and skip the flag check. Retargeted to master.
When __isset drops the last non-temp reference to $this (e.g. $GLOBALS['o'] = 0), the OBJ_RELEASE after the __isset call freed zobj before zend_std_read_property reached the shared uninit_error check at zend_lazy_object_must_init(zobj), a heap-use-after-free. The GC_ADDREF/OBJ_RELEASE pair around __isset has been correct since 2018. The 2023 lazy-object support added a zobj read in the shared fall-through path without extending the isset branch's ref coverage to match. Defer the release via a local flag so zobj stays alive through the lazy-init check and the recursive read on the initialized instance. Route the lazy-init block's exits through a release_zobj_exit label so the deferred release fires on those paths too, while the hot paths that already released inline skip the flag check. Closes phpGH-21776
iliaal
force-pushed
the
fix/gh-21776-isset-uaf
branch
from
f235a27 to
4a843ef
Compare
2 participants
Fixes #21776.
When
__issetdrops the last non-temp reference to$this(e.g.$GLOBALS['o'] = 0), theOBJ_RELEASE(zobj)after the__issetcall freed the object before
zend_std_read_propertyreached theshared
uninit_errorcheck atzend_lazy_object_must_init(zobj).ASAN reports a heap-use-after-free.
Defer the release via a local flag so
zobjstays alive through thelazy-init check and the recursive read on the initialized instance.
Route the two
returns inside the lazy block throughexit:so thedeferred release runs on those paths too.