ext/opcache/jit/zend_jit_trace: fix memory leak in _compile_root_trace() #10146
Conversation
A copy of this piece of code exists in zend_jit_compile_side_trace(), but there, the leak bug does not exist. This bug exists since both copies of this piece of code were added in commit 4bf2d09
| @@ -7099,6 +7099,7 @@ static zend_jit_trace_stop zend_jit_compile_root_trace(zend_jit_trace_rec *trace | |||
| if (t->stack_map_size) { | |||
| zend_jit_trace_stack *shared_stack_map = (zend_jit_trace_stack*)zend_shared_alloc(t->stack_map_size * sizeof(zend_jit_trace_stack)); | |||
| if (!shared_stack_map) { | |||
| efree(t->stack_map); | |||
There was a problem hiding this comment.
should not you set it to NULL after ?
There was a problem hiding this comment.
It's not set to NULL in zend_jit_compile_side_trace()... in other code paths, it is however set to NULL. But I guess those could be removed, because nobody cares - if anywould would care, they would be confused as to whether to free it with efree() or not free it because it's already in shared memory. In any case, this code is unclear and confusing.
There was a problem hiding this comment.
Upon further investigation, t->stack_map is not accessed again in this function and I believe that t / zend_jit_traces[ZEND_JIT_TRACE_NUM] is considered free and will be initialized when reused, so it's probably safe to not set it to NULL.
On the other hand, the performance implications of setting it to NULL are practically 0 here, and this would reduce the confusion / increase consistency with other branches.
There was a problem hiding this comment.
Meanwhile I have written a draft commit (to be submitted as follow-up for #10147) which removes the NULL setters in other branches. I think setting stuff to NULL only adds confusion because it makes the reader believe NULL has a meaning, but here it has none.
|
@dstogov, please review. As I mentioned in review comment #10146 (comment) I have a patch for the master branch which removes the unnecessary NULL setters, which I will submit after #10147 is merged. |
* PHP-8.1: ext/opcache/jit/zend_jit_trace: fix memory leak in _compile_root_trace() (#10146)
* PHP-8.2: ext/opcache/jit/zend_jit_trace: fix memory leak in _compile_root_trace() (#10146)
A copy of this piece of code exists in zend_jit_compile_side_trace(), but there, the leak bug does not exist.
This bug exists since both copies of this piece of code were added in commit 4bf2d09