Fix bug 70470 by bouk · Pull Request #2736 · php/php-src

bouk · 2017-09-04T15:07:34Z

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 · 2017-09-04T19:37:25Z

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

nikic · 2017-09-04T19:44:02Z

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);


/* break missing intentionally */

nikic · 2017-09-04T19:52:02Z

sapi/cli/php_cli_server.c

+		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);


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 · 2017-09-04T19:53:02Z

sapi/cli/php_cli_server.c

+		break;
+	case NONE:
+		// can't happen
+		assert(1);


Should be assert(0)?

nikic · 2017-09-04T19:56:48Z

sapi/cli/php_cli_server.c

+	switch (client->last_header_element) {
+	case NONE:
+		break;
+	case FIELD:


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

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

nikic · 2017-09-04T20:02:29Z

sapi/cli/php_cli_server.c

+	case VALUE:
+		php_cli_server_client_save_header(client);
+	case NONE:
+		client->current_header_name = at;


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

nikic · 2017-09-04T20:07:39Z

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 · 2017-09-04T20:11:37Z

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;


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 · 2017-09-04T20:13:52Z

sapi/cli/php_cli_server.c

 	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;


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

bouk · 2017-09-05T11:57:06Z

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

nikic · 2017-09-05T14:27:05Z

Merged as cd9d90f into 7.0+. Thanks!

nikic · 2017-09-05T16:09:06Z

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 · 2017-09-05T16:10:38Z

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 · 2017-09-05T16:15:27Z

Fixed in 42549b7.

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 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 reviewed Sep 4, 2017

View reviewed changes

bouk force-pushed the bug-70470 branch from d8e4818 to 86d0f3b Compare September 5, 2017 09:11

Fix bug 70470

00c8f5f

bouk force-pushed the bug-70470 branch from 86d0f3b to 00c8f5f Compare September 5, 2017 11:56

nikic closed this Sep 5, 2017

Conversation

bouk commented Sep 4, 2017

Uh oh!

nikic commented Sep 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nikic commented Sep 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bouk commented Sep 5, 2017

Uh oh!

nikic commented Sep 5, 2017

Uh oh!

nikic commented Sep 5, 2017

Uh oh!

nikic commented Sep 5, 2017

Uh oh!

nikic commented Sep 5, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants