Fix php-wasm popen stream leaks - #3679
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR fixes file descriptor/stream leaks in the php-wasm popen() read-mode shim by switching to regular file closing semantics, and adds regression tests to ensure repeated process-output APIs don’t leak /tmp/popen_output streams.
Changes:
- Switch read-mode process-output streams from pipe-backed streams to file-backed streams (closing via
fclose()semantics). - Apply the same behavior to
exec()/shell_exec()/system()paths that consume the same shim. - Add a regression test that runs 100 iterations per API and asserts no increase in open
/tmp/popen_outputstreams (for PHP 8.3 builds).
Reviewed changes
Copilot reviewed 3 out of 7 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/php-wasm/node/src/test/php-part-1.spec.ts | Adds regression coverage asserting no /tmp/popen_output stream leaks across repeated calls. |
| packages/php-wasm/compile/php/php_wasm.c | Switches wasm_php_exec stream creation from pipe semantics to file semantics. |
| packages/php-wasm/compile/php/Dockerfile | Patches upstream PHP sources so read-mode popen/exec use file semantics in wasm builds. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
9e77a00 to
f358f6e
Compare
AI assistance: OpenAI gpt-5.6-terra via OpenCode was used to regenerate PHP-WASM artifacts and verify the stream cleanup coverage.
AI-assisted: gpt-5.6-terra via OpenCode diagnosed the Asyncify stream regression, implemented the cleanup correction, regenerated artifacts, and ran focused matrix tests.
AI assistance: OpenAI gpt-5.6-sol via OpenCode identified and removed formatter-only changes unrelated to the PHP-WASM fix.
|
Weird... GitHub is showing zero changes for this PR. I'm updating it to the latest from trunk to see whether it resets anything. |
Remove unrelated all-version generated build refreshes while retaining the PHP 8.3 Node runtimes used by the regression test. AI assistance: OpenAI gpt-5.6-sol via OpenCode diagnosed the GitHub diff timeout, narrowed the artifact scope, and verified the resulting base comparison. Chris reviewed and directed the work.
|
When I click the "Files Changed" tab, I've gotten different changed file counts this evening. I may just checkout the PR branch and review locally based on a Git diff. |
|
I think there was a glitch here, because I also saw no changes after your previous comment, then it was showing a large diff. After my last push it seems back to normal on my end. |
Summary
popen()read-mode output with regular file-stream semantics because the shim buffers process output into/tmp/popen_output, not a real process pipe.php_exec()paths used byexec(),shell_exec(),system(), andpassthru().Fixes #3678.
Testing
npm exec nx -- run php-wasm-node:test-group-1-asyncify --testNamePattern="closes read-mode process output streams"npm exec nx -- run php-wasm-node:test-group-1-jspi --testNamePattern="closes read-mode process output streams"npm exec nx -- run php-wasm-node:lintnpm exec nx -- run php-wasm-node:typecheckprettier --check packages/php-wasm/node/src/test/php-part-1.spec.ts packages/php-wasm/supported-php-versions.mjsArtifact scope
AI assistance