Refactor CLI SAPI php_cli_server_client struct to use zend_string #8522

Girgias · 2022-05-10T00:22:32Z

No description provided.

kocsismate · 2022-05-12T06:44:10Z

What is the advantage of this change? What I can see now is that one has to use ZSTR_VAL() and ZSTR_LEN() all over the place instead of directly passing the necessary values.

Or do you plan to submit followups so that functions which take the char * parameters are also changed to accept zend_strings (i.e. php_base64_decode())?

iluuu1994

I don't mind these changes. Although I personally think we should avoid too many TODOs and fix those cases before merging (because famously TODOs that hit production never actually get fixed).

sapi/cli/php_cli_server.c

iluuu1994 · 2022-05-12T08:09:00Z

sapi/cli/php_cli_server.c

@@ -623,9 +620,10 @@ static int sapi_cli_server_register_entry_cb(char **entry, int num_args, va_list
 		}
 		spprintf(&real_key, 0, "%s_%s", "HTTP", key);
 		if (strcmp(key, "CONTENT_TYPE") == 0 || strcmp(key, "CONTENT_LENGTH") == 0) {
-			sapi_cli_server_register_variable(track_vars_array, key, *entry);
+			// TODO make a version specialized for zend_string?
+			sapi_cli_server_register_variable(track_vars_array, key, ZSTR_VAL(*entry));


Could at least save a strlen

Yes, but I think ideally I'll get everything else which uses this function to be a zend_string which would indeed save a strlen.

sapi/cli/php_cli_server.c

iluuu1994 · 2022-05-12T09:42:33Z

sapi/cli/php_cli_server.c

+		case HEADER_VALUE: {
+			/* Previous element was part of header value, append content to it */
+			size_t old_length = ZSTR_LEN(client->current_header_value);
+			// TODO Release old value?


Looking at the implementation of zend_string_extend, it returns a reallocated string if the string was non-interned and the refcount was === 1, and a new string otherwise. That's honestly not great design because there's no way to know if the returned string is copied or reallocated and if the old string needs to be released or not without basically performing the same check.

In this particular case, it looks like it is never interned and the refcount is always === 1, so the old string doesn't need to be released. Maybe it would make sense to add a ZEND_ASSERT to be sure.

Will add an assertion, as I was wondering why I wasn't hitting any memory leaks

Did you also run with ASAN? Memory leaks are only reported for non-persistent memory tracked by ZMM.

I don't think I did will do that when I get back.

But I'm also struggling to understand why the memory needs to be allocated persistently in the first place. As I haven't figured out how that memory is freed in the end.

Looking at the implementation of zend_string_extend, it returns a reallocated string if the string was non-interned and the refcount was === 1, and a new string otherwise

This follows a copy on write semantic, I don't think we need to care about the original string after a zend_string_extend(): The other owners will release it.

why the memory needs to be allocated persistently in the first place

If it's used before or after zend_execute_scripts, the non-persistent allocator may not be started, or maybe shutdown.

That's good to know, so everything needs to be persistently allocated then in the SAPI?

Actually I may be wrong here. The allocator is fully available as soon as sapi_cli_server_startup() and is reset in php_cli_server_request_shutdown(). So it's possible to allocate non-persistent things outside of zend_execute_script as long as their lifetime don't extend after php_cli_server_request_shutdown().

Well it does seem that FreeBSD is more picky about this stuff than Linux. But lets see if me not duplicating a string breaks it again.

Nope, I do think every string here needs to be persistently allocated.
I think I will add some _ex variant of some APIs to pass a persistent flag.

Nope, I do think every string here needs to be persistently allocated.

I confirm. I dug more in the cli server, and what I missed originally is that it parses multiple requests concurrently. It results that parsing and script execution (and allocator shutdown) are interleaved.

Girgias · 2022-05-12T15:36:39Z

So the plan is indeed to do follow up PRs to convert more pairs of char* and length fields/args to use zend_string*. Mainly because we have usually better APIs with zend_strings, and we have some internal API which only take a zend_string.
Moreover, passing zend_string will prevent some strlen() recomputation (although one could argue just adding a size_t param is the better way to go).

Ideally, all SAPIs would use zend_strings so that we can change the "higher generic lever" SAPI API to zend_strings to then add and utilize some interned strings such as HTTP methods name, or common header fields, as this is recurrent and slightly suboptimal to always recreate this string when it's a core part of a response.

Hope this clarifies why I started working on this.

Still don't understand why ASAN didn't pick this up, probably need to check with Valgrind

…a IS_POINTER zval

The key has it's refcount increased This reverts commit 4c90ecb.

Maybe this is needed actually This reverts commit 0162cff.

This reverts commit a2d0106.

Girgias · 2022-05-14T11:51:16Z

I think this is now ready, and hopefully addresses all the coments.

sapi/cli/php_cli_server.c

arnaud-lb · 2022-05-14T21:03:19Z

sapi/cli/php_cli_server.c

+		case HEADER_VALUE: {
+			/* Previous element was part of header value, append content to it */
+			size_t old_length = ZSTR_LEN(client->current_header_value);
+			// TODO Release old value?


Nope, I do think every string here needs to be persistently allocated.

I confirm. I dug more in the cli server, and what I missed originally is that it parses multiple requests concurrently. It results that parsing and script execution (and allocator shutdown) are interleaved.

arnaud-lb · 2022-05-14T21:06:56Z

sapi/cli/php_cli_server.c

+			ZEND_FALLTHROUGH;
+		case HEADER_NONE:
+			/* Create new header field */
+			client->current_header_name = zend_string_init(at, length, /* persistent */ false);


This one needs to be persistent too

otherwise it's freed too early in this case:

$fd = stream_socket_client("tcp://localhost:8082"); fwrite($fd, "GET /index.php HTTP/1.0\r\nHost: hello"); $fd2 = stream_socket_client("tcp://localhost:8082"); fwrite($fd2, "GET /index.php HTTP/1.0\r\nConnection: close\r\n\r\n"); stream_copy_to_stream($fd2, STDOUT); fwrite($fd, "\r\n\r\n"); stream_copy_to_stream($fd, STDOUT);

I didn't manage to reproduce this locally. Hopefully I fixed the issue.

Thanks to Arnaud for figuring this out

Girgias · 2022-05-15T12:53:11Z

@cmb69 I don't really understand why Windows is failing, could you have a look into it?

cmb69 · 2022-05-15T15:04:39Z

@Girgias, looks like the culprit are the changes to php_cli_server.inc, especially not closing stderr (apparently, on Windows some spurious log output is sent to stderr, after the server has started, and that lets the test fail). Might be best to stick with the previous redirection of stderr to a pipe, and to close that pipe after the listen address has been identified.

arnaud-lb · 2022-05-15T15:08:02Z

@cmb69 the changes cause php_cli_server.inc to print the server's stderr when proc_get_status()['exitstatus'] is not 0. Is 255 a normal status code after a proc_terminate() on windows ?

cmb69 · 2022-05-15T15:26:58Z

@arnaud-lb, ah, right! The problem is

php-src/ext/standard/proc_open.c

Line 324 in 926407f

RETURN_BOOL(TerminateProcess(proc->childHandle, 255));

…ring (#8522)" This reverts commit 9b19d90. This has broken the ZEND_RC_DEBUG build.

… zend_string (php#8522)"" This reverts commit 82d407b.

Girgias force-pushed the cli-sapi-2 branch 3 times, most recently from 162db64 to 3bd53e3 Compare May 11, 2022 22:00

Girgias marked this pull request as ready for review May 11, 2022 22:00

Girgias requested review from iluuu1994, arnaud-lb and kocsismate May 11, 2022 22:08

Girgias changed the title ~~Refactor CLI SAPI to use zend_string~~ Refactor CLI SAPI php_cli_server_client struct to use zend_string May 11, 2022

iluuu1994 reviewed May 12, 2022

View reviewed changes

Girgias force-pushed the cli-sapi-2 branch 3 times, most recently from 07cf0a2 to 0ce4dab Compare May 13, 2022 22:08

Girgias added 15 commits May 14, 2022 00:27

Convert php_cli_server_client->addr_str to zend_string

5692fa2

Convert php_cli_server_client->current_header_name to zend_string

e770e27

Convert php_cli_server_client->current_header_value to zend_string

bb2b624

Add some assertions

14c592e

Duplicate original name header

ebf6581

Use bool instead of unsigned int with a size of 1 bit

ab212be

Add break again (don't know why I removed that one)

438f032

Add an assertion about the reallocation

a014824

Actually free persistent header value

e5dea7e

Still don't understand why ASAN didn't pick this up, probably need to check with Valgrind

Make request->headers store zend_string as IS_STRING zval instead as …

782e0c9

…a IS_POINTER zval

Revert "Duplicate original name header"

74b2375

The key has it's refcount increased This reverts commit 4c90ecb.

Revert "Revert "Duplicate original name header""

fccca66

Maybe this is needed actually This reverts commit 0162cff.

Do not duplicate add_str, just add a refcount

a2d0106

Revert "Do not duplicate add_str, just add a refcount"

b0246d4

This reverts commit a2d0106.

Improve name + comment

fa21d10

Girgias force-pushed the cli-sapi-2 branch from 0ce4dab to fa21d10 Compare May 13, 2022 23:30

Girgias requested a review from iluuu1994 May 14, 2022 11:50

iluuu1994 reviewed May 14, 2022

View reviewed changes

sapi/cli/php_cli_server.c Show resolved Hide resolved

iluuu1994 approved these changes May 14, 2022

View reviewed changes

arnaud-lb reviewed May 14, 2022

View reviewed changes

Girgias added 8 commits May 15, 2022 11:52

Server should not crash if request is interupted early

cb78ce4

Make header name persistent

36f69ec

Extract common code into a function

c381cea

Output Server errors

5e16552

Thanks to Arnaud for figuring this out

Spelling

877007e

Fix heap use after free

866ffc6

Cleanup tests

ba5d78b

Fix Clang MSAN

0af9a5e

Girgias added 2 commits May 15, 2022 18:02

Cater to Windows proc_terminate status code

cd7d695

bleh

1c821d8

arnaud-lb approved these changes May 15, 2022

View reviewed changes

Girgias merged commit 9b19d90 into php:master May 15, 2022

Girgias deleted the cli-sapi-2 branch May 15, 2022 20:11

nikic added a commit that referenced this pull request May 21, 2022

Revert "Refactor CLI SAPI php_cli_server_client struct to use zend_st…

82d407b

…ring (#8522)" This reverts commit 9b19d90. This has broken the ZEND_RC_DEBUG build.

Girgias added a commit to Girgias/php-src that referenced this pull request May 22, 2022

Revert "Revert "Refactor CLI SAPI php_cli_server_client struct to use…

a15b716

… zend_string (php#8522)"" This reverts commit 82d407b.

Girgias mentioned this pull request May 22, 2022

Refactor (again) CLI SAPI server_client struct to use zend_string* #8605

Merged

Girgias added a commit to Girgias/php-src that referenced this pull request May 22, 2022

Revert "Revert "Refactor CLI SAPI php_cli_server_client struct to use…

3e7cf1a

… zend_string (php#8522)"" This reverts commit 82d407b.

Girgias added a commit to Girgias/php-src that referenced this pull request May 27, 2022

Revert "Revert "Refactor CLI SAPI php_cli_server_client struct to use…

dae6a9b

… zend_string (php#8522)"" This reverts commit 82d407b.

Girgias mentioned this pull request Jul 21, 2022

Convert client->request.request_uri to zend_string #9086

Merged

Refactor CLI SAPI php_cli_server_client struct to use zend_string #8522

Refactor CLI SAPI php_cli_server_client struct to use zend_string #8522

Uh oh!

Conversation

Girgias commented May 10, 2022

Uh oh!

kocsismate commented May 12, 2022

Uh oh!

iluuu1994 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iluuu1994 May 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

arnaud-lb May 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Girgias commented May 12, 2022

Uh oh!

Girgias commented May 14, 2022

Uh oh!

Uh oh!

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!

Girgias commented May 15, 2022

Uh oh!

cmb69 commented May 15, 2022

Uh oh!

arnaud-lb commented May 15, 2022

Uh oh!

cmb69 commented May 15, 2022

Uh oh!

Uh oh!

iluuu1994 May 12, 2022 •

edited

Loading

arnaud-lb May 13, 2022 •

edited

Loading