Fix GH-10168: heap-buffer-overflow at zval_undefined_cv #10500
Conversation
Fixes phpGH-10168 The problem is that we're using the variable_ptr in the opcode handler *after* it has already been destroyed. The solution is to delay executing the destructors until after the variable_ptr is used. To accomplish this we introduce 2 new API functions: * zend_assign_to_variable_delay_garbage_handling(); and * zend_assign_to_variable_handle_garbage() that allows users to delay the garbage handling. zend_assign_to_variable() is now a wrapper for those such that there is no BC break. We only have to apply this to ASSIGN_DIM and ASSIGN. That's because the others rely on properties, in which case the variable_ptr can't be destroyed. The first commit fixes the bug, the second one adds a regression test. Comparing the performance before and after this fix using Valgrind on Zend/bench.php. Instruction counts yields 3,339,632,671 before and 3,340,132,746 after. The difference is only a performance decrease of ~0.015%, which seems negligible.
Co-authored-by: Changochen <[email protected]>
|
I pushed the name change. Thanks. |
There was a problem hiding this comment.
In general the fix is right and performance degradation is not critical, however, I would prefer to try a different approach to fix this.
Instead of delaying destructor, I suggest to create a clone of zend_assign_to_variable() with two destinations (e.g. . zend_assign_to_two_variables) This wouldn't affect assignments without return value and would reduce number of reference-counting for assignments with return values. (Something similar is done in JIT). I can not be sure which approach is better without implementation.
@nielsdos, @iluuu1994 can you try implementing this or I may try to do this myself.
Please respond to let me start or wait for your result. Any other arguments are welcome.
|
@dstogov @iluuu1994 Thanks for checking this. I'll try dstogov's suggestion tonight and create an alternative PR for that. I'll also do the same performance measurement. We can then see which PR gets closed and which PR gets merged :) |
|
Alternative implementation available at GH-10524. |
| static zend_always_inline zval* zend_assign_to_variable(zval *variable_ptr, zval *value, zend_uchar value_type, bool strict) | ||
| { | ||
| zend_refcounted *garbage; | ||
| variable_ptr = zend_assign_to_variable_delay_garbage_handling(variable_ptr, value, value_type, strict, &garbage); | ||
| zend_handle_garbage_from_variable_assignment(garbage); | ||
| return variable_ptr; | ||
| } | ||
|
|
There was a problem hiding this comment.
Please keep the original version of zend_assign_to_variable(). This will eliminate effect on everything except ASSIGN and ASSIGN_DIM.
| value = zend_assign_to_variable(variable_ptr, value, OP2_TYPE, EX_USES_STRICT_TYPES()); | ||
| zend_refcounted *garbage; | ||
| value = zend_assign_to_variable_delay_garbage_handling(variable_ptr, value, OP2_TYPE, EX_USES_STRICT_TYPES(), &garbage); | ||
| if (UNEXPECTED(RETURN_VALUE_USED(opline))) { | ||
| ZVAL_COPY(EX_VAR(opline->result.var), value); | ||
| } | ||
| zend_handle_garbage_from_variable_assignment(garbage); | ||
|
|
There was a problem hiding this comment.
Rewrite this to allow specializer selecting the more efficient version.
if (RETURN_VALUE_USED(opline)) {
zend_refcounted *garbage;
value = zend_assign_to_variable_delay_garbage_handling(variable_ptr, value, OP2_TYPE, EX_USES_STRICT_TYPES(), &garbage);
ZVAL_COPY(EX_VAR(opline->result.var), value);
zend_handle_garbage_from_variable_assignment(garbage);
} else {
zend_assign_to_variable(variable_ptr, value, OP2_TYPE, EX_USES_STRICT_TYPES());
}|
Closing this one as preference was given to the alternative approach. |
Fixes #10168
cc'ing @iluuu1994 since he also was involved in the issue report
The problem is that we're using the variable_ptr in the opcode handler
after it has already been destroyed. The solution is to delay
executing the destructors until after the variable_ptr is used.
To accomplish this we introduce 2 new API functions:
the garbage handling. zend_assign_to_variable() is now a wrapper for
those such that there is no BC break.
We only have to apply this to ASSIGN_DIM and ASSIGN. That's because the
others rely on properties, in which case the variable_ptr can't be
destroyed.
The first commit fixes the bug, the second one adds a regression test.
Comparing the performance before and after this fix using Valgrind on
Zend/bench.php.
Instruction counts yields 3,339,632,671 before and 3,340,132,746 after.
The difference is only a performance decrease of ~0.015%, which seems
negligible.
Additional notes
I do have a local, experimental, patchset that improves performance of ZEND_ASSIGN and even results in a net win, but that's kinda unrelated to this PR and probably master-only material. Just thought I'd mention it since I did measurements of VM performance's tiny decrease.