Preserve UTF-16 fetch units and BOM-shaped SQL payloads

[Saurabh Singh (saurabh500)]

Description

Preserve complete UTF-16 code units when the source and requested ODBC C type are both wide. Shared raw copying covers complete buffered SQLGetData, captured/chunked SQLGetData, direct bound rows and materialized bound/output-parameter fallback. Unpaired surrogates no longer take the lossy decode/reencode path. Materialized fast paths still require even byte lengths; capacities, byte indicators, unaligned writes and termination remain checked.

Bound truncation trims only a real surrogate pair crossing the capacity boundary; bound MAX delivery retains an isolated high surrogate in the final payload slot and checks the first excluded unit across chunks. SQLGetData remains resumable by code unit. Odd-length PLP handling is unchanged/unverified and excluded from this parity claim: no new malformed-wire error policy is introduced.

The user-authorized BOM change makes shared SqlString::decode honor its explicit encoding instead of sniffing/removing a BOM. Two existing encoding_rs calls become decode_without_bom_handling; no dependency or new state. This covers owned and borrowed SQL payloads. Existing malformed-sequence replacement and LCID warning behavior remain unchanged; this is not a global relaxation of Unicode validation. Streaming decoders already disable BOM handling and remain unchanged.

The BOM change also affects JS and Python column strings, not just ODBC: mssql-js's row writer calls SqlString::decode, and mssql-py-core's cursor calls to_utf8_string. Leading BOM-shaped payloads now remain data in the declared encoding instead of being stripped or selecting another encoding, including a leading FE FF no longer switching UTF-16LE to big-endian. The shared TDS README documents this cross-binding effect, with a link from the Python runtime README; preserving raw unpaired units in ODBC wide buffers is separate.

The original 10 cases cover nchar/nvarchar/MAX; malformed/valid pairs; FEFF/FFFE; embedded/trailing NUL; NULL/empty; bound arrays, captured/eight-column buffered rows; output parameters; byte indicators, termination, adjacent capacities, repeated reads/end and reuse. Two cases cover bound/unbound x bounded/MAX CP1252 and UTF-8 BOM-shaped payloads, with ASCII/high-byte controls. No per-driver assertions or reference skips. Earlier installed18.4.1.1 results are not substituted for this measurement.

RawUnitsBesideNvarcharMax and RawUnitsThroughCompleteBufferedRows reproduce the original corruption against SQL Server. BoundTruncationPreservesOnlyCompletePairs checks real-server truncation. The socket-backed Rust mock test additionally forces a high/low pair across a specific PLP wire-chunk boundary that a SQL query cannot control; it is not a substitute for the live reproduction.

Exact BOM probe:

SET NOCOUNT ON;
DECLARE @t TABLE(v varchar(32) COLLATE Latin1_General_100_CI_AS);
INSERT @t VALUES(0xEFBBBF41);
SELECT v,v FROM @t;

First column, SQL_C_BINARY: all implementations return EF BB BF 41, indicator4. Second column, SQL_C_WCHAR:

Implementation	Returned UTF-16LE bytes	Indicator	Execute / Fetch / GetData
Retail18.6.2.1	EF00 BB00 BF00 4100	8	SQL_SUCCESS / SQL_SUCCESS / SQL_SUCCESS
Untouched b0ecac2 and pre-BOM Rust	4100	2	SQL_SUCCESS / SQL_SUCCESS / SQL_SUCCESS
BOM-fixed Rust1a298601	EF00 BB00 BF00 4100	8	SQL_SUCCESS / SQL_SUCCESS / SQL_SUCCESS

The bounded case was red for bound and unbound delivery before the fix; reference, MAX, UTF-8, ASCII and high-byte controls passed. Native tests also check the terminator and exterior canaries.

Review iteration 5f4b03ba: corrected the stale SQLBindCol NULL-unbinding documentation and added a complete-wide-copy debug assertion with short-buffer coverage; the copy itself still executes in release builds. Local validation passed 240 affected debug tests, 2 release-mode copy/progress tests, strict workspace clippy and workspace/excluded-Python formatting. Omitting terminator space in a temporary mutation triggered the new assertion; the mutation was restored. Full SQL/native/Python suites were not rerun for this documentation/debug-assertion iteration, and new-head CI is pending.

