Skip to content

Fix use-after-free of object through __isset() and globals #18852

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+24 −0
Open

Fix use-after-free of object through __isset() and globals #18852

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions Zend/tests/gh18845.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
--TEST--
GH-18845: Use-after-free of object through __isset() and globals
--FILE--
<?php

class C {
public function __isset($x) {
$GLOBALS['c'] = null;
return true;
}
}

$c = new C;
var_dump($c->prop ?? 1);

?>
--EXPECT--
int(1)
6 changes: 6 additions & 0 deletions Zend/zend_object_handlers.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -905,7 +905,13 @@ ZEND_API zval *zend_std_read_property(zend_object *zobj, zend_string *name, int
if (zobj->ce->__get && !((*guard) & IN_GET)) {
goto call_getter;
}

bool obj_is_freed = GC_REFCOUNT(zobj) == 1;
OBJ_RELEASE(zobj);
if (UNEXPECTED(obj_is_freed)) {
retval = &EG(uninitialized_zval);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This reverts the return value of __isset() from true to false.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Effectively, yes. This is later done anyway (see fallthrough path below), at least unless the object is a lazy object. The correct behavior would be to defer the release call until after the zend_lazy_object_must_init() check, which will require some additional branches (only deref when we previously hit the __get branch), which I didn't deem worth it.

If you prefer, I can implement this behavior.

goto exit;
}
} else if (zobj->ce->__get && !((*guard) & IN_GET)) {
goto call_getter_addref;
}
Expand Down