Fix use-after-free of object through __isset() and globals#18852
Fix use-after-free of object through __isset() and globals#18852iluuu1994 wants to merge 2 commits intophp:masterfrom
Conversation
Zend/zend_object_handlers.c
Outdated
| bool obj_is_freed = GC_REFCOUNT(zobj) == 1; | ||
| OBJ_RELEASE(zobj); | ||
| if (UNEXPECTED(obj_is_freed)) { | ||
| retval = &EG(uninitialized_zval); |
There was a problem hiding this comment.
This reverts the return value of __isset() from true to false.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I don't see a big danger (just made a note).
I don't object against this fix (it looks good to me).
@nielsdos please also take a quick look.
There was a problem hiding this comment.
I kinda liked the original approach more because the fix was more "localized". I also don't think it's a big danger, especially because the test is pathological code.
Anyway, one question I had was with the obj_is_freed condition. What if the object is cyclic, and the refcount>1, but after the OBJ_RELEASE call the object is released due to its cycles anyway: is this possible?
I tried to cheat it, but didn't manage to make it crash.
There was a problem hiding this comment.
I also like the original approach.
I think we don't need to set retval here, as it is initialized to this value above, and is not changed after that? The result of __isset is only used to decide whether to call __get, and is not exposed. $c->prop ?? null is always null when the object implements __isset but not __get.
I'm not sure that falling-through when __isset returns true was on purpose. I think we can goto exit in all cases.
There was a problem hiding this comment.
I'm not sure that falling-through when __isset returns true was on purpose. I think we can goto exit in all cases.
This would change behavior for lazy objects, not initializing them and thus potentially changing behavior for dynamic properties created in initializers. I don't know what the desired semantics are, I'll rely on your expertise here.
There was a problem hiding this comment.
I believe the existing behavior was not on purpose / is a bug.
We normally do not initialize lazy objects when there is a magic method to carry the operation. The reason is that we assume that if the magic method depends on the object's state or observes it, its execution will trigger initialization as needed.
In this particular case we call __isset() followed by __get() (if __isset() returned true). But when __get() is not defined, we always return null. Therefore, the object's state is not used after the __isset() call, so we don't need initialization.
|
I adjusted the logic now for lazy objects in a separate commit for comparison. It doesn't touch any hot-paths, so it should be ok. It's a bit more complex though, so I don't have a particular preference. I'm happy with whatever you prefer. /cc @arnaud-lb |
4 participants
Fixes GH-18845