Skip to content

Commit 31f17a3

Browse files
feat: persistent-zval helpers (deep-copy zval trees across threads)
Second step of the split suggested in #2287: land the persistent-zval subsystem as a standalone, reviewable header, independent of background workers. This is the subsystem most likely to hide latent refcount or memory-lifetime bugs; reviewing it in isolation is higher-signal than finding issues inside a 3k-line diff. ## What - persistent_zval.h (renamed from the bg_worker_vars.h draft, prefix dropped for generality): - persistent_zval_validate: whitelist (scalars, arrays of allowed values, enum instances). Everything else fails fast. - persistent_zval_persist: deep-copy request -> persistent (pemalloc) memory. Fast paths baked in: interned strings shared, opcache- immutable arrays passed by pointer without copying or owning. - persistent_zval_free: deep-free; skips interned strings and immutable arrays (borrowed, not owned). - persistent_zval_to_request: deep-copy persistent -> fresh request memory. Enums re-resolved by class + case name on each read. - frankenphp.c: header included only when FRANKENPHP_TEST_HOOKS is defined. First real consumer (background workers) drops the guard. - Test hook gated on FRANKENPHP_TEST_HOOKS: - PHP function frankenphp_test_persist_roundtrip(mixed): mixed runs validate -> persist -> to_request -> free and returns the result. - Registered via zend_register_functions at MINIT so it never appears in ext_functions[] and never ships in production builds. - CI workflows set -DFRANKENPHP_TEST_HOOKS in CGO_CFLAGS (tests.yaml + sanitizers.yaml). windows.yaml is the release build, not a test runner, and stays untouched. ## Notes - Build verified both without the flag (production path, no unused-function warnings) and with it (test path). - The FRANKENPHP_TEST_HOOKS guard around the header include goes away in the PR that lands the first real caller; the test hook itself goes away in that same step once end-to-end tests cover the code paths.
1 parent a05e6dd commit 31f17a3

6 files changed

Lines changed: 463 additions & 4 deletions

File tree

.github/workflows/sanitizers.yaml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -108,7 +108,7 @@ jobs:
108108
- name: Set CGO flags
109109
run: |
110110
{
111-
echo "CGO_CFLAGS=$CFLAGS -I${PWD}/watcher/target/include $(php-config --includes)"
111+
echo "CGO_CFLAGS=$CFLAGS -I${PWD}/watcher/target/include -DFRANKENPHP_TEST_HOOKS $(php-config --includes)"
112112
echo "CGO_LDFLAGS=$LDFLAGS $(php-config --ldflags) $(php-config --libs)"
113113
} >> "$GITHUB_ENV"
114114
- name: Run tests

.github/workflows/tests.yaml

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ jobs:
5959
- name: Install e-dant/watcher
6060
uses: ./.github/actions/watcher
6161
- name: Set CGO flags
62-
run: echo "CGO_CFLAGS=-I${PWD}/watcher/target/include $(php-config --includes)" >> "${GITHUB_ENV}"
62+
run: echo "CGO_CFLAGS=-I${PWD}/watcher/target/include -DFRANKENPHP_TEST_HOOKS $(php-config --includes)" >> "${GITHUB_ENV}"
6363
- name: Build
6464
run: go build
6565
- name: Build testcli binary
@@ -135,7 +135,7 @@ jobs:
135135
echo "GEN_STUB_SCRIPT=${PWD}/php-${PHP_VERSION}/build/gen_stub.php" >> "${GITHUB_ENV}"
136136
- name: Set CGO flags
137137
run: |
138-
echo "CGO_CFLAGS=$(php-config --includes)" >> "${GITHUB_ENV}"
138+
echo "CGO_CFLAGS=-DFRANKENPHP_TEST_HOOKS $(php-config --includes)" >> "${GITHUB_ENV}"
139139
echo "CGO_LDFLAGS=$(php-config --ldflags) $(php-config --libs)" >> "${GITHUB_ENV}"
140140
- name: Install gotestsum
141141
run: go install gotest.tools/gotestsum@latest
@@ -172,7 +172,7 @@ jobs:
172172
- name: Set CGO flags
173173
run: |
174174
{
175-
echo "CGO_CFLAGS=-I/opt/homebrew/include/ $(php-config --includes)"
175+
echo "CGO_CFLAGS=-I/opt/homebrew/include/ -DFRANKENPHP_TEST_HOOKS $(php-config --includes)"
176176
echo "CGO_LDFLAGS=-L/opt/homebrew/lib/ $(php-config --ldflags) $(php-config --libs)"
177177
} >> "${GITHUB_ENV}"
178178
- name: Build

