Skip to content

Commit 3bc8e50

Browse files
Fix issue microsoft#341: Segfault in SQLFreeHandle during GC when cursor outlives connection
- Check connection state before calling SQLFreeHandle in Cursor.close() - Default to SAFE behavior: skip handle cleanup if connection is None/gone/closed - Only free handle if we can CONFIRM connection is alive and open - Add early sys._is_finalizing() check in Cursor.__del__() - Add 5 new tests covering the exact reproduction scenario from the issue This fixes the case where: 1. Connection is closed (ODBC handles freed) 2. Cursor objects still exist in Python memory 3. GC runs during normal execution (not interpreter shutdown) 4. Cursor.__del__() tries to free invalid ODBC statement handle Key insight: The previous fix defaulted to 'not closed' when connection was None, which caused crashes when the connection was garbage collected. Now we default to 'closed' (safe) and only free if we can confirm connection is alive and open. PR microsoft#361 only checked sys.is_finalizing() which doesn't cover GC during normal execution. This fix uses the Connection.closed property (from PR microsoft#398) to detect closed connections.
1 parent 95e0836 commit 3bc8e50

File tree

2 files changed

+266
-6
lines changed

2 files changed

+266
-6
lines changed

mssql_python/cursor.py

Lines changed: 38 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -752,9 +752,31 @@ def close(self) -> None:
752752
logger.warning("Error removing cursor from connection tracking: %s", e)
753753

754754
if self.hstmt:
755-
self.hstmt.free()
756-
self.hstmt = None
757-
logger.debug("SQLFreeHandle succeeded")
755+
# Check if connection is already closed - if so, the ODBC handle is already
756+
# invalid and we should NOT call SQLFreeHandle. This prevents segfaults
757+
# when GC runs after connection.close(). See GitHub issue #341.
758+
# We check connection.closed property (added in PR #398) to determine if
759+
# the connection's ODBC handles have been freed.
760+
#
761+
# IMPORTANT: Default to "closed" (safe to skip free). Only proceed with
762+
# freeing if we can CONFIRM the connection is alive and open.
763+
# If connection is None or garbage collected, we must skip the free.
764+
connection_is_open = False
765+
try:
766+
if hasattr(self, "_connection") and self._connection is not None:
767+
# Only set to open if connection exists AND is not closed
768+
connection_is_open = not getattr(self._connection, "closed", True)
769+
except (ReferenceError, AttributeError):
770+
# Connection object may have been garbage collected or partially destroyed
771+
connection_is_open = False
772+
773+
if connection_is_open:
774+
self.hstmt.free()
775+
self.hstmt = None
776+
logger.debug("SQLFreeHandle succeeded")
777+
else:
778+
logger.debug("Skipping SQLFreeHandle - parent connection already closed or gone")
779+
self.hstmt = None
758780
self._clear_rownumber()
759781
self.closed = True
760782

