FIX: Segmentation Fault in libmsodbcsql-18.5 during SQLFreeHandle() (#415)

subrata-ms · web-flow · commit f2290529bdd8 · 2026-01-30T15:53:19.000+05:30
> [AB#41463](https://sqlclientdrivers.visualstudio.com/c6d89619-62de-46a0-8b46-70b92a84d85e/_workitems/edit/41463)  > GitHub Issue: #341 ------------------------------------------------------------------- ### Summary  This pull request implements a critical fix for a long-standing use-after-free (segmentation fault) bug that occurred when a database connection was closed while statement handles were still alive. The fix ensures that child statement handles are properly tracked and marked as "implicitly freed" when the parent connection is closed, preventing double-free and use-after-free errors. Additionally, comprehensive regression tests are added to verify the fix under various scenarios. **Critical bug fix for handle management:** * The `Connection` class now tracks all child statement handles in a `_childStatementHandles` vector using `weak_ptr` to avoid circular references and memory leaks. When the connection is closed, all child statement handles are marked as "implicitly freed" before the parent handle is released. This prevents the `SqlHandle` destructor from attempting to free handles that have already been freed by the ODBC driver. [[1]](diffhunk://#diff-c74277ea1f36fff8f6bbe9df3fd722f7d270f3d6a3069ac7a747fe6d4235cf13R64-R67) [[2]](diffhunk://#diff-eca696c13d997f510e5f9b16288ed1deb0ad132768c283eda1518f78edf9b6ecR97-R110) [[3]](diffhunk://#diff-eca696c13d997f510e5f9b16288ed1deb0ad132768c283eda1518f78edf9b6ecL176-R203) * The `SqlHandle` class now includes an `_implicitly_freed` flag and a `markImplicitlyFreed()` method. The `free()` method checks this flag and skips ODBC cleanup if the handle was already implicitly freed, preventing use-after-free errors. [[1]](diffhunk://#diff-85167a2d59779df18704284ab7ce46220c3619408fbf22c631ffdf29f794d635R382-R390) [[2]](diffhunk://#diff-dde2297345718ec449a14e7dff91b7bb2342b008ecc071f562233646d71144a1R1147-R1150) [[3]](diffhunk://#diff-dde2297345718ec449a14e7dff91b7bb2342b008ecc071f562233646d71144a1L1172-L1181) * The Python bindings are updated to expose the new `markImplicitlyFreed` method on `SqlHandle`, ensuring that the state can be managed from both C++ and Python layers. **Testing and verification:** * A new test file, `test_016_connection_invalidation_segfault.py`, is added with extensive tests that reproduce the original segfault scenarios, including multiple cursors, uncommitted transactions, prepared statements, and concurrent connection invalidation. These tests confirm that the fix prevents crashes and ensures clean resource management. **Other minor changes:** * Minor whitespace and comment cleanups in unrelated parts of the codebase. [[1]](diffhunk://#diff-dde2297345718ec449a14e7dff91b7bb2342b008ecc071f562233646d71144a1L2896) [[2]](diffhunk://#diff-dde2297345718ec449a14e7dff91b7bb2342b008ecc071f562233646d71144a1L3618-L3619) This change is critical for stability and correctness when using connection pooling, SQLAlchemy, or any code that may close connections before all cursors are explicitly closed.
diff --git a/mssql_python/pybind/connection/connection.cpp b/mssql_python/pybind/connection/connection.cpp
@@ -94,6 +94,47 @@ void Connection::connect(const py::dict& attrs_before) {
 void Connection::disconnect() {
     if (_dbcHandle) {
         LOG("Disconnecting from database");
+
+        // CRITICAL FIX: Mark all child statement handles as implicitly freed
+        // When we free the DBC handle below, the ODBC driver will automatically free
+        // all child STMT handles. We need to tell the SqlHandle objects about this
+        // so they don't try to free the handles again during their destruction.
+        
+        // THREAD-SAFETY: Lock mutex to safely access _childStatementHandles
+        // This protects against concurrent allocStatementHandle() calls or GC finalizers
+        {
+            std::lock_guard<std::mutex> lock(_childHandlesMutex);
+            
+            // First compact: remove expired weak_ptrs (they're already destroyed)
+            size_t originalSize = _childStatementHandles.size();
+            _childStatementHandles.erase(
+                std::remove_if(_childStatementHandles.begin(), _childStatementHandles.end(),
+                               [](const std::weak_ptr<SqlHandle>& wp) { return wp.expired(); }),
+                _childStatementHandles.end());
+            
+            LOG("Compacted child handles: %zu -> %zu (removed %zu expired)",
+                originalSize, _childStatementHandles.size(),
+                originalSize - _childStatementHandles.size());
+            
+            LOG("Marking %zu child statement handles as implicitly freed",
+                _childStatementHandles.size());
+            for (auto& weakHandle : _childStatementHandles) {
+                if (auto handle = weakHandle.lock()) {
+                    // SAFETY ASSERTION: Only STMT handles should be in this vector
+                    // This is guaranteed by allocStatementHandle() which only creates STMT handles
+                    // If this assertion fails, it indicates a serious bug in handle tracking
+                    if (handle->type() != SQL_HANDLE_STMT) {
+                        LOG_ERROR("CRITICAL: Non-STMT handle (type=%d) found in _childStatementHandles. "
+                                  "This will cause a handle leak!", handle->type());
+                        continue;  // Skip marking to prevent leak
+                    }
+                    handle->markImplicitlyFreed();
+                }
+            }
+            _childStatementHandles.clear();
+            _allocationsSinceCompaction = 0;
+        }  // Release lock before potentially slow SQLDisconnect call
+
         SQLRETURN ret = SQLDisconnect_ptr(_dbcHandle->get());
         checkError(ret);
         // triggers SQLFreeHandle via destructor, if last owner
@@ -173,7 +214,36 @@ SqlHandlePtr Connection::allocStatementHandle() {
     SQLHANDLE stmt = nullptr;
     SQLRETURN ret = SQLAllocHandle_ptr(SQL_HANDLE_STMT, _dbcHandle->get(), &stmt);
     checkError(ret);
-    return std::make_shared<SqlHandle>(static_cast<SQLSMALLINT>(SQL_HANDLE_STMT), stmt);
+    auto stmtHandle = std::make_shared<SqlHandle>(static_cast<SQLSMALLINT>(SQL_HANDLE_STMT), stmt);
+
+    // THREAD-SAFETY: Lock mutex before modifying _childStatementHandles
+    // This protects against concurrent disconnect() or allocStatementHandle() calls,
+    // or GC finalizers running from different threads
+    {
+        std::lock_guard<std::mutex> lock(_childHandlesMutex);
+        
+        // Track this child handle so we can mark it as implicitly freed when connection closes
+        // Use weak_ptr to avoid circular references and allow normal cleanup
+        _childStatementHandles.push_back(stmtHandle);
+        _allocationsSinceCompaction++;
+
+        // Compact expired weak_ptrs only periodically to avoid O(n²) overhead
+        // This keeps allocation fast (O(1) amortized) while preventing unbounded growth
+        // disconnect() also compacts, so this is just for long-lived connections with many cursors
+        if (_allocationsSinceCompaction >= COMPACTION_INTERVAL) {
+            size_t originalSize = _childStatementHandles.size();
+            _childStatementHandles.erase(
+                std::remove_if(_childStatementHandles.begin(), _childStatementHandles.end(),
+                               [](const std::weak_ptr<SqlHandle>& wp) { return wp.expired(); }),
+                _childStatementHandles.end());
+            _allocationsSinceCompaction = 0;
+            LOG("Periodic compaction: %zu -> %zu handles (removed %zu expired)",
+                originalSize, _childStatementHandles.size(),
+                originalSize - _childStatementHandles.size());
+        }
+    }  // Release lock
+
+    return stmtHandle;
 }
 
 SQLRETURN Connection::setAttribute(SQLINTEGER attribute, py::object value) {
@@ -308,7 +378,7 @@ bool Connection::reset() {
         disconnect();
         return false;
     }
-    
+
     // SQL_ATTR_RESET_CONNECTION does NOT reset the transaction isolation level.
     // Explicitly reset it to the default (SQL_TXN_READ_COMMITTED) to prevent
     // isolation level settings from leaking between pooled connection usages.
@@ -320,7 +390,7 @@ bool Connection::reset() {
         disconnect();
         return false;
     }
-    
+
     updateLastUsed();
     return true;
 }
diff --git a/mssql_python/pybind/connection/connection.h b/mssql_python/pybind/connection/connection.h
@@ -5,10 +5,19 @@
 #include "../ddbc_bindings.h"
 #include <memory>
 #include <string>
+#include <mutex>
 
 // Represents a single ODBC database connection.
 // Manages connection handles.
 // Note: This class does NOT implement pooling logic directly.
+//
+// THREADING MODEL (per DB-API 2.0 threadsafety=1):
+// - Connections should NOT be shared between threads in normal usage
+// - However, _childStatementHandles is mutex-protected because:
+//   1. Python GC/finalizers can run from any thread
+//   2. Native code may release GIL during blocking ODBC calls
+//   3. Provides safety if user accidentally shares connection
+// - All accesses to _childStatementHandles are guarded by _childHandlesMutex
 
 class Connection {
   public:
@@ -61,6 +70,22 @@ class Connection {
     std::chrono::steady_clock::time_point _lastUsed;
     std::wstring wstrStringBuffer;  // wstr buffer for string attribute setting
     std::string strBytesBuffer;     // string buffer for byte attributes setting
+
+    // Track child statement handles to mark them as implicitly freed when connection closes
+    // Uses weak_ptr to avoid circular references and allow normal cleanup
+    // THREAD-SAFETY: All accesses must be guarded by _childHandlesMutex
+    std::vector<std::weak_ptr<SqlHandle>> _childStatementHandles;
+    
+    // Counter for periodic compaction of expired weak_ptrs
+    // Compact every N allocations to avoid O(n²) overhead in hot path
+    // THREAD-SAFETY: Protected by _childHandlesMutex
+    size_t _allocationsSinceCompaction = 0;
+    static constexpr size_t COMPACTION_INTERVAL = 100;
+    
+    // Mutex protecting _childStatementHandles and _allocationsSinceCompaction
+    // Prevents data races between allocStatementHandle() and disconnect(),
+    // or concurrent GC finalizers running from different threads
+    mutable std::mutex _childHandlesMutex;
 };
 
 class ConnectionHandle {
diff --git a/mssql_python/pybind/ddbc_bindings.cpp b/mssql_python/pybind/ddbc_bindings.cpp
@@ -1144,6 +1144,21 @@ SQLSMALLINT SqlHandle::type() const {
     return _type;
 }
 
+void SqlHandle::markImplicitlyFreed() {
+    // SAFETY: Only STMT handles should be marked as implicitly freed.
+    // When a DBC handle is freed, the ODBC driver automatically frees all child STMT handles.
+    // Other handle types (ENV, DBC, DESC) are NOT automatically freed by parents.
+    // Calling this on wrong handle types will cause silent handle leaks.
+    if (_type != SQL_HANDLE_STMT) {
+        // Log error but don't throw - we're likely in cleanup/destructor path
+        LOG_ERROR("SAFETY VIOLATION: Attempted to mark non-STMT handle as implicitly freed. "
+                  "Handle type=%d. This will cause handle leak. Only STMT handles are "
+                  "automatically freed by parent DBC handles.", _type);
+        return;  // Refuse to mark - let normal free() handle it
+    }
+    _implicitly_freed = true;
+}
+
 /*
  * IMPORTANT: Never log in destructors - it causes segfaults.
  * During program exit, C++ destructors may run AFTER Python shuts down.
@@ -1169,16 +1184,19 @@ void SqlHandle::free() {
             return;
         }
 
-        // Always clean up ODBC resources, regardless of Python state
+        // CRITICAL FIX: Check if handle was already implicitly freed by parent handle
+        // When Connection::disconnect() frees the DBC handle, the ODBC driver automatically
+        // frees all child STMT handles. We track this state to avoid double-free attempts.
+        // This approach avoids calling ODBC functions on potentially-freed handles, which
+        // would cause use-after-free errors.
+        if (_implicitly_freed) {
+            _handle = nullptr;  // Just clear the pointer, don't call ODBC functions
+            return;
+        }
+
+        // Handle is valid and not implicitly freed, proceed with normal freeing
         SQLFreeHandle_ptr(_type, _handle);
         _handle = nullptr;
-
-        // Only log if Python is not shutting down (to avoid segfault)
-        if (!pythonShuttingDown) {
-            // Don't log during destruction - even in normal cases it can be
-            // problematic If logging is needed, use explicit close() methods
-            // instead
-        }
     }
 }
 
@@ -2893,7 +2911,6 @@ SQLRETURN SQLGetData_wrap(SqlHandlePtr StatementHandle, SQLUSMALLINT colCount, p
 
     // Cache decimal separator to avoid repeated system calls
 
-
     for (SQLSMALLINT i = 1; i <= colCount; ++i) {
         SQLWCHAR columnName[256];
         SQLSMALLINT columnNameLen;
@@ -3615,8 +3632,6 @@ SQLRETURN FetchBatchData(SQLHSTMT hStmt, ColumnBuffers& buffers, py::list& colum
             columnInfos[col].processedColumnSize + 1;  // +1 for null terminator
     }
 
-
-
     // Performance: Build function pointer dispatch table (once per batch)
     // This eliminates the switch statement from the hot loop - 10,000 rows × 10
     // cols reduces from 100,000 switch evaluations to just 10 switch
@@ -4033,8 +4048,8 @@ SQLRETURN FetchMany_wrap(SqlHandlePtr StatementHandle, py::list& rows, int fetch
             lobColumns.push_back(i + 1);  // 1-based
         }
     }
-    
-    // Initialized to 0 for LOB path counter; overwritten by ODBC in non-LOB path; 
+
+    // Initialized to 0 for LOB path counter; overwritten by ODBC in non-LOB path;
     SQLULEN numRowsFetched = 0;
     // If we have LOBs → fall back to row-by-row fetch + SQLGetData_wrap
     if (!lobColumns.empty()) {
@@ -4066,7 +4081,7 @@ SQLRETURN FetchMany_wrap(SqlHandlePtr StatementHandle, py::list& rows, int fetch
         LOG("FetchMany_wrap: Error when binding columns - SQLRETURN=%d", ret);
         return ret;
     }
-    
+
     SQLSetStmtAttr_ptr(hStmt, SQL_ATTR_ROW_ARRAY_SIZE, (SQLPOINTER)(intptr_t)fetchSize, 0);
     SQLSetStmtAttr_ptr(hStmt, SQL_ATTR_ROWS_FETCHED_PTR, &numRowsFetched, 0);
 
diff --git a/mssql_python/pybind/ddbc_bindings.h b/mssql_python/pybind/ddbc_bindings.h
@@ -379,9 +379,24 @@ class SqlHandle {
     SQLSMALLINT type() const;
     void free();
 
+    // Mark this handle as implicitly freed (freed by parent handle)
+    // This prevents double-free attempts when the ODBC driver automatically
+    // frees child handles (e.g., STMT handles when DBC handle is freed)
+    //
+    // SAFETY CONSTRAINTS:
+    // - ONLY call this on SQL_HANDLE_STMT handles
+    // - ONLY call this when the parent DBC handle is about to be freed
+    // - Calling on other handle types (ENV, DBC, DESC) will cause HANDLE LEAKS
+    // - The ODBC spec only guarantees automatic freeing of STMT handles by DBC parents
+    //
+    // Current usage: Connection::disconnect() marks all tracked STMT handles
+    // before freeing the DBC handle.
+    void markImplicitlyFreed();
+
   private:
     SQLSMALLINT _type;
     SQLHANDLE _handle;
+    bool _implicitly_freed = false;  // Tracks if handle was freed by parent
 };
 using SqlHandlePtr = std::shared_ptr<SqlHandle>;
 
diff --git a/tests/test_016_connection_invalidation_segfault.py b/tests/test_016_connection_invalidation_segfault.py