frankenphp.c

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,12 @@
3737

3838
#include "_cgo_export.h"
3939
#include "frankenphp_arginfo.h"
40+
#ifdef FRANKENPHP_TEST_HOOKS
41+
/* The persistent_zval helpers are only compiled in when a consumer needs
42+
* them. The step that lands the first real caller (background workers)
43+
* will drop this guard. */
44+
#include "persistent_zval.h"
45+
#endif
4046

4147
#if defined(PHP_WIN32) && defined(ZTS)
4248
ZEND_TSRMLS_CACHE_DEFINE()
@@ -708,12 +714,55 @@ PHP_FUNCTION(frankenphp_log) {
708714
}
709715
}
710716

717+
#ifdef FRANKENPHP_TEST_HOOKS
718+
/* Test-only entry point that exercises persistent_zval.h end-to-end:
719+
* validate -> persist (request -> persistent memory) ->
720+
* to_request (persistent -> fresh request memory) -> free persistent copy.
721+
* Compiled only when FRANKENPHP_TEST_HOOKS is defined; never registered
722+
* in production builds. */
723+
ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(
724+
arginfo_frankenphp_test_persist_roundtrip, 0, 1, IS_MIXED, 0)
725+
ZEND_ARG_TYPE_INFO(0, value, IS_MIXED, 0)
726+
ZEND_END_ARG_INFO()
727+
728+
PHP_FUNCTION(frankenphp_test_persist_roundtrip) {
729+
zval *input;
730+
ZEND_PARSE_PARAMETERS_START(1, 1)
731+
Z_PARAM_ZVAL(input)
732+
ZEND_PARSE_PARAMETERS_END();
733+
734+
if (!persistent_zval_validate(input)) {
735+
zend_throw_exception(spl_ce_LogicException,
736+
"persistent_zval: value type not supported "
737+
"(only scalars, arrays, and enums are allowed)",
738+
0);
739+
RETURN_THROWS();
740+
}
741+
742+
zval persistent;
743+
persistent_zval_persist(&persistent, input);
744+
persistent_zval_to_request(return_value, &persistent);
745+
persistent_zval_free(&persistent);
746+
}
747+
748+
static const zend_function_entry frankenphp_test_hook_functions[] = {
749+
PHP_FE(frankenphp_test_persist_roundtrip,
750+
arginfo_frankenphp_test_persist_roundtrip) PHP_FE_END};
751+
#endif
752+
711753
PHP_MINIT_FUNCTION(frankenphp) {
712754
register_frankenphp_symbols(module_number);
713755
#ifndef PHP_WIN32
714756
pthread_atfork(NULL, NULL, frankenphp_fork_child);
715757
#endif
716758

759+
#ifdef FRANKENPHP_TEST_HOOKS
760+
if (zend_register_functions(NULL, frankenphp_test_hook_functions, NULL,
761+
MODULE_PERSISTENT) == FAILURE) {
762+
return FAILURE;
763+
}
764+
#endif
765+
717766
zend_function *func;
718767

719768
// Override putenv

persistent_zval.h

Lines changed: 272 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,272 @@
1+
/* persistent_zval.h - Deep-copy zval trees to and from persistent memory.
2+
*
3+
* Provides a small, self-contained toolkit for moving zval trees across
4+
* thread boundaries. The supported shape is a whitelist: scalars, arrays,
5+
* and enums. Everything else is rejected by persistent_zval_validate so
6+
* callers can fail fast before allocating.
7+
*
8+
* Fast paths:
9+
* - Interned strings: shared memory, no copy.
10+
* - Opcache-immutable arrays: shared pointer, no copy, no free.
11+
*
12+
* Included by frankenphp.c; not a standalone compilation unit. */
13+
14+
#ifndef PERSISTENT_ZVAL_H
15+
#define PERSISTENT_ZVAL_H
16+
17+
#include <Zend/zend_enum.h>
18+
19+
/* Enum payload stored in persistent memory: the class name + case name
20+
* are kept as persistent zend_strings and the case object is re-resolved
21+
* via zend_lookup_class + zend_enum_get_case_cstr on each read. */
22+
typedef struct {
23+
zend_string *class_name;
24+
zend_string *case_name;
25+
} persistent_zval_enum_t;
26+
27+
static void persistent_zval_free(zval *z);
28+
static void persistent_zval_to_request(zval *dst, zval *src);
29+
30+
/* Whitelist check: only scalars, arrays of allowed values, and enum
31+
* instances pass. Returns false for objects other than enums, resources,
32+
* closures, references, etc. */
33+
static bool persistent_zval_validate(zval *z) {
34+
switch (Z_TYPE_P(z)) {
35+
case IS_NULL:
36+
case IS_FALSE:
37+
case IS_TRUE:
38+
case IS_LONG:
39+
case IS_DOUBLE:
40+
case IS_STRING:
41+
return true;
42+
case IS_OBJECT:
43+
return (Z_OBJCE_P(z)->ce_flags & ZEND_ACC_ENUM) != 0;
44+
case IS_ARRAY: {
45+
/* Opcache-immutable arrays are compile-time constants in shared
46+
* memory; their leaves are guaranteed scalars or further immutable
47+
* arrays. The copy/free paths below already trust this flag, so a
48+
* recursive walk here would just be cycles. */
49+
if ((GC_FLAGS(Z_ARRVAL_P(z)) & IS_ARRAY_IMMUTABLE) != 0)
50+
return true;
51+
zval *val;
52+
ZEND_HASH_FOREACH_VAL(Z_ARRVAL_P(z), val) {
53+
if (!persistent_zval_validate(val))
54+
return false;
55+
}
56+
ZEND_HASH_FOREACH_END();
57+
return true;
58+
}
59+
default:
60+
return false;
61+
}
62+
}
63+
64+
/* Deep-copy a zval from request memory into persistent (pemalloc) memory.
65+
* Callers must have already passed persistent_zval_validate on src.
66+
*
67+
* Storage convention for enums: dst becomes IS_PTR holding a
68+
* persistent_zval_enum_t. This is an internal representation; the caller
69+
* should never expose a persistent zval to PHP directly, only via
70+
* persistent_zval_to_request. */
71+
static void persistent_zval_persist(zval *dst, zval *src) {
72+
switch (Z_TYPE_P(src)) {
73+
case IS_NULL:
74+
case IS_FALSE:
75+
case IS_TRUE:
76+
ZVAL_COPY_VALUE(dst, src);
77+
break;
78+
case IS_LONG:
79+
ZVAL_LONG(dst, Z_LVAL_P(src));
80+
break;
81+
case IS_DOUBLE:
82+
ZVAL_DOUBLE(dst, Z_DVAL_P(src));
83+
break;
84+
case IS_STRING: {
85+
zend_string *s = Z_STR_P(src);
86+
if (ZSTR_IS_INTERNED(s)) {
87+
ZVAL_STR(dst, s); /* interned strings live process-wide */
88+
} else {
89+
ZVAL_NEW_STR(dst, zend_string_init(ZSTR_VAL(s), ZSTR_LEN(s), 1));
90+
}
91+
break;
92+
}
93+
case IS_OBJECT: {
94+
/* Must be an enum (validated earlier). */
95+
zend_class_entry *ce = Z_OBJCE_P(src);
96+
persistent_zval_enum_t *e = pemalloc(sizeof(*e), 1);
97+
e->class_name =
98+
ZSTR_IS_INTERNED(ce->name)
99+
? ce->name
100+
: zend_string_init(ZSTR_VAL(ce->name), ZSTR_LEN(ce->name), 1);
101+
zval *case_name_zval = zend_enum_fetch_case_name(Z_OBJ_P(src));
102+
zend_string *case_str = Z_STR_P(case_name_zval);
103+
e->case_name =
104+
ZSTR_IS_INTERNED(case_str)
105+
? case_str
106+
: zend_string_init(ZSTR_VAL(case_str), ZSTR_LEN(case_str), 1);
107+
ZVAL_PTR(dst, e);
108+
break;
109+
}
110+
case IS_ARRAY: {
111+
HashTable *src_ht = Z_ARRVAL_P(src);
112+
if ((GC_FLAGS(src_ht) & IS_ARRAY_IMMUTABLE) != 0) {
113+
/* Opcache-immutable arrays live for the process lifetime and are
114+
* safe to share across threads by pointer. Zero-copy, zero-free. */
115+
ZVAL_ARR(dst, src_ht);
116+
break;
117+
}
118+
HashTable *dst_ht = pemalloc(sizeof(HashTable), 1);
119+
zend_hash_init(dst_ht, zend_hash_num_elements(src_ht), NULL, NULL, 1);
120+
ZVAL_ARR(dst, dst_ht);
121+
122+
zend_string *key;
123+
zend_ulong idx;
124+
zval *val;
125+
ZEND_HASH_FOREACH_KEY_VAL(src_ht, idx, key, val) {
126+
zval pval;
127+
persistent_zval_persist(&pval, val);
128+
if (key) {
129+
if (ZSTR_IS_INTERNED(key)) {
130+
zend_hash_add_new(dst_ht, key, &pval);
131+
} else {
132+
zend_string *pkey = zend_string_init(ZSTR_VAL(key), ZSTR_LEN(key), 1);
133+
zend_hash_add_new(dst_ht, pkey, &pval);
134+
zend_string_release(pkey);
135+
}
136+
} else {
137+
zend_hash_index_add_new(dst_ht, idx, &pval);
138+
}
139+
}
140+
ZEND_HASH_FOREACH_END();
141+
break;
142+
}
143+
default:
144+
ZVAL_NULL(dst);
145+
break;
146+
}
147+
}
148+
149+
/* Deep-free a persistent zval tree. Idempotent on scalars. Skips
150+
* interned strings and immutable arrays (they are borrowed, not owned). */
151+
static void persistent_zval_free(zval *z) {
152+
switch (Z_TYPE_P(z)) {
153+
case IS_STRING:
154+
if (!ZSTR_IS_INTERNED(Z_STR_P(z))) {
155+
zend_string_free(Z_STR_P(z));
156+
}
157+
break;
158+
case IS_PTR: {
159+
persistent_zval_enum_t *e = (persistent_zval_enum_t *)Z_PTR_P(z);
160+
if (!ZSTR_IS_INTERNED(e->class_name))
161+
zend_string_free(e->class_name);
162+
if (!ZSTR_IS_INTERNED(e->case_name))
163+
zend_string_free(e->case_name);
164+
pefree(e, 1);
165+
break;
166+
}
167+
case IS_ARRAY: {
168+
HashTable *ht = Z_ARRVAL_P(z);
169+
if ((GC_FLAGS(ht) & IS_ARRAY_IMMUTABLE) != 0) {
170+
/* Borrowed from opcache, do not touch. */
171+
break;
172+
}
173+
zval *val;
174+
ZEND_HASH_FOREACH_VAL(ht, val) { persistent_zval_free(val); }
175+
ZEND_HASH_FOREACH_END();
176+
zend_hash_destroy(ht);
177+
pefree(ht, 1);
178+
break;
179+
}
180+
default:
181+
break;
182+
}
183+
}
184+
185+
/* Deep-copy a persistent zval tree back into request memory. Enums are
186+
* resolved from their class+case names on each call. If the enum class
187+
* or case can't be found in the current thread's class table, an
188+
* exception is thrown and dst is set to IS_NULL. */
189+
static void persistent_zval_to_request(zval *dst, zval *src) {
190+
switch (Z_TYPE_P(src)) {
191+
case IS_NULL:
192+
case IS_FALSE:
193+
case IS_TRUE:
194+
ZVAL_COPY_VALUE(dst, src);
195+
break;
196+
case IS_LONG:
197+
ZVAL_LONG(dst, Z_LVAL_P(src));
198+
break;
199+
case IS_DOUBLE:
200+
ZVAL_DOUBLE(dst, Z_DVAL_P(src));
201+
break;
202+
case IS_STRING:
203+
if (ZSTR_IS_INTERNED(Z_STR_P(src))) {
204+
ZVAL_STR(dst, Z_STR_P(src));
205+
} else {
206+
ZVAL_STRINGL(dst, Z_STRVAL_P(src), Z_STRLEN_P(src));
207+
}
208+
break;
209+
case IS_PTR: {
210+
persistent_zval_enum_t *e = (persistent_zval_enum_t *)Z_PTR_P(src);
211+
zend_class_entry *ce = zend_lookup_class(e->class_name);
212+
if (!ce || !(ce->ce_flags & ZEND_ACC_ENUM)) {
213+
zend_throw_exception_ex(spl_ce_LogicException, 0,
214+
"persistent_zval: enum class \"%s\" not found",
215+
ZSTR_VAL(e->class_name));
216+
ZVAL_NULL(dst);
217+
break;
218+
}
219+
zend_object *enum_obj = zend_enum_get_case_cstr(ce, ZSTR_VAL(e->case_name));
220+
if (!enum_obj) {
221+
zend_throw_exception_ex(spl_ce_LogicException, 0,
222+
"persistent_zval: enum case \"%s::%s\" not found",
223+
ZSTR_VAL(e->class_name), ZSTR_VAL(e->case_name));
224+
ZVAL_NULL(dst);
225+
break;
226+
}
227+
ZVAL_OBJ_COPY(dst, enum_obj);
228+
break;
229+
}
230+
case IS_ARRAY: {
231+
HashTable *src_ht = Z_ARRVAL_P(src);
232+
if ((GC_FLAGS(src_ht) & IS_ARRAY_IMMUTABLE) != 0) {
233+
/* Zero-copy: immutable arrays are safe to expose directly. */
234+
ZVAL_ARR(dst, src_ht);
235+
break;
236+
}
237+
array_init_size(dst, zend_hash_num_elements(src_ht));
238+
HashTable *dst_ht = Z_ARRVAL_P(dst);
239+
240+
zend_string *key;
241+
zend_ulong idx;
242+
zval *val;
243+
ZEND_HASH_FOREACH_KEY_VAL(src_ht, idx, key, val) {
244+
zval rval;
245+
persistent_zval_to_request(&rval, val);
246+
if (EG(exception)) {
247+
zval_ptr_dtor(&rval);
248+
break;
249+
}
250+
if (key) {
251+
if (ZSTR_IS_INTERNED(key)) {
252+
zend_hash_add_new(dst_ht, key, &rval);
253+
} else {
254+
zend_string *rkey = zend_string_init(ZSTR_VAL(key), ZSTR_LEN(key), 0);
255+
ZSTR_H(rkey) = ZSTR_H(key);
256+
zend_hash_add_new(dst_ht, rkey, &rval);
257+
zend_string_release(rkey);
258+
}
259+
} else {
260+
zend_hash_index_add_new(dst_ht, idx, &rval);
261+
}
262+
}
263+
ZEND_HASH_FOREACH_END();
264+
break;
265+
}
266+
default:
267+
ZVAL_NULL(dst);
268+
break;
269+
}
270+
}
271+
272+
#endif /* PERSISTENT_ZVAL_H */

0 commit comments

Comments
 (0)