Fix stream invalidation when statement goes out of scope (GH#1443) (#1598)

* FIX: Freeing metadata entries and clearing the vector

* Resolving comments

* Fix stream invalidation when statement goes out of scope (GH#1443)

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

* Add cleanup validation tests for stream-outlives-stmt fix

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.

* Removed newly added lines from changelog

Author: jahnvi480

source/shared/core_sqlsrv.h

@@ -1390,13 +1390,14 @@ struct sqlsrv_stream {
     SQLUSMALLINT field_index;
     SQLSMALLINT sql_type;
     sqlsrv_stmt* stmt;
+    zend_resource* stmt_res;     // prevent statement from being freed while stream is active
 
     sqlsrv_stream( _In_opt_ zval* str_z, _In_ SQLSRV_ENCODING enc ) :
-        stream_z( str_z ), encoding( enc ), field_index( 0 ), sql_type( SQL_UNKNOWN_TYPE ), stmt( NULL )
+        stream_z( str_z ), encoding( enc ), field_index( 0 ), sql_type( SQL_UNKNOWN_TYPE ), stmt( NULL ), stmt_res( NULL )
     {
     }
 
-    sqlsrv_stream() : stream_z( NULL ), encoding( SQLSRV_ENCODING_INVALID ), field_index( 0 ), sql_type( SQL_UNKNOWN_TYPE ), stmt( NULL )
+    sqlsrv_stream() : stream_z( NULL ), encoding( SQLSRV_ENCODING_INVALID ), field_index( 0 ), sql_type( SQL_UNKNOWN_TYPE ), stmt( NULL ), stmt_res( NULL )
     {
     }
 };
@@ -1724,6 +1725,7 @@ struct sqlsrv_stmt : public sqlsrv_context {
     zval field_cache;                     // cache for a single row of fields, to allow multiple and out of order retrievals
     zval col_cache;                       // Used by get_field_as_string not to call SQLColAttribute()  after every fetch.
     zval active_stream;                   // the currently active stream reading data from the database
+    zend_resource* zend_res;              // the zend_resource for this statement (used by streams to prevent premature stmt destruction)
 
     sqlsrv_params_container params_container;       // holds all parameters and references used for SQLBindParameter
 

source/shared/core_stmt.cpp

@@ -132,7 +132,8 @@ sqlsrv_stmt::sqlsrv_stmt( _In_ sqlsrv_conn* c, _In_ SQLHANDLE handle, _In_ error
     decimal_places(NO_CHANGE_DECIMAL_PLACES),     // the default is no formatting to resultset required
     data_classification(false),
     buffered_query_limit( sqlsrv_buffered_result_set::BUFFERED_QUERY_LIMIT_INVALID ),
-    send_streams_at_exec( true )
+    send_streams_at_exec( true ),
+    zend_res( NULL )
 {
     ZVAL_UNDEF( &active_stream );
 
@@ -1522,6 +1523,14 @@ void core_get_field_common( _Inout_ sqlsrv_stmt* stmt, _In_ SQLUSMALLINT field_i
             ss->sql_type = static_cast<SQLUSMALLINT>( sql_type );
             ss->encoding = static_cast<SQLSRV_ENCODING>( sqlsrv_php_type.typeinfo.encoding );
 
+            // If the statement has a zend_resource, hold a reference to it.
+            // This prevents the statement from being destroyed while the stream
+            // is still alive (e.g. when $stmt goes out of scope but $stream is returned).
+            if( stmt->zend_res != NULL ) {
+                ss->stmt_res = stmt->zend_res;
+                GC_ADDREF( ss->stmt_res );
+            }
+
             zval_auto_ptr return_value_z;
             return_value_z = ( zval * )sqlsrv_malloc( sizeof( zval ));
             ZVAL_UNDEF( return_value_z );

source/shared/core_stream.cpp

@@ -34,9 +34,18 @@ int sqlsrv_stream_close( _Inout_ php_stream* stream, int /*close_handle*/ )
     // UNDEF the stream zval and delete our reference count to it.
     ZVAL_UNDEF( &( ss->stmt->active_stream ) );
 
+    // Save stmt_res before freeing ss, then release the statement reference.
+    // This may trigger the statement destructor if this was the last reference,
+    // which is safe because active_stream was already set to UNDEF above.
+    zend_resource* saved_stmt_res = ss->stmt_res;
+
     sqlsrv_free( ss );
     stream->abstract = NULL;
 
+    if( saved_stmt_res != NULL ) {
+        zend_list_delete( saved_stmt_res );
+    }
+
     return 0;
 }
 

source/sqlsrv/conn.cpp

@@ -1131,6 +1131,9 @@ PHP_FUNCTION( sqlsrv_prepare )
         // register the statement with the PHP runtime
         ss::zend_register_resource( stmt_z, stmt, ss_sqlsrv_stmt::descriptor, ss_sqlsrv_stmt::resource_name );
 
+        // store the zend_resource on the statement so streams can prevent premature stmt destruction
+        stmt->zend_res = Z_RES(stmt_z);
+
         // store the resource id with the connection so the connection
         // can release this statement when it closes.
         zend_long next_index = zend_hash_next_free_element( conn->stmts );
@@ -1255,6 +1258,10 @@ PHP_FUNCTION( sqlsrv_query )
 
         // register the statement with the PHP runtime
         ss::zend_register_resource(stmt_z, stmt, ss_sqlsrv_stmt::descriptor, ss_sqlsrv_stmt::resource_name);
+
+        // store the zend_resource on the statement so streams can prevent premature stmt destruction
+        stmt->zend_res = Z_RES(stmt_z);
+
         // store the resource id with the connection so the connection
         // can release this statement when it closes.
         zend_ulong next_index = zend_hash_next_free_element( conn->stmts );

test/functional/sqlsrv/sqlsrv_stream_outlives_stmt.phpt

@@ -0,0 +1,160 @@
+--TEST--
+GitHub issue 1443 - stream remains valid after statement goes out of scope
+--DESCRIPTION--
+When a stream is returned from a function where the statement variable goes out
+of scope, the stream should remain valid as long as it has references. Previously
+the statement destructor would close the stream, making it invalid.
+Also validates that both the stream and statement are properly cleaned up after use,
+with no resource leaks detected via memory_get_usage over repeated iterations.
+--SKIPIF--
+<?php require('skipif.inc'); ?>
+--FILE--
+<?php
+require_once("MsCommon.inc");
+
+// Test 1: Stream returned from a function where $stmt goes out of scope
+function getStream($conn)
+{
+    $stmt = sqlsrv_query($conn, "SELECT CONVERT(VARBINARY(32), 0x48656C6C6F)");
+    if ($stmt === false) {
+        fatalError("Query failed in getStream.");
+    }
+    sqlsrv_fetch($stmt);
+    // $stmt will go out of scope when this function returns
+    return sqlsrv_get_field($stmt, 0, SQLSRV_PHPTYPE_STREAM(SQLSRV_ENC_BINARY));
+}
+
+$conn = connect();
+if ($conn === false) {
+    fatalError("Could not connect.");
+}
+
+$stream = getStream($conn);
+if ($stream === false) {
+    fatalError("Failed to get stream.");
+}
+
+// The stream should still be valid even though $stmt went out of scope
+$data = fread($stream, 100);
+if ($data === false) {
+    fatalError("fread failed on stream.");
+}
+
+// CONVERT(VARBINARY(32), 0x48656C6C6F) should return the bytes "Hello"
+if ($data === "Hello") {
+    echo "Test 1 passed: stream data read correctly after stmt out of scope.\n";
+} else {
+    echo "Test 1 FAILED: expected 'Hello', got '" . bin2hex($data) . "'.\n";
+}
+
+fclose($stream);
+
+// Test 2: Stream from sqlsrv_prepare + sqlsrv_execute
+function getStreamPrepared($conn)
+{
+    $stmt = sqlsrv_prepare($conn, "SELECT CONVERT(VARBINARY(32), 0x576F726C64)");
+    if ($stmt === false) {
+        fatalError("Prepare failed in getStreamPrepared.");
+    }
+    sqlsrv_execute($stmt);
+    sqlsrv_fetch($stmt);
+    return sqlsrv_get_field($stmt, 0, SQLSRV_PHPTYPE_STREAM(SQLSRV_ENC_BINARY));
+}
+
+$stream2 = getStreamPrepared($conn);
+if ($stream2 === false) {
+    fatalError("Failed to get stream from prepared stmt.");
+}
+
+$data2 = fread($stream2, 100);
+if ($data2 === "World") {
+    echo "Test 2 passed: stream from prepared stmt works after stmt out of scope.\n";
+} else {
+    echo "Test 2 FAILED: expected 'World', got '" . bin2hex($data2) . "'.\n";
+}
+
+fclose($stream2);
+
+// Test 3: Multiple reads from the stream
+function getStreamLargeData($conn)
+{
+    $stmt = sqlsrv_query($conn, "SELECT CONVERT(VARBINARY(MAX), REPLICATE(CONVERT(VARBINARY(MAX), 0x41), 1000))");
+    if ($stmt === false) {
+        fatalError("Query failed in getStreamLargeData.");
+    }
+    sqlsrv_fetch($stmt);
+    return sqlsrv_get_field($stmt, 0, SQLSRV_PHPTYPE_STREAM(SQLSRV_ENC_BINARY));
+}
+
+$stream3 = getStreamLargeData($conn);
+if ($stream3 === false) {
+    fatalError("Failed to get stream for large data.");
+}
+
+$allData = '';
+while (!feof($stream3)) {
+    $chunk = fread($stream3, 100);
+    if ($chunk === false) {
+        break;
+    }
+    $allData .= $chunk;
+}
+
+if (strlen($allData) === 1000 && $allData === str_repeat('A', 1000)) {
+    echo "Test 3 passed: large stream data read correctly.\n";
+} else {
+    echo "Test 3 FAILED: expected 1000 bytes of 'A', got " . strlen($allData) . " bytes.\n";
+}
+
+fclose($stream3);
+
+// Test 4: Verify cleanup - no resource leak over repeated iterations.
+// Run the stream-outlives-stmt pattern in a loop and check that memory
+// usage stabilizes, confirming the statement is freed when the stream closes.
+$leakDetected = false;
+for ($i = 0; $i < 20; $i++) {
+    $s = getStream($conn);
+    $d = fread($s, 100);
+    fclose($s);
+    unset($s);
+    unset($d);
+
+    if ($i === 0) {
+        $memBaseline = memory_get_usage();
+    }
+}
+$memAfter = memory_get_usage();
+// Allow a small tolerance (32 KB) for PHP internal allocations.
+// A real leak would grow ~proportionally to iteration count.
+if (($memAfter - $memBaseline) < 32768) {
+    echo "Test 4 passed: no resource leak detected over repeated iterations.\n";
+} else {
+    echo "Test 4 FAILED: possible leak, memory grew by " . ($memAfter - $memBaseline) . " bytes.\n";
+}
+
+// Test 5: After all streams are closed, the connection should still be
+// fully functional — proving statements were cleaned up properly.
+$stmt = sqlsrv_query($conn, "SELECT 1 AS alive");
+if ($stmt === false) {
+    echo "Test 5 FAILED: connection unusable after cleanup.\n";
+} else {
+    sqlsrv_fetch($stmt);
+    $val = sqlsrv_get_field($stmt, 0);
+    if ($val == 1) {
+        echo "Test 5 passed: connection works after all streams and statements cleaned up.\n";
+    } else {
+        echo "Test 5 FAILED: unexpected value $val.\n";
+    }
+    sqlsrv_free_stmt($stmt);
+}
+
+sqlsrv_close($conn);
+echo "Done.\n";
+?>
+--EXPECT--
+Test 1 passed: stream data read correctly after stmt out of scope.
+Test 2 passed: stream from prepared stmt works after stmt out of scope.
+Test 3 passed: large stream data read correctly.
+Test 4 passed: no resource leak detected over repeated iterations.
+Test 5 passed: connection works after all streams and statements cleaned up.
+Done.