Skip to content

Commit e02e053

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 e02e053

6 files changed

Lines changed: 468 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: 277 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,277 @@
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+
/* Whitelist check: only scalars, arrays of allowed values, and enum
28+
* instances pass. Returns false for objects other than enums, resources,
29+
* closures, references, etc. */
30+
static bool persistent_zval_validate(zval *z) {
31+
switch (Z_TYPE_P(z)) {
32+
case IS_NULL:
33+
case IS_FALSE:
34+
case IS_TRUE:
35+
case IS_LONG:
36+
case IS_DOUBLE:
37+
case IS_STRING:
38+
return true;
39+
case IS_OBJECT:
40+
return (Z_OBJCE_P(z)->ce_flags & ZEND_ACC_ENUM) != 0;
41+
case IS_ARRAY: {
42+
/* Opcache-immutable arrays are compile-time constants in shared
43+
* memory; their leaves are guaranteed scalars or further immutable
44+
* arrays. The copy/free paths below already trust this flag, so a
45+
* recursive walk here would just be cycles. */
46+
if ((GC_FLAGS(Z_ARRVAL_P(z)) & IS_ARRAY_IMMUTABLE) != 0)
47+
return true;
48+
zval *val;
49+
ZEND_HASH_FOREACH_VAL(Z_ARRVAL_P(z), val) {
50+
if (!persistent_zval_validate(val))
51+
return false;
52+
}
53+
ZEND_HASH_FOREACH_END();
54+
return true;
55+
}
56+
default:
57+
return false;
58+
}
59+
}
60+
61+
/* Deep-copy a zval from request memory into persistent (pemalloc) memory.
62+
* Callers must have already passed persistent_zval_validate on src.
63+
*
64+
* Storage convention for enums: dst becomes IS_PTR holding a
65+
* persistent_zval_enum_t. This is an internal representation; the caller
66+
* should never expose a persistent zval to PHP directly, only via
67+
* persistent_zval_to_request. */
68+
static void persistent_zval_persist(zval *dst, zval *src) {
69+
switch (Z_TYPE_P(src)) {
70+
case IS_NULL:
71+
case IS_FALSE:
72+
case IS_TRUE:
73+
ZVAL_COPY_VALUE(dst, src);
74+
break;
75+
case IS_LONG:
76+
ZVAL_LONG(dst, Z_LVAL_P(src));
77+
break;
78+
case IS_DOUBLE:
79+
ZVAL_DOUBLE(dst, Z_DVAL_P(src));
80+
break;
81+
case IS_STRING: {
82+
zend_string *s = Z_STR_P(src);
83+
if (ZSTR_IS_INTERNED(s)) {
84+
ZVAL_STR(dst, s); /* interned strings live process-wide */
85+
} else {
86+
ZVAL_NEW_STR(dst, zend_string_init(ZSTR_VAL(s), ZSTR_LEN(s), 1));
87+
}
88+
break;
89+
}
90+
case IS_OBJECT: {
91+
/* Must be an enum (validated earlier). */
92+
zend_class_entry *ce = Z_OBJCE_P(src);
93+
persistent_zval_enum_t *e = pemalloc(sizeof(*e), 1);
94+
e->class_name =
95+
ZSTR_IS_INTERNED(ce->name)
96+
? ce->name
97+
: zend_string_init(ZSTR_VAL(ce->name), ZSTR_LEN(ce->name), 1);
98+
zval *case_name_zval = zend_enum_fetch_case_name(Z_OBJ_P(src));
99+
zend_string *case_str = Z_STR_P(case_name_zval);
100+
e->case_name =
101+
ZSTR_IS_INTERNED(case_str)
102+
? case_str
103+
: zend_string_init(ZSTR_VAL(case_str), ZSTR_LEN(case_str), 1);
104+
ZVAL_PTR(dst, e);
105+
break;
106+
}
107+
case IS_ARRAY: {
108+
HashTable *src_ht = Z_ARRVAL_P(src);
109+
if ((GC_FLAGS(src_ht) & IS_ARRAY_IMMUTABLE) != 0) {
110+
/* Opcache-immutable arrays live for the process lifetime and are
111+
* safe to share across threads by pointer. Zero-copy, zero-free. */
112+
ZVAL_ARR(dst, src_ht);
113+
break;
114+
}
115+
HashTable *dst_ht = pemalloc(sizeof(HashTable), 1);
116+
zend_hash_init(dst_ht, zend_hash_num_elements(src_ht), NULL, NULL, 1);
117+
ZVAL_ARR(dst, dst_ht);
118+
119+
zend_string *key;
120+
zend_ulong idx;
121+
zval *val;
122+
ZEND_HASH_FOREACH_KEY_VAL(src_ht, idx, key, val) {
123+
zval pval;
124+
persistent_zval_persist(&pval, val);
125+
if (key) {
126+
if (ZSTR_IS_INTERNED(key)) {
127+
zend_hash_add_new(dst_ht, key, &pval);
128+
} else {
129+
zend_string *pkey = zend_string_init(ZSTR_VAL(key), ZSTR_LEN(key), 1);
130+
/* Iteration guarantees the source key has its hash set.
131+
* Propagating it lets zend_hash_add_new skip the re-hash. */
132+
ZSTR_H(pkey) = ZSTR_H(key);
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+
/* Unreachable: persistent_zval_validate is the gatekeeper. */
145+
ZEND_UNREACHABLE();
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 (EG(exception)) {
213+
/* Autoloader threw; let that exception propagate untouched. */
214+
ZVAL_NULL(dst);
215+
break;
216+
}
217+
if (!ce || !(ce->ce_flags & ZEND_ACC_ENUM)) {
218+
zend_throw_exception_ex(spl_ce_LogicException, 0,
219+
"persistent_zval: enum class \"%s\" not found",
220+
ZSTR_VAL(e->class_name));
221+
ZVAL_NULL(dst);
222+
break;
223+
}
224+
zend_object *enum_obj = zend_enum_get_case_cstr(ce, ZSTR_VAL(e->case_name));
225+
if (!enum_obj) {
226+
zend_throw_exception_ex(spl_ce_LogicException, 0,
227+
"persistent_zval: enum case \"%s::%s\" not found",
228+
ZSTR_VAL(e->class_name), ZSTR_VAL(e->case_name));
229+
ZVAL_NULL(dst);
230+
break;
231+
}
232+
ZVAL_OBJ_COPY(dst, enum_obj);
233+
break;
234+
}
235+
case IS_ARRAY: {
236+
HashTable *src_ht = Z_ARRVAL_P(src);
237+
if ((GC_FLAGS(src_ht) & IS_ARRAY_IMMUTABLE) != 0) {
238+
/* Zero-copy: immutable arrays are safe to expose directly. */
239+
ZVAL_ARR(dst, src_ht);
240+
break;
241+
}
242+
array_init_size(dst, zend_hash_num_elements(src_ht));
243+
HashTable *dst_ht = Z_ARRVAL_P(dst);
244+
245+
zend_string *key;
246+
zend_ulong idx;
247+
zval *val;
248+
ZEND_HASH_FOREACH_KEY_VAL(src_ht, idx, key, val) {
249+
zval rval;
250+
persistent_zval_to_request(&rval, val);
251+
if (EG(exception)) {
252+
zval_ptr_dtor(&rval);
253+
break;
254+
}
255+
if (key) {
256+
if (ZSTR_IS_INTERNED(key)) {
257+
zend_hash_add_new(dst_ht, key, &rval);
258+
} else {
259+
zend_string *rkey = zend_string_init(ZSTR_VAL(key), ZSTR_LEN(key), 0);
260+
ZSTR_H(rkey) = ZSTR_H(key);
261+
zend_hash_add_new(dst_ht, rkey, &rval);
262+
zend_string_release(rkey);
263+
}
264+
} else {
265+
zend_hash_index_add_new(dst_ht, idx, &rval);
266+
}
267+
}
268+
ZEND_HASH_FOREACH_END();
269+
break;
270+
}
271+
default:
272+
/* Unreachable: only types produced by persistent_zval_persist land here. */
273+
ZEND_UNREACHABLE();
274+
}
275+
}
276+
277+
#endif /* PERSISTENT_ZVAL_H */

0 commit comments

Comments
 (0)