Related Issues

Fixes #604

Checklist

• cargo bfmt passes
• cargo bclippy passes
• Full cargo btest passes locally (earlier run had 30 matched baseline failures; this iteration ran focused tests)
• Both cross-repo jobs pass on latest head with Python pin 2a86fc1f0306accecdbc6c563ca2408fad9e1fd6 (passed on prior head; awaiting new-head CI)
• New/changed functionality has tests
• Public API changes are documented (no driver public API changes; shared TDS, Python runtime and ODBC READMEs document decoding behavior)

[Copilot]

Copilot review overview

🔵 Needs a closer look

Bound truncation issues remain, and native differential and full-suite validation are still pending.

Review effort: Balanced
Findings: 2

Open (2)

• Preserve isolated high surrogates in bound PLP delivery · New
• Preserve surrogate pairs at bound truncation boundaries · New

What changed in this PR

Preserves raw UTF-16 code units during wide ODBC delivery, avoiding lossy surrogate replacement.

Changes:

• Adds shared UTF-16LE copy logic.
• Uses raw-unit delivery across SQLGetData and bound fetches.
• Adds regression tests and documents the behavior.

File	Description
mssql-odbc/​src/​api/​util.rs	Adds raw UTF-16LE copying.
mssql-odbc/​src/​api/​get_data.rs	Preserves units in SQLGetData.
mssql-odbc/​src/​api/​fetch_scroll.rs	Extends raw copying to bound delivery.
mssql-odbc/​README.md	Documents wide-string behavior.

---

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Copilot review overview

🟡 Changes recommended

Streaming wide PLP paths can still silently discard an odd trailing byte.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1

Open (1)

• Reject odd-byte UTF-16 tails in wide PLP and SQLGetData paths · New

Resolved since last review (2)

• Preserve surrogate pairs at bound truncation boundaries
• Preserve isolated high surrogates in bound PLP delivery

[Copilot]

Copilot review overview

🔵 Needs a closer look

It modifies several unsafe FFI and streaming paths, has an unresolved test-fixture issue, and new-head CI remains pending.

Review effort: Balanced
Findings: None

Resolved since last review (1)

• Reject odd-byte UTF-16 tails in wide PLP and SQLGetData paths

Previously missed (1)

In code that hasn't changed since last review

Preserve no-connection skip behavior in derived fixture setup

mssql-odbc/​tests/​e2e/​tests/​get_data_test.cpp:279

This assertion changes the base fixture's no-connection behavior from a skip to a failure. As a result, every new GetDataUtf16Test fails when the E2E binary is run without ODBC_TEST_SERVER or ODBC_TEST_CONNSTR, while the existing GetDataLiveTest cases skip. Preserve that gating in the derived setup before calling the base setup.

[Saurabh Singh (saurabh500)]

Confirmed the review-body fixture finding: without connection configuration, the 8 new GetDataUtf16Test cases failed while 75 existing GetDataLiveTest cases skipped; fixed in 1be0db3 by retaining the base setup and returning when it reports IsSkipped(). Without configuration all 83 now skip, while configured Rust/reference runs still pass all 12 parity cases twice (48 executions, zero failures/skips); FetchScroll deliberately inherits its base no-connection failure contract and is unchanged, and this commit changes no product code.

[Copilot]

Copilot review overview

🔵 Needs a closer look

It changes unsafe FFI and streaming behavior while latest cross-platform CI and full validation remain incomplete.

Review effort: Balanced
Findings: None

[github-actions]

📊 Code Coverage Report

🔥 Diff Coverage 99%

🎯 Overall Coverage 94.0%

📦 Project: mssql-tds + mssql-odbc + mssql-py-core ℹ️ Note: diff coverage is reported, not enforced.

---

Diff Coverage

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

• mssql-mock-tds/src/protocol.rs (83.3%): Missing lines 953
• mssql-mock-tds/src/query_response.rs (84.6%): Missing lines 46,73
• mssql-odbc/src/api/fetch_scroll.rs (100%)
• mssql-odbc/src/api/get_data.rs (100%)
• mssql-odbc/src/api/util.rs (100%)
• mssql-tds/src/datatypes/sql_string.rs (100%)

Summary

