Fix PLP carry handling across SQLGetData target switches

[Saurabh Singh (saurabh500)]

Description

Switching the SQLGetData C target while a PLP column has converted output pending can strand the previous target's carry and return 01004 indefinitely.

Match msodbcsql's carry handling: both text targets drain pending converted bytes verbatim, while binary bypasses the carry and completes on wire exhaustion. Complete decoder-held source characters before returning, even after earlier output, across UTF-16-to-UTF-8, narrow-to-UTF-8, and narrow-to-WCHAR conversion. Internal reads preserve the buffer offset and call-level length accounting; widening fills remaining output capacity before reporting truncation. Preserve zero-length binary probes and consuming binary-to-text continuations. Both output-offset sites preserve null targets so completion/refill retries cannot bypass no-copy guards.

Completion is bounded to the original partial character. If malformed input starts another character during completion, preserve its bytes in the raw-input buffer rather than extending the retry chain. Subsequent text or binary reads consume that prefix with correct remaining-length accounting. This covers consecutive high surrogates and malformed UTF-8 leads without buffering the entire value.

The verbatim carry copy is intentional: changing from UTF-8 output to UTF-16 does not re-encode bytes already converted. Source-traced in InternalGetColData (odbc/sqlcdata.h) and the completion gate (odbc/sqlcdata.cpp), and measured against retail 18.6.2.1 (SQL_DRIVER_VER=18.06.0002). Windows narrow output retains its existing client-codepage difference.

One reference-driver quirk is intentionally preserved: a WCHAR probe with no payload room on an exhausted UTF-16 source returns 01004 with indicator 0 even when converted carry remains. Supplying payload room drains the carry and completes; repeatedly issuing zero-capacity probes does not.

Review coverage includes odd carry with aligned/byte-offset destinations, mock varchar(max)/GBK refill and decoder completion, malformed-input replay, and null-target retries. The output helper already uses byte copies and write_unaligned; its two focused wide-copy tests passed under Miri (nightly-2026-09-06, seed 0). Native odd-carry cases pass against Rust and Linux msodbcsql 18.6.2.1. Disabling the GBK retry behaviors makes their regressions fail. All three new null-target cases reproduced SIGSEGV before the null-preserving fix and pass afterward.

Optimizing repeated binary staging is deferred: an intervening text read can exhaust the wire while leaving carry, so a one-time nonempty-binary-read flag would break the untouched-buffer guarantee. A regression pins that sequence.

Merged main through #631 in c706633a. The SQLGetData conflict resolution preserves this PR's carry, bounded completion, and null-target handling while integrating resolved collation decoders, CHAR-buffer refill, and conversion-aware length indicators. The decoder-state query now handles OEM encodings, with added OEM target-switch coverage. Both parents' regressions are retained; truncation fixtures were adjusted where CHAR refill now completes the original input in one call.

Required checks passed on pre-merge head f812b4e3. Validation on fa3f0f53 later hit a Docker-download/agent timeout in the cross-repo Python stage; that infrastructure failure was retried. Latest local validation on c706633a: 413 targeted Rust tests, workspace formatting, affected-crate all-feature/all-target clippy, and the driver build passed. Native get-data/fetch suites passed 159 cases with the Rust driver (4 SQL Server 2022 JSON skips), and 135 cases with retail msodbcsql 18.06.0002 (28 existing skips). Latest-head CI remains to be confirmed. The checked validation items below record completed pre-merge validation, not a claim that latest-head CI has finished.

Related Issues

Fixes AB#48046

https://sqlclientdrivers.visualstudio.com/mssql-rs/_workitems/edit/48046

Checklist

• cargo bfmt passes
• cargo bclippy passes
• cargo btest passes
• New/changed functionality has tests
• Public API changes are documented

[Copilot]

Copilot review overview

🟡 Changes recommended

WCHAR truncation can report a zero indicator while converted bytes remain pending, causing indicator-sized retries to stall.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1

Open (1)

• WCHAR indicator omits pending carry, causing endless retries · New

What changed in this PR

Fixes PLP streaming when SQLGetData switches C target types while converted output remains buffered.

Changes:

• Generalizes converted carry storage and delivery across text targets.
• Preserves binary probe and wire-exhaustion behavior.
• Adds Rust and native target-switch regression tests and documentation.

File	Description
mssql-odbc/​src/​api/​get_data.rs	Updates PLP carry, retry, completion, and indicator logic.
mssql-odbc/​src/​handles/​stmt.rs	Generalizes pending converted bytes state.
mssql-odbc/​tests/​e2e/​tests/​get_data_test.cpp	Adds native target-switch coverage.
mssql-odbc/​README.md	Documents target-switch behavior and parity.

---

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

[Copilot]

Copilot review overview

🟡 Changes recommended

Decoder input can still be stranded when a partial character follows output, causing data loss after switching to binary.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1

Open (1)

• Retry loses split character after prior output · New

Resolved since last review (1)

• WCHAR indicator omits pending carry, causing endless retries

[Copilot]

Copilot review overview

🔵 Needs a closer look

The low-level FFI streaming changes require human review and completion of the pending full clippy and coverage runs.

Review effort: Balanced
Findings: None

Resolved since last review (1)

• Retry loses split character after prior output

[Copilot]

Copilot review overview

🔵 Needs a closer look

The FFI streaming state changes are intricate, and full clippy and coverage validation remains pending.

Review effort: Balanced
Findings: None

[github-actions]

📊 Code Coverage Report

🔥 Diff Coverage 93%

🎯 Overall Coverage 94.2%

📦 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 (66.7%): Missing lines 951
• mssql-mock-tds/src/query_response.rs (83.3%): Missing lines 50,80
• mssql-odbc/src/api/get_data.rs (93.9%): Missing lines 1987-1993,2008-2010,2012,2016-2017,2131,2134-2135,2138-2142,2144,2163-2164,2170-2174,2180,2192-2194,2207,2234,2238-2239,2270-2271,2274,2276,2280,2298-2299,2317,2320-2321,2330-2331,2404,2407,2502,2505,2514-2515,2572,2577-2579,2582,2627-2628,2647
• mssql-odbc/src/handles/stmt.rs (97.6%): Missing lines 241
• mssql-tds/src/datatypes/sql_string/encoding.rs (100%)

