Skip to content

Commit 89f46e7

Browse files
Enfiladeguideclaude
andcommitted
Fix GH-24661: FPM pools with different disable_functions share cached scripts
FPM applies disable_functions and extension of a pool in its workers, after the master has finalized the system id, and a web server can pass disable_functions in PHP_ADMIN_VALUE. Scripts cached by the workers of one pool were then used by the workers of other pools, although they depend on the internal functions they were compiled with (see GH-17733). Recompute the system id when FPM changes the internal functions, so that the file cache uses a separate directory, and keep scripts in shared memory apart by the system id, like root_hash does for chroot. User functions declared by preloading are added after the system id is finalized, so they are skipped. With preloading, scripts in shared memory are not kept apart, as preloaded scripts must stay visible to all processes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1 parent 810ce5e commit 89f46e7

7 files changed

Lines changed: 311 additions & 9 deletions

‎Zend/zend_system_id.c‎

Lines changed: 21 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,6 @@ void zend_startup_system_id(void)
6464

6565
void zend_finalize_system_id(void)
6666
{
67-
unsigned char digest[16];
6867
uint8_t hooks = 0;
6968

7069
if (zend_ast_process) {
@@ -90,22 +89,37 @@ void zend_finalize_system_id(void)
9089
}
9190
}
9291

92+
finalized = 1;
93+
zend_update_system_id();
94+
}
95+
96+
/* Called again by FPM, which applies disable_functions and extension after
97+
* startup, from the pool configuration or from FastCGI parameters. */
98+
void zend_update_system_id(void)
99+
{
100+
PHP_MD5_CTX ctx = context;
101+
unsigned char digest[16];
102+
103+
ZEND_ASSERT(finalized);
104+
93105
/* Cached scripts can refer to offsets of internal functions.
94106
* Effects of disable_functions and frameless functions must be taken into account.
95107
* Therefore, embed the function names in-order.
96108
* Updating third party extensions may change function signatures, arginfo, offsets, ...
97109
* Therefore, also embed the loaded modules and their versions. */
98-
ZEND_HASH_MAP_FOREACH_STR_KEY(CG(function_table), zend_string *name) {
99-
PHP_MD5Update(&context, ZSTR_VAL(name), ZSTR_LEN(name) + 1);
110+
ZEND_HASH_MAP_FOREACH_STR_KEY_PTR(CG(function_table), zend_string *name, zend_function *func) {
111+
/* Skip user functions added by preloading */
112+
if (func->type == ZEND_INTERNAL_FUNCTION) {
113+
PHP_MD5Update(&ctx, ZSTR_VAL(name), ZSTR_LEN(name) + 1);
114+
}
100115
} ZEND_HASH_FOREACH_END();
101116
ZEND_HASH_MAP_FOREACH_PTR(&module_registry, zend_module_entry *mod) {
102-
PHP_MD5Update(&context, mod->name, strlen(mod->name) + 1);
117+
PHP_MD5Update(&ctx, mod->name, strlen(mod->name) + 1);
103118
if (EXPECTED(mod->version != NO_VERSION_YET)) {
104-
PHP_MD5Update(&context, mod->version, strlen(mod->version) + 1);
119+
PHP_MD5Update(&ctx, mod->version, strlen(mod->version) + 1);
105120
}
106121
} ZEND_HASH_FOREACH_END();
107122

108-
PHP_MD5Final(digest, &context);
123+
PHP_MD5Final(digest, &ctx);
109124
php_hash_bin2hex(zend_system_id, digest, sizeof digest);
110-
finalized = 1;
111125
}

‎Zend/zend_system_id.h‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,13 +20,14 @@
2020
#include "zend_types.h"
2121

2222
BEGIN_EXTERN_C()
23-
/* True global; Write-only during MINIT/startup */
23+
/* True global; Write-only during MINIT/startup, see also zend_update_system_id() */
2424
extern ZEND_API char zend_system_id[32];
2525

2626
ZEND_API zend_result zend_add_system_entropy(const char *module_name, const char *hook_name, const void *data, size_t size);
2727
END_EXTERN_C()
2828

2929
void zend_startup_system_id(void);
3030
void zend_finalize_system_id(void);
31+
void zend_update_system_id(void);
3132

3233
#endif /* ZEND_SYSTEM_ID_H */

‎ext/opcache/ZendAccelerator.c‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,7 @@
4646
#include "zend_accelerator_util_funcs.h"
4747
#include "zend_accelerator_hash.h"
4848
#include "zend_file_cache.h"
49+
#include "zend_system_id.h"
4950
#include "ext/pcre/php_pcre.h"
5051
#include "ext/standard/basic_functions.h"
5152

@@ -116,6 +117,8 @@ zend_accel_shared_globals *accel_shared_globals = NULL;
116117
/* true globals, no need for thread safety */
117118
#ifdef ZEND_WIN32
118119
char accel_uname_id[32];
120+
#else
121+
static char accel_startup_system_id[32];
119122
#endif
120123
bool accel_startup_ok = false;
121124
static const char *zps_failure_reason = NULL;
@@ -2707,6 +2710,14 @@ ZEND_RINIT_FUNCTION(zend_accelerator)
27072710
} else {
27082711
ZCG(root_hash) = 0;
27092712
}
2713+
2714+
/* The system id changes if a process changes its internal functions or
2715+
* extensions after startup, see zend_update_system_id(). Scripts compiled
2716+
* with different ones are kept apart, like root_hash does for chroot.
2717+
* Preloaded scripts must stay visible to all processes. */
2718+
if (!ZCSG(preload_script) && UNEXPECTED(memcmp(zend_system_id, accel_startup_system_id, 32) != 0)) {
2719+
ZCG(root_hash) ^= zend_hash_func(zend_system_id, 32);
2720+
}
27102721
#endif
27112722