• Total: 417 lines
• Missing: 3 lines
• Coverage: 99%

mssql-mock-tds/src/protocol.rs

  949             result.put_u16_le(
  950                 if col.data_type == crate::query_response::SqlDataType::NVarCharMax {
  951                     PLP_TYPE_LENGTH_MARKER
  952                 } else {
! 953                     MAX_BOUNDED_STRING_BYTES
  954                 },
  955             );
  956             result.put_slice(&[0x09, 0x04, 0xD0, 0x00, 0x34]); // SQL_Latin1_General_CP1_CI_AS
  957         } else {

---

mssql-mock-tds/src/query_response.rs

  42             SqlDataType::TinyInt => 1,
  43             SqlDataType::SmallInt => 2,
  44             SqlDataType::Int => 4,
  45             SqlDataType::BigInt => 8,
! 46             SqlDataType::NVarChar | SqlDataType::NVarCharMax => 255, // Handled specially
  47         }
  48     }
  49 }

---

  69             ColumnValue::SmallInt(_) => SqlDataType::SmallInt,
  70             ColumnValue::Int(_) => SqlDataType::Int,
  71             ColumnValue::BigInt(_) => SqlDataType::BigInt,
  72             ColumnValue::NVarChar(_) => SqlDataType::NVarChar,
! 73             ColumnValue::NVarCharMax(_) => SqlDataType::NVarCharMax,
  74             ColumnValue::Null => SqlDataType::Int, // Default to Int for NULL
  75         }
  76     }

---

---

🔗 Quick Links

View Azure DevOps Build · Coverage Report

[Copilot]

Copilot review overview

🔵 Needs a closer look

Unsafe FFI buffer handling, PLP streaming, and cross-driver parity changes require final human validation.

Review effort: Balanced
Findings: None

Resolved since last review (1)

• Cross-repository test pin differs from the validated PR revision

[Copilot]

Copilot review overview

🔵 Needs a closer look

Unsafe FFI buffer handling, PLP streaming semantics, and a public enum compatibility concern require human review.

Review effort: Balanced
Findings: 1

Open (1)

• Adding enum variants breaks downstream exhaustive matches · New

[David Engel (David-Engel)]

Automated review

Replaces UTF-16 validation with raw code-unit copying on every same-encoding wide delivery path (complete buffered SQLGetData, captured/chunked SQLGetData, bound row slots, bound PLP streaming, output parameters), and makes SqlString::decode honor its declared encoding instead of sniffing a BOM. I traced each changed path against its callers and the truncation/indicator conventions around it; the shared helpers preserve the previous null-pointer, zero-capacity, and truncation semantics, and the new surrogate-boundary trimming in copy_bound_utf16le_with_nul and the PLP wide branch agree with each other case-for-case. Fixes #604 satisfies the linked-issue requirement.

No blocking issues found. Four non-blocking findings are left inline:

• mssql-odbc/src/api/util.rs — the complete-buffered fast path lost its bulk copy_nonoverlapping (performance concern, unmeasured).
• mssql-odbc/src/api/fetch_scroll.rs — copy_bound_utf16le_with_nul documents an even-length contract it doesn't enforce.
• mssql-odbc/src/api/get_data.rs — a checked usize::try_from result is re-derived with as.
• mssql-tds/README.md — the cross-binding behavior change has no CHANGELOG.md entry.

Verification performed

In a detached worktree at 5f4b03ba, against merge-base 3248bcef:

• cargo fmt -- --check — clean.
• cargo clippy --workspace --all-features --all-targets -- -D warnings — clean.
• cargo nextest run -p mssqlodbc --lib — 1664/1664 pass, including the new wide_get_data_preserves_raw_units_and_progress, complete_wide_copy_rejects_odd_bytes_without_writing, narrow_get_data_keeps_lossy_surrogate_conversion, bound_wide_copies_preserve_raw_units_on_both_paths, and the socket-backed bound_wide_plp_preserves_units_across_wire_chunks.
• cargo nextest run -p mssql-tds -p mssql-mock-tds --lib — 2217/2224 pass; decode_preserves_bom_shaped_sql_payloads passes. The 7 failures are all certificate_validator / win_tls::validate cert-fixture tests, unrelated to this change and environmental on this machine.
• Integration/e2e suites were not run (no .env, no live SQL Server, no Driver Manager leg), so the new GetDataUtf16Test / FetchScrollUtf16Test C++ cases are unverified here.