@@ -2573,16 +2595,26 @@ def __del__(self):
25732595
This is a safety net to ensure resources are cleaned up
25742596
even if close() was not called explicitly.
25752597
If the cursor is already closed, it will not raise an exception during cleanup.
2598+
2599+
IMPORTANT: This handles the case where GC runs after the parent connection
2600+
is closed. In this scenario, the ODBC statement handle is already invalid
2601+
because the connection handle was freed first. Attempting to call
2602+
SQLFreeHandle on an invalid handle causes a segfault. See GitHub issue #341.
25762603
"""
2604+
import sys
2605+
2606+
# Check for interpreter shutdown first
2607+
if sys is None or (hasattr(sys, "_is_finalizing") and sys._is_finalizing()):
2608+
# During interpreter shutdown, skip cleanup to avoid crashes
2609+
return
2610+
25772611
if "closed" not in self.__dict__ or not self.closed:
25782612
try:
25792613
self.close()
25802614
except Exception as e: # pylint: disable=broad-exception-caught
25812615
# Don't raise an exception in __del__, just log it
25822616
# If interpreter is shutting down, we might not have logging set up
2583-
import sys
2584-
2585-
if sys and sys._is_finalizing():
2617+
if hasattr(sys, "_is_finalizing") and sys._is_finalizing():
25862618
# Suppress logging during interpreter shutdown
25872619
return
25882620
logger.debug("Exception during cursor cleanup in __del__: %s", e)

tests/test_005_connection_cursor_lifecycle.py

Lines changed: 228 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -692,3 +692,231 @@ def aggressive_worker(thread_id):
692692
result.returncode == 0
693693
), f"Expected clean exit, but got exit code {result.returncode}. STDERR: {result.stderr}"
694694
assert "Exiting abruptly with active threads and pending queries" in result.stdout
695+
696+
697+
def test_cursor_gc_after_connection_close_no_segfault(conn_str):
698+
"""
699+
Test for GitHub issue #341: Segfault in SQLFreeHandle during GC.
700+
701+
This test verifies that when:
702+
1. A connection is closed (ODBC handles freed)
703+
2. Cursor objects still exist in Python (not yet garbage collected)
704+
3. GC runs later and cursor.__del__() is called
705+
706+
The cursor cleanup should NOT call SQLFreeHandle on the now-invalid handle,
707+
preventing a segfault. The fix checks connection.closed before freeing handles.
708+
"""
709+
import gc
710+
711+
# Create connection and multiple cursors
712+
conn = connect(conn_str)
713+
cursors = []
714+
for i in range(5):
715+
cursor = conn.cursor()
716+
cursor.execute(f"SELECT {i}")
717+
cursor.fetchall()
718+
cursors.append(cursor)
719+
720+
# Keep a reference to one cursor
721+
orphan_cursor = cursors[2]
722+
723+
# Connection should be open
724+
assert conn.closed is False, "Connection should be open"
725+
726+
# Close connection - cursor.close() will now see connection.closed == True
727+
conn.close()
728+
729+
# Verify connection is closed
730+
assert conn.closed is True, "Connection should be closed after close()"
731+
732+
# Clear cursor list but keep orphan_cursor alive
733+
cursors.clear()
734+
gc.collect() # GC runs here but orphan_cursor is still alive
735+
736+
# Now delete orphan_cursor - this triggers __del__ -> close()
737+
# close() should check connection.closed and skip SQLFreeHandle
738+
del orphan_cursor
739+
gc.collect() # Should not segfault!
740+
741+
# If we get here without a segfault, the test passed
742+
743+
744+
def test_cursor_checks_connection_closed_before_free(conn_str):
745+
"""
746+
Test that cursor.close() checks connection.closed property before freeing handles.
747+
748+
This is a unit test for the fix to GitHub issue #341.
749+
The fix uses the Connection.closed property (added in PR #398) to determine
750+
if the connection's ODBC handles have already been freed.
751+
"""
752+
conn = connect(conn_str)
753+
754+
# Create multiple cursors
755+
cursor1 = conn.cursor()
756+
cursor2 = conn.cursor()
757+
cursor3 = conn.cursor()
758+
759+
# Execute queries to allocate statement handles
760+
cursor1.execute("SELECT 1")
761+
cursor1.fetchall()
762+
cursor2.execute("SELECT 2")
763+
cursor2.fetchall()
764+
cursor3.execute("SELECT 3")
765+
cursor3.fetchall()
766+
767+
# Verify connection is open
768+
assert conn.closed == False
769+
770+
# Close one cursor explicitly - should succeed
771+
cursor1.close()
772+
assert cursor1.closed == True
773+
774+
# Close connection
775+
conn.close()
776+
assert conn.closed == True
777+
778+
# Remaining cursors should be closed by connection.close()
779+
assert cursor2.closed == True
780+
assert cursor3.closed == True
781+
782+
783+
def test_cursor_close_skips_free_when_connection_closed(conn_str):
784+
"""
785+
Test that Cursor.close() skips SQLFreeHandle when connection is already closed.
786+
787+
This tests the safety mechanism for GitHub issue #341 by simulating
788+
the scenario where a cursor's close() is called after the connection is closed.
789+
"""
790+
# Create connection and cursor
791+
conn = connect(conn_str)
792+
cursor = conn.cursor()
793+
cursor.execute("SELECT 1")
794+
cursor.fetchall()
795+
796+
# Close connection first - this frees the ODBC connection handle
797+
conn.close()
798+
799+
# Verify connection is closed
800+
assert conn.closed is True
801+
802+
# The cursor was already closed by connection.close(), but calling close() again
803+
# should be safe (idempotent) and should not try to free an invalid handle
804+
cursor.close()
805+
806+
# If we get here without error, the test passed
807+
808+
809+
def test_issue_341_gc_during_normal_execution_no_segfault(conn_str):
810+
"""
811+
Test for GitHub issue #341: Segfault during GC in normal execution.
812+
813+
This test reproduces the exact scenario reported in the issue:
814+
https://github.com/microsoft/mssql-python/issues/341
815+
816+
The issue occurs when:
817+
1. Connection is closed (ODBC connection handle freed)
818+
2. Cursor objects still exist in Python memory (not yet garbage collected)
819+
3. GC runs during NORMAL execution (not interpreter shutdown)
820+
- This is the key difference from PR #361's fix which only checked sys.is_finalizing()
821+
4. Cursor.__del__() is invoked and tries to free its ODBC statement handle
822+
5. SQLFreeHandle() is called on an invalid handle → SEGFAULT
823+
824+
The scenario from the issue was:
825+
- SQLAlchemy test_reconnect.py tests close connections and throw away cursors
826+
- GC runs during engine creation in a subsequent test (normal execution)
827+
- Cursor finalizers execute while handles are invalid
828+
- sys.is_finalizing() returns False, so PR #361's fix didn't help
829+
830+
The fix checks connection.closed before calling SQLFreeHandle.
831+
"""
832+
import gc
833+
834+
# Simulate the SQLAlchemy reconnect test scenario
835+
836+
# Phase 1: Create connection and cursors, simulate "disconnect" scenario
837+
conn1 = connect(conn_str)
838+
cursors = []
839+
for i in range(3):
840+
cursor = conn1.cursor()
841+
cursor.execute(f"SELECT {i}")
842+
cursor.fetchall()
843+
cursors.append(cursor)
844+
845+
# Keep a reference to cursors (simulating SQLAlchemy keeping cursor refs)
846+
orphan_cursors = cursors.copy()
847+
848+
# Close connection - simulates disconnect/reconnect scenario
849+
# This frees the ODBC connection handle, making statement handles invalid
850+
conn1.close()
851+
852+
# At this point:
853+
# - conn1.closed == True
854+
# - ODBC connection handle is freed
855+
# - orphan_cursors still have references to invalid statement handles
856+
# - sys.is_finalizing() returns False (we're in normal execution)
857+
858+
# Phase 2: Create a NEW connection - simulates "reconnect" scenario
859+
# This is when GC typically runs (during object allocation)
860+
conn2 = connect(conn_str)
861+
862+
# Trigger GC explicitly to simulate what happens during engine creation
863+
# In the real scenario, GC runs implicitly during object allocation
864+
gc.collect()
865+
866+
# The orphan_cursors.__del__() should have been called by now
867+
# If the fix works, it should have checked connection.closed and skipped SQLFreeHandle
868+
869+
# Phase 3: Verify the new connection works
870+
cursor2 = conn2.cursor()
871+
cursor2.execute("SELECT 'reconnect_test'")
872+
result = cursor2.fetchone()
873+
assert result[0] == "reconnect_test", f"Expected 'reconnect_test', got {result[0]}"
874+
875+
# Clean up properly
876+
cursor2.close()
877+
conn2.close()
878+
879+
# Clear the orphan cursors list and run GC again
880+
orphan_cursors.clear()
881+
cursors.clear()
882+
gc.collect()
883+
884+
# If we get here without a segfault, the test passed
885+
886+
887+
def test_issue_341_multiple_reconnect_cycles_no_segfault(conn_str):
888+
"""
889+
Test for GitHub issue #341: Multiple reconnect cycles without segfault.
890+
891+
This simulates what SQLAlchemy's test_reconnect.py does - multiple
892+
connect/disconnect cycles with cursors being garbage collected at
893+
unpredictable times.
894+
"""
895+
import gc
896+
897+
all_orphan_cursors = []
898+
899+
# Simulate multiple reconnect cycles
900+
for cycle in range(5):
901+
conn = connect(conn_str)
902+
cursors = []
903+
for i in range(3):
904+
cursor = conn.cursor()
905+
cursor.execute(f"SELECT {cycle * 10 + i}")
906+
cursor.fetchall()
907+
cursors.append(cursor)
908+
909+
# Keep references to cursors (simulating refs kept elsewhere)
910+
all_orphan_cursors.extend(cursors)
911+
912+
# Close connection without closing cursors
913+
conn.close()
914+
915+
# Trigger GC - this is when the segfault would occur
916+
gc.collect()
917+
918+
# Final cleanup
919+
all_orphan_cursors.clear()
920+
gc.collect()
921+
922+
# If we get here without a segfault, the test passed

0 commit comments

Comments
 (0)