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