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

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;
 }

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 {

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);
 

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>;