Summary

• Total: 1097 lines
• Missing: 67 lines
• Coverage: 93%

mssql-mock-tds/src/protocol.rs

  947         ) {
  948             // Required to support string responses (e.g., @@USERAGENT).
  949             // TDS ColMetadata mandates a 5-byte collation suffix for variable-length types.
  950             result.put_u16_le(
! 951                 if matches!(
  952                     col.data_type,
  953                     crate::query_response::SqlDataType::NVarCharMax
  954                         | crate::query_response::SqlDataType::VarCharMax
  955                 ) {

---

mssql-mock-tds/src/query_response.rs

  46             SqlDataType::SmallInt => 2,
  47             SqlDataType::Int => 4,
  48             SqlDataType::BigInt => 8,
  49             SqlDataType::NVarChar | SqlDataType::NVarCharMax => 255, // Handled specially
! 50             SqlDataType::VarCharMax => 255,
  51         }
  52     }
  53 }

---

  76             ColumnValue::Int(_) => SqlDataType::Int,
  77             ColumnValue::BigInt(_) => SqlDataType::BigInt,
  78             ColumnValue::NVarChar(_) => SqlDataType::NVarChar,
  79             ColumnValue::NVarCharMax(_) => SqlDataType::NVarCharMax,
! 80             ColumnValue::VarCharMax(_) => SqlDataType::VarCharMax,
  81             ColumnValue::Null => SqlDataType::Int, // Default to Int for NULL
  82         }
  83     }

---

mssql-odbc/src/api/get_data.rs

  1983                 0,
  1984                 "Buffer length too small to hold a single character and null terminator",
  1985             );
  1986         } else if let Ok(mut s) = stmt.inner.lock() {
! 1987             post_sql_error(
! 1988                 &mut s,
! 1989                 SQLSTATE_HY090,
! 1990                 0,
! 1991                 "Buffer length too small to hold a single character and null terminator",
! 1992             );
! 1993         }
  1994         return SQL_ERROR;
  1995     }
  1996 
  1997     // msodbcsql's InternalGetColData (sqlcdata.h) copies its conversion

---

  2004     let prefix_bytes = if target_type != SQL_C_BINARY && carried_bytes > 0 {
  2005         let mut state = match retained_stmt_state.take() {
  2006             Some(state) => state,
  2007             None => {
! 2008                 let Ok(state) = stmt.inner.lock() else {
! 2009                     error!("SQLGetData: stmt mutex poisoned while delivering PLP carry");
! 2010                     return SQL_ERROR;
  2011                 };
! 2012                 state
  2013             }
  2014         };
  2015         let Some(stream) = state.active_plp.as_mut() else {
! 2016             error!("SQLGetData: PLP stream vanished while delivering carry");
! 2017             return SQL_ERROR;
  2018         };
  2019         let capacity = if target_type == SQL_C_WCHAR {
  2020             payload_capacity & !1
  2021         } else {

---

  2127                 stream.take_prefetch_error(),
  2128                 stream.read_prefetched_wire(payload),
  2129             )
  2130         } else {
! 2131             (None, None)
  2132         }
  2133     } else {
! 2134         let Ok(mut stmt_state) = stmt.inner.lock() else {
! 2135             error!("SQLGetData: stmt mutex poisoned while reading prefetched PLP bytes");
  2136             return SQL_ERROR;
  2137         };
! 2138         if let Some(stream) = stmt_state.active_plp.as_mut() {
! 2139             (
! 2140                 stream.take_prefetch_error(),
! 2141                 stream.read_prefetched_wire(payload),
! 2142             )
  2143         } else {
! 2144             (None, None)
  2145         }
  2146     };
  2147 
  2148     let read_result = if let Some(error) = prefetch_error {

---

  2159         let dbc = stmt.parent_dbc();
  2160         let mut dbc_state = match dbc.inner.lock() {
  2161             Ok(state) => state,
  2162             Err(_) => {
! 2163                 error!("SQLGetData: dbc mutex poisoned while reading PLP stream");
! 2164                 return SQL_ERROR;
  2165             }
  2166         };
  2167         if let Some(busy_stmt) = dbc_state.active_stmt
  2168             && busy_stmt != statement_handle

---

  2166         };
  2167         if let Some(busy_stmt) = dbc_state.active_stmt
  2168             && busy_stmt != statement_handle
  2169         {
! 2170             drop(dbc_state);
! 2171             if let Ok(mut s) = stmt.inner.lock() {
! 2172                 post_diag(&mut s, ERR_CONNECTION_BUSY);
! 2173             }
! 2174             return SQL_ERROR;
  2175         }
  2176         let buffered_read = {
  2177             let Some(client) = dbc_state.client.as_mut() else {
  2178                 drop(dbc_state);

---

  2176         let buffered_read = {
  2177             let Some(client) = dbc_state.client.as_mut() else {
  2178                 drop(dbc_state);
  2179                 if let Ok(mut s) = stmt.inner.lock() {
! 2180                     post_diag(&mut s, ERR_NO_ACTIVE_TDS_CLIENT);
  2181                 }
  2182                 return SQL_ERROR;
  2183             };
  2184             client.try_read_active_plp_chunk(payload)

---

  2188             Ok(CursorPoll::Ready(chunk)) => {
  2189                 drop(dbc_state);
  2190                 Ok(chunk)
  2191             }
! 2192             Err(error) => {
! 2193                 drop(dbc_state);
! 2194                 Err(error)
  2195             }
  2196             Ok(CursorPoll::Pending) => {
  2197                 let Some(mut client) = dbc_state.client.take() else {
  2198                     drop(dbc_state);

---

  2203                 };
  2204                 drop(dbc_state);
  2205                 let mut prefetch_scratch = {
  2206                     let Ok(mut stmt_state) = stmt.inner.lock() else {
! 2207                         error!("SQLGetData: stmt mutex poisoned while taking PLP prefetch buffer");
  2208                         return SQL_ERROR;
  2209                     };
  2210                     std::mem::take(&mut stmt_state.plp_prefetch_scratch)
  2211                 };

---

  2230                         Ok(tail) => {
  2231                             let carry = (prefetch_scratch, tail, chunk.total_read);
  2232                             Ok((chunk, Some(carry), None, None))
  2233                         }
! 2234                         Err(error) => Ok((chunk, None, Some(prefetch_scratch), Some(error))),
  2235                     }
  2236                 });
  2237                 let Ok(mut dbc_state) = dbc.inner.lock() else {
! 2238                     error!("SQLGetData: dbc mutex poisoned after PLP read");
! 2239                     return SQL_ERROR;
  2240                 };
  2241                 dbc_state.client = Some(client);
  2242                 dbc_state.active_stmt = Some(statement_handle);
  2243                 drop(dbc_state);

---

  2266                         } else if let Some(buffer) = unused_scratch {
  2267                             stmt_state.plp_prefetch_scratch = buffer;
  2268                         }
  2269                         if let Some(error) = prefetch_error {
! 2270                             let Some(stream) = stmt_state.active_plp.as_mut() else {
! 2271                                 error!(
  2272                                     "SQLGetData: PLP stream vanished while saving prefetch error"
  2273                                 );
! 2274                                 return SQL_ERROR;
  2275                             };
! 2276                             stream.set_prefetch_error(error);
  2277                         }
  2278                         Ok(chunk)
  2279                     }