CI note

The failing ADO Build MacOS job is infrastructure, not this PR: the Rust Clippy Lint (workspace + mssql-py-core) task fails with error: 'cargo-clippy' is not installed for the toolchain '1.95-x86_64-apple-darwin'. Worth a retry once the agent image is fixed rather than a code change. Several other ADO legs are still pending, and the checklist boxes for full cargo btest and the cross-repo jobs on this head are correctly left unchecked.

Things I checked and found correct

• The SQLBindCol doc correction in exports.rs matches sql_bind_col_safe, which unbinds on a null TargetValuePtr regardless of the indicator and cites msodbcsql's UnbindParam for it.
• copy_bound_utf16le_with_nul's capacity - usize::from(splits_pair) cannot underflow: splits_pair requires copied > 0, which requires capacity >= 2.
• try_write_direct_captured_string_chunk's &bytes[offset * 2..] is safe — offset is clamped to total_units immediately above, so the slice index is always in range and the multiply cannot overflow.
• Dropping the early slot.is_null() / buf_elements == 0 returns from deliver_encoded_string is behavior-preserving; copy_utf16le_with_nul returns the same values for both cases.
• The narrow-widening branch in deliver_bound_plp doesn't need the new cross-chunk pair check — encoding_rs emits both halves of a scalar value in the same decoded chunk, so a pair can never straddle decoded_units boundaries.
• EncodingType::encoding() is only consumed in non-test code to build new_decoder_without_bom_handling, so the streaming and materialized paths now agree on BOM handling instead of diverging.

[Copilot]

Copilot review overview

🔵 Needs a closer look

It changes shared decoding and unsafe ODBC delivery paths while latest-head full validation remains pending.

Review effort: Balanced
Findings: None

Resolved since last review (1)

• Adding enum variants breaks downstream exhaustive matches

[David Engel (David-Engel)]

Automated review — generated by GitHub Copilot on behalf of David Engel (@David-Engel). This is not an approval and does not satisfy the human review requirement. Findings may be incomplete or wrong; push back on anything that looks off.

Summary

Wide same-encoding delivery now copies raw UTF-16 code units instead of validating and re-encoding them, across complete-buffered SQLGetData, captured/chunked SQLGetData, direct bound rows, bound PLP and the materialized bound/output-parameter fallback, with a bound-only helper that trims a real surrogate pair straddling the capacity boundary. SqlString::decode stops BOM-sniffing, which is correctly documented as a cross-binding (mssql-js, mssql-py-core) change. The shared surrogate predicates, the buffer_bytes reuse in try_write_complete_buffered_string, and the corrected SQLBindCol unbinding doc all read cleanly, and the earlier rounds of feedback on this PR look addressed. One documentation-scope finding below; nothing blocking.

Verification

