Skip to content
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.

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

cli server crashes on SIGINT when compiled with ZEND_RC_DEBUG=1 #11716

Closed
ju1ius opened this issue Jul 16, 2023 · 4 comments
Closed

cli server crashes on SIGINT when compiled with ZEND_RC_DEBUG=1 #11716

ju1ius opened this issue Jul 16, 2023 · 4 comments

Comments

@ju1ius
Copy link
Contributor

ju1ius commented Jul 16, 2023

Description

Steps to reproduce:

  • PHP must be compiled with --enable-debug and CFLAGS="-DZEND_RC_DEBUG=1"
  • launch a cli server like php -S localhost:8888
  • terminate the process with a SIGINT (i.e. CTRL+C)

Here's the gdb backtrace:

__pthread_kill_implementation (threadid=<optimized out>, signo=signo@entry=6, no_tid=no_tid@entry=0) at ./nptl/pthread_kill.c:44
0x00007fd65fdba15f in __pthread_kill_internal (signo=6, threadid=<optimized out>) at ./nptl/pthread_kill.c:78
0x00007fd65fd6c472 in __GI_raise (sig=sig@entry=6) at ../sysdeps/posix/raise.c:26
0x00007fd65fd564b2 in __GI_abort () at ./stdlib/abort.c:79
0x00007fd65fd563d5 in __assert_fail_base (fmt=0x7fd65fecadc8 "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n",
    assertion=assertion@entry=0x5582e4bcde80 "(zval_gc_flags((p)->u.type_info) & ((1<<7)|(1<<8))) != (1<<7)",
    file=file@entry=0x5582e4bcde18 "/usr/src/php-src/Zend/zend_types.h", line=line@entry=1289,
    function=function@entry=0x5582e4bce468 <__PRETTY_FUNCTION__.62> "zend_gc_delref") at ./assert/assert.c:92
0x00007fd65fd653a2 in __assert_fail (
    assertion=0x5582e4bcde80 "(zval_gc_flags((p)->u.type_info) & ((1<<7)|(1<<8))) != (1<<7)",
    file=0x5582e4bcde18 "/usr/src/php-src/Zend/zend_types.h", line=1289,
    function=0x5582e4bce468 <__PRETTY_FUNCTION__.62> "zend_gc_delref") at ./assert/assert.c:101
0x00005582e4867e74 in zend_gc_delref (p=0x5582e52c8210) at /usr/src/php-src/Zend/zend_types.h:1289
0x00005582e4868109 in zend_string_release (s=0x5582e52c8210) at /usr/src/php-src/Zend/zend_string.h:345
 0x00005582e486e022 in zend_hash_destroy (ht=0x5582e50237a8 <server+648>) at /usr/src/php-src/Zend/zend_hash.c:1738
0x00005582e49d7e23 in php_cli_server_dtor (server=0x5582e5023520 <server>) at /usr/src/php-src/sapi/cli/php_cli_server.c:2333
0x00005582e49d8f82 in do_cli_server (argc=4, argv=0x5582e5150770) at /usr/src/php-src/sapi/cli/php_cli_server.c:2864
0x00005582e49cefeb in main (argc=4, argv=0x5582e5150770) at /usr/src/php-src/sapi/cli/php_cli.c:1340

Looks like this assertion:

ZEND_ASSERT((zval_gc_flags((p)->u.type_info) & (GC_PERSISTENT|GC_PERSISTENT_LOCAL)) != GC_PERSISTENT);

fails when calling

zend_hash_destroy(&server->extension_mime_types);

PHP Version

php8.3-dev

Operating System

debian sid

@nielsdos
Copy link
Member

nielsdos commented Jul 16, 2023

I guess this works:

diff --git a/Zend/zend_hash.c b/Zend/zend_hash.c
index a8571af75c..4cdf681761 100644
--- a/Zend/zend_hash.c
+++ b/Zend/zend_hash.c
@@ -898,6 +898,11 @@ static zend_always_inline zval *_zend_hash_str_add_or_update_i(HashTable *ht, co
 	ht->nNumOfElements++;
 	p = ht->arData + idx;
 	p->key = key = zend_string_init(str, len, GC_FLAGS(ht) & IS_ARRAY_PERSISTENT);
+#if ZEND_RC_DEBUG
+	if (GC_FLAGS(ht) & GC_PERSISTENT_LOCAL) {
+		GC_MAKE_PERSISTENT_LOCAL(key);
+	}
+#endif
 	p->h = ZSTR_H(key) = h;
 	HT_FLAGS(ht) &= ~HASH_FLAG_STATIC_KEYS;
 	if (flag & HASH_LOOKUP) {
diff --git a/sapi/cli/php_cli_server.c b/sapi/cli/php_cli_server.c
index 9c71b83580..5bcf736969 100644
--- a/sapi/cli/php_cli_server.c
+++ b/sapi/cli/php_cli_server.c
@@ -2320,6 +2320,7 @@ static void php_cli_server_mime_type_ctor(php_cli_server *server, const php_cli_
 	const php_cli_server_ext_mime_type_pair *pair;
 
 	zend_hash_init(&server->extension_mime_types, 0, NULL, NULL, 1);
+	GC_MAKE_PERSISTENT_LOCAL(&server->extension_mime_types);
 
 	for (pair = mime_type_map; pair->ext; pair++) {
 		size_t ext_len = strlen(pair->ext);

CLI is single threaded, so clearly that hashtable is also local.
And also I think that persistent local hashtables should mark the strings they create as persistent local as well, that seems to make sense. Since GC_PERSISTENT_LOCAL is only used for debugging I guarded it with an #if.
But this is a bit of a shot in the dark, so I'd like confirmation from someone more accustomed to Zend.

@Girgias
Copy link
Member

Girgias commented Jul 16, 2023

Assigning @arnaud-lb as they helped me figure out this sort of issue with strings last time I was doing something with the CLI

@arnaud-lb
Copy link
Member

@nielsdos this looks good. Do you want to make a PR? Otherwise, I will do. We may have the same issue with req->headers and req->headers_original_case in php_cli_server_request_ctor().

The cli server appears to be the only place where we destroy persistent hash tables before zend_rc_debug is set to false by php_module_shutdown(). This explains why we didn't need to take care of GC_PERSISTENT_LOCAL before in zend_hash.

@nielsdos
Copy link
Member

@arnaud-lb Thank you for checking. I can create the PR soon and check the other cases.

nielsdos added a commit to nielsdos/php-src that referenced this issue Jul 21, 2023
nielsdos added a commit to nielsdos/php-src that referenced this issue Jul 21, 2023
nielsdos added a commit that referenced this issue Jul 21, 2023
* PHP-8.1:
  Fix GH-11716: cli server crashes on SIGINT when compiled with ZEND_RC_DEBUG=1
nielsdos added a commit that referenced this issue Jul 21, 2023
* PHP-8.2:
  Fix GH-11716: cli server crashes on SIGINT when compiled with ZEND_RC_DEBUG=1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants