From 8ede55712cb27b074badf9bd613d3f5b8f25c38b Mon Sep 17 00:00:00 2001 From: Gaurav Sharma Date: Fri, 10 Oct 2025 11:06:04 +0000 Subject: [PATCH] FIX: Core Dump during shutdown --- mssql_python/connection.py | 14 +++++- mssql_python/pybind/connection/connection.cpp | 43 ++++++++++++++++--- tests/test_005_connection_cursor_lifecycle.py | 32 ++++++++++++++ 3 files changed, 81 insertions(+), 8 deletions(-) diff --git a/mssql_python/connection.py b/mssql_python/connection.py index 832d2aac..82052356 100644 --- a/mssql_python/connection.py +++ b/mssql_python/connection.py @@ -1244,7 +1244,17 @@ def __del__(self): """ if "_closed" not in self.__dict__ or not self._closed: try: + import sys + # If the interpreter is finalizing, skip cleanup that might call into + # C extension destructors which use the Python C-API. + if sys and getattr(sys, "_is_finalizing", lambda: False)(): + return self.close() except Exception as e: - # Dont raise exceptions from __del__ to avoid issues during garbage collection - log('error', f"Error during connection cleanup: {e}") \ No newline at end of file + # During interpreter shutdown avoid logging or raising further errors + try: + if not (hasattr(sys, "_is_finalizing") and sys._is_finalizing()): + log('error', f"Error during connection cleanup: {e}") + except Exception: + # If logging isn't available (e.g., during shutdown), silently ignore + pass \ No newline at end of file diff --git a/mssql_python/pybind/connection/connection.cpp b/mssql_python/pybind/connection/connection.cpp index 3311c697..382494ea 100644 --- a/mssql_python/pybind/connection/connection.cpp +++ b/mssql_python/pybind/connection/connection.cpp @@ -45,7 +45,21 @@ Connection::Connection(const std::wstring& conn_str, bool use_pool) } Connection::~Connection() { - disconnect(); // fallback if user forgets to disconnect + // During interpreter shutdown, destructors must not throw or call into Python runtime. + // Call disconnect() but swallow exceptions to avoid terminate() during stack unwinding. + try { + disconnect(); // fallback if user forgets to disconnect + } catch (const std::exception &e) { + // Avoid using LOG (acquires GIL). Write to stderr directly only if Python is alive. + if (Py_IsInitialized()) { + std::cerr << "Exception during Connection::~Connection cleanup: " << e.what() << std::endl; + } + // Swallow - can't safely call Python APIs here. + } catch (...) { + if (Py_IsInitialized()) { + std::cerr << "Unknown exception during Connection::~Connection cleanup" << std::endl; + } + } } // Allocates connection handle @@ -89,12 +103,22 @@ void Connection::connect(const py::dict& attrs_before) { void Connection::disconnect() { if (_dbcHandle) { + // Use LOG for normal operations where Python is available LOG("Disconnecting from database"); - SQLRETURN ret = SQLDisconnect_ptr(_dbcHandle->get()); - checkError(ret); + try { + SQLRETURN ret = SQLDisconnect_ptr(_dbcHandle->get()); + // checkError may throw; catch and write to stderr instead of calling LOG + try { + checkError(ret); + } catch (const std::exception &e) { + std::cerr << "SQLDisconnect reported error: " << e.what() << std::endl; + // swallow - destructor callers must not throw + } + } catch (const std::exception &e) { + std::cerr << "Exception during SQLDisconnect call: " << e.what() << std::endl; + } _dbcHandle.reset(); // triggers SQLFreeHandle via destructor, if last owner - } - else { + } else { LOG("No connection handle to disconnect"); } } @@ -270,8 +294,15 @@ ConnectionHandle::ConnectionHandle(const std::string& connStr, bool usePool, con } ConnectionHandle::~ConnectionHandle() { + // Make destructor noexcept-safe: close but swallow exceptions. if (_conn) { - close(); + try { + close(); + } catch (const std::exception &e) { + LOG("Exception during ConnectionHandle destructor close: {}", e.what()); + } catch (...) { + LOG("Unknown exception during ConnectionHandle destructor close"); + } } } diff --git a/tests/test_005_connection_cursor_lifecycle.py b/tests/test_005_connection_cursor_lifecycle.py index df777c08..c4cf0787 100644 --- a/tests/test_005_connection_cursor_lifecycle.py +++ b/tests/test_005_connection_cursor_lifecycle.py @@ -506,6 +506,38 @@ def test_sql_syntax_error_no_segfault_on_shutdown(conn_str): # Should not segfault (exit code 139 on Unix, 134 on macOS) assert result.returncode == 1, f"Expected exit code 1 due to syntax error, but got {result.returncode}. STDERR: {result.stderr}" + +def test_connection_error_then_shutdown_no_core_dump(conn_str): + """Test that connection-level errors followed by interpreter shutdown do not cause core dumps or pybind11 aborts""" + escaped_conn_str = conn_str.replace('\\', '\\\\').replace('"', '\\"') + code = f""" +from mssql_python import connect + +# Create a connection, trigger an error on the native side, then let interpreter shutdown handle cleanup +conn = connect("{escaped_conn_str}") +cursor = conn.cursor() + +# Execute an invalid query to create an error state on the STMT/DBC +try: + cursor.execute('syntax error at shutdown_test') +except Exception: + # Intentionally ignore the Python-level exception; we want to ensure teardown is safe + pass + +# Don't close cursor or connection explicitly - allow objects to be cleaned up during interpreter finalization +print('done') +""" + + result = subprocess.run([sys.executable, "-c", code], capture_output=True, text=True) + + # Expect a non-zero return value due to the syntax error, but not a crash/abort + assert result.returncode != 0, f"Expected non-zero exit due to syntax error, got 0. STDERR: {result.stderr}" + + # Ensure there is no segmentation fault or pybind11 abort text in stderr + assert "Segmentation fault" not in result.stderr + assert "pybind11::error_already_set" not in result.stderr + assert "sys.meta_path is None" not in result.stderr + def test_multiple_sql_syntax_errors_no_segfault(conn_str): """Test multiple SQL syntax errors don't cause segfault during cleanup""" escaped_conn_str = conn_str.replace('\\', '\\\\').replace('"', '\\"')