! 2280                     Err(error) => Err(error),
  2281                 }
  2282             }
  2283         }
  2284     };

---

  2294             if let Some(mut s) = retained_stmt_state.take() {
  2295                 s.clear_state(STMT_STATE_CURSOR_OPEN);
  2296                 post_tds_error(&mut s, &e, SQLSTATE_HY000);
  2297             } else if let Ok(mut s) = stmt.inner.lock() {
! 2298                 s.clear_state(STMT_STATE_CURSOR_OPEN);
! 2299                 post_tds_error(&mut s, &e, SQLSTATE_HY000);
  2300             }
  2301             return SQL_ERROR;
  2302         }
  2303     };

---

  2313         // split across a chunk boundary is carried rather than corrupted.
  2314         let buf_elements = (buffer_length as usize) / std::mem::size_of::<SqlWChar>();
  2315         let emitted = {
  2316             let Ok(mut ss) = stmt.inner.lock() else {
! 2317                 return SQL_ERROR;
  2318             };
  2319             let Some(stream) = ss.active_plp.as_mut() else {
! 2320                 error!("SQLGetData: narrow PLP stream vanished mid-call");
! 2321                 return SQL_ERROR;
  2322             };
  2323             stream.ensure_narrow_decoder();
  2324             let ActivePlpStream {
  2325                 narrow_decoder,

---

  2326                 pending_units,
  2327                 ..
  2328             } = stream;
  2329             let Some(decoder) = narrow_decoder.as_mut() else {
! 2330                 error!("SQLGetData: narrow PLP stream has no encoding to widen through");
! 2331                 return SQL_ERROR;
  2332             };
  2333             let pending_before = pending_units.len();
  2334             let emit = widen_into_pending(
  2335                 decoder,

---

  2400         // than the buffer holds. Transcode the whole chunk, copy only what
  2401         // fits, and keep the rest in pending_utf8 for the next call.
  2402         {
  2403             let Ok(mut ss) = stmt.inner.lock() else {
! 2404                 return SQL_ERROR;
  2405             };
  2406             let Some(stream) = ss.active_plp.as_mut() else {
! 2407                 return SQL_ERROR;
  2408             };
  2409             let ActivePlpStream {
  2410                 pending_byte,
  2411                 pending_high_surrogate,

---

  2498         // and pending_utf8 carries output the caller's buffer had no room for,
  2499         // since decoding expands: CP1252 0x80 is three UTF-8 bytes.
  2500         {
  2501             let Ok(mut ss) = stmt.inner.lock() else {
! 2502                 return SQL_ERROR;
  2503             };
  2504             let Some(stream) = ss.active_plp.as_mut() else {
! 2505                 return SQL_ERROR;
  2506             };
  2507             stream.ensure_narrow_decoder();
  2508             let ActivePlpStream {
  2509                 narrow_decoder,

---

  2510                 pending_bytes: pending_utf8,
  2511                 ..
  2512             } = stream;
  2513             let Some(decoder) = narrow_decoder.as_mut() else {
! 2514                 error!("SQLGetData: narrow PLP stream has no encoding to convert through");
! 2515                 return SQL_ERROR;
  2516             };
  2517             let pending_before = pending_utf8.len();
  2518             let emit = transcode_narrow_into_pending(
  2519                 decoder,

---

  2568             // the wire, so verbatim is the conversion.
  2569             Some(PlpEncoding::SingleByteText) => copy_verbatim(),
  2570             // json — UTF-8 on the wire; delivered verbatim to SQL_C_CHAR. Must
  2571             // stay distinct from SingleByteText (see above).
! 2572             Some(PlpEncoding::Utf8Text) => copy_verbatim(),
  2573             // Utf16Text/Binary/None never reach this branch: the compatibility
  2574             // gate rejects them or an earlier arm handles them. Assert the
  2575             // invariant in debug/tests; fall back to a verbatim copy in release
  2576             // rather than panicking across the FFI boundary (which would be UB).
! 2577             other => {
! 2578                 debug_assert!(
! 2579                     false,
  2580                     "SQL_C_CHAR PLP delivery reached with unexpected encoding {other:?}"
  2581                 );
! 2582                 copy_verbatim();
  2583             }
  2584         }
  2585     }

---

  2623         {
  2624             // Completion reads one byte. Output plus a non-neutral decoder
  2625             // means that byte started a new character after malformed input.
  2626             let Some(&byte) = payload.first() else {
! 2627                 error!("SQLGetData: narrow character completion has no source byte");
! 2628                 return SQL_ERROR;
  2629             };
  2630             source_prefix[0] = byte;
  2631             source_prefix_len = 1;
  2632             stream.narrow_decoder = None;

---

  2643         }
  2644         progress.retry_bytes = if transcode_utf16_to_utf8 {
  2645             progress.completing_surrogate = stream.pending_high_surrogate.is_some();
  2646             if stream.pending_byte.is_some() {
! 2647                 1
  2648             } else if stream.pending_high_surrogate.is_some() {
  2649                 2
  2650             } else {
  2651                 0

---

mssql-odbc/src/handles/stmt.rs

  237             .field("column", &self.column)
  238             .field("encoding", &self.encoding)
  239             .field("pending_byte", &self.pending_byte)
  240             .field("pending_high_surrogate", &self.pending_high_surrogate)
! 241             .field("pending_bytes", &self.pending_bytes.len())
  242             .field("narrow_decoder", &self.narrow_decoder.is_some())
  243             .field("pending_units", &self.pending_units.len())
  244             .field(
  245                 "prefetched_wire_remaining",

---

---

🔗 Quick Links

View Azure DevOps Build · Coverage Report

[Vahid Beiranvand (Vahid-b)]

This is an automated review generated by GitHub Copilot. It may be incomplete or incorrect — please verify findings before acting on them.

Summary

This replaces pending_utf8 with a general pending_bytes carry, drains that carry verbatim as a prefix for both text targets (binary bypasses it), and adds an outer retry loop (stream_active_plp_chunk → stream_active_plp_chunk_once + PlpReadProgress) so a decoder-held partial source character is completed before returning, even after earlier output. Indicator accounting is re-routed through a per-call chunk_indicator and summed into the application's pointer at the end. The layering, the parity rationale, and the deliberately preserved msodbcsql zero-capacity WCHAR probe quirk are all documented in mssql-odbc/README.md, and the two earlier Copilot threads were addressed substantively.

One blocking issue: the new carry prefix can advance the caller's buffer pointer by an odd number of bytes, after which the narrow→UTF-16 widening arm performs SQLWCHAR-typed writes at a misaligned address. I reproduced this on this head.

Verification performed

• Confirmed repo microsoft/mssql-rs, author saurabh500, head 2b02b90fa427f239f2e1ebf038116223c1c8eb7c, state OPEN / non-draft.
• Merge base against current origin/main (120eab88): f5c408b8c4ff0bb49acf2ebdfea4f45a8705de92. Reviewed the full diff (4 files, +903/-90) in a dedicated worktree with CARGO_TARGET_DIR redirected; worktree removed afterwards.
• Read the PR body, the linked AB#48046 reference, both existing review threads (both resolved), and the coverage-report comment (97% diff / 94.0% overall).
• Read policy from origin/main: .github/copilot-instructions.md, .github/instructions/pr-workflow.instructions.md, .github/instructions/mssql-odbc.instructions.md. .github/skills/code-review/SKILL.md was not present in the checkout, so the repo's own review skill could not be applied.
• gh pr checks 628: all 18 required checks pass, including both msodbcsql comparison legs, the cross-repo mssql-python suite, Kerberos, and coverage merge.
• Measured the blocking finding with a probe test built on the PR's own mock-PLP harness (cargo test -p mssqlodbc --lib, debug):
  • wire 80 80 41 00, stream retyped to PlpEncoding::SingleByteText + WINDOWS_1252 (the mock cannot serve varchar(max); everything else is organic);
  • SQLGetData(SQL_C_CHAR, buf[3]) → 01004, emits e2 82, leaving pending_bytes = [ac] — odd, asserted;
  • SQLGetData(SQL_C_WCHAR, …) into a genuinely SQLWCHAR-aligned [u16; 16] → SQL_SUCCESS, indicator 7, bytes ac | ac 20 | 41 00 | 00 00.
  • The 0x20AC, 0x0041, 0x0000 units are written at byte offsets 1, 3, 5 of an even-addressed buffer.
  • Probe removed; worktree clean.
• Confirmed prefix_bytes / wrapping_add do not exist on origin/main, so this pointer offset is new to this PR.
• Confirmed .config/nextest.toml [profile.miri-odbc] default-filter is test(::memory_safety::) | test(conversion::param_buffer::tests::misaligned_), so the CI Miri leg does not reach api::get_data (and cannot — these tests start Tokio and a mock server).

Not verified: no msodbcsql or dotnet/SqlClient checkout exists on this host, so the parity citations to odbc/sqlcdata.h / sqlcdata.cpp and the retail 18.6.2.1 measurements rest on your reported runs and the green comparison legs, not on anything I re-derived. I could not run Miri against the PLP paths for the reason above.

Findings

Blocking

mssql-odbc/src/api/get_data.rs:2040-2043 and :2318-2322 — misaligned SQLWCHAR writes after an odd carry prefix

The carry drain advances the caller's pointer by prefix_bytes:

let target_value_ptr: SqlPointer = target_value_ptr
    .cast::<u8>()
    .wrapping_add(prefix_bytes)
    .cast();

prefix_bytes is carried_bytes.min(capacity), and carried_bytes is byte-granular — transcode_narrow_into_pending returns out_bytes.min(pending.len()), so a SQL_C_CHAR read that splits a 3-byte UTF-8 sequence leaves a 1-byte carry. For SQL_C_WCHAR the capacity is even, so when capacity >= carried_bytes the emitted prefix is the odd carry itself.

The widening arm then does SQLWCHAR-typed writes through that now-odd pointer:

copy_with_nul(
    target_value_ptr as *mut SqlWChar,
    buf_elements,
    &pending_units[..emit],
);

copy_with_nul (api/util.rs:41-58) uses ptr::copy_nonoverlapping plus dst.add(copy_len).write(..), both of which require the destination to be aligned for T. With T = u16 at an odd address that is UB, regardless of whether x86 tolerates it in practice. The retry iterations inherit the same odd offset via progress.written.

This does not reproduce as a runtime panic because copy_nonoverlapping's alignment precondition is check_language_ub, which only fires under Miri — and the Miri profile's filter excludes api::get_data. So neither the debug test run nor CI can catch it.

Application-level sequence, no white-box access needed: varchar(max) under a non-UTF-8 collation (the AB#47566 path), SQLGetData(SQL_C_CHAR, …) with payload room that splits a multi-byte character, then SQLGetData(SQL_C_WCHAR, …).

The sibling arm already handles this correctly — target_type == SQL_C_WCHAR && !hex_stream copies as u8 and terminates with write_unaligned. Suggest the widening arm do the same: copy pending_units[..emit] as bytes into the u8 pointer and write the terminator with write_unaligned, so the verbatim-carry parity is preserved without an alignment assumption. copy_with_nul(target_value_ptr as *mut SqlWChar, …) at :1100 and in the hex_stream arm are safe today only because carry is always zero there; worth a comment pinning that, or routing all SQLWCHAR output through one unaligned-safe helper.

Suggestion

mssql-odbc/tests/e2e/tests/get_data_test.cpp — no coverage for CHAR→WCHAR on a non-UTF-8 narrow collation

The new target-switch cases cover the combination that reaches the defect only partially:

• PlpTargetSwitchCompletesWideningAfterPriorOutput and PlpWideningFillsRemainingCapacityWithoutOverreading use Latin1_General_100_CI_AS_SC_UTF8, so transcode_narrow_to_utf8 is excluded and a SQL_C_CHAR read leaves no carry;
• PlpTargetSwitchCompletesDbcsAfterPriorOutput uses Chinese_PRC_CI_AS — which does carry — but switches CHAR→BINARY, which bypasses the prefix entirely.

Nothing exercises non-UTF-8 narrow + CHAR→WCHAR, which is exactly where the odd prefix meets the widening arm. A case on SQL_Latin1_General_CP1_CI_AS (or Chinese_PRC_CI_AS) reading SQL_C_CHAR into a buffer that splits a multi-byte character and then switching to SQL_C_WCHAR would pin both the byte layout and the fix above. The SKIP_IF_COMPARING_MSODBCSQL_ON_WINDOWS() guards on the other new cases look correct — they match the documented Windows client-codepage difference and leave the Linux comparison leg asserting.

Nit

PR description / checklist is stale. cargo bclippy and cargo btest are unchecked and the body says "Full workspace clippy and coverage test runs remain pending", but all 18 required checks are green on this head, including coverage-report and both comparison legs. Worth updating so a reader doesn't treat the gate as unrun.

Required CI

No required check is failing on 2b02b90f — all 18 pass.

[David Engel (David-Engel)]

Automated review (unattended run)

Reviewed the full diff against merge-base f5c408b and ran cargo nextest run -p mssqlodbc --lib in an isolated worktree (1673 passed). Required CI is green on 2b02b90, so no findings restate CI output. I did not re-file the two Copilot threads that saurabh500 already answered (zero-capacity WCHAR probe indicator; stranded decoder input after output).

Carry/retry design reads sound to me. I traced the new outer loop for termination (every retry requires read > 0 && !reached_end, and payload_capacity shrinks by progress.written each pass), confirmed narrow_decoder_has_partial_character is safe for every encoding lcid_to_encoding can return (all are SingleByte/Utf8/Gb18030/Big5/EucKr/ShiftJis, so latin1_byte_compatible_up_to never short-circuits on the "never Latin1-byte-compatible" arm, and new_decoder_without_bom_handling keeps the life cycle Converting), and checked that the odd-offset copy_with_nul writes stay unaligned-safe and in bounds.

Three non-blocking comments inline, two of them mutation-proven test gaps:

1. get_data.rs:2605 — the WCHAR refill retry survives mutation; no Rust test covers it.
2. get_data.rs:2588 — the narrow-decoder half of the partial-character completion survives mutation; only the UTF-16 half has Rust coverage.
3. get_data.rs:2060 — staged_binary_read is sticky for the rest of the stream once text carry exists.

Process note (not attachable to a diff line): the PR is out of draft but cargo bclippy and cargo btest are still unchecked in the description, and the body says full clippy/coverage runs "remain pending". Both ADO validation and the GitHub coverage-report job are green on this head, so the checklist looks stale rather than accurate — worth ticking before human review per .github PR workflow guidance.

[Saurabh Singh (saurabh500)]

misaligned SQLWCHAR writes after an odd carry prefix

This does not apply to the reviewed head. copy_with_nul at 2b02b90f already casts both pointers to u8 for copy_nonoverlapping and uses write_unaligned for the terminator. The widening arm calls that helper, so an odd destination offset does not introduce an alignment requirement. This implementation predates this PR.

I ran the actual helper under Miri (nightly-2026-09-06, seed 0): wide_copies_respect_capacity_at_a_byte_offset and wide_copies_initialize_only_the_written_prefix both passed. The first explicitly asserts a misaligned destination and exercises the production helper across capacities and repeated writes. This verifies the writer, not the full Tokio/PLP path under Miri.

Added the suggested native odd-carry regression in c6aefe4: CP1252 CHAR-to-WCHAR with aligned and byte-offset application buffers, exact byte layouts, indicators, terminators, and guard bytes. It passes against this driver and Linux retail msodbcsql 18.6.2.1. No production rewrite was needed. The PR description now distinguishes the completed CI on 2b02b90 from CI for this test-only follow-up.

[Copilot]

Copilot review overview

🟡 Changes recommended

Character-completion retries can consume and buffer an entire malformed PLP stream in one call.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1

Open (1)

• Bound retry buffering for consecutive high surrogates · New

[ttk (Theekshna)]

Unattended hourly review sweep (first pass on this PR from me). Checked out fresh at 0a437d80 (merge-base f5c408b8 against main, 6 files, +1080/-92). Read all prior discussion first: two Copilot bot reviews, saurabh500's own replies, and human reviews from Vahid-b and David-Engel at an earlier head (2b02b90f) plus the newest bot review at this head.

David-Engel's three mutation-proven test-gap comments (get_data.rs:2605, :2588, :2060) and both anchored bot threads (the zero-capacity WCHAR indicator question, and stranded-decoder-input) are all resolved — saurabh500 added the corresponding tests/fixes in c6aefe47/0a437d80 and I didn't re-check that work. The bot's newest thread (get_data.rs:2584, unbounded pending_bytes growth for consecutive malformed high surrogates, High) is still open and unresolved as of this review — I did not re-file it, just noting it stays a blocker for merge alongside the finding below.

Vahid-b's Blocking finding from the 2b02b90f review was never addressed and is still present on this head. It has no visible GitHub thread because it was posted as prose inside the review body rather than as an anchored inline comment, so it generated no notification for the author to reply to — the same non-anchored-finding gap this sweep has seen elsewhere. I re-verified it directly against the current source (see inline comment) rather than taking the original review at its word: copy_with_nul<T> (api/util.rs:111-132) does its bulk copy via std::ptr::copy_nonoverlapping, whose documented precondition is that both pointers are properly aligned for T; the new carry-prefix and retry-progress offsets applied to target_value_ptr before reaching the SQL_C_WCHAR call site are byte-granular and can be odd, so the precondition is violated. Vahid-b's own probe test already measured the odd byte offset in a real SQLGetData(SQL_C_WCHAR, …) call on this exact code (not just a source read) — I could not independently re-run it, since this sandbox's cargo nextest run -p mssqlodbc --lib fails to compile for the unrelated, pre-existing crc32fast AVX-512/vpclmulqdq codegen issue (E9012/E9013) already noted on PRs #631/#620/#599.

Check 3 (divergence documented): the zero-capacity WCHAR probe quirk this PR's description asks about is properly recorded in mssql-odbc/README.md:255-259 ("SQLGetData target switches"), with the same sqlcdata.h/.cpp citation as the PR body. Confirmed Sql\Ntdbms\sqlncli\odbc\sqlcdata.{h,cpp} exists in the reference tree and contains InternalGetColData's completion path; I did not re-derive the exact indicator-zero behavior myself since the author's reply to the original bot thread already cites a real retail-binary measurement (SQL_DRIVER_VER=18.06.0002 on Linux) rather than a source read, and two independent human reviews left it unchallenged.

Check 4 (description currency): matches the diff; linked to AB#48046; checklist boxes are honest — ADO's "mssql-rs Pull request validation" is still pending on this exact head, which the body itself says ("CI for the latest follow-up remains to be confirmed").

Check 5 (slop): no padded comments found in the new carry/retry code; the safety comments explain invariants (buffer bounds, why a flag stays conservative) rather than restating lines.

Check 6 (evidence audit): the PR's own numbers (1673 local tests, 97% diff / 94.0% overall coverage, 91 native SQLGetData cases, Miri seed 0) rest on the author's and David-Engel's local runs — I could not re-derive any of them locally for the crc32fast reason above, so I'm relying on gh pr checks (green on the prior head, ADO pending on this one) rather than re-running the suite myself. The class of input this review adds evidence about — an odd byte offset reaching a SQLWCHAR-typed bulk copy — is exactly the neighbor none of the 91 native cases or the new Rust regressions cover: every new WCHAR-target regression I can see uses either a UTF-8 collation (no carry at all) or completes on a 2-byte-aligned boundary.

This is an unattended automated review; findings may be incomplete or wrong, push back on anything that looks off.

Severity	Count
Blocking	1
Suggestion	0
Nit	0

[ttk (Theekshna)]

Duplicate of #628 (review) — posted twice by a tooling error in the same automated run. Please disregard this review; the findings and marker live on the other one.

[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

Unattended re-review of the complete diff (6 files, +1372/-96) against merge-base f5c408b8 on head f812b4e3, focused on the newest commit (f812b4e3, null TargetValuePtr preserved across retry passes) since everything before it already has review history here. No new findings.

The null-target blocker I filed on 00eb41e4 is fixed: with emit forced to 0 for a null target, prefix_bytes is 0 as well, so both offset sites leave the pointer null and every downstream is_null() guard keeps working. I re-ran the two throwaway probes that reproduced 0xC0000005 on the previous head and they now pass, and the three new regressions (plp_null_target_survives_utf16_completion_after_output, ..._narrow_completion_after_output, ..._widening_refill) cover both offset sites plus the refill path.

I also re-checked the parts of the carry/retry machinery the newest commit touches rather than trusting the earlier pass: progress.written can never exceed the caller's buffer_length (each arm's chunk_indicator is bounded by that pass's payload_capacity, which is itself recomputed after subtracting prior progress), so the buffer_length.saturating_sub(...) on an i64 cannot go negative and be cast to a huge usize; the outer loop still terminates because every retry requires read > 0 && !reached_end and the malformed-input replay clears the state that asked for the retry; and narrow_decoder_has_partial_character remains safe for every encoding lcid_to_encoding can return (all SingleByte, Utf8, Gb18030, Big5, ShiftJis or EucKr variants, none of which return None unconditionally from latin1_byte_compatible_up_to).

I did not re-file findings already raised and answered on this PR: the zero-capacity WCHAR probe indicator, stranded decoder input, the two mutation-proven test gaps, sticky binary staging, unbounded completion retries on malformed input, and the odd-offset copy_with_nul alignment question.

Verification

cargo nextest run -p mssqlodbc --lib --no-fail-fast in the assigned isolated worktree with the assigned CARGO_TARGET_DIR: 1685 passed, 0 failed. cargo clippy -p mssqlodbc -p mssql-mock-tds --all-features --all-targets -- -D warnings: clean.

On top of that I wrote five throwaway differential probes against mssql-mock-tds (reverted, not committed) that drain a whole PLP value one chunk at a time for every buffer size from 2 to 16 bytes and compare the concatenated output against the expected bytes, with a hard call-count cap to catch a stalled stream: UTF-16 source to SQL_C_CHAR (including a surrogate pair and a BMP character), GBK varchar(max) to SQL_C_CHAR and to SQL_C_WCHAR, a UTF-8 collation column to SQL_C_WCHAR, and the GBK case again with the value split across four PLP chunks so every character boundary falls inside a chunk split. All passed with no lost, duplicated or reordered bytes and no stall.

Findings

Blocking

None.

Suggestion

None.

Nit

None.

This looks ready for human review, but this comment is not an approval — the FFI streaming state machine and the msodbcsql parity decisions still want a human reviewer's judgement, and the workspace-wide cargo btest / cargo bclippy checklist items are still open in the PR description.

[Copilot]

Copilot review overview

🟡 Changes recommended

One critical API compatibility issue and four moderate data-handling issues remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1

Open (1)

• Adding SqlDataType variant breaks exhaustive downstream matches · New

[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

Unattended re-review of the complete PR diff (6 files, +1372/-96) against merge-base 4a72e500 on head fa3f0f53. fa3f0f53 is a merge of main that carries no change to the PR's own files (git diff f812b4e3..fa3f0f53 touches only code and docs that arrived from main), so this pass re-verified the carry/retry machinery on the new base rather than re-reading unchanged commits. No new findings.

What I re-checked on this head, rather than trusting the earlier pass:

• Loop termination. Every retry requires read > 0 && !reached_end, so each pass consumes wire; the surrogate and narrow completions each clear the state that asked for the retry (or replay the newly started character through restore_source_prefix), and the widening refill is bounded by the output slots left in the caller's buffer.
• Buffer accounting across passes. Each arm's chunk_indicator is bounded by that pass's payload_capacity, which is recomputed after subtracting progress.written and prefix_bytes, so the SqlLen subtractions cannot go negative and be cast to a huge usize, and the offsets stay inside the caller's buffer.
• Carry ordering. Leftover pending_bytes after a partial prefix drain can only happen when prefix_bytes == text_capacity, which leaves the post-prefix payload_capacity at 0 — so no conversion arm can emit newer output ahead of an older, undelivered carry.
• restore_source_prefix. The replayed prefix is spliced ahead of any surviving prefetched tail, prefetched_total_read_before is rebased to total_read - len, and total_read / progress.wire_read are decremented in step, so the truncation indicator's total_read - progress.wire_read stays the pre-call consumed count.
• narrow_decoder_has_partial_character. Safe for every encoding lcid_to_encoding can reach (WINDOWS_125x/WINDOWS_874 SingleByte, plus GBK, BIG5, EUC_KR, SHIFT_JIS, UTF_8): in encoding_rs variant.rs those arms return None only when the decoder is non-neutral, never unconditionally, and new_decoder_without_bom_handling keeps the life cycle Converting.
• Odd-offset wide writes. copy_with_nul casts to *mut u8 for the bulk copy and uses write_unaligned for the terminator, and <*mut T>::add has no alignment precondition, so the odd prefix_bytes offset is sound.

I did not re-file findings already raised and answered here: the zero-capacity WCHAR probe indicator, stranded decoder input after output, sticky binary staging, unbounded completion retries on malformed input, the odd-offset alignment question, and the two mutation-proven test gaps (both now covered by mock GBK regressions).

The mock-server changes are additive: ColumnDefinition::collation has a Latin1 default and the struct is only built through new(), so existing callers are unaffected.

Verification

cargo nextest run -p mssqlodbc --lib api::get_data in the assigned isolated worktree with the assigned CARGO_TARGET_DIR: 121 passed, 0 failed.

Mutation proof of the newest commit's fix, since it is the one change without prior review history: dropping the null guard from the progress.written offset site (restoring the unconditional wrapping_add) makes all three new null-target regressions abort with 0xC0000005, and the guard restores them. That pins plp_null_target_survives_utf16_completion_after_output, ..._narrow_completion_after_output and ..._widening_refill as real guards rather than passing tests. Worktree restored afterwards.

Findings

Blocking

None.

Suggestion

None.

Nit

None.

This looks ready for human review, but this comment is not an approval — the FFI streaming state machine and the msodbcsql parity decisions still want a human reviewer's judgement. ADO validation on fa3f0f53 is still pending (9 pass / 8 pending, none failing), and the PR description still says the workspace-wide checklist items remain open while the checklist boxes are all ticked.

[ttk (Theekshna)]

Head moved only because main was merged in (fa3f0f53); byte-compared the PR's own diff against its merge-base before (f5c408b8..f812b4e3) and after (4a72e500..fa3f0f53) for every file the PR touches (get_data.rs, fetch_scroll.rs, stmt.rs, mock-tds protocol.rs/query_response.rs, README, e2e get_data_test.cpp) — content is identical, only diff-hunk line numbers shifted. No new PR-authored work since my last review at f812b4e3 (findings=0); that review's coverage stands. CI on this head is passing/pending with no failures, and the PR description already documents the merge and its own re-validation of 227 local tests accurately.

[Copilot]

Copilot review overview

🔵 Needs a closer look

Three moderate carry and replay defects remain unresolved.

Review effort: Balanced
Findings: None

Resolved since last review (1)

• Adding SqlDataType variant breaks exhaustive downstream 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

Unattended review of c706633a, the merge of main (through #631) into this branch. The prior automated pass was clean on fa3f0f53; this head is a conflict-resolving merge — git show --cc touches 5 files and ~1175 lines in get_data.rs alone — so this pass focused on the reconciliation itself rather than re-reading the eight PR commits underneath it.

The reconciliation looks right. Specifically re-derived rather than assumed:

• Refill bound. The widening-only payload_capacity / size_of::<SqlWChar>() became max_read_for(payload_capacity - chunk_written, 0, 0), with the gate broadened to transcode_utf16_to_utf8 || transcode_narrow_to_utf8 as well. The capacity arithmetic is exact: the next pass recomputes payload_capacity from buffer_length - progress.written, and since progress.written grows by prefix_bytes + chunk_written, the next pass's post-prefix capacity is precisely this pass's minus chunk_written. Zero remaining capacity maps to 0 on all three arms (narrow_max_read/utf16le_max_read return 0 when remaining == 0; widening floors to 0 units), so the loop cannot spin on an exhausted buffer. The refill only fires when both carries are empty, which — given emit = out_bytes.min(pending.len()) — makes read > 0 && chunk_written == 0 unreachable there, so each retry pass strictly consumes capacity.
• narrow_decoder_has_partial_character over ResolvedDecoder. The encoding_rs arm keeps the original latin1_byte_compatible_up_to(&[]) probe and its ASCII-compatibility debug_assert; the new Oem437/Oem850 arm returns false, which is correct — the table codec is single-byte and buffers no source. That also means the completion retry and the restore_source_prefix replay are inert for OEM, matching oem_plp_target_switch_preserves_carry_and_refills_text.
• Indicator merge. The PR's carry term and main's SqlLen::try_from overflow guard were combined rather than one dropped, and the transcode_narrow_to_utf8 arm was rebased from main's emitted_utf8 onto progress.written, which is the cross-pass equivalent. hex_scale still multiplies the carry term, but carry_before is provably 0 on a hex stream (the hex arm carries nothing between calls, and no conversion path populates pending_* for a Binary column), so that is unreachable rather than a double-count.
• ColumnDefinition::collation has a Latin1 default and the struct is only built through new(), so the mock additions stay source-compatible for existing callers; the SqlDataType/ColumnValue variant additions were already settled on this PR against the unpublished 0.2.0 boundary.

I did not re-file anything already raised and answered here — the zero-capacity WCHAR probe indicator, stranded decoder input after output, sticky binary staging, unbounded completion on malformed input, the odd-offset alignment question, the mock-crate API break, or the two earlier mutation-proven gaps (both now covered).

The PR description is accurate about this head, including the conflict-resolution summary and the adjusted truncation fixtures, and AB#48046 is linked and matches the change.

Verification

cargo nextest run -p mssqlodbc --lib --no-fail-fast in the assigned isolated worktree with the assigned CARGO_TARGET_DIR: 1712 passed, 0 failed.

Three mutations against the merge's own decisions, since those are the part without prior review history:

Mutation	Result
Narrow the refill gate back to widen_narrow_to_utf16 only	6 tests fail — well guarded
Drop held_converted_bytes from the known-length indicator	1712 pass — gap, see inline
Drop the pending_units term from the narrow indicator	1712 pass — gap, see inline

The worktree was restored after each mutation and is clean.

Findings

Blocking

None.

Suggestion

1. get_data.rs — the held_converted_bytes term in the known-length indicator has no regression test, and the path that needs it is reachable. Details inline.

Nit

1. get_data.rs — the pending_units half of the narrow indicator's pending count is likewise uncovered. Details inline.

Both findings are test coverage, not behavior: the implementation is what I would expect in each case. Nothing here blocks merging, but the first one is the kind of term that a later refactor silently drops.

CI on this head is still in progress (Test MacOS and coverage-report running; everything else green), so latest-head validation is not yet confirmed.

[Copilot]

Copilot review overview

🔵 Needs a closer look

The intricate stateful conversion changes and unconfirmed latest-head CI warrant final human review.

Review effort: Balanced
Findings: None

[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

This head (7c7f5021) adds only the two indicator regressions requested on the previous pass; the production diff is unchanged since c706633a. I reviewed the whole net diff against the merge base (92f8f041) rather than only the new commit: the split of stream_active_plp_chunk into an outer retry driver plus stream_active_plp_chunk_once, the pending_utf8 → pending_bytes rename and verbatim carry delivery before either text target, the bounded partial-character completion with restore_source_prefix replay, the widening refill, the null-target guards on both output-offset sites, ResolvedDecoder::has_pending_narrow_character, and the mock VarCharMax/collation support. No blocking or behavioral finding.

Things I re-derived rather than assumed, since they are the load-bearing parts of the new control flow: the outer loop terminates because every retry either strictly consumes output capacity or hands the newly started character back to the raw stream and sets retry_bytes = 0; progress.wire_read and the restore_source_prefix decrements keep consumed_before = total_read - progress.wire_read equal to the pre-call wire position across internal passes; the carry prefix either drains fully or sets prefix_fills_buffer, so pending_bytes never ends up holding newly converted output ahead of older carry; and has_pending_narrow_character is sound for every encoding that can actually reach it — latin1_byte_compatible_up_to returns None unconditionally only for REPLACEMENT/UTF_16LE/UTF_16BE (encoding_rs 0.8.35 variant.rs), which is exactly the set its is_ascii_compatible debug assert excludes, and narrow_encoding is built from EncodingType::LcidBased(...), which never yields them. The Finished lifecycle panic is likewise unreachable because every query is gated on !reached_end.

I did not re-file anything already raised and answered here: the zero-capacity WCHAR probe indicator, stranded decoder input after prior output, sticky binary staging, unbounded completion on malformed input, the odd-offset alignment question, the mock crate's 0.2.0 API break, or the two mutation-proven indicator gaps that this head closes. The description matches the diff and AB#48046 is linked.

Verification

Read-only pass plus targeted source checks in the assigned worktree: full net diff against the merge base, encoding_rs 0.8.35 lib.rs/variant.rs for latin1_byte_compatible_up_to semantics, lcid_encoding.rs/sql_string.rs for which encodings can reach the narrow decoder, and gh pr checks (every completed required check green on this head; the coverage merge and the mssql-python macOS stage are still running, none failing).

I did not re-run the crate suite or re-run mutations on this head — the two mutations from the previous pass are exactly what 7c7f5021 adds coverage for, and the run budget went to reading the merged control flow instead. So latest-head test evidence here is CI's, not mine.

Findings

Blocking

None.

Suggestion

None.

Nit

1. mssql-odbc/src/api/get_data.rs — narrow_decoder_has_partial_character is a pass-through wrapper that can be dropped. Details inline.