- Notifications
You must be signed in to change notification settings - Fork27
FIX: Access Token Handling, Shutdown Safety, and Connection Attribute Tests#266
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
… bewithgaurav/fix_segfault_cleanup_sql_failures
…s://github.com/microsoft/mssql-python into bewithgaurav/fix_segfault_cleanup_sql_failures
… bewithgaurav/fix_segfault_cleanup_sql_failures
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Pull Request Overview
This PR adds comprehensive test coverage for custom connection attributes, focusing on theattrs_before parameter functionality and implementing defensive fixes for ODBC driver segfaults.
- Adds extensive test coverage for the
attrs_beforeparameter with various data types and edge cases - Implements defensive protection against ODBC driver crashes with short access tokens (< 32 bytes)
- Adds tests for Python shutdown scenarios that previously caused segfaults with SQL syntax errors
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_008_auth.py | Adds test for access token length validation to prevent ODBC driver crashes |
| tests/test_005_connection_cursor_lifecycle.py | Adds comprehensive tests for cursor cleanup scenarios during Python shutdown |
| tests/test_003_connection.py | Adds extensive test coverage for attrs_before parameter with various data types and configurations |
| mssql_python/pybind/ddbc_bindings.cpp | Implements Python shutdown detection and safe cleanup to prevent segfaults |
| mssql_python/pybind/connection/connection.cpp | Adds access token length validation and secure memory handling for sensitive data |
Tip: Customize your code reviews with copilot-instructions.md.Create the file orlearn how to get started.
| cursor.execute("syntax error") | ||
| # Don't explicitly close cursor/connection - let Python shutdown handle cleanup | ||
| print("Script completed, shutting down...") # This would NOT print anyways |
CopilotAISep 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The comment contains a grammatical error. Should be 'This would NOT print anyway' (without the 's' in 'anyways').
| print("Script completed, shutting down...")# This would NOT printanyways | |
| print("Script completed, shutting down...")# This would NOT printanyway |
| }catch (...) { | ||
| std::cerr <<"Error occurred while checking Python finalization state." << std::endl; |
CopilotAISep 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The error message is too generic and doesn't provide enough context about what specific error occurred. Consider logging the actual exception details or being more specific about the type of error.
| }catch (...) { | |
| std::cerr <<"Error occurred while checking Python finalization state." << std::endl; | |
| }catch (const std::exception& e) { | |
| std::cerr <<"Error occurred while checking Python finalization state:" << e.what() << std::endl; | |
| // Be conservative - don't assume shutdown on any exception | |
| // Only return true if we're absolutely certain Python is shutting down | |
| returnfalse; | |
| }catch (...) { | |
| std::cerr <<"Unknown error occurred while checking Python finalization state." << std::endl; |
Uh oh!
There was an error while loading.Please reload this page.
| if (attribute == SQL_COPT_SS_ACCESS_TOKEN && buffer.size() <32) { | ||
| LOG("Access token too short (< 32 bytes) - protecting against ODBC driver crash"); | ||
| return SQL_ERROR;// Return error instead of letting ODBC crash | ||
| } |
CopilotAISep 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The function returns SQL_ERROR but doesn't set any specific error message that can be retrieved by the caller. This makes debugging difficult. Consider setting a more descriptive error state or returning a custom error code.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
github-actionsbot commentedOct 6, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
📊 Code Coverage Report
Diff CoverageDiff: main...HEAD, staged and unstaged changes
Summary
📋 Files Needing Attention📉Files with overall lowest coverage (click to expand)mssql_python.ddbc_bindings.py: 68.5%mssql_python.pybind.ddbc_bindings.cpp: 69.4%mssql_python.pybind.connection.connection_pool.cpp: 78.9%mssql_python.cursor.py: 79.6%mssql_python.pybind.connection.connection.cpp: 80.1%mssql_python.connection.py: 81.7%mssql_python.helpers.py: 84.7%mssql_python.auth.py: 85.3%mssql_python.type.py: 86.8%mssql_python.pooling.py: 88.8% 🔗 Quick Links
|
Uh oh!
There was an error while loading.Please reload this page.
Work Item / Issue Reference
Summary
This pull request introduces important safety and robustness improvements to the handling of ODBC connection attributes, particularly focusing on access token authentication and defensive programming against known driver bugs. It also adds comprehensive test coverage for these changes, ensuring correct behavior and memory safety in various edge cases.
Defensive fixes and error handling
Connection::setAttribute(C++), which blocks access tokens shorter than 32 bytes to prevent a known crash in Microsoft ODBC Driver 18. An informative error is raised if a short token is detected.SqlHandle::~SqlHandleto avoid callingSQLFreeHandleduring Python interpreter shutdown, preventing potential crashes or undefined behavior.Test coverage for connection attributes
tests/test_003_connection.pyto verify the behavior of theattrs_beforeparameter, including handling of supported and unsupported attribute types, invalid keys, empty dictionaries, multiple attributes, memory safety with binary data, and compatibility with connection pooling and autocommit.tests/test_008_auth.pyto verify the defensive fix for short access tokens, ensuring that tokens under 32 bytes are blocked and legitimate tokens are allowed (with expected authentication errors if invalid). These tests run in subprocesses to isolate potential segfaults.Minor test cleanups