Skip to content

Commit d8fe645

Browse files
bwoebibwoebi
committed
Fix valgrind errors in phpdbg
Revert "We cannot safely assume that all op array will be refcount 0 after execution" This reverts commit b6936ad. This change turns out to not have been a clever idea and was causing more weirdness than it helped...
1 parent b209531 commit d8fe645

File tree

12 files changed

+561
-516
lines changed

12 files changed

+561
-516
lines changed

‎Zend/zend_compile.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -726,7 +726,7 @@ ZEND_API zend_op_array *compile_filename(int type, zval *filename);
726726
ZEND_API int zend_execute_scripts(int type, zval *retval, int file_count, ...);
727727
ZEND_API int open_file_for_scanning(zend_file_handle *file_handle);
728728
ZEND_API void init_op_array(zend_op_array *op_array, zend_uchar type, int initial_ops_size);
729-
ZEND_API zend_bool destroy_op_array(zend_op_array *op_array);
729+
ZEND_API void destroy_op_array(zend_op_array *op_array);
730730
ZEND_API void zend_destroy_file_handle(zend_file_handle *file_handle);
731731
ZEND_API void zend_cleanup_user_class_data(zend_class_entry *ce);
732732
ZEND_API void zend_cleanup_internal_class_data(zend_class_entry *ce);

‎Zend/zend_opcode.c‎

Lines changed: 3 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -335,7 +335,7 @@ void zend_class_add_ref(zval *zv)
335335
ce->refcount++;
336336
}
337337

338-
ZEND_API zend_bool destroy_op_array(zend_op_array *op_array)
338+
ZEND_API void destroy_op_array(zend_op_array *op_array)
339339
{
340340
zval *literal = op_array->literals;
341341
zval *end;
@@ -353,12 +353,8 @@ ZEND_API zend_bool destroy_op_array(zend_op_array *op_array)
353353
op_array->run_time_cache = NULL;
354354
}
355355

356-
if (!op_array->refcount) {
357-
return 1;
358-
}
359-
360-
if (--(*op_array->refcount) > 0) {
361-
return 0;
356+
if (!op_array->refcount || --(*op_array->refcount) > 0) {
357+
return;
362358
}
363359

364360
efree_size(op_array->refcount, sizeof(*(op_array->refcount)));
@@ -419,8 +415,6 @@ ZEND_API zend_bool destroy_op_array(zend_op_array *op_array)
419415
}
420416
efree(arg_info);
421417
}
422-
423-
return 1;
424418
}
425419

426420
void init_op(zend_op *op)

‎Zend/zend_vm_def.h‎

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -2364,9 +2364,8 @@ ZEND_VM_HELPER(zend_leave_helper, ANY, ANY)
23642364
ZEND_VM_LEAVE();
23652365
} else if (ZEND_CALL_KIND_EX(call_info) == ZEND_CALL_NESTED_CODE) {
23662366
zend_detach_symbol_table(execute_data);
2367-
if (EXPECTED(destroy_op_array(&EX(func)->op_array) != 0)) {
2368-
efree_size(EX(func), sizeof(zend_op_array));
2369-
}
2367+
destroy_op_array(&EX(func)->op_array);
2368+
efree_size(EX(func), sizeof(zend_op_array));
23702369
old_execute_data = execute_data;
23712370
execute_data = EG(current_execute_data) = EX(prev_execute_data);
23722371
zend_vm_stack_free_call_frame_ex(call_info, old_execute_data);
@@ -5455,7 +5454,7 @@ ZEND_VM_HANDLER(73, ZEND_INCLUDE_OR_EVAL, CONST|TMPVAR|CV, ANY)
54555454
}
54565455

54575456
call->prev_execute_data = execute_data;
5458-
i_init_code_execute_data(call, new_op_array, return_value);
5457+
i_init_code_execute_data(call, new_op_array, return_value);
54595458
if (EXPECTED(zend_execute_ex == execute_ex)) {
54605459
ZEND_VM_ENTER();
54615460
} else {
@@ -5464,9 +5463,8 @@ ZEND_VM_HANDLER(73, ZEND_INCLUDE_OR_EVAL, CONST|TMPVAR|CV, ANY)
54645463
zend_vm_stack_free_call_frame(call);
54655464
}
54665465

