perf: move sandboxed environment to the C side#2058
perf: move sandboxed environment to the C side#2058AlliBalliBaba wants to merge 50 commits intomainfrom
Conversation
* removes backoff. * Adjusts comment. * Suggestions by @dunglas * Removes 'max_consecutive_failures' * Removes 'max_consecutive_failures' * Adjusts warning. * Disables the logger in tests. * Revert "Adjusts warning." This reverts commit e93a6a9. * Revert "Removes 'max_consecutive_failures'" This reverts commit ba28ea0. * Revert "Removes 'max_consecutive_failures'" This reverts commit 32e649c. * Only fails on max failures again. * Restores failure timings.
# Conflicts: # phpmainthread_test.go
# Conflicts: # phpthread.go # threadworker.go
# Conflicts: # phpthread.go # threadinactive.go # threadregular.go # threadworker.go # worker.go
# Conflicts: # phpmainthread_test.go # threadregular.go
# Conflicts: # env.go # frankenphp.c # phpmainthread.go # phpthread.go # threadregular.go
# Conflicts: # env.go # frankenphp.c # phpmainthread.go # phpthread.go # threadregular.go
There was a problem hiding this comment.
Pull request overview
This PR shifts environment sandboxing ($_ENV/$_SERVER + getenv/putenv) from Go into the C/Zend side by snapshotting the process environment once and using zend_array_dup / zend_hash_copy to avoid per-request CGO overhead.
Changes:
- Add a main-thread environment snapshot (
main_thread_env) in C and use it to populate$_SERVER/$_ENV. - Rework
frankenphp_putenv/frankenphp_getenvto use a thread-local sandbox HashTable (sandboxed_env) and reset it at script termination. - Remove Go-side sandbox env caches/cleanup and update tests/fixtures accordingly.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
env.go |
Replaces Go map-based env snapshot with a C HashTable initializer called from C. |
frankenphp.c |
Implements C-side env snapshotting + sandboxed getenv/putenv, and updates $_SERVER/$_ENV registration. |
phpthread.go |
Removes per-thread Go-side sandbox env storage. |
phpmainthread.go |
Removes main-thread Go-side sandbox env storage. |
threadregular.go |
Drops Go-side sandbox env clearing for regular threads. |
threadworker.go |
Drops Go-side sandbox env clearing during worker script setup. |
worker_test.go |
Adjusts worker env test to rely on package TestMain setup. |
frankenphp_test.go |
Sets a required env var in TestMain and updates expected env test output. |
testdata/env/test-env.php |
Extends env behavior checks (putenv shouldn’t affect $_SERVER, invalid key handling). |
caddy/caddy_test.go |
Moves env var setup for tests into package TestMain. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| zkey := C.frankenphp_init_persistent_string(toUnsafeChar(key), C.size_t(len(key))) | ||
| zvalStr := C.frankenphp_init_persistent_string(toUnsafeChar(val), C.size_t(len(val))) | ||
|
|
||
| // copy the main thread env to the thread specific env | ||
| func cloneSandboxedEnv(thread *phpThread) { | ||
| if thread.sandboxedEnv != nil { | ||
| return | ||
| } | ||
| thread.sandboxedEnv = make(map[string]*C.zend_string, len(mainThread.sandboxedEnv)) | ||
| for key, value := range mainThread.sandboxedEnv { | ||
| thread.sandboxedEnv[key] = value | ||
| var zval C.zval | ||
| *(*uint32)(unsafe.Pointer(&zval.u1)) = C.IS_INTERNED_STRING_EX | ||
| *(**C.zend_string)(unsafe.Pointer(&zval.value)) = zvalStr | ||
| C.zend_hash_update(mainThreadEnv, zkey, &zval) | ||
| } |
There was a problem hiding this comment.
go_init_os_env constructs a C.zval by writing directly into zval.u1 / zval.value using unsafe.Pointer. This relies on Zend Engine internal layout details and is likely to break across PHP versions/architectures (and can lead to subtle memory corruption). Prefer adding a small C helper (e.g., using ZVAL_STR/ZVAL_INTERNED_STR + zend_hash_update) and call that from Go, rather than manually poking the struct fields.
There was a problem hiding this comment.
I would avoid implementing this suggestion because it would involve a CGO call.
There was a problem hiding this comment.
I added a wrapper function that does both ZVAL_STR and zend_hash_update to keep it at 1 Cgo call.
| /* take a snapshot of the environment for sandboxing */ | ||
| if (main_thread_env == NULL) { | ||
| main_thread_env = malloc(sizeof(HashTable)); | ||
| zend_hash_init(main_thread_env, 8, NULL, NULL, 1); | ||
| go_init_os_env(main_thread_env); | ||
| } |
There was a problem hiding this comment.
main_thread_env is allocated with malloc(sizeof(HashTable)) and initialized, but it is never destroyed/freed during shutdown. If FrankenPHP is used as an embedded library or in test harnesses that initialize/shutdown multiple times, this becomes a real leak (and it also bypasses Zend's persistent allocator conventions). Consider allocating via pemalloc(..., 1)/zend_new_array() and releasing it (e.g., zend_hash_destroy(main_thread_env) + free/pefree) during module/SAPI shutdown.
There was a problem hiding this comment.
I don't think we ever want to free this.
There was a problem hiding this comment.
Yeah these strings should never be freed. I changed it to pemalloc(..., 1) to keep with convention
3 participants
This PR uses
zend_array_dupto simplify the environment sandboxing logic. It removes the CGO overhead from$_ENVand$_SERVERregistration in regular threads and saves some memory on many threads. (wip)