Fix GH-17747: Exception on reading property in register-based FETCH_OBJ_R breaks JIT #17749
Conversation
…H_OBJ_R breaks JIT When read_property fails, it may return `&EG(uninitialized_zval)`, and the exception is handled in the VM. The VM will try to `zval_ptr_dtor_nogc` the result, but the result was never set, resulting in dtor'ing garbage data. To solve this, we check when a different zval* was returned and initialize the result with UNDEF. We don't need to copy as the slow_ex handler return values are used directly in a register.
ext/opcache/jit/zend_jit_helpers.c
Outdated
| @@ -2017,7 +2022,12 @@ static zval* ZEND_FASTCALL zend_jit_fetch_obj_is_slow_ex(zend_object *zobj) | |||
| void **cache_slot = CACHE_ADDR(opline->extended_value & ~ZEND_FETCH_OBJ_FLAGS); | |||
|
|
|||
| retval = zobj->handlers->read_property(zobj, name, BP_VAR_IS, cache_slot, result); | |||
| if (retval == result && UNEXPECTED(Z_ISREF_P(retval))) { | |||
| if (retval != result) { | |||
There was a problem hiding this comment.
I could also check for &EG(uninitialized_zval) but that check may be more expensive and we need to compare retval and result anyway for the zend_unwrap_reference below.
On the other hand, the current approach may cause more memory writes.
ext/opcache/jit/zend_jit_helpers.c
Outdated
| if (retval == result && UNEXPECTED(Z_ISREF_P(retval))) { | ||
| if (retval != result) { | ||
| /* On exception, we may free the result in the ZEND_HANDLE_EXCEPTION VM handler | ||
| * so we need to initialize the result. | ||
| * We don't need to copy because we use the return value directly as a register. */ | ||
| ZVAL_UNDEF(result); | ||
| } else if (UNEXPECTED(Z_ISREF_P(retval))) { |
There was a problem hiding this comment.
This may cause problems for case without exceptions, because the type in memory is changed and it may be inconsistent with inferred type.
I think, it's better to use zend_jit_check_exception_undef_result() instead of zend_jit_check_exception() when Z_MODE(res_addr) == IS_REG.
There was a problem hiding this comment.
This may cause problems for case without exceptions, because the type in memory is changed and it may be inconsistent with inferred type.
I thought this didn't matter as the value isn't actually used (the bytes are uninitialized anyway in this case).
I think, it's better to use zend_jit_check_exception_undef_result() instead of zend_jit_check_exception() when Z_MODE(res_addr) == IS_REG.
I didn't know this helper existed, I should've looked better. This idea is actually something I briefly thought of doing myself but didn't know it existed already. Thanks for pointing out.
When read_property fails, it may return
&EG(uninitialized_zval), and the exception is handled in the VM. The VM will try tozval_ptr_dtor_nogcthe result, but the result was never set, resulting in dtor'ing garbage data. To solve this, we check when a different zval* was returned and initialize the result with UNDEF. We don't need to copy as the slow_ex handler return values are used directly in a register.