27122723
HANDLE_BLOCK_INTERRUPTIONS();
@@ -3256,6 +3267,10 @@ static zend_result accel_post_startup(void)
32563267
}
32573268
}
32583269

3270+
#ifndef ZEND_WIN32
3271+
memcpy(accel_startup_system_id, zend_system_id, 32);
3272+
#endif
3273+
32593274
/********************************************/
32603275
/* End of non-SHM dependent initializations */
32613276
/********************************************/

‎sapi/fpm/fpm/fpm_php.c‎

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
#include "php_main.h"
1111
#include "php_ini.h"
1212
#include "ext/standard/dl.h"
13+
#include "zend_system_id.h"
1314

1415
#include "fastcgi.h"
1516

@@ -111,15 +112,26 @@ int fpm_php_apply_defines_ex(struct key_value_s *kv, int mode) /* {{{ */
111112
#endif
112113

113114
zend_interned_strings_switch_storage(1);
114-
return Z_TYPE(zv) == IS_TRUE ? FPM_PHP_INI_EXTENSION_LOADED : FPM_PHP_INI_EXTENSION_FAILED;
115+
if (Z_TYPE(zv) != IS_TRUE) {
116+
return FPM_PHP_INI_EXTENSION_FAILED;
117+
}
118+
/* Scripts cached by other pools may depend on the internal functions */
119+
zend_update_system_id();
120+
return FPM_PHP_INI_EXTENSION_LOADED;
115121
}
116122

117123
if (fpm_php_zend_ini_alter_master(name, name_len, value, value_len, mode, PHP_INI_STAGE_ACTIVATE) == FAILURE) {
118124
return FPM_PHP_INI_ALTERING_ERROR;
119125
}
120126

