Fix GH-17736: Assertion failure zend_reference_destroy() #17739
Conversation
The cache slot for FETCH_OBJ_W in function `test` is primed with the class for C. The next call uses a simplexml instance and reuses the same cache slot. simplexml's get_property_ptr handler does not use the cache slot, so the old values remain in the cache slot. When `zend_handle_fetch_obj_flags` is called this is not guarded by a check for the class entry. So we end up using the prop_info from the property C::$a instead of the simplexml property. This patch adds a check for the class entry. I placed the check as late as possible to avoid as much overhead as possible. An alternative solution is to write NULLs to the cache slot in the get_property_ptr handlers of extensions that don't use the cache slot, but that is not general: not only simplexml would need changes, maybe even third party extensions would need changes as well.
|
Does this need handling in the JIT? Specifically, |
|
Probably, hang on. |
|
Pushed the equivalent change for JIT, I could reproduce it with function JIT. |
| if (prop_info) { | ||
| if (prop_info && EXPECTED(zobj->ce == CACHED_PTR_EX(cache_slot))) { |
There was a problem hiding this comment.
There is something wrong.
This check should be done in zend_get_property_offset().
There was a problem hiding this comment.
May be sxe_property_get_adr should invalidate the cache.
There was a problem hiding this comment.
@dstogov Niels referenced this solution here:
An alternative solution is to write NULLs to the cache slot in the get_property_ptr handlers of extensions that don't use the cache slot, but that is not general: not only simplexml would need changes, maybe even third party extensions would need changes as well.
I think it's not obvious that handlers that don't use the cache need to clear it. It's probably the better solution, but we should document it clearly (maybe in the comments of the handler for best visibility, and then in UPGRADING.INTERNALS).
There was a problem hiding this comment.
@dstogov Niels referenced this solution here:
An alternative solution is to write NULLs to the cache slot in the get_property_ptr handlers of extensions that don't use the cache slot, but that is not general: not only simplexml would need changes, maybe even third party extensions would need changes as well.
I think it's not obvious that handlers that don't use the cache need to clear it. It's probably the better solution, but we should document it clearly (maybe in the comments of the handler for best visibility, and then in UPGRADING.INTERNALS).
I'm not sure about the best way to fix this, but the current solution that keeps run-time cache in inconsistent state looks like a hack. Especially for zend_fetch_property_address() we may clear it right here (after the first failed check).
The cache slot for FETCH_OBJ_W in function
testis primed forC::$a. The next call uses a simplexml instance and reuses the same cache slot. simplexml's get_property_ptr handler does not use the cache slot, so the old values remain in the cache slot. Whenzend_handle_fetch_obj_flagsis called this is not guarded by a check for the class entry. So we end up using the prop_info from the propertyC::$ainstead of the simplexml property.This patch adds a check for the class entry. I placed the check as late as possible to avoid as much overhead as possible. An alternative solution is to write NULLs to the cache slot in the get_property_ptr handlers of extensions that don't use the cache slot, but that is not general: not only simplexml would need changes, maybe even third party extensions would need changes as well.