Skip to content

fix: don't segfault when the PHP CLI is executed while FrankenPHP is running - #2593

Closed
ousamabenyounes wants to merge 3 commits into
php:mainfrom
ousamabenyounes:fix/issue-2342
Closed

fix: don't segfault when the PHP CLI is executed while FrankenPHP is running#2593
ousamabenyounes wants to merge 3 commits into
php:mainfrom
ousamabenyounes:fix/issue-2342

Conversation

@ousamabenyounes

Copy link
Copy Markdown
Contributor

What

frankenphp.ExecuteScriptCLI() (and ExecutePHPCode()) crash the whole process
with a segmentation fault when called after frankenphp.Init() in the same
process (issue #2342):

FrankenPHP started 🐘  php_version=8.5.10-dev ...
signal: segmentation fault

Why

Both functions start the embedded PHP CLI SAPI through php_embed_init(). That
runs sapi_startup() + php_module_startup() for the embed SAPI. When FrankenPHP
is already running, its own SAPI and the PHP engine are already started, so
booting a second SAPI on top corrupts global engine state and the process
crashes.

What this PR does

The minimal, safe fix: prevent the crash. When FrankenPHP is already running,
ExecuteScriptCLI/ExecutePHPCode now log an error and return a non-zero exit
status instead of booting the embedded CLI SAPI and segfaulting. The standalone
CLI path (no prior Init) is untouched.

This intentionally does not add a new way to run CLI scripts inside a running
server (the broader idea discussed on the issue, e.g. running on a dedicated
thread/child process); it only turns an undefined-behaviour crash into a defined,
recoverable error. That larger feature can be built on top later.

Thread-safety note: the guard reads isRunning (and globalLogger) without an
extra lock, consistent with how Init/Shutdown already manage isRunning and
how these globals are read elsewhere in the package. CLI-after-Init is a
sequential misuse, not concurrent access.

Test verification (RED → GREEN)

Run in the dev image (dev.Dockerfile), PHP 8.5 ZTS, -tags nowatcher.

RED — unmodified main (only the repro/test applied):

--- FAIL: TestExecuteScriptCLIWhileRunning (0.18s)
    cli_test.go:89: process was killed by a signal instead of exiting cleanly:
        signal: segmentation fault (core dumped)
    cli_test.go:91: Not equal: expected: 1  actual: -1
FAIL	github.com/dunglas/frankenphp

(A standalone Init() + ExecuteScriptCLI() program likewise crashes with
signal: segmentation fault, exit 139.)

GREEN — with the fix (go test -race):

=== RUN   TestRefuseCLIWhileRunning
--- PASS: TestRefuseCLIWhileRunning (0.03s)
=== RUN   TestExecuteScriptCLI
--- PASS: TestExecuteScriptCLI (0.08s)
=== RUN   TestExecuteCLICode
--- PASS: TestExecuteCLICode (0.01s)
=== RUN   TestExecuteScriptCLIWhileRunning
--- PASS: TestExecuteScriptCLIWhileRunning (0.03s)
PASS
ok  	github.com/dunglas/frankenphp	1.225s

Existing standalone CLI tests still pass (no regression). refuseCLIWhileRunning
shows 100% coverage; go build, go vet, gofmt, the Caddy module build and
go mod tidy -diff are all clean.

ousamabenyounes and others added 2 commits August 5, 2026 23:51
…running

ExecuteScriptCLI and ExecutePHPCode start the embedded PHP CLI SAPI via
php_embed_init(). When FrankenPHP is already running in the same process
(Init has been called), booting the embedded SAPI on top of the running
FrankenPHP SAPI corrupts the PHP engine and crashes the whole process with
a segmentation fault.

Refuse the call cleanly instead: when FrankenPHP is running, log an error and
return a non-zero exit status rather than crashing. The standalone CLI path
(no prior Init) is unchanged.

Fixes php#2342

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
staticcheck QF1008: exec.ExitError embeds *os.ProcessState, so Exited()
is promoted (as ExitCode() is already used on the next line).
@henderkes

Copy link
Copy Markdown
Contributor

But this can never happen? There's no code wiring to create this scenario.

The godoc already noted that ExecuteScriptCLI/ExecutePHPCode must not run
while FrankenPHP is up. Spell out the actionable alternatives: run the CLI
before Init(), after Shutdown(), or in a separate process (the same binary
in "php-cli" mode), which gives a real CLI SAPI and process isolation.

Refs php#2342

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@ousamabenyounes

Copy link
Copy Markdown
Contributor Author

You're right that nothing in-tree chains Init() + ExecuteScriptCLI()caddy/app.go calls Init() and never the CLI, caddy/php-cli.go the reverse.

But both are exported public API, so an external embedder can create the sequence — which is what @burgesQ did. My argument for the guard: an exported function that SIGSEGVs the whole process on misuse should fail loudly instead. Reproduced on PHP 8.5 ZTS (--enable-embed), guard removed:

FrankenPHP started 🐘 php_version=8.5.10-dev ...
Segmentation fault (core dumped)   # exit 139

Root cause as @burgesQ analysed: a second php_embed_init() on top of the SAPI/TSRM already started by Init(). The refusal path is covered by a test.

The guard logs an explicit error and returns a non-zero exit status — this keeps the exported int signature and the existing test coverage intact; a panic would only be warranted if you'd rather treat this as a hard programmer error.

I did look at making the call actually run while the server is up — executing the script on the FrankenPHP SAPI instead of a second embed SAPI — and I don't think it's a safe fix: PHP_SAPI would stay "frankenphp" (breaks console tooling that special-cases cli), and mutating process-global SAPI/ini/signal state while requests are in flight is racy. The legitimate "single binary that serves HTTP and also runs console commands" case is better served by a child process running the same binary in php-cli mode — as @AlliBalliBaba suggested above — real CLI SAPI + isolation, and it already works since that path never calls Init().

I've spelled out the lifecycle constraint in the godoc of both functions (run CLI execution before Init(), after Shutdown(), or in a child process).

@henderkes

Copy link
Copy Markdown
Contributor

For the love of god, @ousamabenyounes please stop slopping the slop responses everywhere. I don't want to read that.

@ousamabenyounes

Copy link
Copy Markdown
Contributor Author

Really sorry i had a bug on my agent workflow...i ll do my best to fix it ....

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants