ZTS (Zend Thread Safety) full support for FrankenPHP #347
Conversation
ioaniftimesei
commented
Nov 7, 2025
ioaniftimesei
commented
- Refactor to ZTS globals for FrankenPHP initial support
- Update Action.cpp to use SAPI headers directly for ZTS compatibility
- Replace C++17 structured bindings with iterator loops for compatibility
- Add FrankenPHP detection and installation support
- Add thread ID logging support
![aikido-pr-checks[bot]](https://avatars.githubusercontent.com/in/898896?s=60&v=4){kind=small}
…readability and performance in multiple files
ioaniftimesei
force-pushed
the
zts-thread-safety-module-globals
branch
from
e90deb1 to
cc173d4
Compare
…ion, so destroy the stats map after RequestProcessor to prevent use-after-free.
…ensure proper cleanup and prevent use-after-free issues.
… and enhance shell execution statistics
… zts extension binaries in rpm
tudor-timcu
changed the title
| } | ||
| } | ||
| return "unknown"; | ||
| zval_ptr_dtor(&retval); |
There was a problem hiding this comment.
Calling zval_ptr_dtor(&retval) when 'retval' may be uninitialized in GetSqlDialectFromPdo(zval*), risking dereferencing invalid memory
Details
✨ AI Reasoning
1) Function GetSqlDialectFromPdo(zval *pdo_object) declares a local zval 'retval' and then unconditionally calls zval_ptr_dtor(&retval) before returning; if CallPhpFunctionWithOneParam(...) returns false, 'retval' is uninitialized and passing it to zval_ptr_dtor can dereference invalid memory, causing a segmentation fault.
2) This change replaced an earlier return path that avoided calling zval_ptr_dtor when the call failed, so the modification introduced the unsafe unconditional destructor call.
🔧 How do I fix it?
Add null checks before dereferencing pointers, validate array bounds before access, avoid using pointers after free/delete, don't write to string literals, and prefer smart pointers in modern C++.
More info - Comment @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
| } | ||
| } | ||
| return json(); | ||
| zval_ptr_dtor(&retval); |
There was a problem hiding this comment.
Calling zval_ptr_dtor(&retval) when 'retval' may be uninitialized in CallPhpFunctionParseUrl, risking dereferencing invalid memory
Details
✨ AI Reasoning
1) Function CallPhpFunctionParseUrl declares local zval 'retval' and always calls zval_ptr_dtor(&retval) before returning; if CallPhpFunctionWithOneParam(...) returns false, 'retval' is uninitialized and zval_ptr_dtor may access invalid memory, producing a segmentation fault.
2) Previously the function returned early without dtor in failing cases; the new unconditional dtor call introduced the unsafe behavior.
🔧 How do I fix it?
Add null checks before dereferencing pointers, validate array bounds before access, avoid using pointers after free/delete, don't write to string literals, and prefer smart pointers in modern C++.
More info - Comment @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
| FrankenPHP's Caddyfile env directive only populates $_SERVER, not the process environment. | ||
| This function reads environment variables from $_SERVER for FrankenPHP compatibility. | ||
| */ | ||
| std::string GetFrankenEnvVariable(const std::string& env_key) { |
There was a problem hiding this comment.
Function GetFrankenEnvVariable duplicates env-retrieval and logging logic already implemented by GetPhpEnvVariable/GetSystemEnvVariable
Details
🔧 How do I fix it?
Delete extra code. Extract repeated code sequences into reusable functions or methods. Use loops or data structures to eliminate repetitive patterns.
More info - Comment @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
| AIKIDO_LOG_ERROR("Failed to initialize the request processor: %s!\n", dlerror()); | ||
| return false; | ||
| } | ||
| if (this->requestProcessorInstance == nullptr && this->createInstanceFn != nullptr) { |
There was a problem hiding this comment.
Function RequestProcessor::RequestInit has grown to handle multiple responsibilities (per-thread instance creation, Go init, SAPI-specific config reload, env checks), making it too long and complex.
Details
✨ AI Reasoning
1) Identification: The diff modifies the method RequestProcessor::RequestInit in lib/php-extension/RequestProcessor.cpp by adding substantial logic for creating per-thread RequestProcessor instances, initializing them, handling FrankenPHP/apace2handler SAPI differences, reloading config per-request, and added env-check logic.
- Analysis of the function body (post-change):
- Total lines: the RequestInit function body spans well over 70 lines after the changes (many new blocks were inserted).
- Distinct concerns: initialization of native library instance; creation of per-thread Go instance; loading and initialization of Go RequestProcessor; SAPI-specific control flow for config reload; environment flag checking; token reload loop behavior; request counting and stats reporting — at least 5 distinct responsibilities.
- Cognitive complexity: multiple nested conditionals, platform/SAPI-specific branches (apache2handler vs frankenphp vs others), dynamic symbol calls, and instance lifecycle management interleaved with request-counting and configuration logic.
- Confidence calculation (starting at base 0.5):
- +0.4 (function is 70+ lines or mixes 5+ concerns) => 0.9
- +0.2 (diff added substantial new logic 10+ lines representing a new concern — per-thread instance creation and initialization) => 1.1
- cap to 1.0 (maximum confidence)
3.x adjustments: (applied above) added +0.4 and +0.2 because the function exceeds the size threshold and the diff introduced a new, separable concern (per-thread Go instance lifecycle) of substantial size.
Conclusion: The RequestProcessor::RequestInit function was made noticeably longer and more complex by this diff, reducing maintainability and mixing several concerns in one method.
🔧 How do I fix it?
Break down long functions into smaller helper functions. Aim for functions under 60 lines with fewer than 10 local variables.
More info - Comment @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
tudor-timcu
changed the title
