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.

[Copilot]

connection.h now uses std::vector<std::weak_ptr<SqlHandle>> but doesn’t include <vector>. Please add the direct include to avoid relying on transitive includes from ddbc_bindings.h (which can break compilation if headers change).

[Copilot]

The new Python-exposed method name markImplicitlyFreed is camelCase, but other ddbc_bindings.Connection methods are exported in snake_case (e.g., set_autocommit, alloc_statement_handle). Consider exporting this as mark_implicitly_freed (optionally keeping markImplicitlyFreed as a temporary alias) to keep the binding API consistent.

[Copilot]

These tests call conn.close(), but Connection.close() actively closes all tracked cursors and frees their statement handles before closing the DBC handle. That means this likely doesn’t exercise the implicit-free path the fix targets (DBC freed while STMT handles are still alive). Consider adding a regression test that keeps a SqlHandle STMT alive across a low-level ddbc_bindings.Connection.close() (e.g., via alloc_statement_handle) and then triggers GC/free on the still-referenced STMT handle.

[Copilot]

Variable cursors is not used.

Suggested change

	cursors = None
	del cursors

[Copilot]

Variable connections is not used.

Suggested change

	connections = None
	all_cursors = None
	del connections
	del all_cursors

[Copilot]

Variable all_cursors is not used.