Repository navigation
Conversation
Free OP_DATA when appending to a full array, and when converting a float or resource key throws, matching the VM.
329267a to
fa6e85c
Compare
iliaal
left a comment
iliaal
left a comment
There was a problem hiding this comment.
Please address the inline double-free.
| free_op_data: | ||
| if (opline->opcode == ZEND_ASSIGN_DIM | ||
| && ((opline+1)->op1_type & (IS_VAR | IS_TMP_VAR))) { | ||
| zval_ptr_dtor_nogc(EX_VAR((opline+1)->op1.var)); |
There was a problem hiding this comment.
This helper is also called from zend_jit_assign_dim_helper() after converting false to an array. If float or resource key conversion throws, OP_DATA is freed here and then freed again by the caller's unconditional jit_FREE_OP().
With $a = false; $a[1.5] = new D; and an error handler that suppresses the false-to-array deprecation but throws on the key-conversion warning, the PR head aborts in a debug build and Valgrind reports invalid reads. The base revision completes cleanly.
Please keep OP_DATA ownership with the caller and perform the missing cleanup in the direct generated fetch path. This also needs false-container variants of the float and resource tests.
There was a problem hiding this comment.
Thanks for catching it. I pushed another commit. Not sure about the new helper approach, whether it's acceptable solution, but it keeps the balance between code size and new instructions.
zend_jit_fetch_dim_w_helper() no longer frees OP_DATA, which zend_jit_assign_dim_helper() frees again after converting false to an array. The generated ASSIGN_DIM path selects dedicated fetch and append-failure helpers at compile time that free OP_DATA, so generated code size is unchanged.
iliaal
left a comment
iliaal
left a comment
There was a problem hiding this comment.
That seems to do the trick, thanks!
Looks good to me, plz wait for @ndossche or @arnaud-lb to confirm before merging.
| return 1; | ||
| } | ||
|
|
||
| static int zend_jit_assign_dim_cannot_add_element_stub(zend_jit_ctx *jit) |
There was a problem hiding this comment.
I don't think you need this new stub, why do we need this indirection?
|
|
||
| static zval* ZEND_FASTCALL zend_jit_assign_dim_fetch_w_helper(zend_array *ht, zval *dim) | ||
| { | ||
| zend_execute_data *execute_data = EG(current_execute_data); |
There was a problem hiding this comment.
it would be better to move the execute_data and opline loads to the !retval error path IMO. To deal with opline changing, then check for an exception and use EG(opline_before_exception).
Same remark for the other helper
Free OP_DATA when appending to a full array, and when converting a float or resource key throws, matching the VM.