Skip to content

Fix bug 70470 #2736

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

Jump to bottom
Closed
bouk wants to merge 1 commit into from
Closed

Fix bug 70470 #2736

bouk wants to merge 1 commit into from

Conversation

@bouk
Copy link
Contributor

@bouk bouk commented Sep 4, 2017

This fixes https://bugs.php.net/bug.php?id=70470 by merging together repeat invocations of on_header_field and on_header_value by appending the strings together into one. After this PR the XFAILing testcase passes

cc @esnunes

@nikic
Copy link
Member

nikic commented Sep 4, 2017

The ext/soap/tests/bug73037.phpt test failure might be legitimate.

nikic
nikic reviewed Sep 4, 2017
View reviewed changes
sapi/cli/php_cli_server.c
zend_string_release(orig_header_name);
switch (client->last_header_element) {
case VALUE:
php_cli_server_client_save_header(client);
Copy link
Member

@nikic nikic Sep 4, 2017

Choose a reason for hiding this comment

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

/* break missing intentionally */

nikic
nikic reviewed Sep 4, 2017
View reviewed changes
sapi/cli/php_cli_server.c Outdated
if (client->current_header_name_allocated) {
size_t new_length = client->current_header_name_len + length;
client->current_header_name = perealloc(client->current_header_name, new_length + 1, 1);
strncpy(client->current_header_name + client->current_header_name_len, at, length);
Copy link
Member

@nikic nikic Sep 4, 2017

Choose a reason for hiding this comment

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

I think it's safer to use memcpy here and below, to avoid uninitialized memory in case there are null bytes (I have no idea if it can actually happen).

nikic
nikic reviewed Sep 4, 2017
View reviewed changes
sapi/cli/php_cli_server.c Outdated
break;
case NONE:
// can't happen
assert(1);
Copy link
Member

@nikic nikic Sep 4, 2017

Choose a reason for hiding this comment

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

Should be assert(0)?

nikic
nikic reviewed Sep 4, 2017
View reviewed changes
sapi/cli/php_cli_server.c Outdated
switch (client->last_header_element) {
case NONE:
break;
case FIELD:
Copy link
Member

@nikic nikic Sep 4, 2017

Choose a reason for hiding this comment

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

Can the FIELD case occur here and if so, will it be handled correctly?

Copy link
Contributor Author

@bouk bouk Sep 5, 2017

Choose a reason for hiding this comment

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

Yes it can occur and it needs to be handled appropriately, fixed

nikic
nikic reviewed Sep 4, 2017
View reviewed changes
sapi/cli/php_cli_server.c Outdated
case VALUE:
php_cli_server_client_save_header(client);
case NONE:
client->current_header_name = at;
Copy link
Member

@nikic nikic Sep 4, 2017

Choose a reason for hiding this comment

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

This discards a const qualifier, so needs a (char *) cast.

@nikic
Copy link
Member

nikic commented Sep 4, 2017

The soap failure is legitimate. Valgrind:

==9578== Conditional jump or move depends on uninitialised value(s)
==9578==    at 0xD7E4BF: php_cli_server_client_save_header (php_cli_server.c:1583)
==9578==    by 0xD7E55A: php_cli_server_client_read_request_on_header_field (php_cli_server.c:1598)
==9578==    by 0xD78FCB: php_http_parser_execute (php_http_parser.c:1121)
==9578==    by 0xD7EB26: php_cli_server_client_read_request (php_cli_server.c:1734)
==9578==    by 0xD80AD6: php_cli_server_recv_event_read_request (php_cli_server.c:2352)
==9578==    by 0xD80FA6: php_cli_server_do_event_for_each_fd_callback (php_cli_server.c:2451)
==9578==    by 0xD7C80C: php_cli_server_poller_iter_on_active (php_cli_server.c:846)
==9578==    by 0xD8103C: php_cli_server_do_event_for_each_fd (php_cli_server.c:2469)
==9578==    by 0xD810C4: php_cli_server_do_event_loop (php_cli_server.c:2479)
==9578==    by 0xD8148B: do_cli_server (php_cli_server.c:2581)
==9578==    by 0xD7717B: main (php_cli.c:1407)
==9578==
==9578== Conditional jump or move depends on uninitialised value(s)
==9578==    at 0xD7E4BF: php_cli_server_client_save_header (php_cli_server.c:1583)
==9578==    by 0xD7E7C5: php_cli_server_client_read_request_on_headers_complete (php_cli_server.c:1658)
==9578==    by 0xD79661: php_http_parser_execute (php_http_parser.c:1329)
==9578==    by 0xD7EB26: php_cli_server_client_read_request (php_cli_server.c:1734)
==9578==    by 0xD80AD6: php_cli_server_recv_event_read_request (php_cli_server.c:2352)
==9578==    by 0xD80FA6: php_cli_server_do_event_for_each_fd_callback (php_cli_server.c:2451)
==9578==    by 0xD7C80C: php_cli_server_poller_iter_on_active (php_cli_server.c:846)
==9578==    by 0xD8103C: php_cli_server_do_event_for_each_fd (php_cli_server.c:2469)
==9578==    by 0xD810C4: php_cli_server_do_event_loop (php_cli_server.c:2479)
==9578==    by 0xD8148B: do_cli_server (php_cli_server.c:2581)
==9578==    by 0xD7717B: main (php_cli.c:1407)
==9578==

Line 1583 corresponds to line 1588 in this PR: https://github.com/php/php-src/pull/2736/files#diff-d705e80d519f022dee38ccc6c177a926R1588

nikic
nikic reviewed Sep 4, 2017
View reviewed changes
sapi/cli/php_cli_server.c
client->last_header_element = NONE;
client->current_header_name = NULL;
client->current_header_name_len = 0;
client->current_header_name_allocated = 0;
Copy link
Member

@nikic nikic Sep 4, 2017

Choose a reason for hiding this comment

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

This initialization should not be removed. Alternatively or additionally the variable should be initialized in https://github.com/php/php-src/pull/2736/files#diff-d705e80d519f022dee38ccc6c177a926R1636.

nikic
nikic reviewed Sep 4, 2017
View reviewed changes
sapi/cli/php_cli_server.c Outdated
unsigned int current_header_name_allocated:1;
char *current_header_value;
size_t current_header_value_len;
enum { NONE=0, FIELD, VALUE } last_header_element;
Copy link
Member

@nikic nikic Sep 4, 2017

Choose a reason for hiding this comment

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

I'd suggest prefixing the enum members, e.g. HEADER_NONE, HEADER_FIELD, HEADER_VALUE.

@bouk
Copy link
Contributor Author

bouk commented Sep 5, 2017

I think I've addressed all your comments adequately, thanks for the thorough review

@nikic
Copy link
Member

nikic commented Sep 5, 2017

Merged as cd9d90f into 7.0+. Thanks!

@nikic nikic closed this Sep 5, 2017
@nikic
Copy link
Member

nikic commented Sep 5, 2017

Hrm, looks like a new test failure showed up on AppVeyor:

========DIFF========
003+ Date: Tue, 05 Sep 2017 14:58:09 +0000
004+ Connection: close
005+ X-Powered-By: PHP/7.1.10-dev
006+ Content-type: text/html; charset=UTF-8
007+ 
009-   string(5) "close"
014+   string(7) "closent"
========DONE========
FAIL Bug #70470 (Built-in server truncates headers spanning over TCP packets) [C:\projects\php-src\sapi\cli\tests\bug70470.phpt]

@nikic
Copy link
Member

nikic commented Sep 5, 2017

Valgrind:

==22753== Conditional jump or move depends on uninitialised value(s)
==22753==    at 0x4C30F78: strlen (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==22753==    by 0xC708D1: zif_apache_request_headers (php_cli_server.c:380)
==22753==    by 0xBFBC5E: ZEND_DO_ICALL_SPEC_HANDLER (zend_vm_execute.h:586)
==22753==    by 0xBFB43C: execute_ex (zend_vm_execute.h:414)
==22753==    by 0xBFB63D: zend_execute (zend_vm_execute.h:458)
==22753==    by 0xB923CB: zend_execute_scripts (zend.c:1443)
==22753==    by 0xC757A3: php_cli_server_dispatch_router (php_cli_server.c:2123)
==22753==    by 0xC75A31: php_cli_server_dispatch (php_cli_server.c:2162)
==22753==    by 0xC76433: php_cli_server_recv_event_read_request (php_cli_server.c:2379)
==22753==    by 0xC767EB: php_cli_server_do_event_for_each_fd_callback (php_cli_server.c:2464)
==22753==    by 0xC71F5C: php_cli_server_poller_iter_on_active (php_cli_server.c:834)
==22753==    by 0xC76881: php_cli_server_do_event_for_each_fd (php_cli_server.c:2482)
==22753==

@nikic
Copy link
Member

nikic commented Sep 5, 2017

Fixed in 42549b7.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nikic nikic nikic left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@bouk @nikic