FIX: Store deferred connect-attribute values in member buffers (#594)

[saurabh500]

Fixes #594.

Problem

PR #568 changed Connection::setAttribute to copy bytes/string attribute values into stack-local buffers before calling SQLSetConnectAttr, on the theory that the released GIL could otherwise expose a race with another thread mutating the member buffer.

That premise is unsound for deferred attributes and introduced a use-after-free.

SQL_COPT_SS_ACCESS_TOKEN (1256) is a deferred ODBC connect attribute: the MS driver stores the caller's pointer at SQLSetConnectAttr time and dereferences it later, during SQLDriverConnect, to build the FedAuth login packet. With the stack-local copy, by the time the driver reads the pointer the buffer has already been freed.

Observed symptoms (issue #594)

Platform	Symptom
macOS arm64	Fatal Python error: Bus error (SIGBUS)
Windows x64	ProgrammingError: Authentication token is missing in the federated authentication message
Azure SQL DB	OperationalError: Communication link failure; TCP Provider Error code 0x2746 (server reset)

All three are the same root cause — the driver dereferences a pointer to a freed stack frame; the symptom depends on what the OS leaves on that page.

Fix

Store the value in the Connection-owned member buffers (strBytesBuffer for bytes/bytearray, wstrStringBuffer for str) so the memory remains valid for the lifetime of the Connection, which covers the deferred dereference during SQLDriverConnect.

PR #568's gil_scoped_release around SQLSetConnectAttr is preserved.

Why the 'race with another thread' rationale from PR #568 doesn't apply

1. attrs_before is applied once, sequentially, during connect(). No other thread is mutating attributes at that moment.
2. Concurrent setAttribute() on the same Connection from different threads is not a supported pattern — ODBC connection handles aren't designed for concurrent attribute mutation.
3. 1.6.0 used the persistent member buffer with no reported issue.
4. Even granting the race premise, a stack local that is freed before the driver finishes using the pointer is strictly worse than a member that at least lives as long as the connection.

Comments in the patched code document both the deferred-read requirement and the non-thread-safe contract.

Verification

Built the patched .so from this branch on macOS arm64, overlaid into the installed 1.7.1 wheel, and ran the issue's repro script against an Azure SQL DB with an AzureCliCredential access token:

Build	Result
Stock 1.7.1	Fatal Python error: Bus error (SIGBUS) / OperationalError: ... TCP Provider Error code 0x2746 (run-to-run variance)
This fix	Clean OperationalError: Login failed for user '<token-identified principal>' surfaced from the server (token transmitted intact; server-side rejection because that test target has no AAD admin configured)

Files touched

• mssql_python/pybind/connection/connection.cpp — both the py::bytes/py::bytearray branch and the py::str branch of Connection::setAttribute reverted to use the persistent member buffers.

[saurabh500]

Pushed 2f8db0b3: replaced the two shared Connection member buffers (wstrStringBuffer, strBytesBuffer) with std::unordered_map<SQLINTEGER, ...> keyed by attribute id, so a second deferred string/bytes attribute can't invalidate the pointer the driver stashed for an earlier one. Only SQL_COPT_SS_ACCESS_TOKEN (1256) is deferred in practice today, so the previous single-buffer form was sufficient to fix #594 as observed; this hardens the fix against any future deferred string/bytes attribute. Re-verified end-to-end against an Azure SQL DB with an AzureCliCredential token — same clean server-side OperationalError, no SIGBUS / TCP reset.

[github-actions]

📊 Code Coverage Report

🔥 Diff Coverage 55%

🎯 Overall Coverage 25%

📈 Total Lines Covered: 7038 out of 27155 📁 Project: mssql-python

---

Diff Coverage

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

• mssql_python/pybind/connection/connection.cpp (55.6%): Missing lines 405-408

Summary

• Total: 9 lines
• Missing: 4 lines
• Coverage: 55%

mssql_python/pybind/connection/connection.cpp

Lines 401-412

  401             //
  402             // Note: attrs_before is applied once, sequentially, during
  403             // connect(); concurrent setAttribute() on the same Connection
  404             // from different threads is not a supported pattern.
! 405             auto& buf = this->_attrBytesBuffers[attribute];
! 406             buf = value.cast<std::string>();
! 407             SQLPOINTER ptr = const_cast<char*>(buf.data());
! 408             SQLINTEGER length = static_cast<SQLINTEGER>(buf.size());
  409 
  410             SQLRETURN ret;
  411             {
  412                 py::gil_scoped_release release;

---

---

📋 Files Needing Attention

📉 Files with overall lowest coverage (click to expand)

mssql_python.pybind.build._deps.simdutf-src.src.haswell.implementation.cpp: 0.4%
mssql_python.pybind.build._deps.simdutf-src.src.implementation.cpp: 6.7%
mssql_python.pybind.build._deps.simdutf-src.include.simdutf.implementation.h: 10.4%
mssql_python.pybind.build._deps.simdutf-src.include.simdutf.scalar.utf16_to_utf8.utf16_to_utf8.h: 25.3%
mssql_python.pybind.logger_bridge.cpp: 59.2%
mssql_python.pybind.ddbc_bindings.h: 59.7%
mssql_python.pybind.build._deps.simdutf-src.include.simdutf.internal.isadetection.h: 65.3%
mssql_python.row.py: 70.5%
mssql_python.pybind.logger_bridge.hpp: 70.8%
mssql_python.pybind.ddbc_bindings.cpp: 74.2%

---

🔗 Quick Links

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

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[saurabh500]

Pushed 031c7175 to expand the comments around the new per-attribute buffers, explaining the lifetime contract more fully.

Why _attrStringBuffers / _attrBytesBuffers are NOT cleared on Connection::reset() (was a reviewer question):

SQL_ATTR_RESET_CONNECTION is the pool-checkin reset. It asks the MS ODBC driver to wipe per-session state (temp tables, open cursors, SET options, isolation level, etc.) on the next use. It does not:

1. tear down the underlying TCP/TLS socket,
2. discard the driver-side authentication context, or
3. release the deferred connect attributes the driver has stashed (notably the SQL_COPT_SS_ACCESS_TOKEN pointer used to build the FedAuth Login7 packet).

The driver may still dereference those stashed pointers after the reset — most importantly during Idle Connection Resiliency: if the underlying TCP connection drops while the connection sits idle in the pool, the driver transparently re-establishes it on the next use and re-runs the Login7 / FedAuth handshake, reading the same access-token pointer it stashed at original-connect time.

If we cleared the per-attribute buffers on reset() we'd reintroduce issue #594 in a new and harder-to-debug form: a UAF during silent reconnect rather than initial connect. The buffers are therefore intentionally retained for the lifetime of the Connection object and freed implicitly via the destructors of the unordered_maps in ~Connection().

Memory cost is bounded: one entry per unique attribute id ever set on the connection (in practice = 1, for token 1256). Token rotation overwrites the same map slot in place, so it's O(1) memory per connection regardless of how often the token is refreshed.