Fix stream invalidation when statement goes out of scope (GH#1443)#1598
Merged
Fix stream invalidation when statement goes out of scope (GH#1443)#1598
Conversation
When a stream is created from sqlsrv_get_field, the stream now holds a reference (GC_ADDREF) to the statement's zend_resource. This prevents PHP from destroying the statement while the stream is still alive. Previously, if $stmt went out of scope (e.g. returned from a function), the statement destructor would close the stream, making it invalid even though userland code still held a reference to it. Changes: - Added zend_resource* stmt_res to sqlsrv_stream struct to hold a reference to the parent statement resource - Added zend_resource* zend_res to sqlsrv_stmt struct so the statement knows its own resource handle - On stream creation, increment statement resource refcount via GC_ADDREF - On stream close, release the reference via zend_list_delete after clearing active_stream (prevents double-close in statement destructor) - Store zend_resource on statement after registration in both sqlsrv_prepare and sqlsrv_query paths Fixes: #1443
There was a problem hiding this comment.
Pull request overview
This PR fixes a lifecycle bug in the SQLSRV driver where a binary stream returned from a statement could become invalid if the statement resource was destroyed while the stream was still in use, by keeping the statement’s zend_resource alive for the stream’s lifetime.
Changes:
- Track the statement’s
zend_resourceonsqlsrv_stmtand retain/release it fromsqlsrv_streamto prevent premature statement destruction while a stream exists. - Update stream close logic to drop the held statement resource reference safely.
- Add a functional regression test covering streams that outlive their originating statement scope, and update the changelog.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/functional/sqlsrv/sqlsrv_stream_outlives_stmt.phpt | Regression test ensuring streams remain readable after $stmt goes out of scope. |
| source/sqlsrv/conn.cpp | Stores the statement’s zend_resource after resource registration for later use by streams. |
| source/shared/core_stream.cpp | Releases the held statement resource reference when the stream is closed. |
| source/shared/core_stmt.cpp | Adds a statement-resource retain when creating a stream from a field. |
| source/shared/core_sqlsrv.h | Extends sqlsrv_stmt and sqlsrv_stream structs to track the statement’s zend_resource. |
| CHANGELOG.md | Adds release note entry for the stream invalidation fix. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
David-Engel
previously approved these changes
Apr 7, 2026
Test 4: Memory leak detection - runs the stream-outlives-stmt pattern 20 times and verifies memory usage stays stable, confirming both the stream and statement are properly freed when the stream is closed. Test 5: Connection health - verifies the connection remains fully functional after all streams/statements are cleaned up, proving no dangling ODBC handles remain.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #1598 +/- ##
==========================================
+ Coverage 85.73% 85.74% +0.01%
==========================================
Files 23 23
Lines 7212 7221 +9
==========================================
+ Hits 6183 6192 +9
Misses 1029 1029
🚀 New features to boost your workflow:
|
David-Engel
approved these changes
Apr 8, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request addresses a critical bug where binary streams returned from a statement could become invalid if the statement object went out of scope, even though the stream was still in use. The changes ensure the statement resource remains alive as long as any associated stream is referenced, preventing premature destruction and stream invalidation. Additionally, a regression test is added to verify the fix.
Bug Fixes and Resource Management:
sqlsrv_streamandsqlsrv_stmtstructures to track and retain a reference to the statement'szend_resource, ensuring the statement is not destroyed while a stream is active.zend_resourceis stored during bothsqlsrv_prepareandsqlsrv_queryso streams can access it.Testing and Documentation:
sqlsrv_stream_outlives_stmt.phpt) to confirm that streams remain valid after their originating statement goes out of scope, covering multiple scenarios.