Fixed JIT startup failure not being respected by new threads in ZTS

[dktapps]

Fixes #16851

@cmb69 would you be able to verify if this fixes the issue you had with ext-parallel?

[cmb69]

With opcache.jit=tracing and opcache.jit_buffer_size=0, prior to this patch I get 7 VirtualProtect() test failures running the ext/parallel test suite; with the patch all tests succeed. Using a true global here should be fine since we have a lock on the SHM when the variable is assigned to.

I wonder, though, what happens when there are multiple processes involved (e.g. FCGI without FPM). Could that cause the same issue? Do we need to promote the global to SHM?

[dktapps]

I'm not familiar enough with FCGI to know for sure if this would be a problem. Possibly the same issue could arise in a forked process, but I think this global will be sufficient to fix it.

[dstogov]

The following looks suspicious to me: "when a second thread is started, INI is loaded again, setting JIT_G(enabled) = 1 and JIT_G(on) = 1, but this time accel_post_startup() does not override these settings".

Why INI is loaded again?

In ZTS build (without threaded extension) INI directives are inherited from parent process using zend_copy_ini_directives().

[dktapps]

zend_jit_config() gets triggered during new thread startup because of this:

php-src/Zend/zend.c

Line 875 in ff5b42b

zend_ini_refresh_caches(ZEND_INI_STAGE_STARTUP);

In the case of threading extensions, because the threads are created post-startup, zend_jit_config() would run with ZEND_INI_STAGE_STARTUP after accel_post_startup() and cause this bug.

That being said, I'd expect problems without threading extensions too. Even if all threads' INI were set up before accel_post_startup(), accel_post_startup() only sets JIT_G(enabled) = 0 on one thread. Unless every thread were to invoke accel_post_startup() (which afaik they don't), then other uses of ZTS should be affected too. However this fix would not address those cases. For that, JIT_G(enabled) would have to be turned into a true global and checked everywhere alongside JIT_G(on).

[dktapps]

I've pushed an alternative fix using RINIT to set the values, which should cover the case where threads were started before accel_post_startup().

It's not the cleanest fix (since really it should only run when threads are started and not on request start), but judging by this code it looks like it's the way to go:

php-src/ext/opcache/ZendAccelerator.c

Lines 2631 to 2634 in fc1db70

	if (!ZCG(enabled) || !accel_startup_ok) {
	ZCG(accelerator_enabled) = false;
	return SUCCESS;
	}

[dstogov]

That being said, I'd expect problems without threading extensions too.

Right. Threads in ZTS build seems ignoring jit startup failure.
I afraid this is not a single place where zend_ini_refresh_caches() may cause problems.
Actually, any INI value modified during MINIT is going to be lost in new threads. Right?

[cmb69]

While there might be a more general problem, the issue at hand is just that we try to protect the dasm_buf even though it is NULL. There is no need to protect a non-existent buffer, so I've applied the following patch:

 ext/opcache/jit/zend_jit.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/ext/opcache/jit/zend_jit.c b/ext/opcache/jit/zend_jit.c
index ffcb8daaf5..a4d74872b0 100644
--- a/ext/opcache/jit/zend_jit.c
+++ b/ext/opcache/jit/zend_jit.c
@@ -3424,7 +3424,7 @@ void zend_jit_unprotect(void)
 #else
 		new = PAGE_READWRITE;
 #endif
-		if (!VirtualProtect(dasm_buf, dasm_size, new, &old)) {
+		if (dasm_buf && !VirtualProtect(dasm_buf, dasm_size, new, &old)) {
 			DWORD err = GetLastError();
 			char *msg = php_win32_error_to_msg(err);
 			fprintf(stderr, "VirtualProtect() failed [%u] %s\n", err, msg);
@@ -3451,7 +3451,7 @@ void zend_jit_protect(void)
 	if (!(JIT_G(debug) & (ZEND_JIT_DEBUG_GDB|ZEND_JIT_DEBUG_PERF_DUMP))) {
 		DWORD old;
 
-		if (!VirtualProtect(dasm_buf, dasm_size, PAGE_EXECUTE_READ, &old)) {
+		if (dasm_buf && !VirtualProtect(dasm_buf, dasm_size, PAGE_EXECUTE_READ, &old)) {
 			DWORD err = GetLastError();
 			char *msg = php_win32_error_to_msg(err);
 			fprintf(stderr, "VirtualProtect() failed [%u] %s\n", err, msg);

That lets the ext/parallel test suite pass (regardless of the value of opcache.jit_buffer_size). Maybe mprotect() would need that guard as well, but possibly it is fine being called as mprotect(NULL, …).

[dktapps]

Yeah, I did consider that solution too @cmb69 but I don't know what other bugs might be lurking on account of this missing initialization. Thought it was best to cover all bases.

Right. Threads in ZTS build seems ignoring jit startup failure. I afraid this is not a single place where zend_ini_refresh_caches() may cause problems. Actually, any INI value modified during MINIT is going to be lost in new threads. Right?

@dstogov Yep, I think so. I guess few extensions do this in a significant way so it hasn't been noticed before.

[dktapps]

I don't see an obvious general fix for this problem. Since the problem really affects the TS globals and not the INI values themselves. The TS global struct would have to be copied from the main thread, but that might include pointers to thread-local objects that shouldn't be copied.

For this PR, it's possible using zend_alter_ini_entry() might solve the problem, but it depends whether child threads are started before or after accel_post_startup(). Not well-versed enough in the webserver side of ZTS to answer that.

[dstogov]

OK let fix the problem in this way.
Can you please rename zend_jit_startup_failed to zend_jit_startup_ok (for consistency with accel_startup_ok).
Everything else looks fine.

[cmb69]

Thank you! Applied to PHP-8.2 and merged up.

[cmb69]

The merge into PHP-8.4 went bad, so I reverted for now. Will have a look shortly; AFK for now.

[dktapps]

I think the return value for zend_jit_startup() is gone in 8.4, that's probably why it conflicts.

[cmb69]

Indeed, thanks! So an extraneous else sneaked in. Should be fixed via da81b5c.