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

[subrata-ms]

AB#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] [2] [3]

• 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] [2] [3]

• 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] [2]

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.

[Copilot]

Pull request overview

Fixes a use-after-free / double-free crash seen with msodbcsql 18.5 when a connection (DBC) is freed while statement (STMT) handles are still alive, by explicitly tracking child statement handles and preventing later cleanup from calling into ODBC on already-freed handles.

Changes:

• Track statement handles created by a Connection via a weak_ptr list and mark them “implicitly freed” during disconnect().
• Add an _implicitly_freed flag + markImplicitlyFreed() to SqlHandle, and skip SQLFreeHandle when set.
• Add a new regression test suite for connection invalidation / GC cleanup scenarios and expose the new method via pybind11.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
tests/test_016_connection_invalidation_segfault.py	Adds regression tests targeting the historical segfault scenario during GC cleanup after connection invalidation.
mssql_python/pybind/ddbc_bindings.h	Extends SqlHandle API/state to support “implicitly freed” tracking.
mssql_python/pybind/ddbc_bindings.cpp	Implements the implicit-free guard in SqlHandle::free() and exports the new method to Python.
mssql_python/pybind/connection/connection.h	Adds per-connection tracking for child statement handles.
mssql_python/pybind/connection/connection.cpp	Marks tracked STMT handles as implicitly freed on disconnect() and records new STMT handles on allocation.

Comments suppressed due to low confidence (1)

mssql_python/pybind/connection/connection.cpp:114

• checkError(ret) can throw on SQLDisconnect_ptr, which means _dbcHandle.reset() will never run. At that point the child statement handles were already marked implicitly freed and the tracking vector cleared, leaving the connection handle alive while STMT handles are now flagged to skip freeing (resource leak / inconsistent state). Restructure disconnect() so handle-marking + _dbcHandle.reset() are exception-safe (e.g., always reset in a scope guard/finally; only mark children when you're definitely freeing the DBC handle).

        SQLRETURN ret = SQLDisconnect_ptr(_dbcHandle->get());
        checkError(ret);
        // triggers SQLFreeHandle via destructor, if last owner
        _dbcHandle.reset();

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[bewithgaurav]

one minor comment to help pass the CI checks, else lgtm
will approve once CI is green

[github-actions]

📊 Code Coverage Report

🔥 Diff Coverage 78%

🎯 Overall Coverage 76%

📈 Total Lines Covered: 5472 out of 7137 📁 Project: mssql-python

---

Diff Coverage

Diff: main...HEAD, staged and unstaged changes

• mssql_python/pybind/connection/connection.cpp (90.7%): Missing lines 127-130
• mssql_python/pybind/ddbc_bindings.cpp (38.5%): Missing lines 1154-1158,1193-1195

Summary

• Total: 56 lines
• Missing: 12 lines
• Coverage: 78%

mssql_python/pybind/connection/connection.cpp

Lines 123-134

  123                     // SAFETY ASSERTION: Only STMT handles should be in this vector
  124                     // This is guaranteed by allocStatementHandle() which only creates STMT handles
  125                     // If this assertion fails, it indicates a serious bug in handle tracking
  126                     if (handle->type() != SQL_HANDLE_STMT) {
! 127                         LOG_ERROR("CRITICAL: Non-STMT handle (type=%d) found in _childStatementHandles. "
! 128                                   "This will cause a handle leak!", handle->type());
! 129                         continue;  // Skip marking to prevent leak
! 130                     }
  131                     handle->markImplicitlyFreed();
  132                 }
  133             }
  134             _childStatementHandles.clear();

---

mssql_python/pybind/ddbc_bindings.cpp

Lines 1150-1162

  1150     // Other handle types (ENV, DBC, DESC) are NOT automatically freed by parents.
  1151     // Calling this on wrong handle types will cause silent handle leaks.
  1152     if (_type != SQL_HANDLE_STMT) {
  1153         // Log error but don't throw - we're likely in cleanup/destructor path
! 1154         LOG_ERROR("SAFETY VIOLATION: Attempted to mark non-STMT handle as implicitly freed. "
! 1155                   "Handle type=%d. This will cause handle leak. Only STMT handles are "
! 1156                   "automatically freed by parent DBC handles.", _type);
! 1157         return;  // Refuse to mark - let normal free() handle it
! 1158     }
  1159     _implicitly_freed = true;
  1160 }
  1161 
  1162 /*

---

Lines 1189-1199

  1189         // frees all child STMT handles. We track this state to avoid double-free attempts.
  1190         // This approach avoids calling ODBC functions on potentially-freed handles, which
  1191         // would cause use-after-free errors.
  1192         if (_implicitly_freed) {
! 1193             _handle = nullptr;  // Just clear the pointer, don't call ODBC functions
! 1194             return;
! 1195         }
  1196 
  1197         // Handle is valid and not implicitly freed, proceed with normal freeing
  1198         SQLFreeHandle_ptr(_type, _handle);
  1199         _handle = nullptr;

---

---

📋 Files Needing Attention

📉 Files with overall lowest coverage (click to expand)

mssql_python.pybind.logger_bridge.hpp: 58.8%
mssql_python.pybind.logger_bridge.cpp: 59.2%
mssql_python.row.py: 66.2%
mssql_python.pybind.ddbc_bindings.cpp: 69.3%
mssql_python.pybind.ddbc_bindings.h: 69.7%
mssql_python.pybind.connection.connection.cpp: 75.3%
mssql_python.ddbc_bindings.py: 79.6%
mssql_python.pybind.connection.connection_pool.cpp: 79.6%
mssql_python.connection.py: 84.1%
mssql_python.cursor.py: 84.7%

---

🔗 Quick Links

⚙️ Build Summary	📋 Coverage Details
View Azure DevOps Build	Browse Full Coverage Report