121127
if (!strcmp(name, "disable_functions") && *value) {
128+
uint32_t num_functions = zend_hash_num_elements(CG(function_table));
129+
122130
zend_disable_functions(value);
131+
/* Scripts cached by other pools may depend on the internal functions */
132+
if (zend_hash_num_elements(CG(function_table)) != num_functions) {
133+
zend_update_system_id();
134+
}
123135
return FPM_PHP_INI_APPLIED;
124136
}
125137

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,63 @@
1+
--TEST--
2+
FPM: GH-24661 - disable_functions passed in PHP_ADMIN_VALUE must not reuse cached scripts
3+
--EXTENSIONS--
4+
opcache
5+
--SKIPIF--
6+
<?php include "skipif.inc"; ?>
7+
--FILE--
8+
<?php
9+
10+
require_once "tester.inc";
11+
12+
$cfg = <<<EOT
13+
[global]
14+
error_log = {{FILE:LOG}}
15+
[unconfined]
16+
listen = {{ADDR}}
17+
pm = static
18+
pm.max_children = 1
19+
EOT;
20+
21+
/* Disabling zend_version() moves the functions registered after it in the
22+
* function table, var_export() among them. */
23+
$code = <<<'EOT'
24+
<?php
25+
function zend_version_or_error(): string {
26+
try {
27+
return gettype(zend_version());
28+
} catch (Error $e) {
29+
return $e->getMessage();
30+
}
31+
}
32+
var_dump(is_array(opcache_get_status(false)));
33+
echo var_export(true, true), "\n";
34+
var_dump(function_exists('zend_version'));
35+
echo zend_version_or_error(), "\n";
36+
EOT;
37+
38+
$opcache = ini_get('extension_dir') . DIRECTORY_SEPARATOR . 'opcache.' . PHP_SHLIB_SUFFIX;
39+
$extraArgs = is_file($opcache) ? ['-dzend_extension=' . $opcache] : [];
40+
41+
$tester = new FPM\Tester($cfg, $code);
42+
$tester->start($extraArgs, iniEntries: ['opcache.file_update_protection' => '0']);
43+
$tester->expectLogStartNotices();
44+
$tester->request()->expectBody(['bool(true)', 'true', 'bool(true)', 'string']);
45+
/* The worker's function table changes before the request starts. */
46+
$restricted = ['PHP_ADMIN_VALUE' => 'disable_functions=zend_version'];
47+
$tester->request(headers: $restricted)
48+
->expectBody(['bool(true)', 'true', 'bool(false)', 'Call to undefined function zend_version()']);
49+
$tester->request(headers: $restricted)
50+
->expectBody(['bool(true)', 'true', 'bool(false)', 'Call to undefined function zend_version()']);
51+
$tester->terminate();
52+
$tester->expectLogTerminatingNotices();
53+
$tester->close();
54+
55+
?>
56+
Done
57+
--EXPECT--
58+
Done
59+
--CLEAN--
60+
<?php
61+
require_once "tester.inc";
62+
FPM\Tester::clean();
63+
?>
Lines changed: 117 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,117 @@
1+
--TEST--
2+
FPM: GH-24661 - Pools with different disable_functions must not share cached scripts
3+
--EXTENSIONS--
4+
opcache
5+
--SKIPIF--
6+
<?php include "skipif.inc"; ?>
7+
--FILE--
8+
<?php
9+
10+
require_once "tester.inc";
11+
12+
$cfg = <<<EOT
13+
[global]
14+
error_log = {{FILE:LOG}}
15+
[unrestricted]
16+
listen = {{ADDR[unrestricted]}}
17+
pm = static
18+
pm.max_children = 1
19+
[restricted]
20+
listen = {{ADDR[restricted]}}
21+
pm = static
22+
pm.max_children = 1
23+
php_admin_value[disable_functions] = zend_version
24+
EOT;
25+
26+
/* Disabling zend_version() moves the functions registered after it in the
27+
* function table, var_export() among them. */
28+
$code = <<<'EOT'
29+
<?php
30+
function zend_version_or_error(): string {
31+
try {
32+
return gettype(zend_version());
33+
} catch (Error $e) {
34+
return $e->getMessage();
35+
}
36+
}
37+
var_dump(is_array(opcache_get_status(false)));
38+
echo var_export(true, true), "\n";
39+
var_dump(function_exists('zend_version'));
40+
echo zend_version_or_error(), "\n";
41+
EOT;
42+
43+
$expected = [
44+
'unrestricted' => ['bool(true)', 'true', 'bool(true)', 'string'],
45+
'restricted' => ['bool(true)', 'true', 'bool(false)', 'Call to undefined function zend_version()'],
46+
];
47+
48+
$opcache = ini_get('extension_dir') . DIRECTORY_SEPARATOR . 'opcache.' . PHP_SHLIB_SUFFIX;
49+
$extraArgs = is_file($opcache) ? ['-dzend_extension=' . $opcache] : [];
50+
51+
function remove_file_cache(string $dir): void {
52+
if (!is_dir($dir)) {
53+
return;
54+
}
55+
$it = new RecursiveIteratorIterator(
56+
new RecursiveDirectoryIterator($dir, FilesystemIterator::SKIP_DOTS),
57+
RecursiveIteratorIterator::CHILD_FIRST
58+
);
59+
foreach ($it as $file) {
60+
$file->isDir() ? rmdir($file->getPathname()) : unlink($file->getPathname());
61+
}
62+
rmdir($dir);
63+
}
64+
65+
$fileCache = __DIR__ . '/gh24661-file-cache';
66+
$caches = [
67+
[],
68+
['opcache.file_cache' => $fileCache, 'opcache.file_cache_only' => '1'],
69+
];
70+
71+
foreach ($caches as $iniEntries) {
72+
/* The pool that compiles the script first must not matter. */
73+
foreach ([['unrestricted', 'restricted'], ['restricted', 'unrestricted']] as $pools) {
74+
if ($iniEntries) {
75+
remove_file_cache($fileCache);
76+
mkdir($fileCache);
77+
}
78+
$tester = new FPM\Tester($cfg, $code);
79+
$tester->start($extraArgs, iniEntries: $iniEntries + ['opcache.file_update_protection' => '0']);
80+
$tester->expectLogStartNotices();
81+
foreach ([...$pools, ...$pools] as $pool) {
82+
$tester->request(address: "{{ADDR[$pool]}}")->expectBody($expected[$pool]);
83+
}
84+
$tester->terminate();
85+
$tester->expectLogTerminatingNotices();
86+
$tester->close();
87+
if ($iniEntries) {
88+
/* One system ID directory for each function table. */
89+
$systemIds = glob($fileCache . '/*', GLOB_ONLYDIR);
90+
if (count($systemIds) !== 2) {
91+
echo "File cache directories: ", count($systemIds), "\n";
92+
}
93+
remove_file_cache($fileCache);
94+
}
95+
}
96+
}
97+
98+
?>
99+
Done
100+
--EXPECT--
101+
Done
102+
--CLEAN--
103+
<?php
104+
require_once "tester.inc";
105+
FPM\Tester::clean();
106+
$fileCache = __DIR__ . '/gh24661-file-cache';
107+
if (is_dir($fileCache)) {
108+
$it = new RecursiveIteratorIterator(
109+
new RecursiveDirectoryIterator($fileCache, FilesystemIterator::SKIP_DOTS),
110+
RecursiveIteratorIterator::CHILD_FIRST
111+
);
112+
foreach ($it as $file) {
113+
$file->isDir() ? rmdir($file->getPathname()) : unlink($file->getPathname());
114+
}
115+
rmdir($fileCache);
116+
}
117+
?>

0 commit comments

Comments
 (0)