`JIT_G(enabled)` not set correctly on other threads (leads to memory protection errors) (ZTS)

dktapps commented 4 hours ago

Description

Disclaimer: I encountered this issue using a threading extension. Well aware this may not be the best source of truth on issues like this, especially due to the difficulty of reproducing.

With the following INI:

opcache.jit=tracing
opcache.jit_buffer_size=0

the following happens:

INI is loaded on the main thread and sets JIT_G(enabled) = 1 and JIT_G(on) = 1
accel_post_startup() is called during process init, and overrides thread-locals here to force-disable JIT: https://github.com/php/php-src/blob/6dec6a6dbaa14ef5ebd998f9ed75919c6dd83661/ext/opcache/ZendAccelerator.c#L3279-L3280
dasm_buf is therefore null because zend_jit_startup() wasn't called due to jit_buffer_size=0: https://github.com/php/php-src/blob/6dec6a6dbaa14ef5ebd998f9ed75919c6dd83661/ext/opcache/ZendAccelerator.c#L3277-L3286 & https://github.com/php/php-src/blob/6dec6a6dbaa14ef5ebd998f9ed75919c6dd83661/ext/opcache/jit/zend_jit.c#L3675
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
finally, the following calls fail on Windows with errors like VirtualProtect() failed [87] The parameter is incorrect during zend_accel_persist_script() because dasm_buf is NULL - this is because JIT_G(on) = 1 although it was supposed to be forcibly disabled: https://github.com/php/php-src/blob/6dec6a6dbaa14ef5ebd998f9ed75919c6dd83661/ext/opcache/zend_persist.c#L1384-L1386 & https://github.com/php/php-src/blob/6dec6a6dbaa14ef5ebd998f9ed75919c6dd83661/ext/opcache/zend_persist.c#L1419-L1424

Proposed solution

JIT_G(enabled) needs to become a true global
zend_jit_config() needs to respect the existing value of JIT_G(enabled) (or its replacement true global) instead of overwriting it like it currently does
JIT_G(enabled) (or its replacement true global) should be set to true in accel_post_startup() after zend_jit_startup() returns a success code

PHP Version

8.1, 8.2, 8.3, 8.4, master

Operating System

Windows

cmb69 commented 3 hours ago

See also https://github.com/krakjoe/parallel/issues/266#issuecomment-2457353827. It seems to me the real problem is that you can enable JIT (opcache.jit) without enabling it (opcache.jit_buffer_size=0).

dktapps commented 3 hours ago

Yeah, that looks like the same issue. Unfortunately these are the default values for opcache.jit and opcache.jit_buffer_size so you can run into this just by loading opcache and not even touching JIT.

cmb69 commented 32 minutes ago

Unfortunately these are the default values for opcache.jit and opcache.jit_buffer_size

Note that as of PHP 8.4.0, the defaults are opcache.jit=disable and opcache.jit_buffer_size=64M (see https://wiki.php.net/rfc/jit_config_defaults), though.

php / php-src