Skip to content

Update interface.c#7376

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
AlexeyZamorov wants to merge 1 commit into from
Closed

Update interface.c #7376

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions ext/curl/interface.c
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -1625,6 +1625,7 @@ static int curl_debug(CURL *cp, curl_infotype type, char *buf, size_t buf_len, v
if (type == CURLINFO_HEADER_OUT) {
if (ch->header.str) {
zend_string_release_ex(ch->header.str, 0);
ch->header.str = NULL;

@nikicnikicAug 17, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the problematic case where buf_len == 0, which means that this releases ch->header.str, but due to the if in the following line, it will not be overwritten by a new buffer?

@AlexeyZamorovAlexeyZamorovAug 17, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found this problem in versions 5.5-5.6. The script crashed with the message "zend_mm_heap is corrupted", but support for these versions was already completed, and I did not check new versions.

Then the fall was caused by freeing memory without resetting the string length to zero. As I understand it, the re-request caused an attempt to free memory that was no longer owned by the variable. But I'm not sure since I'm not an expert.

We have now switched to versions 7 and 8 and the same problem occurred. The script crashes steadily on the second request with the message "Bus error".

I will try to write some code to check for this error.

@AlexeyZamorovAlexeyZamorovAug 26, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, I still cannot localize the error and make an example. On a working program without "ch>header.str = NULL;" the script lives no more than 10 minutes.

I am attaching info from gdb. It looks like the problem does not occur when using the curl_debug function, but after in the garbage collector.

Assertion failed: (source_list->ptr == prop), function zend_ref_del_type_source, file /root/source/php/php-8.0.9-debug/Zend/zend_execute.c, line 3306.

(gdb) bt
#0 0x0000000803194b8c in thr_kill () from /lib/libc.so.7
#1 0x00000008032319eb in abort () from /lib/libc.so.7
#2 0x000000080321ad85 in __assert () from /lib/libc.so.7
#3 0x0000000000967113 in zend_ref_del_type_source (source_list=0x810d64d18, prop=0x80377b9c0) at /root/source/php/php-8.0.9-debug/Zend/zend_execute.c:3306
#4 0x0000000000a12e08 in zend_object_std_dtor (object=0x8106cd000) at /root/source/php/php-8.0.9-debug/Zend/zend_objects.c:67
#5 0x0000000000a1a1c1 in zend_objects_store_del (object=0x8106cd000) at /root/source/php/php-8.0.9-debug/Zend/zend_objects_API.c:193
#6 0x0000000000933e64 in rc_dtor_func (p=0x8106cd000) at /root/source/php/php-8.0.9-debug/Zend/zend_variables.c:57
#7 0x0000000000a13029 in i_zval_ptr_dtor (zval_ptr=0x803678578) at zend_variables.h:44
#8 0x0000000000a12e11 in zend_object_std_dtor (object=0x803678500) at /root/source/php/php-8.0.9-debug/Zend/zend_objects.c:70
#9 0x00000000009fcf60 in zend_gc_collect_cycles () at /root/source/php/php-8.0.9-debug/Zend/zend_gc.c:1584
#10 0x00000000009536dc in zif_gc_collect_cycles (execute_data=0x803615330, return_value=0x7fffffffca40) at /root/source/php/php-8.0.9-debug/Zend/zend_builtin_functions.c:93
#11 0x000000000096e71d in ZEND_DO_FCALL_BY_NAME_SPEC_RETVAL_UNUSED_HANDLER (execute_data=0x8036150a0) at zend_vm_execute.h:1460
#12 0x00000000009ea16c in execute_ex (ex=0x803615020) at zend_vm_execute.h:54331
#13 0x00000000009ea2a4 in zend_execute (op_array=0x80365d280, return_value=0x0) at zend_vm_execute.h:58871
#14 0x0000000000938446 in zend_execute_scripts (type=8, retval=0x0, file_count=3) at /root/source/php/php-8.0.9-debug/Zend/zend.c:1680
#15 0x00000000008a28de in php_execute_script (primary_file=0x7fffffffe730) at /root/source/php/php-8.0.9-debug/main/main.c:2524
#16 0x0000000000a26690 in do_cli (argc=4, argv=0x7fffffffe9e8) at /root/source/php/php-8.0.9-debug/sapi/cli/php_cli.c:949
#17 0x0000000000a274ec in main (argc=4, argv=0x7fffffffe9e8) at /root/source/php/php-8.0.9-debug/sapi/cli/php_cli.c:1337

@nikicnikicAug 31, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't manage to reproduce anything either, but I found a related issue, which is fixed by 30e791e.

@nikicnikicAug 31, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I merged a variant of your fix in 8c292a2. Rather than nulling out header.str, it drops the empty string check to make sure the old pointer is always overwritten.

@AlexeyZamorovAlexeyZamorovSep 2, 2021•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I applied this fix on 8.0.10 and the problem disappeared completely. Thanks.

}
if (buf_len > 0) {
ch->header.str = zend_string_init(buf, buf_len, 0);
Expand DownExpand Up@@ -3468,6 +3469,7 @@ static void curl_free_obj(zend_object *object)
zval_ptr_dtor(&ch->handlers.std_err);
if (ch->header.str) {
zend_string_release_ex(ch->header.str, 0);
ch->header.str = NULL;

@nikicnikicAug 17, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is unlikely to be necessary, as this function destroys ch and it cannot be used afterwards anymore.

@AlexeyZamorovAlexeyZamorovAug 17, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about the usefulness of this line. You, as a developer, know better whether need it here. My code works fine with it.

}

zval_ptr_dtor(&ch->handlers.write_header->stream);
Expand Down