• Reviewed git diff $(git merge-base origin/main HEAD)..HEAD in an isolated worktree at f9d56f75, plus the unchanged callers: deliver_bound, deliver_encoded_string, deliver_bound_plp, try_write_direct_captured_string_chunk, copy_with_nul/copy_utf16_with_nul, bind_col.rs (for the exports.rs doc correction) and every decode_without_bom_handling / EncodingType::encoding() consumer.
• cargo nextest run -p mssqlodbc -p mssql-tds --lib --no-fail-fast: 3866 tests, 3859 passed. The 7 failures are the known certificate_validator / win_tls::validate fixture failures that need tests/test_certificates/*.pem, unrelated to this diff.
• cargo clippy --workspace --all-features --all-targets -- -D warnings: clean.
• Mutation-checked the three new guards rather than trusting green:
  • neutralising the splits_pair capacity reduction in copy_bound_utf16le_with_nul fails bound_wide_copies_preserve_raw_units_on_both_paths;
  • commenting out the out_units.pop() in deliver_bound_plp fails bound_wide_plp_preserves_units_across_wire_chunks (left: 55357, right: 0);
  • reverting both decode_without_bom_handling calls to decode fails decode_preserves_bom_shaped_sql_payloads.
    All mutations were reverted; the worktree is clean.
• Traced the bound PLP wide loop by hand for the boundary cases the mock cannot enumerate (pair fully inside capacity, pair split at end-of-chunk, value ending exactly at capacity on an isolated high surrogate, capacity_elements == 0); the first-excluded-unit check is correct in each and only ever needs one trim.
• Required CI is pending on this head with no failures; the Linux/Windows/macOS ARM builds, Kerberos and the cross-repo mssql-python ODBC job have already passed.

Findings

Blocking

None.

Suggestion

1. mssql-odbc/README.md:230 — the bound pair-trim guarantee is stated unconditionally but only holds when the column value is already UTF-16. Details inline, with the probe output.

Nit

None.

[Vahid Beiranvand (Vahid-b)]

Unattended automated review — generated by GitHub Copilot on behalf of Saurabh Singh (@saurabh500)'s reviewers. Findings were not checked by a human before posting. This is not an approval and does not satisfy the human review requirement. Push back on anything that looks wrong.

Summary

Wide same-encoding ODBC delivery now copies complete UTF-16LE code units instead of round-tripping through Unicode scalars, and SqlString::decode honors its declared encoding instead of sniffing a BOM. I reviewed the full merge-base diff (3248bcef..f9d56f75, 14 files), linked issue #604, all prior reviews, the 29 inline comments, the three top-level bot/author comments, and current checks.

The delivery logic holds up. copy_utf16le_with_nul reproduces copy_with_nul's null/zero-capacity/truncation contract exactly, so dropping the explicit early-outs from deliver_encoded_string and try_write_direct_captured_string_chunk is behavior-preserving. copy_bound_utf16le_with_nul and the deliver_bound_plp wide branch implement the same "trim only a real pair at the boundary" rule case-for-case, and the PLP cross-chunk pop() is safe against re-filling because the if truncated { ... continue; } guard at the top of the chunk loop stops all further appends for the rest of the value. The new deliver_bound wide branch reuses the file's existing truncated || (slot.is_null() && !bytes.is_empty()) null-slot rule verbatim, so it is internally consistent with the generic path directly below it. The f9d56f75 delta (reusing the checked buffer_bytes) is correct: buffer_bytes >= bytes.len() + 2 implies buffer_bytes / 2 - 1 >= units, so the copy cannot truncate, which is what the new debug_assert! pins.

I found one non-blocking issue, in the shared mssql-tds crate rather than in the ODBC delivery paths.

Verification performed

Detached worktree at f9d56f75, merge base 3248bcef, with a dedicated CARGO_TARGET_DIR. The primary checkout was untouched.

• cargo nextest run -p mssqlodbc --lib --no-fail-fast → 1664 passed, 0 failed, 0 skipped (25.1s), including wide_get_data_preserves_raw_units_and_progress, complete_wide_copy_rejects_odd_bytes_without_writing, narrow_get_data_keeps_lossy_surrogate_conversion, bound_wide_copies_preserve_raw_units_on_both_paths, utf16le_copy_preserves_every_single_code_unit and the socket-backed bound_wide_plp_preserves_units_across_wire_chunks.
• Measured mutation for the finding below: changed encoding_agrees_with_to_utf8_string_for_lcid_bytes's payload from [0xCF, 0xF0, 0xE8, 0xE2, 0xE5, 0xF2] to [0xEF, 0xBB, 0xBF, 0xCF, 0xF0, 0xE8, 0xE2, 0xE5, 0xF2] → FAIL, left: "\u{FFFD}\u{FFFD}\u{FFFD}\u{FFFD}\u{FFFD}\u{FFFD}" vs right: "п»їПривет". Restored afterwards; git status --porcelain is clean.
• Sibling sweep on the BOM change. Grepped every encoding_rs decode construction across mssql-tds, mssql-odbc, mssql-js, mssql-py-core, mssql-tds-cli and mssql-mock-tds. All non-test sites now use decode_without_bom_handling or new_decoder_without_bom_handling; the streaming and materialized paths agree.
• Fixture-contract check. FetchScrollLiveTest::SetUp uses FAIL() on a missing connection while GetDataLiveTest::SetUp uses GTEST_SKIP(), so GetDataUtf16Test's added if (IsSkipped()) return; and FetchScrollUtf16Test's omission of it are each correct for their own base. No GTEST_SKIP(), #[ignore] or SKIP_IF_COMPARING_MSODBCSQL() is used to mask any new case.
• Description check. Utf16TestData::Values() has 11 payloads across 12 new TEST_F cases (8 GetDataUtf16Test + 4 FetchScrollUtf16Test), of which 2 are the CP1252/UTF-8 BOM cases — matching the "original 10 cases" plus "two cases" wording. Fixes #604 satisfies the linked-issue requirement. Every changed file maps to a description bullet, including the .pipeline/mssql-python-revision.txt pin.
• gh pr checks 612 — no failing check; coverage-report still pending, so I did not re-derive CI results locally.

Not verified here: the live GetDataUtf16Test / FetchScrollUtf16Test results and the retail msodbcsql 18.6.2.1 comparison rest on your reported runs and the CI legs — I had no SQL Server and no native driver in this run. I did not build the C++ e2e binary.

Findings

Blocking

None.

Suggestion

mssql-tds/src/datatypes/sql_string.rs — EncodingType::encoding()'s documented agreement with to_utf8_string is now conditional, and the test that guards it cannot see the difference.

encoding_agrees_with_to_utf8_string_for_lcid_bytes states the invariant in its own comment: "The accessor has to decode to the same text to_utf8_string would, otherwise a writer using it would silently diverge from the owned path." It asserts EncodingType::encoding().decode(&bytes) — the BOM-sniffing encoding_rs entry point — against to_utf8_string().

On the merge base both sides called the same encoding.decode, so that held for every payload. After this PR to_utf8_string goes through decode_without_bom_handling, so it holds only for payloads with no BOM-shaped prefix — and the test's payload (CF F0 E8 E2 E5 F2) has none, so it stays green. Measured on this head with the payload prefixed by EF BB BF:

left:  "������"        // encoding().decode(...) — sniffed a UTF-8 BOM, switched encodings
right: "п»їПривет"     // to_utf8_string()

Note the divergence is not just a stripped BOM: sniffing re-selects the encoding, so the Windows-1251 tail is then reinterpreted as invalid UTF-8 and the whole value becomes U+FFFD.

This is not a live defect. The only non-test consumers of encoding() are mssql-odbc/src/api/fetch_scroll.rs:1791 (feeding new_decoder_without_bom_handling at :1799) and mssql-odbc/src/api/get_data.rs:1710 (feeding handles/stmt.rs:139, likewise without BOM handling), and neither mssql-js nor mssql-py-core calls it. But encoding() is pub, and its doc comment currently warns only about the UTF-8 panic asymmetry with to_utf8_string — not about this new one. A future writer reaching for the natural encoding().decode(...) would diverge silently, which is exactly the failure this test exists to prevent.

Concrete resolution: extend the encoding() doc comment to state that callers must use decode_without_bom_handling / new_decoder_without_bom_handling to match to_utf8_string, and add a BOM-shaped case to encoding_agrees_with_to_utf8_string_for_lcid_bytes asserting against the no-BOM-handling call so the invariant is pinned where it is actually documented.

Nit

None.

Required CI failures

None. All required checks that have reported are passing on this head (f9d56f75), including Windows, Windows ARM, Linux, Linux ARM, macOS, CodeQL, Kerberos and the cross-repo mssql-python suite on mssql-odbc driver job. coverage-report is still pending at the time of this review.

Deferred / to file

• Pre-existing, not introduced here and deliberately not filed against this PR: SqlString::decode's EncodingType::Utf8 arm still does String::from_utf8(bytes.to_vec()).unwrap(), a panic on wire data reachable from mssql-py-core's cursor and mssql-js's row writer. It is already documented on EncodingType::encoding(), and the shape exists unchanged on main; worth its own issue rather than scope here.
• Already answered in-thread and not re-raised: odd-length PLP handling, the null TargetValuePtr question, the copy_bound_utf16le_with_nul odd-length contract, the lost bulk copy_nonoverlapping (measured by the author as memcpy-equivalent), the mssql-mock-tds public-enum break, the CHANGELOG.md entry, and the Python revision pin.

The PR appears otherwise ready for human review. It has not been approved here.