In ZTS, JIT code refers to zend_internal_function* that may not exist after the end of the request

[dktapps]

Description

I don't have a reproducing test case for this with vanilla PHP yet. I encountered this while working on pmmp/pthreads.

In ZTS, internal functions are duplicated here: https://github.com/php/php-src/blob/PHP-8.0/Zend/zend.c#L665
and at the end of the request, they are freed here: https://github.com/php/php-src/blob/PHP-8.0/Zend/zend.c#L701
This means that when compiling functions on a particular thread, any zend_internal_function referenced are actually specific to that thread.

If this jitted code is subsequently referenced from OPcache by another thread/request, a UAF may occur if the caller has some reason to access the zend_function from the call stack (I ran into this with a closure calling var_dump() - UAF occurred because var_dump() wanted to get the current filename and ran across a freed zend_internal_function from another thread).

This appears not to reproduce on Windows, since ASLR prevents referring directly to function addresses anyway, so the JIT already has alternative measures for that.

It would appear that the solution to this would be to just not use const addresses in JIT code on ZTS. It's already doing this for Windows anyway because of ASLR.

PHP Version

8.0.27 (ZTS)

Operating System

WSL2 Ubuntu 20.04

[dstogov]

compiler_globals_ctor is not called for each request. It's only called for new thread startup.
It seems like this doesn't make a problem for vanilla PHP because all workers threads are started before the first request.
I suppose, you have problems, because your extension starts new threads at runtime...

This is not an easy problem. I'm not sure if we still need to duplicate internal functions for each thread in master branch (I thought they are immutable). Of course, we may change JIT to produce less efficient code for ZTS, but I'll need to do some research first.

@iluuu1994 @cmb69 @krakjoe may be you have related thoughts?

[dktapps]

I suppose, you have problems, because your extension starts new threads at runtime...

Yeah, that's correct. I imagine extensions like krakjoe/parallel will experience the same problem.

It's only called for new thread startup.

Still, jitted code is shared between all threads, right? So even if it doesn't fault, it's still potentially doing thread-unsafe stuff. (I'm not sure on the exact inner workings of zend_internal_function, but there had to be some reason these were copied?)

[dktapps]

Here is an example reproducing case using pmmp/pthreads:

Using 255e08a and pmmp/ext-pmmpthread@118a823

config.nice:

#! /bin/sh
#
# Created by configure

'./configure' \
'--disable-all' \
'--enable-cli' \
'--enable-zts' \
'--enable-pthreads' \
'--enable-opcache' \
'--enable-opcache-jit' \
'--enable-debug' \
'--with-valgrind' \
"$@"

Test php.ini:

extension_dir=./modules
zend_extension=opcache.so
opcache.enable_cli=1
opcache.jit=tracing
opcache.jit_buffer_size=16M

Test command line:

sapi/cli/php run-tests.php -j16 -P -m -c ./php.ini --show-diff ext/pthreads/tests/object-comparison.phpt

Test output:
object-comparison.mem.txt

The destroyed internal function is (I believe) var_dump().

I also tested this with opcache.jit_debug=256 to verify the problem by looking at the assembly, so I'm sure this is not pthreads' fault.

[dstogov]

#10517 should fix the problem.
However, this is a radical change and it may be merged only to master.

[dktapps]

Thanks, I'll test the PR with pthreads later.

For other branches I guess it should be sufficient to use ASLR-like workarounds for ZTS. It'll cost some performance, but since ZTS isn't commonly used on *nix (?) I don't think there will be many complaints.

[dstogov]

Fixed in master via 3b75f07