Skip to content

Commit 028da64

Browse files
author
Shiwani Gupta
committed
mssql-odbc: declare and bound data-at-execution parameters from ParameterType
SQLBindParameter's ParameterType and ColumnSize were ignored for data-at-execution parameters: every value streamed as a `max` type whatever the application declared, and no length bound was enforced. Declaration follows ParameterType. `dae_plan` decides stream-vs-buffer from the SQL type's PLP-ability, mirroring msodbcsql's IsPartialLenType (sqlcprot.h:1421), which keys on the SQL type alone rather than on ColumnSize. A fixed-width target is now collected and converted whole through the materializing path instead of being forced into a `max` stream it cannot represent. ColumnSize bounds the value. `dae_streamed_declaration` narrows the @params declaration and `dae_length_limit` enforces the accumulated total in SQLPutData, applied before the streamed/buffered split and against the application buffer -- where msodbcsql runs ValidatePutDataLength (sqlccmd.cpp:4571), so 22001 lands on the call that overflows rather than at close. Trailing pad units are trimmed rather than rejected, including one split across two calls, and trimmed padding does not consume the declaration's budget. A narrow buffer is measured in UTF-16 units, the unit the materialized path uses, so the two paths agree on what fits. The declaration is only narrowed when that bound can be enforced: a streamed parameter never reaches a close-time conversion, so promising a length nothing checks would leave the overflow to the server. Transcoding is streamed, not deferred. `DaeTranscode` converts each chunk on the way out, carrying a trailing partial character into the next call -- the shape of msodbcsql's ConvertLongData (sqlccnvt.cpp:841) with its cbTruncatedCharsInConvBuf carry and end-of-value flush (sqlccmd.cpp:5999-6001). Narrow encoding reuses encode_narrow, so the streamed and materialized paths agree on the wire bytes. A mixed sequence collects only what it must. Parameters are offered in bind order, as msodbcsql offers them, and the RPC opens as soon as no parameter still to be visited needs collecting -- so a PLP-capable value bound after a fixed-width one streams rather than being held whole. text/ntext/image keep their `max` substitution, tracked under AB#47592. AB#47590
1 parent ce029d8 commit 028da64

14 files changed

Lines changed: 2723 additions & 613 deletions

File tree

