Add alternative to zend_string_dup for persistent strings

[TysonAndre]

And add ZEND_ASSERT statement so that misuse of zend_string_dup can be detected.
See comments in 9eb295f

• Keep the existing functionality, so that behavior stays consistent across php versions and so the PECL authors fix bugs in the same way across versions

E.g. PECLs may be affected by this since the zend_string file isn't well documented, e.g. phpv8/v8js#432 (not personally affected by it)

[TysonAndre]

It seems as if I can reproduce it, after all, with the httpd api?

Calls to shell_exec could unrelatedly have their error handling improved, and the typo in 021.phpt could be fixed

FAIL Bug #37276 (problems witch $_POST array) [tests/basic/021.phpt] 
php: Zend/zend_string.h:213: zend_string *zend_string_dup(zend_string *, _Bool): Assertion `!persistent || (zval_gc_flags((s)->gc.u.type_info) & (1<<7))' failed.
Aborted
ERROR: Worker 1 reported unexpected E_DEPRECATED: explode(): Passing null to parameter #2 ($string) of type string is deprecated in /tmp/cirrus-ci-build/run-tests.php on line 3739

[TysonAndre]

Oh, right, I forgot about permanent being distinct from persistent. I think I should check for both.

This seems to be related to the jit being enabled in ci for some tests. sapi/cli/php run-tests.php -d zend_extension=opcache.so -d opcache.enable=1 -d opcache.enable_cli=1 -d opcache.jit=function -d opcache.jit_function_size=16M tests having opcache loaded and enabled on ZTS, also affecting php-cgi tests such as tests/basic/002.phpt without the jit

• I also saw cirrus failures on NTS but don't have a similar enough setup to reproduce it

I'm not familiar with enough with ZTS to know why the ZTS-only function zend_copy_constants needs persistent zend_string instances for constants for a given thread.

I guess persistent is safe to use where permanent is needed (opcache won't delete it until module shutdown?)

Related code for persistent strings

Related snippets, gdb output (click to expand)

static void copy_zend_constant(zval *zv)
{
	zend_constant *c = Z_PTR_P(zv);

	ZEND_ASSERT(ZEND_CONSTANT_FLAGS(c) & CONST_PERSISTENT);
	Z_PTR_P(zv) = pemalloc(sizeof(zend_constant), 1);
	memcpy(Z_PTR_P(zv), c, sizeof(zend_constant));

	c = Z_PTR_P(zv);
	c->name = zend_string_copy(c->name);
	if (Z_TYPE(c->value) == IS_STRING) {
		Z_STR(c->value) = zend_string_dup(Z_STR(c->value), 1);
	}
}

(gdb) bt
#0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
#1  0x00007ffff7c37859 in __GI_abort () at abort.c:79
#2  0x00007ffff7c37729 in __assert_fail_base (fmt=0x7ffff7dcd588 "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n", 
    assertion=0x555555e3d5e8 "!persistent || (zval_gc_flags((s)->gc.u.type_info) & (1<<7))", 
    file=0x555555e3d5b8 "/path/to/php-src/Zend/zend_string.h", line=213, function=<optimized out>)
    at assert.c:92
#3  0x00007ffff7c48fd6 in __GI___assert_fail (
    assertion=0x555555e3d5e8 "!persistent || (zval_gc_flags((s)->gc.u.type_info) & (1<<7))", 
    file=0x555555e3d5b8 "/path/to/php-src/Zend/zend_string.h", line=213, 
    function=0x555555e3db10 <__PRETTY_FUNCTION__.10707> "zend_string_dup") at assert.c:101
#4  0x0000555555aeae30 in zend_string_dup (s=0x555556817348, persistent=true)
    at /path/to/php-src/Zend/zend_string.h:213
#5  0x0000555555aeb7b7 in copy_zend_constant (zv=0x5555564f58b0) at /path/to/php-src/Zend/zend_constants.c:72
#6  0x0000555555b2bc67 in zend_hash_copy (target=0x555556376f10, source=0x555556376b40, 
    pCopyConstructor=0x555555aeb6f9 <copy_zend_constant>) at /path/to/php-src/Zend/zend_hash.c:2202
#7  0x0000555555aeb7ef in zend_copy_constants (target=0x555556376f10, source=0x555556376b40)
    at /path/to/php-src/Zend/zend_constants.c:79
#8  0x0000555555b0b3bf in executor_globals_ctor (executor_globals=0x555556375400)
    at /path/to/php-src/Zend/zend.c:766
#9  0x0000555555b0be1d in zend_post_startup () at /path/to/php-src/Zend/zend.c:1072
#10 0x0000555555a4dad3 in php_module_startup (sf=0x555556357580 <cli_sapi_module>, additional_module=0x0)
    at /path/to/php-src/main/main.c:2229
#11 0x0000555555c97433 in php_cli_startup (sapi_module=0x555556357580 <cli_sapi_module>)
    at /path/to/php-src/sapi/cli/php_cli.c:410
#12 0x0000555555c993f0 in main (argc=79, argv=0x555556371230) at /path/to/php-src/sapi/cli/php_cli.c:1300

...
(gdb) print c->value->value.str->gc
$7 = {refcount = 2, u = {type_info = 342}}
(gdb) print (char*) c->value->value.str->val
$8 = 0x555556817360 "8.3.0-dev"
(gdb) print (char*) c->name->val
$9 = 0x555556817338 "PHP_VERSION"

type_info=0x156
GC_NOT_COLLECTABLE, IS_STR_INTERNED(GC_IMMUTABLE), IS_STR_PERMANENT, 6=IS_STRING

// ext/opcache/ZendAccelerator.c accel_new_interned_string_for_php (using shared memory, not malloc)
	GC_TYPE_INFO(s) = GC_STRING | ((IS_STR_INTERNED | IS_STR_PERMANENT) << GC_FLAGS_SHIFT);
// elsewhere
	/* We can reuse init_interned_string_for_php for the "init_existing_interned" case,
	 * because the function does not create new interned strings at runtime. */
	zend_interned_strings_set_request_storage_handlers(
		accel_new_interned_string_for_php,
		accel_init_interned_string_for_php,
		accel_init_interned_string_for_php);

[arnaud-lb]

It is my understanding that ZSTR_IS_INTERNED(s) implies IS_STR_PERSISTENT|IS_STR_PERMANENT, unless the string was interned during a request, which is not supposed to happen except when using dl(), according to

php-src/Zend/zend_string.c

Lines 254 to 260 in 33dd226

	#if ZEND_RC_DEBUG
	if (zend_rc_debug) {
	/* PHP shouldn't create persistent interned string during request,
	* but at least dl() may do this */
	ZEND_ASSERT(!(GC_FLAGS(str) & GC_PERSISTENT));
	}
	#endif

Are there other cases where ZSTR_IS_INTERNED(s) does not imply IS_STR_PERSISTENT|IS_STR_PERMANENT?

Since this targets master, we might as well change the behavior of zend_string_dup() directly?

[dstogov]

@TysonAndre do you still like to merge this? It looks like the original problem was in invalid zend_string_dup() usage.