5467-
if (EXPECTED(destroy_op_array(new_op_array) != 0)) {
5468-
efree_size(new_op_array, sizeof(zend_op_array));
5469-
}
5466+
destroy_op_array(new_op_array);
5467+
efree_size(new_op_array, sizeof(zend_op_array));
54705468
if (UNEXPECTED(EG(exception) != NULL)) {
54715469
zend_throw_exception_internal(NULL);
54725470
HANDLE_EXCEPTION();

‎Zend/zend_vm_execute.h‎

Lines changed: 11 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -503,9 +503,8 @@ static ZEND_OPCODE_HANDLER_RET ZEND_FASTCALL zend_leave_helper_SPEC(ZEND_OPCODE_
503503
ZEND_VM_LEAVE();
504504
} else if (ZEND_CALL_KIND_EX(call_info) == ZEND_CALL_NESTED_CODE) {
505505
zend_detach_symbol_table(execute_data);
506-
if (EXPECTED(destroy_op_array(&EX(func)->op_array) != 0)) {
507-
efree_size(EX(func), sizeof(zend_op_array));
508-
}
506+
destroy_op_array(&EX(func)->op_array);
507+
efree_size(EX(func), sizeof(zend_op_array));
509508
old_execute_data = execute_data;
510509
execute_data = EG(current_execute_data) = EX(prev_execute_data);
511510
zend_vm_stack_free_call_frame_ex(call_info, old_execute_data);
@@ -3670,7 +3669,7 @@ static ZEND_OPCODE_HANDLER_RET ZEND_FASTCALL ZEND_INCLUDE_OR_EVAL_SPEC_CONST_HAN
36703669
}
36713670

36723671
call->prev_execute_data = execute_data;
3673-
i_init_code_execute_data(call, new_op_array, return_value);
3672+
i_init_code_execute_data(call, new_op_array, return_value);
36743673
if (EXPECTED(zend_execute_ex == execute_ex)) {
36753674
ZEND_VM_ENTER();
36763675
} else {
@@ -3679,9 +3678,8 @@ static ZEND_OPCODE_HANDLER_RET ZEND_FASTCALL ZEND_INCLUDE_OR_EVAL_SPEC_CONST_HAN
36793678
zend_vm_stack_free_call_frame(call);
36803679
}
36813680

3682-
if (EXPECTED(destroy_op_array(new_op_array) != 0)) {
3683-
efree_size(new_op_array, sizeof(zend_op_array));
3684-
}
3681+
destroy_op_array(new_op_array);
3682+
efree_size(new_op_array, sizeof(zend_op_array));
36853683
if (UNEXPECTED(EG(exception) != NULL)) {
36863684
zend_throw_exception_internal(NULL);
36873685
HANDLE_EXCEPTION();
@@ -29068,7 +29066,7 @@ static ZEND_OPCODE_HANDLER_RET ZEND_FASTCALL ZEND_INCLUDE_OR_EVAL_SPEC_CV_HANDLE
2906829066
}
2906929067

2907029068
call->prev_execute_data = execute_data;
29071-
i_init_code_execute_data(call, new_op_array, return_value);
29069+
i_init_code_execute_data(call, new_op_array, return_value);
2907229070
if (EXPECTED(zend_execute_ex == execute_ex)) {
2907329071
ZEND_VM_ENTER();
2907429072
} else {
@@ -29077,9 +29075,8 @@ static ZEND_OPCODE_HANDLER_RET ZEND_FASTCALL ZEND_INCLUDE_OR_EVAL_SPEC_CV_HANDLE
2907729075
zend_vm_stack_free_call_frame(call);
2907829076
}
2907929077

29080-
if (EXPECTED(destroy_op_array(new_op_array) != 0)) {
29081-
efree_size(new_op_array, sizeof(zend_op_array));
29082-
}
29078+
destroy_op_array(new_op_array);
29079+
efree_size(new_op_array, sizeof(zend_op_array));
2908329080
if (UNEXPECTED(EG(exception) != NULL)) {
2908429081
zend_throw_exception_internal(NULL);
2908529082
HANDLE_EXCEPTION();
@@ -40480,7 +40477,7 @@ static ZEND_OPCODE_HANDLER_RET ZEND_FASTCALL ZEND_INCLUDE_OR_EVAL_SPEC_TMPVAR_HA
4048040477
}
4048140478

4048240479
call->prev_execute_data = execute_data;
40483-
i_init_code_execute_data(call, new_op_array, return_value);
40480+
i_init_code_execute_data(call, new_op_array, return_value);
4048440481
if (EXPECTED(zend_execute_ex == execute_ex)) {
4048540482
ZEND_VM_ENTER();
4048640483
} else {
@@ -40489,9 +40486,8 @@ static ZEND_OPCODE_HANDLER_RET ZEND_FASTCALL ZEND_INCLUDE_OR_EVAL_SPEC_TMPVAR_HA
4048940486
zend_vm_stack_free_call_frame(call);
4049040487
}
4049140488

40492-
if (EXPECTED(destroy_op_array(new_op_array) != 0)) {
40493-
efree_size(new_op_array, sizeof(zend_op_array));
40494-
}
40489+
destroy_op_array(new_op_array);
40490+
efree_size(new_op_array, sizeof(zend_op_array));
4049540491
if (UNEXPECTED(EG(exception) != NULL)) {
4049640492
zend_throw_exception_internal(NULL);
4049740493
HANDLE_EXCEPTION();

‎sapi/phpdbg/phpdbg.c‎

Lines changed: 3 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -213,8 +213,8 @@ static PHP_RSHUTDOWN_FUNCTION(phpdbg) /* {{{ */
213213
zend_hash_destroy(&PHPDBG_G(bp)[PHPDBG_BREAK_METHOD]);
214214
zend_hash_destroy(&PHPDBG_G(bp)[PHPDBG_BREAK_COND]);
215215
zend_hash_destroy(&PHPDBG_G(bp)[PHPDBG_BREAK_MAP]);
216-
zend_hash_destroy(&PHPDBG_G(seek));
217216
zend_hash_destroy(&PHPDBG_G(file_sources));
217+
zend_hash_destroy(&PHPDBG_G(seek));
218218
zend_hash_destroy(&PHPDBG_G(registered));
219219
zend_hash_destroy(&PHPDBG_G(watchpoints));
220220
zend_llist_destroy(&PHPDBG_G(watchlist_mem));
@@ -234,12 +234,6 @@ static PHP_RSHUTDOWN_FUNCTION(phpdbg) /* {{{ */
234234
PHPDBG_G(oplog) = NULL;
235235
}
236236

237-
if (PHPDBG_G(ops)) {
238-
destroy_op_array(PHPDBG_G(ops));
239-
efree(PHPDBG_G(ops));
240-
PHPDBG_G(ops) = NULL;
241-
}
242-
243237
if (PHPDBG_G(oplog_list)) {
244238
phpdbg_oplog_list *cur = PHPDBG_G(oplog_list);
245239
do {
@@ -558,8 +552,8 @@ static PHP_FUNCTION(phpdbg_get_executable)
558552
phpdbg_file_source *source = zend_hash_find_ptr(&PHPDBG_G(file_sources), name);
559553
if (source) {
560554
phpdbg_oplog_fill_executable(
561-
source->op_array,
562-
phpdbg_add_empty_array(Z_ARR_P(return_value), source->op_array->filename),
555+
&source->op_array,
556+
phpdbg_add_empty_array(Z_ARR_P(return_value), source->op_array.filename),
563557
by_opcode);
564558
}
565559
} ZEND_HASH_FOREACH_END();

‎sapi/phpdbg/phpdbg_list.c‎

Lines changed: 8 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -294,35 +294,28 @@ zend_op_array *phpdbg_compile_file(zend_file_handle *file, int type) {
294294
zend_op_array *phpdbg_init_compile_file(zend_file_handle *file, int type) {
295295
char *filename = (char *)(file->opened_path ? ZSTR_VAL(file->opened_path) : file->filename);
296296
char resolved_path_buf[MAXPATHLEN];
297-
zend_op_array *ret;
297+
zend_op_array *op_array;
298298
phpdbg_file_source *dataptr;
299299

300300
if (VCWD_REALPATH(filename, resolved_path_buf)) {
301301
filename = resolved_path_buf;
302302
}
303303

304-
ret = PHPDBG_G(init_compile_file)(file, type);
304+
op_array = PHPDBG_G(init_compile_file)(file, type);
305305

306-
if (ret == NULL) {
306+
if (op_array == NULL) {
307307
return NULL;
308308
}
309309

310310
dataptr = zend_hash_str_find_ptr(&PHPDBG_G(file_sources), filename, strlen(filename));
311311
ZEND_ASSERT(dataptr != NULL);
312312

313-
dataptr->op_array = ret;
314-
dataptr->destroy_op_array = 1;
315-
if (dataptr->op_array) {
316-
if (dataptr->op_array->refcount) {
317-
++*dataptr->op_array->refcount;
318-
} else {
319-
dataptr->op_array->refcount = emalloc(sizeof(uint32_t));
320-
*dataptr->op_array->refcount = 2;
321-
dataptr->destroy_op_array = 0;
322-
}
313+
dataptr->op_array = *op_array;
314+
if (dataptr->op_array.refcount) {
315+
efree(op_array);
323316
}
324317

325-
return ret;
318+
return &dataptr->op_array;
326319
}
327320

328321
void phpdbg_free_file_source(zval *zv) {
@@ -332,12 +325,7 @@ void phpdbg_free_file_source(zval *zv) {
332325
efree(data->buf);
333326
}
334327

335-
if (!data->destroy_op_array) {
336-
efree(data->op_array->refcount);
337-
}
338-
if (!data->destroy_op_array || destroy_op_array(data->op_array)) {
339-
efree(data->op_array);
340-
}
328+
destroy_op_array(&data->op_array);
341329

342330
efree(data);
343331
}

‎sapi/phpdbg/phpdbg_list.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ typedef struct {
4848
#if HAVE_MMAP
4949
void *map;
5050
#endif
51-
zend_op_array *op_array;
51+
zend_op_array op_array;
5252
zend_bool destroy_op_array;
5353
uint lines;
5454
uint line[1];

0 commit comments

Comments
 (0)