.github/instructions/mssql-odbc.instructions.md

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -353,6 +353,22 @@ does not grow every time a new msodbcsql build is measured.
353353
`ColAttributeLiveTest.EmptyVariantProbeConsumesValueButKeepsBaseType`
354354
accepts either so the parity leg still compares the base type and the
355355
`SQL_NO_DATA` re-read.
356+
14. **A cross-family data-at-execution parameter is converted rather than
357+
refused.** `SQL_C_CHAR`/`SQL_C_WCHAR` against a fixed-width target such as
358+
`SQL_INTEGER` is collected whole and converted by the same code the
359+
materialized path uses, so text supplied in chunks parses to an integer
360+
exactly as it does when bound directly. msodbcsql accepts the pairing at
361+
`SQLExecute` and then refuses it at `SQLPutData` with `HY019` ("Processing
362+
of fixed length targets cannot be spread over multiple calls to
363+
SQLPutData"), so a value this driver sends is one msodbcsql rejects.
364+
Deliberate: `ParameterType` decides how a data-at-execution parameter is
365+
declared, and a pairing the materialized path can convert should not become
366+
unsupported merely because the value arrives in chunks. Note this *widens*
367+
what is accepted — an application relying on the refusal gets a successful
368+
execute instead, though only for a binding msodbcsql never let complete
369+
either. `CrossFamilyDataAtExecutionConvertsToInteger` (in both
370+
`execute_test.cpp` and `param_conversions_test.cpp`) carries
371+
`SKIP_IF_COMPARING_MSODBCSQL()`. Tracked in AB#47590.
356372

357373
## No panics
358374

mssql-odbc/src/api/cancel.rs

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -87,19 +87,13 @@ unsafe fn sql_cancel_impl(statement_handle: SqlHandle) -> SqlReturn {
8787
#[cfg(test)]
8888
mod tests {
8989
use super::*;
90-
use crate::api::odbc_types::{SQL_C_CHAR, SQL_INVALID_HANDLE, SQL_VARCHAR};
90+
use crate::api::odbc_types::SQL_INVALID_HANDLE;
9191
use crate::handles::stmt::{DaeParam, DaeState, STMT_STATE_EXEC_STARTED};
9292
use crate::test_support::TestHandles;
9393

9494
fn dae_with_one_param(cursor: Option<usize>) -> DaeState {
9595
DaeState::for_test(
96-
vec![DaeParam {
97-
value_ptr: std::ptr::null_mut(),
98-
expected_len: None,
99-
needs_transcode: false,
100-
c_type: SQL_C_CHAR,
101-
sql_type: SQL_VARCHAR,
102-
}],
96+
vec![DaeParam::unbounded(0, std::ptr::null_mut(), None)],
10397
cursor,
10498
)
10599
}

mssql-odbc/src/api/exec_common.rs

Lines changed: 246 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ use mssql_tds::connection::tds_client::{
1616
CursorPoll, ExecuteOptions, ResultSet, StatementId, TdsClient,
1717
};
1818
use mssql_tds::error::{Error as TdsError, TimeoutErrorType};
19-
use mssql_tds::message::parameters::rpc_parameters::RpcParameter;
19+
use mssql_tds::message::parameters::rpc_parameters::{RpcParameter, StreamedSqlType};
2020

2121
use super::ird::populate_ird;
2222
use super::sqlstate::*;
@@ -25,7 +25,8 @@ use crate::api::odbc_types::{
2525
SQL_SUCCESS_WITH_INFO, SqlHandle, SqlLen, SqlReturn,
2626
};
2727
use crate::conversion::param_convert::{
28-
ParamBuildError, bound_param_to_rpc, dae_placeholder_type, is_data_at_exec_indicator,
28+
DaePlan, DaeTranscode, ParamBuildError, bound_param_to_rpc, buffered_dae_to_rpc,
29+
dae_length_limit, dae_plan, dae_streamed_declaration, is_data_at_exec_indicator,
2930
};
3031
use crate::error::post_sql_error;
3132
use crate::handles::dbc::ConnectionState;
@@ -124,9 +125,29 @@ pub(super) fn park_dae_client(
124125
client: TdsClient,
125126
prepared: Option<PreparedPlan>,
126127
orphaned: Option<StatementId>,
127-
dae_params: Vec<DaeParam>,
128+
mut dae_params: Vec<DaeParam>,
128129
op: &str,
129130
) -> SqlReturn {
131+
// The wire encoding of a narrow target comes from the database collation,
132+
// which is only knowable with the connection in hand — `write_streamed_chunk`
133+
// writes bytes verbatim, so the re-encoding has to happen before them
134+
// (AB#47590).
135+
let collation = client.get_collation();
136+
for param in &mut dae_params {
137+
if !param.plan.is_buffered() {
138+
let transcode =
139+
DaeTranscode::new(param.binding.c_type, param.binding.sql_type, collation);
140+
// A pairing whose buffer bytes already are the wire's bytes keeps
141+
// `None`, so `SQLPutData` forwards its chunks borrowed instead of
142+
// copying each one through a conversion that would return them
143+
// unchanged. Skipping the transcode also skips the close-time
144+
// flush, which is right: a passthrough parameter never holds back a
145+
// partial character to carry.
146+
if !transcode.is_passthrough() {
147+
param.transcode = Some(transcode);
148+
}
149+
}
150+
}
130151
let Ok(mut stmt_state) = stmt.inner.lock() else {
131152
// The client has nowhere to go: the statement that owns it is
132153
// unreachable and the DBC still records it as busy.
@@ -137,6 +158,35 @@ pub(super) fn park_dae_client(
137158
SQL_NEED_DATA
138159
}
139160

161+
/// Parks a sequence whose execute is deferred: at least one parameter buffers,
162+
/// so no RPC has been opened and the client sits idle on the statement until
163+
/// the last `SQLParamData` builds the complete parameter list and runs it.
164+
///
165+
/// The connection still counts as busy for the duration, exactly as the
166+
/// streaming sequence does, so an application cannot start another command on it
167+
/// mid-sequence.
168+
#[allow(clippy::too_many_arguments)]
169+
pub(super) fn park_deferred_dae(
170+
stmt: &StmtHandle,
171+
client: TdsClient,
172+
prepared: Option<PreparedPlan>,
173+
orphaned: Option<StatementId>,
174+
dae_params: Vec<DaeParam>,
175+
prebuilt: Vec<RpcParameter>,
176+
sql: Option<String>,
177+
timeout_secs: u32,
178+
op: &str,
179+
) -> SqlReturn {
180+
let Ok(mut stmt_state) = stmt.inner.lock() else {
181+
error!("{op}: stmt mutex poisoned while parking deferred DAE state");
182+
return SQL_ERROR;
183+
};
184+
stmt_state.dae = Some(
185+
DaeState::new(client, prepared, orphaned, dae_params).deferred(prebuilt, sql, timeout_secs),
186+
);
187+
SQL_NEED_DATA
188+
}
189+
140190
/// Aborts a data-at-execution sequence with a diagnostic. Always `SQL_ERROR`.
141191
pub(super) fn abort_dae_with_diag(
142192
dbc: &DbcHandle,
@@ -644,27 +694,88 @@ pub(super) unsafe fn build_named_params(
644694
};
645695

646696
if let Some(indicator) = dae_indicator {
647-
let dae_stream = match dae_placeholder_type(bound_param.c_type, bound_param.sql_type) {
648-
Ok(t) => t,
697+
let plan = match dae_plan(bound_param.c_type, bound_param.sql_type) {
698+
Ok(plan) => plan,
649699
Err(e) => {
650700
error!(
651-
"{op}: parameter {} DAE type not streamable: {}",
701+
"{op}: parameter {} cannot be supplied at execution: {}",
652702
i + 1,
653703
e.diag().text
654704
);
655705
post_diag(stmt_state, e.diag());
656706
return Err(SQL_ERROR);
657707
}
658708
};
659-
let rpc =
660-
RpcParameter::data_at_exec(Some(name), StatusFlags::NONE, dae_stream.sql_type);
661-
dae_params.push(DaeParam {
662-
value_ptr: bound_param.parameter_value_ptr,
663-
expected_len: dae_expected_length(indicator),
664-
needs_transcode: dae_stream.needs_transcode,
665-
c_type: bound_param.c_type,
666-
sql_type: bound_param.sql_type,
667-
});
709+
// A bound that can be measured in buffer bytes is applied as the
710+
// chunks arrive, so an overflow is reported by the `SQLPutData`
711+
// that carries it rather than at close (msodbcsql parity).
712+
let length_limit = match dae_length_limit(
713+
bound_param.c_type,
714+
bound_param.sql_type,
715+
bound_param.column_size,
716+
) {
717+
Ok(limit) => limit,
718+
Err(e) => {
719+
error!(
720+
"{op}: parameter {} ColumnSize invalid: {}",
721+
i + 1,
722+
e.diag().text
723+
);
724+
post_diag(stmt_state, e.diag());
725+
return Err(SQL_ERROR);
726+
}
727+
};
728+
dae_params.push(DaeParam::new(
729+
i,
730+
dae_expected_length(indicator),
731+
plan,
732+
length_limit,
733+
bound_param,
734+
));
735+
// Nothing to declare for a buffered parameter yet: its bytes are
736+
// not in, so its type and length are not known. The slot is filled
737+
// to keep parameter positions lined up and is rebuilt by
738+
// `rebuild_deferred_params` before anything reaches the wire.
739+
let rpc = match plan {
740+
DaePlan::Stream(streamed) => {
741+
let param = RpcParameter::data_at_exec(Some(name), StatusFlags::NONE, streamed);
742+
// The body is PLP whatever `ColumnSize` says, but the
743+
// variable it lands in is declared from `ParameterType`,
744+
// matching the materialized path and msodbcsql (AB#47590).
745+
//
746+
// Only narrowed when the bound above can actually be
747+
// enforced. A streamed parameter never reaches a close-time
748+
// conversion, so an unenforced bound would have the client
749+
// declare `varchar(n)` and then stream past it, leaving the
750+
// overflow to the server instead of the `22001` the
751+
// declaration implies. Staying `max` keeps the value intact
752+
// until the bound is measurable on this path too
753+
// (AB#47590).
754+
let declaration = if length_limit.is_some() {
755+
dae_streamed_declaration(bound_param.sql_type, bound_param.column_size)
756+
} else {
757+
Ok(None)
758+
};
759+
match declaration {
760+
Ok(Some(declaration)) => param.with_streamed_declaration(declaration),
761+
Ok(None) => param,
762+
Err(e) => {
763+
error!(
764+
"{op}: parameter {} declaration invalid: {}",
765+
i + 1,
766+
e.diag().text
767+
);
768+
post_diag(stmt_state, e.diag());
769+
return Err(SQL_ERROR);
770+
}
771+
}
772+
}
773+
DaePlan::Buffer => RpcParameter::data_at_exec(
774+
Some(name),
775+
StatusFlags::NONE,
776+
StreamedSqlType::VarBinaryMax,
777+
),
778+
};
668779
params.push(rpc);
669780
} else {
670781
match unsafe { bound_param_to_rpc(name, &bound_param) } {
@@ -690,6 +801,66 @@ pub(super) unsafe fn build_named_params(
690801
Ok(ParamsWithDae { params, dae_params })
691802
}
692803

804+
/// Rebuilds the RPC parameter list once every data-at-execution value has been
805+
/// collected, for a sequence that deferred its execute.
806+
///
807+
/// Only the data-at-execution slots are rebuilt, from the bytes
808+
/// [`buffered_dae_to_rpc`] converts; every other parameter is the one
809+
/// `build_named_params` already materialized at execute time and is kept
810+
/// verbatim. A buffered value therefore still goes through the same conversion
811+
/// the materialized path uses, so it is declared and bounded exactly like a
812+
/// value supplied in a single buffer (AB#47590).
813+
///
814+
/// Nothing here reads application memory. Re-reading it would be unsound rather
815+
/// than merely redundant: `bound_params` is an execute-time snapshot that
816+
/// `SQLFreeStmt(SQL_RESET_PARAMS)` leaves in place while releasing the bindings
817+
/// it describes, so an application that resets its parameters mid-sequence --
818+
/// which this driver explicitly supports -- would have freed buffers
819+
/// dereferenced here.
820+
pub(super) fn rebuild_deferred_params(
821+
stmt_state: &mut StmtState,
822+
prebuilt: Vec<RpcParameter>,
823+
collected: &[(usize, Vec<u8>, bool)],
824+
dae_params: &[DaeParam],
825+
op: &str,
826+
) -> Result<Vec<RpcParameter>, SqlReturn> {
827+
let mut params = prebuilt;
828+
829+
for (index, bytes, is_null) in collected {
830+
let Some(dae) = dae_params.iter().find(|p| p.bound_index == *index) else {
831+
error!(
832+
"{op}: collected value for parameter {} has no binding",
833+
index + 1
834+
);
835+
post_diag(stmt_state, ERR_UNBOUND_PARAMETER);
836+
return Err(SQL_ERROR);
837+
};
838+
let Some(slot) = params.get_mut(*index) else {
839+
error!(
840+
"{op}: collected value for parameter {} has no slot",
841+
index + 1
842+
);
843+
post_diag(stmt_state, ERR_UNBOUND_PARAMETER);
844+
return Err(SQL_ERROR);
845+
};
846+
let name = format!("@P{}", index + 1);
847+
match buffered_dae_to_rpc(name, &dae.binding, bytes, *is_null) {
848+
Ok(param) => *slot = param,
849+
Err(e) => {
850+
error!(
851+
"{op}: parameter {} conversion failed: {}",
852+
index + 1,
853+
e.diag().text
854+
);
855+
post_diag(stmt_state, e.diag());
856+
return Err(SQL_ERROR);
857+
}
858+
}
859+
}
860+
861+
Ok(params)
862+
}
863+
693864
/// Captures result metadata after a successful execution and finalizes the
694865
/// statement/connection state.
695866
///
@@ -1637,16 +1808,9 @@ mod tests {
16371808

16381809
let dae = unsafe { build_named_params(&mut state, 3, "test") }.unwrap();
16391810
assert_eq!(dae.params.len(), 3);
1640-
assert_eq!(
1641-
dae.dae_params,
1642-
vec![DaeParam {
1643-
value_ptr: std::ptr::null_mut(),
1644-
expected_len: None,
1645-
needs_transcode: false,
1646-
c_type: SQL_C_CHAR,
1647-
sql_type: SQL_VARCHAR
1648-
}]
1649-
);
1811+
assert_eq!(dae.dae_params.len(), 1);
1812+
assert_eq!(dae.dae_params[0].bound_index, 1);
1813+
assert_eq!(dae.dae_params[0].expected_len, None);
16501814
}
16511815

16521816
/// `SQL_LEN_DATA_AT_EXEC(n)` promises `n` bytes, which the closing
@@ -1672,16 +1836,63 @@ mod tests {
16721836
}));
16731837

16741838
let dae = unsafe { build_named_params(&mut state, 1, "test") }.unwrap();
1675-
assert_eq!(
1676-
dae.dae_params,
1677-
vec![DaeParam {
1678-
value_ptr: std::ptr::null_mut(),
1679-
expected_len: Some(7),
1680-
needs_transcode: false,
1681-
c_type: SQL_C_CHAR,
1682-
sql_type: SQL_VARCHAR
1683-
}]
1684-
);
1839+
assert_eq!(dae.dae_params.len(), 1);
1840+
assert_eq!(dae.dae_params[0].bound_index, 0);
1841+
assert_eq!(dae.dae_params[0].expected_len, Some(7));
1842+
}
1843+
1844+
/// A streamed parameter is only declared at a narrowed length when the
1845+
/// bound that length implies is one `SQLPutData` can actually enforce.
1846+
///
1847+
/// The two decisions are made independently, so nothing but this guard stops
1848+
/// them disagreeing. Where they would, the cost is real: a streamed value
1849+
/// never reaches the close-time conversion that bounds a buffered one, so a
1850+
/// narrowed declaration with no bound behind it has the client promise the
1851+
/// server a length and then stream past it.
1852+
#[test]
1853+
fn build_named_params_only_narrows_a_declaration_it_can_enforce() {
1854+
for (c_type, sql_type, expect_narrowed) in [
1855+
// Same unit on both sides: measurable, so narrowing is honoured.
1856+
(SQL_C_CHAR, SQL_VARCHAR, true),
1857+
// A wideness mismatch is measurable too - the unit is the
1858+
// declaration's and the count is of the source.
1859+
(crate::api::odbc_types::SQL_C_WCHAR, SQL_VARCHAR, true),
1860+
(SQL_C_CHAR, crate::api::odbc_types::SQL_WVARCHAR, true),
1861+
// Cross-family is not: a binary byte is not a character, so the
1862+
// declaration stays `max` rather than claiming a bound.
1863+
(SQL_C_CHAR, crate::api::odbc_types::SQL_VARBINARY, false),
1864+
] {
1865+
let h = TestHandles::with_env_dbc_stmt();
1866+
let stmt = unsafe { handle_from_raw::<StmtHandle>(h.stmt) };
1867+
1868+
let mut ind: SqlLen = SQL_DATA_AT_EXEC;
1869+
let mut state = stmt.inner.lock().unwrap();
1870+
state.bound_params.push(Some(BoundParam {
1871+
input_output_type: SQL_PARAM_INPUT,
1872+
c_type,
1873+
sql_type,
1874+
column_size: 10,
1875+
decimal_digits: 0,
1876+
parameter_value_ptr: std::ptr::null_mut(),
1877+
buffer_length: 0,
1878+
strlen_or_ind_ptr: &mut ind as *mut SqlLen,
1879+
octet_length_ptr: &mut ind as *mut SqlLen,
1880+
}));
1881+
1882+
let dae = unsafe { build_named_params(&mut state, 1, "test") }.unwrap();
1883+
let bounded = dae.dae_params[0].length_limit.is_some();
1884+
assert_eq!(
1885+
bounded, expect_narrowed,
1886+
"{c_type} -> {sql_type}: the bound decides whether narrowing is honest"
1887+
);
1888+
// The declaration is only narrowed when the bound backs it, so the
1889+
// two travel together rather than being decided apart.
1890+
let narrowed = format!("{:?}", dae.params[0]).contains("streamed_declaration: Some");
1891+
assert_eq!(
1892+
narrowed, expect_narrowed,
1893+
"{c_type} -> {sql_type}: declaration narrowing must follow the bound"
1894+
);
1895+
}
16851896
}
16861897

16871898
/// Without an indicator pointer there is nothing to carry a

0 commit comments

Comments
 (0)