diff --git a/.github/workflows/build.yaml b/.github/workflows/build.yaml index 60a6fd1..eeb6a0a 100644 --- a/.github/workflows/build.yaml +++ b/.github/workflows/build.yaml @@ -261,6 +261,8 @@ jobs: run: cargo +nightly fuzz run utf16 --target x86_64-unknown-linux-gnu -- -max_total_time=30 - name: Fuzz column_value run: cargo +nightly fuzz run column_value --target x86_64-unknown-linux-gnu -- -max_total_time=30 + - name: Fuzz parse_attributes + run: cargo +nightly fuzz run parse_attributes --target x86_64-unknown-linux-gnu -- -max_total_time=30 # Verifies the crate can actually be packaged, without publishing anything. # diff --git a/AGENTS.md b/AGENTS.md index 98ecfca..2010600 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -1324,20 +1324,35 @@ let ptr = unsafe { arena.as_mut_ptr().cast::().add(1) }.cast::(); ### Fuzzing `fuzz/` holds [cargo-fuzz](https://github.com/rust-fuzz/cargo-fuzz) targets for -the memory-marshalling hot paths, `write_column_value` and `utf16`. Each -allocates its output buffer at exactly the caller-declared length, so -AddressSanitizer catches any overrun that clippy cannot see. It is its own -Cargo workspace, because libFuzzer needs nightly, so the root build ignores it. -A short smoke run of both targets runs on every PR (the `fuzz` job in -`build.yaml`). +the raw-pointer paths: `write_column_value`, `utf16` and +`ffi::setup::parse_attributes_w`. The first two allocate their output buffer at +exactly the caller-declared length, so AddressSanitizer catches any overrun that +clippy cannot see; the third walks a Driver-Manager pointer looking for a +terminator. It is its own Cargo workspace, because libFuzzer needs nightly, so +the root build ignores it. A short smoke run of every target runs on every PR +(the `fuzz` job in `build.yaml`). ```bash cargo install cargo-fuzz cargo +nightly fuzz run utf16 cargo +nightly fuzz run column_value +cargo +nightly fuzz run parse_attributes ``` -See [`fuzz/README.md`](fuzz/README.md) for what is and is not worth fuzzing. +**A fuzz target is for `unsafe` code.** Where the code is safe Rust the worst +outcome is a panic or a wrong answer, and a `proptest` suite next to the code +finds both on stable, in far less CPU time, on every `cargo test` rather than in +a 30-second smoke run. `escape`, `types::connect_params`, `param_convert`, +`numeric_convert` and `types::conversions` are covered that way, with an oracle +rather than only a never-panics assertion wherever one exists. + +Reaching a `pub(crate)` item from `fuzz/`, which is a separate crate, goes +through a wrapper gated on the default-off `test-support` feature. +`parse_attributes_summary_w` is the example to copy. + +See [`fuzz/README.md`](fuzz/README.md) for what is and is not worth fuzzing, and +for why a fuzz target must terminate the buffer it hands to a parser whose +contract says it is terminated. ### Benchmarks diff --git a/CHANGELOG.md b/CHANGELOG.md index ec88b2f..0242412 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,22 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- `test_support::parse_attributes_summary_w`, behind the default-off + `test-support` feature. It reports what the `ConfigDSNW` attribute-list parser + read from a raw `*const u16`, so the new `parse_attributes` fuzz target can + reach a `pub(crate)` parser from a separate crate. A driver has no reason to + call it, and a build without the feature does not export it. +- A `parse_attributes` fuzz target, covering that parser over aligned, + deliberately misaligned and overlong-segment buffers. It joins the per-PR + ASAN smoke run. +- Property tests for the paths that carry no `unsafe` and so are not worth a + fuzz target: the escape translator against a grammar of escape tokens and four + dialects, `ConnectParams` round-tripping a whole connection string rather than + one pair, the two parameter-conversion tables, and every `*_from_raw` swept + across its entire input domain. + ## [0.1.0] — 2026-08-04 First release, so this section describes what the crate offers rather than what diff --git a/Cargo.toml b/Cargo.toml index b2302ec..39d825c 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -39,6 +39,9 @@ exclude = [ "rustfmt.toml", ".pre-commit-config.yaml", ".markdownlint.yaml", + # Saved `proptest` failure seeds. Test-only data, and the tests that read it + # are not compiled from the published tarball. + "proptest-regressions/", ] # docs.rs builds for a single target by default, which would leave `ConfigDSNW` @@ -52,14 +55,18 @@ targets = ["x86_64-pc-windows-msvc", "x86_64-unknown-linux-gnu"] features = ["test-support"] [features] -# Test support for driver crates: the `conformance` module, which drives -# `SQLGetInfoW` through the real C ABI to check an info type's return shape. +# Test support for driver crates and for `fuzz/`. Two things: +# +# - the `conformance` module, which drives `SQLGetInfoW` through the real C ABI +# to check an info type's return shape; +# - `test_support::parse_attributes_summary_w`, which lets the `parse_attributes` +# fuzz target reach a `pub(crate)` raw-pointer parser from a separate crate. # # Default-off because it is test code. Compiled unconditionally it lands in # every driver's production binary, and it reaches an `unreachable!()` through a # public `unsafe fn` taking a caller-supplied `u16` — a panic path a shipped # driver has no reason to carry. Driver test suites enable it under -# `[dev-dependencies]`. +# `[dev-dependencies]`; `fuzz/Cargo.toml` enables it on its path dependency. test-support = [] [dependencies] diff --git a/fuzz/Cargo.toml b/fuzz/Cargo.toml index fbee2c8..40625d7 100644 --- a/fuzz/Cargo.toml +++ b/fuzz/Cargo.toml @@ -25,7 +25,16 @@ test = false doc = false bench = false +[[bin]] +name = "parse_attributes" +path = "fuzz_targets/parse_attributes.rs" +test = false +doc = false +bench = false + [dependencies] arbitrary = { version = "1", features = ["derive"] } libfuzzer-sys = "0.4" -stackable-odbc-core = { path = ".." } +# `test-support` is what makes `ffi::setup::parse_attributes_summary_w` visible. +# The attribute parser itself is `pub(crate)`, and this is a separate crate. +stackable-odbc-core = { path = "..", features = ["test-support"] } diff --git a/fuzz/README.md b/fuzz/README.md index f2515f1..8b442ec 100644 --- a/fuzz/README.md +++ b/fuzz/README.md @@ -1,25 +1,58 @@ # Fuzz targets -These targets cover the `unsafe` pointer-marshalling paths in -`stackable-odbc-core`, which is where AddressSanitizer catches what clippy -cannot see. Each one allocates its output buffer at exactly the size a correct -application would, so any write past the end is a reported error rather than a -silent overrun into neighbouring memory. +These targets cover the `unsafe` raw-pointer paths in `stackable-odbc-core`, +which is where AddressSanitizer catches what clippy cannot see. -That size is the `BufferLength` argument for a variable-length C target, and the -C type's own size for a fixed-length one, which ignores `BufferLength` -altogether. +The two marshalling targets allocate their output buffer at exactly the size a +correct application would, so any write past the end is a reported error rather +than a silent overrun into neighbouring memory. That size is the `BufferLength` +argument for a variable-length C target, and the C type's own size for a +fixed-length one, which ignores `BufferLength` altogether. + +The third goes the other way: it hands a parser an *input* buffer sized exactly +to its contents, so a read that runs past the terminator is reported rather than +finding more of the same allocation. - `utf16` covers `utf16_to_string` and `write_utf16`. - `column_value` covers `write_column_value` across every marshallable value variant and C target type, which is the full coercion matrix. +- `parse_attributes` covers `ffi::setup::parse_attributes_w`, the attribute-list + walk behind `ConfigDSNW`, over aligned, deliberately misaligned and + overlong-segment buffers. + +## What belongs here, and what does not + +The line is `unsafe`, not importance. A target earns its nightly toolchain and +its ASAN build when the failure it is hunting is a read or write outside an +allocation. Where the code is safe Rust, the worst outcome is a panic or a wrong +answer, and a property test finds both on stable, in a fraction of the CPU time, +on every PR rather than in a 30-second smoke run. + +So the pure-safe parsers are covered by [`proptest`](https://docs.rs/proptest) +suites next to the code instead, asserting never-panics *and* an oracle wherever +one exists: -The pure-safe parsers, `translate_escapes`, `ConnectParams::parse` and the -drivers' own type-name parsers, contain no `unsafe`, so AddressSanitizer adds -nothing over property tests. They are covered by -[`proptest`](https://docs.rs/proptest) suites next to the code, which run on -stable under an ordinary `cargo test` and assert both never-panics and -round-trip invariants. +| Module | What the properties assert | +| --- | --- | +| `escape` | A grammar of escape-ish tokens against four dialects; an unknown escape and its surroundings survive byte for byte | +| `types::connect_params` | Every pair survives `parse` ∘ `to_connection_string`, and no keyword is injected by a value containing `}` | +| `param_convert` | Rendering never expands past `MAX_DECIMAL_EXPANSION_DIGITS`; rendering round-trips; `to_integer` agrees with `i128::from_str`; a `SQL_NUMERIC_STRUCT` reconstructs its literal | +| `numeric_convert` | The whole *C to SQL: Numeric* table is total; integers reach their target exactly when they are in range | +| `types::conversions` | Every `*_from_raw` swept across its entire 16-bit domain, round-tripping | + +`parse_attributes` is on this side of the line because it steps a raw `*const +u16` looking for a terminator, and because the Driver Manager gives it no +alignment guarantee. + +### Terminate the buffer + +The parser's safety contract is that the pointer is null or double-null +terminated. A fuzz target that hands it an unterminated buffer will get an ASAN +report, and the report will be about the target: the read past the end is the +caller breaking a contract, not the parser exceeding one. `parse_attributes` +appends the terminator itself and fuzzes what comes before it, and reaches the +per-segment scan limit with a run that is long but still inside a real +allocation. ## Running @@ -30,8 +63,14 @@ libFuzzer does. cargo install cargo-fuzz cargo +nightly fuzz run utf16 cargo +nightly fuzz run column_value +cargo +nightly fuzz run parse_attributes ``` +`parse_attributes` reaches `parse_attributes_w`, which is `pub(crate)`, through +`test_support::parse_attributes_summary_w`. That wrapper is gated behind the +default-off `test-support` feature, which this crate enables on its dependency, +so a shipped driver never exports it. + If cargo-fuzz fails with "sanitizer is incompatible with statically linked libc", it picked a musl target. Pin the gnu triple explicitly, which is what CI does: diff --git a/fuzz/fuzz_targets/parse_attributes.rs b/fuzz/fuzz_targets/parse_attributes.rs new file mode 100644 index 0000000..586b24a --- /dev/null +++ b/fuzz/fuzz_targets/parse_attributes.rs @@ -0,0 +1,120 @@ +//! `ConfigDSNW`'s attribute-list parser, driven over raw pointers. +//! +//! `test_support::parse_attributes_summary_w` wraps +//! `ffi::setup::parse_attributes_w`, which walks a `*const u16` the Driver +//! Manager supplied, hunting for the double null that ends the list. It is the +//! one parser in core whose failure mode is a read past the end of an +//! allocation rather than a panic, which is what makes AddressSanitizer worth +//! the nightly toolchain here. +//! +//! # Every buffer is terminated +//! +//! The parser's safety contract is that the pointer is null, or points to a +//! valid double-null-terminated `u16` sequence. So each shape below appends that +//! terminator itself and fuzzes what comes *before* it. Handing the parser an +//! unterminated buffer would certainly produce an ASAN report, and it would be a +//! report about this file: the read past the end would be the caller breaking a +//! contract it agreed to, not the parser exceeding one. What is worth fuzzing is +//! the walk over a buffer that is terminated but says nothing else sensible. +//! +//! The parser bounds each segment at `i16::MAX` code units precisely so a caller +//! that gets this wrong is contained rather than unbounded, and the +//! `OverlongSegment` shape reaches that bound from inside a real allocation. + +#![no_main] + +use arbitrary::Arbitrary; +use libfuzzer_sys::fuzz_target; +use stackable_odbc_core::test_support::parse_attributes_summary_w; + +/// One code unit past the parser's own per-segment scan limit, which is +/// `i16::MAX`. Declared here rather than imported because it is crate-private, +/// and a copy that drifts makes this shape stop reaching the bound rather than +/// start failing, so the assertion below checks the bound was actually hit. +const PAST_SEGMENT_SCAN_LIMIT: usize = i16::MAX as usize + 1; + +#[derive(Arbitrary, Debug)] +enum Shape { + /// A `u16`-aligned buffer, which is the ordinary case. + Aligned, + /// A buffer whose `u16` sequence starts at an odd byte address. + /// + /// The Driver Manager promises no alignment, and the parser reads every code + /// unit with `read_unaligned` for that reason. An aligned read of this + /// pointer is undefined behaviour, and in a debug build it aborts without + /// unwinding, which no panic hook can contain. A regression to + /// `slice::from_raw_parts` would be caught here and nowhere else. + Unaligned, + /// A segment longer than the parser will scan, so the scan limit fires with + /// every read still inside the allocation. + OverlongSegment, +} + +#[derive(Arbitrary, Debug)] +struct Input { + shape: Shape, + units: Vec, +} + +fuzz_target!(|input: Input| { + match input.shape { + Shape::Aligned => { + let mut buf = input.units; + buf.extend_from_slice(&[0, 0]); + // SAFETY: `buf` is non-empty and ends in two zero `u16`s, so it is a + // double-null-terminated sequence, and it outlives the call. + let _ = unsafe { parse_attributes_summary_w(buf.as_ptr()) }; + } + + Shape::Unaligned => { + // One byte of padding in front, so the `u16` sequence begins at an + // odd address. Everything after it stays a whole number of code + // units, which keeps the terminator two aligned-to-the-sequence + // zeros rather than a split pair. + let mut bytes = vec![0u8]; + for unit in &input.units { + bytes.extend_from_slice(&unit.to_le_bytes()); + } + bytes.extend_from_slice(&[0, 0, 0, 0]); + + // SAFETY: offset 1 is inside `bytes`, which holds `1 + 2n + 4` + // bytes, so the sequence from there is `n + 2` whole `u16`s ending + // in two zeros. The pointer is read only with `read_unaligned`, so + // the odd address is sound, and `bytes` outlives the call. + let ptr = unsafe { bytes.as_ptr().add(1) }.cast::(); + let _ = unsafe { parse_attributes_summary_w(ptr) }; + } + + Shape::OverlongSegment => { + // The fuzzed units come first, so their bytes still drive real + // segments, and the overlong run is appended behind a separator. + let mut buf = input.units; + buf.push(0); + + // Decided on the prefix alone, before the run and the terminator are + // appended: those end the list by construction, so a check made + // after them would be true every time and assert nothing. + let ends_early = ends_the_list(&buf); + + buf.resize(buf.len() + PAST_SEGMENT_SCAN_LIMIT, u16::from(b'A')); + buf.extend_from_slice(&[0, 0]); + + // SAFETY: as the aligned case; `buf` ends in two zero `u16`s. + let (_, _, syntax_error) = unsafe { parse_attributes_summary_w(buf.as_ptr()) }; + + // Where the run is reached at all, the scan limit must have fired. + assert!( + syntax_error || ends_early, + "a segment of {PAST_SEGMENT_SCAN_LIMIT} code units must trip the scan limit" + ); + } + } +}); + +/// Whether the parser stops inside `units` rather than walking off its end. +/// +/// It stops at an empty segment, which is a leading null or two consecutive +/// ones. +fn ends_the_list(units: &[u16]) -> bool { + units.first() == Some(&0) || units.windows(2).any(|pair| pair == [0, 0]) +} diff --git a/proptest-regressions/param_convert.txt b/proptest-regressions/param_convert.txt new file mode 100644 index 0000000..9aa3bdc --- /dev/null +++ b/proptest-regressions/param_convert.txt @@ -0,0 +1,10 @@ +# Seeds for failure cases proptest has generated in the past. It is +# automatically read and these particular cases re-run before any +# novel cases are generated. +# +# It is recommended to check this file in to source control so that +# everyone who runs the test benefits from these saved cases. +cc b397f155c0024183da7ee227f989dce4f106122c05ddcd6d0a0fd6ea193de960 # shrinks to text = "0e-1048577" +cc 14cac6f9937a37466197f0f46da27ef8f7bf8f4e4dcd139f7ff5b9985a5b79eb # shrinks to text = "0e-2147483647" +cc a63740d711d952f1a19c605c2d34fd42607826298239bcf9051d443bead7282b # shrinks to text = "0e-1048577" +cc c1710352100792598c4ba42876c7f85a26ded650c3c2b3acd0d4a4dcd3163a54 # shrinks to text = ".214748364721474836472147483647", wide = 2, narrow = 3 diff --git a/src/backend.rs b/src/backend.rs index efc491b..780adc2 100644 --- a/src/backend.rs +++ b/src/backend.rs @@ -3074,6 +3074,10 @@ mod tests { /// a test naming the info type rather than surviving review, which is how /// `SQL_GROUP_BY`, `SQL_CORRELATION_NAME` and `SQL_SUBQUERIES` come to be /// decided in the wrong place. + #[cfg_attr( + miri, + ignore = "two full SQLGetInfo sweeps; no unsafe in this module for Miri to check" + )] #[test] fn default_get_info_answers_are_backend_derived_or_declared_core_facts() { use crate::test_utils::MockAltBackend; diff --git a/src/escape.rs b/src/escape.rs index 7143131..e2934f7 100644 --- a/src/escape.rs +++ b/src/escape.rs @@ -826,6 +826,10 @@ mod tests { format!("{}x{}", "{fn UCASE(".repeat(depth), ")}".repeat(depth)) } + #[cfg_attr( + miri, + ignore = "MAX_ESCAPE_DEPTH-deep input is slow under Miri; no unsafe here to check" + )] #[test] fn nesting_within_the_depth_limit_still_translates() { // Real SQL nests escapes a handful deep at most; the limit must not be @@ -835,6 +839,10 @@ mod tests { assert!(out.contains('x')); } + #[cfg_attr( + miri, + ignore = "MAX_ESCAPE_DEPTH-deep input is slow under Miri; no unsafe here to check" + )] #[test] fn nesting_within_the_depth_limit_is_linear_through_the_fn_argument_path() { // Exactly `MAX_ESCAPE_DEPTH`, the deepest input the limit accepts, and @@ -850,12 +858,20 @@ mod tests { assert_eq!(out.matches("upper(").count(), MAX_ESCAPE_DEPTH); } + #[cfg_attr( + miri, + ignore = "MAX_ESCAPE_DEPTH-deep input is slow under Miri; no unsafe here to check" + )] #[test] fn nesting_beyond_the_depth_limit_is_rejected_not_overflowed() { let err = translate_escapes(&nested_oj(MAX_ESCAPE_DEPTH + 1), &dialect()).unwrap_err(); assert_eq!(err.sqlstate().as_str(), "42000"); } + #[cfg_attr( + miri, + ignore = "MAX_ESCAPE_DEPTH-deep input is slow under Miri; no unsafe here to check" + )] #[test] fn nesting_beyond_the_depth_limit_is_rejected_through_the_fn_argument_path() { let err = translate_escapes(&nested_fn(MAX_ESCAPE_DEPTH + 1), &dialect()).unwrap_err(); @@ -1012,20 +1028,202 @@ mod proptests { use super::*; use proptest::prelude::*; + /// The dialects the sweep runs against, chosen by index. + /// + /// Pinning `ansi_default` would leave `remap_scalar_fn`, `rewrite_scalar_fn` + /// and the three renderers on their no-op or ANSI settings for every + /// generated input, so the `{fn}` rewrite path and the `{d}`/`{t}`/`{ts}` + /// renderers would never be reached with anything but the identity. These + /// four vary the identifier quoting, both scalar hooks and the renderers. + /// + /// They are core's own dialects, built through the public builders. The + /// composition of this parser with a *real* backend dialect belongs in that + /// driver's repository, which is the only place that knows what its rewrite + /// hooks are supposed to produce. + fn dialect_by_index(index: usize) -> EscapeDialect { + /// Rewrites to a longer name, so a rewrite that mismanaged its output + /// buffer cannot be masked by the replacement happening to fit. + fn remap(name: &str) -> Option<&'static str> { + match name.to_ascii_uppercase().as_str() { + "UCASE" => Some("upper_case_of"), + "LCASE" => Some("lower"), + _ => None, + } + } + fn rewrite(name: &str, args: &str) -> Option { + match name.to_ascii_uppercase().as_str() { + // Re-emits the argument text twice, so an off-by-one in how + // `args` is delimited shows up doubled rather than not at all. + "CONCAT" => Some(format!("({args}) || ({args})")), + "CURDATE" => Some("current_date".to_owned()), + _ => None, + } + } + fn brace_date(x: &str) -> String { + // Deliberately re-introduces a brace into the output. Nothing + // rescans it, and this pins that. + format!("date{{{x}}}") + } + fn plain_time(x: &str) -> String { + format!("time {x}") + } + fn plain_timestamp(x: &str) -> String { + format!("ts {x}") + } + + match index % 4 { + 0 => EscapeDialect::ansi_default(), + 1 => EscapeDialect::ansi_default().with_identifier_quotes(&[('`', '`')]), + 2 => EscapeDialect::ansi_default() + .with_identifier_quotes(&[('[', ']'), ('"', '"')]) + .with_remap_scalar_fn(remap), + _ => EscapeDialect::ansi_default() + .with_remap_scalar_fn(remap) + .with_rewrite_scalar_fn(rewrite) + .with_datetime_renderers(brace_date, plain_time, plain_timestamp), + } + } + + /// Fragments that look like the things this scanner has to tell apart. + /// + /// A `.*` strategy is why the existing never-panics test is weaker than it + /// looks: it has to spell `{fn CONVERT(x, SQL_INTEGER)}` out of thin air to + /// reach the scalar-function path, and it essentially never does. Even a + /// balanced `{ts '...'}` is out of reach. Emitting whole tokens instead + /// puts every escape keyword, both comment forms and all three quote + /// characters into the input by construction, and lets them nest and + /// interleave in ways a hand-written test would not think to try. + fn escape_token() -> impl Strategy { + prop_oneof![ + Just("{fn ".to_owned()), + Just("{fn CONVERT(".to_owned()), + Just("{fn UCASE(".to_owned()), + Just("{fn CONCAT(".to_owned()), + Just("{d ".to_owned()), + Just("{t ".to_owned()), + Just("{ts ".to_owned()), + Just("{oj ".to_owned()), + Just("{escape ".to_owned()), + Just("{call ".to_owned()), + Just("{?= call ".to_owned()), + Just("{".to_owned()), + Just("}".to_owned()), + Just("'".to_owned()), + Just("''".to_owned()), + Just("\"".to_owned()), + Just("`".to_owned()), + Just("[".to_owned()), + Just("]".to_owned()), + Just("--".to_owned()), + Just("/*".to_owned()), + Just("*/".to_owned()), + Just("\n".to_owned()), + Just("(".to_owned()), + Just(")".to_owned()), + Just(", ".to_owned()), + Just("SQL_INTEGER".to_owned()), + Just("'2020-01-01 12:00:00'".to_owned()), + "[a-zA-Z_][a-zA-Z0-9_]{0,5}", + ] + } + + /// Text with no brace in it, so it cannot open an escape of its own. Still + /// carries the quote and comment characters, because the scanner has to + /// carry those across untouched. + fn brace_free_text() -> impl Strategy { + proptest::collection::vec( + prop_oneof![ + Just("'".to_owned()), + Just("\"".to_owned()), + Just("`".to_owned()), + Just("--".to_owned()), + Just("/*".to_owned()), + Just("*/".to_owned()), + Just("\n".to_owned()), + Just(" ".to_owned()), + "[a-zA-Z0-9_(),.]{0,6}", + ], + 0..8, + ) + .prop_map(|parts| parts.concat()) + } + proptest! { - // The escape scanner must never panic on any input, however malformed - // the escapes, because a panic would cross the FFI boundary. + /// The escape scanner must never panic on any input, however malformed + /// the escapes, because a panic would cross the FFI boundary. + /// + /// Kept alongside the grammar below rather than replaced by it: the two + /// reach different things. This one produces the arbitrary characters, + /// multi-byte ones included, that a grammar of hand-picked tokens never + /// emits. #[test] fn translate_escapes_never_panics(s in ".*") { let _ = translate_escapes(&s, &EscapeDialect::ansi_default()); } - // Plain text with no escape braces, quotes or comments is copied through - // unchanged. + /// The same never-panics property, but over inputs that actually reach + /// the escape machinery, against all four dialects. + /// + /// An `Err` is a perfectly good outcome here: `{call}` and `{?= call}` + /// are refused with 'HYC00', and nesting past `MAX_ESCAPE_DEPTH` is + /// refused too, which is reachable because the token count runs past + /// that depth. + #[test] + fn escape_grammar_never_panics( + tokens in proptest::collection::vec(escape_token(), 0..80), + dialect_index in 0..4usize, + ) { + let sql = tokens.concat(); + let _ = translate_escapes(&sql, &dialect_by_index(dialect_index)); + } + + /// Plain text with no escape braces, quotes or comments is copied + /// through unchanged. #[test] fn plain_text_is_unchanged(s in "[a-zA-Z0-9 ]*") { let out = translate_escapes(&s, &EscapeDialect::ansi_default()).ok(); prop_assert_eq!(out, Some(s)); } + + /// An escape whose keyword is not one this driver knows is copied out + /// verbatim, and so is everything around it. + /// + /// This is the oracle `plain_text_is_unchanged` cannot be: an input + /// with no `{` in it returns on the early-out at the top of + /// [`translate_escapes`] without the scanner running at all, so that + /// test pins the fast path only. Putting one unrecognised escape in the + /// middle forces the full character walk, over text carrying the quote + /// and comment characters it has to step across, and the whole input + /// must still come back byte for byte. + /// + /// The body is the one part held to plain characters. A quote or a + /// comment opener inside it swallows the escape's own closing brace, + /// which makes the escape genuinely unterminated and an `Err` the + /// correct answer rather than a lost byte. The prefix and suffix keep + /// the full character set, because an unclosed quote out there puts the + /// `{` inside a literal, where it is not an escape at all and the copy + /// is verbatim either way. + #[test] + fn an_unknown_escape_and_its_surroundings_survive_verbatim( + prefix in brace_free_text(), + keyword in "[a-z]{4,8}", + body in "[a-zA-Z0-9_ ,.()]{0,12}", + suffix in brace_free_text(), + dialect_index in 0..4usize, + ) { + // Never one of the seven keywords the translator acts on, and + // never the `?=` form, so the escape has to take the pass-through + // arm. `prop_assume!` rather than a narrower regex: the excluded + // set is short enough to reject and long enough to be unreadable + // as a character class. + prop_assume!(!matches!( + keyword.as_str(), + "fn" | "d" | "t" | "ts" | "oj" | "escape" | "call" + )); + + let sql = format!("{prefix}{{{keyword} {body}}}{suffix}"); + let out = translate_escapes(&sql, &dialect_by_index(dialect_index)); + prop_assert_eq!(out.ok(), Some(sql)); + } } } diff --git a/src/numeric_convert.rs b/src/numeric_convert.rs index e01ccd3..df1e96b 100644 --- a/src/numeric_convert.rs +++ b/src/numeric_convert.rs @@ -1284,3 +1284,208 @@ mod tests { ); } } + +#[cfg(test)] +mod proptest_numeric_convert { + use super::*; + use proptest::prelude::*; + + /// How a bound numeric parameter was supplied, before it is canonicalised. + /// + /// The strategies generate this rather than a [`NumericParam`] directly, + /// because proptest has to be able to print a failing case and + /// `NumericParam` carries no `Debug`. Deriving one on it would be the + /// smaller change here and the wrong one there: this type holds the *value* + /// of a bound parameter, and a `Debug` impl is how such a value ends up + /// interpolated into a log line by someone who was only trying to trace a + /// conversion. + #[derive(Debug, Clone)] + enum ParamSpec { + /// Every integer C type. + Integer(i128), + /// `SQL_C_NUMERIC`, which arrives as the text its struct renders to. + Text(String), + /// `SQL_C_FLOAT` when `single`, `SQL_C_DOUBLE` otherwise. + Approx { value: f64, single: bool }, + } + + impl ParamSpec { + /// `None` where the ODBC path would have raised a diagnostic instead of + /// producing a parameter, which is only the unparseable text case. + fn build(&self) -> Option { + match self { + ParamSpec::Integer(value) => Some(NumericParam::exact_integer(*value)), + ParamSpec::Text(text) => NumericParam::exact_text(text), + ParamSpec::Approx { value, single } => Some(NumericParam::approx(*value, *single)), + } + } + } + + /// Every shape a bound numeric parameter can arrive in. + /// + /// The float arm carries NaN and both infinities on purpose. They are the + /// values row 4 is explicitly allowed to accept and every other row has to + /// refuse, and they are also the values that compare false against every + /// bound, so a range test written as a pair of comparisons rather than an + /// `is_finite` check lets them through. + fn param_spec() -> impl Strategy { + prop_oneof![ + any::().prop_map(ParamSpec::Integer), + any::().prop_map(|v| ParamSpec::Integer(i128::from(v))), + "-?[0-9]{1,30}(\\.[0-9]{0,20})?([eE]-?[0-9]{1,5})?".prop_map(ParamSpec::Text), + (any::(), any::()) + .prop_map(|(value, single)| ParamSpec::Approx { value, single }), + prop_oneof![ + Just(f64::NAN), + Just(f64::INFINITY), + Just(f64::NEG_INFINITY), + Just(0.0_f64), + Just(-0.0_f64), + Just(f64::MIN_POSITIVE), + Just(f64::MAX), + ] + .prop_map(|value| ParamSpec::Approx { + value, + single: false + }), + ] + } + + /// SQL type codes, biased onto the ranges this table branches on and left + /// open elsewhere, so the unsupported-target path is reached too. + fn sql_type() -> impl Strategy { + prop_oneof![ + (-12_i16..15).prop_map(SqlDataType), + (88_i16..116).prop_map(SqlDataType), + any::().prop_map(SqlDataType), + ] + } + + proptest! { + /// The whole table is total: no combination of value, target type, + /// column size, scale and interval precision panics. + /// + /// All five arguments are the application's. `SQLBindParameter` takes + /// the last four straight from the caller and does not require them to + /// agree with each other or with the value, so a scale can be negative, + /// a column size can be `usize::MAX` and the type code need not be one + /// this table knows. + #[test] + fn numeric_to_sql_type_is_total( + spec in param_spec(), + sql_type in sql_type(), + col_size in prop_oneof![0_usize..40, Just(usize::MAX), any::()], + decimal_digits in any::(), + interval_precision in any::(), + ) { + let Some(value) = spec.build() else { + return Ok(()); + }; + let _ = numeric_to_sql_type(value, sql_type, col_size, decimal_digits, interval_precision); + } + + /// An exact integer reaches its integer target when it fits, and is + /// refused as out of range when it does not. + /// + /// The bounds are written out rather than taken from `try_from`, which + /// is what the conversion itself uses: an oracle that calls the same + /// function agrees with it by construction. Nothing is truncated on + /// this path either, an integer having no fraction to lose, so a + /// warning here would be a warning the application cannot act on. + #[test] + fn an_exact_integer_reaches_its_target_or_is_refused_by_range( + value in prop_oneof![ + any::(), + any::().prop_map(i128::from), + any::().prop_map(i128::from), + -300_i128..300, + ], + which in 0_usize..4, + ) { + let (sql_type, low, high) = match which { + 0 => (SqlDataType::EXT_TINY_INT, i128::from(i8::MIN), i128::from(i8::MAX)), + 1 => (SqlDataType::SMALLINT, i128::from(i16::MIN), i128::from(i16::MAX)), + 2 => (SqlDataType::INTEGER, i128::from(i32::MIN), i128::from(i32::MAX)), + _ => (SqlDataType::EXT_BIG_INT, i128::from(i64::MIN), i128::from(i64::MAX)), + }; + + let converted = numeric_to_sql_type( + NumericParam::exact_integer(value), + sql_type, + 0, + 0, + 0, + ); + + let fits = (low..=high).contains(&value); + prop_assert_eq!( + converted.is_ok(), + fits, + "{} against {:?}", + value, + sql_type + ); + + if let Ok(converted) = converted { + prop_assert!( + converted.warning.is_none(), + "an integer source has no fraction to truncate" + ); + let expected = match which { + 0 => ColumnValue::I8(value as i8), + 1 => ColumnValue::I16(value as i16), + 2 => ColumnValue::I32(value as i32), + _ => ColumnValue::I64(value as i64), + }; + prop_assert_eq!(converted.value, expected); + } + } + + /// A character target accepts exactly the values whose rendering fits + /// the declared column size, and hands over that same rendering. + /// + /// The length the table checks has to be the length of the text that is + /// sent, not of some canonical form of the number, which is why the + /// assertion reads the returned string rather than re-rendering the + /// value. A declared size of zero means the application declared none. + #[test] + fn a_character_target_accepts_exactly_what_fits( + spec in param_spec(), + col_size in 0_usize..48, + ) { + let Some(value) = spec.build() else { + return Ok(()); + }; + let rendered = value.render(); + let converted = numeric_to_sql_type(value, SqlDataType::VARCHAR, col_size, 0, 0); + + let fits = col_size == 0 || rendered.chars().count() <= col_size; + prop_assert_eq!( + converted.is_ok(), + fits, + "{:?} ({} characters) against a declared size of {}", + rendered, + rendered.chars().count(), + col_size + ); + + if let Ok(converted) = converted { + prop_assert_eq!(converted.value, ColumnValue::String(rendered)); + } + } + + /// `exact_text` accepts a string exactly when it is a *numeric-literal*. + /// + /// It is the entry point for `SQL_C_NUMERIC`, whose text core renders + /// itself, so the property that matters is that it is not stricter than + /// the parser it delegates to and does not panic on text that is not a + /// number at all. + #[test] + fn exact_text_accepts_what_the_parser_accepts(text in "[-+.eE0-9 ]{0,24}") { + prop_assert_eq!( + NumericParam::exact_text(&text).is_some(), + parse_numeric_literal(&text).is_some() + ); + } + } +} diff --git a/src/param_convert.rs b/src/param_convert.rs index e12dc2b..9372f50 100644 --- a/src/param_convert.rs +++ b/src/param_convert.rs @@ -2389,3 +2389,336 @@ mod tests { ); } } + +#[cfg(test)] +mod proptest_param_convert { + use super::*; + use crate::column_value::NumericTarget; + use proptest::prelude::*; + + /// Text shaped like the *numeric-literal* grammar, and like the things that + /// are one token away from it. + /// + /// The tokens are chosen for where the arithmetic in this module is + /// delicate rather than for where the grammar is: `i32::MAX` and + /// `i32::MIN` as exponents, because [`parse_numeric_literal`] subtracts the + /// exponent from the fraction length and a wrapping subtraction there is a + /// scale nothing downstream can render; long runs of zeros, because a zero + /// mantissa at a huge exponent is the denial of service + /// [`DecimalLiteral::to_integer`] guards against by hand; and the sign, + /// point and `e` in isolation, because every one of them can appear twice + /// or without digits. + fn numeric_text() -> impl Strategy { + // Weighted towards the soup, because the extreme exponents render up to + // `MAX_DECIMAL_EXPANSION_DIGITS` characters each and there is no point + // paying for that on the cases that are only checking totality. + prop_oneof![4 => token_soup(), 1 => extreme_exponent_literal()] + } + + /// Well-formed literals whose exponent sits on the boundaries the expansion + /// bound is drawn around. + /// + /// [`token_soup`] reaches `1e-1048576` only by assembling four particular + /// tokens in one particular order, which it essentially never does, so on + /// its own it would leave `rendering_never_expands_past_the_bound` asserting + /// a bound nothing had approached. These construct the interesting + /// exponents outright: either side of [`MAX_DECIMAL_EXPANSION_DIGITS`], + /// either side of the `i32` the parser's scale arithmetic has to survive, + /// and the small ones in between. + fn extreme_exponent_literal() -> impl Strategy { + ( + "-?[0-9]{1,6}", + prop_oneof![ + Just(i64::from(i32::MIN)), + Just(i64::from(i32::MIN) + 1), + Just(-1_048_577), + Just(-1_048_576), + Just(-1_048_575), + Just(-1_i64), + Just(0), + Just(1), + Just(1_048_575), + Just(1_048_576), + Just(1_048_577), + Just(i64::from(i32::MAX) - 1), + Just(i64::from(i32::MAX)), + ], + ) + .prop_map(|(mantissa, exponent)| format!("{mantissa}e{exponent}")) + } + + fn token_soup() -> impl Strategy { + proptest::collection::vec( + prop_oneof![ + Just("-".to_owned()), + Just("+".to_owned()), + Just(".".to_owned()), + Just("e".to_owned()), + Just("E".to_owned()), + Just(" ".to_owned()), + Just("0".to_owned()), + Just("000000".to_owned()), + Just("2147483647".to_owned()), + Just("2147483648".to_owned()), + Just("1048576".to_owned()), + "[0-9]{1,20}", + ], + 0..8, + ) + .prop_map(|parts| parts.concat()) + } + + /// Plain integers, including ones a few digits wider than `i128` holds, so + /// the oracle below covers the rejection as well as the acceptance. + fn integer_text() -> impl Strategy { + prop_oneof![ + "-?[0-9]{1,45}", + any::().prop_map(|v| v.to_string()), + any::().prop_map(|v| v.to_string()), + ] + } + + /// One exact decimal value written one way, so two renderings of it can be + /// compared without a decimal library. + /// + /// Trailing fractional zeros carry no value and neither does a sign on + /// zero, which is the whole of the difference: `0.0` and `0` are the same + /// number, and a conversion is free to prefer either. Trailing zeros are + /// stripped only past a decimal point, because in `100` they are the value. + fn same_value(rendered: &str) -> String { + let trimmed = if rendered.contains('.') { + rendered.trim_end_matches('0').trim_end_matches('.') + } else { + rendered + }; + if trimmed.trim_start_matches('-').bytes().all(|b| b == b'0') { + return "0".to_owned(); + } + trimmed.to_owned() + } + + /// SQL type codes, biased onto the ranges this module branches on: the + /// concise types around zero, the datetime types at 91 to 93 and the + /// interval types at 101 to 113. The unbiased arm keeps the codes that fall + /// through to the default arm in play. + fn sql_type() -> impl Strategy { + prop_oneof![ + (-12_i16..15).prop_map(SqlDataType), + (88_i16..116).prop_map(SqlDataType), + any::().prop_map(SqlDataType), + ] + } + + proptest! { + /// Nothing in the [`DecimalLiteral`] family panics, whatever the text + /// parsed into it and whatever precision and scale it is asked for. + /// + /// This is not a formality. These run under `SQLBindParameter` and + /// `SQLPutData` on a buffer the application filled, so a panic here is + /// a panic unwinding through `extern "system"`, and the arithmetic is + /// `i128` and `i32` scale shifting where a subtraction can wrap and a + /// slice index can be built from a length that underflowed. + #[test] + fn every_decimal_literal_method_is_total( + text in numeric_text(), + truncate_to in 0_usize..80, + precision in any::(), + target_scale in any::(), + ) { + let Some(literal) = parse_numeric_literal(&text) else { + return Ok(()); + }; + + let _ = literal.to_decimal_string(); + let _ = literal.to_integer(); + let _ = literal.required_scale(); + let _ = literal.whole_digits(); + let _ = literal.fraction_is_zero(); + let _ = literal.significant(); + let _ = literal.is_zero(); + let _ = literal.expansion_is_bounded(); + let _ = literal.truncated_to_scale(truncate_to); + let _ = literal.to_numeric_struct(NumericTarget { + precision, + scale: target_scale, + ..NumericTarget::default() + }); + } + + /// Rendering is bounded by the input plus + /// [`MAX_DECIMAL_EXPANSION_DIGITS`], never by the exponent. + /// + /// The bound is the whole point of that constant: `1e2147483647` must + /// not become a two-gigabyte `String` inside an FFI call. Asserting the + /// length rather than merely completing the call is what makes the + /// property visible, because an unbounded expansion that happens to fit + /// in memory still passes a never-panics test. + #[test] + fn rendering_never_expands_past_the_bound(text in numeric_text()) { + let Some(literal) = parse_numeric_literal(&text) else { + return Ok(()); + }; + let rendered = literal.to_decimal_string(); + prop_assert!( + rendered.len() <= text.len() + MAX_DECIMAL_EXPANSION_DIGITS + 32, + "{:?} rendered {} characters", + text, + rendered.len() + ); + } + + /// A rendered literal parses back to a literal that renders the same. + /// + /// [`DecimalLiteral::to_decimal_string`] is what a backend interpolates + /// into SQL, so this says the exponent expansion is exact: no digit is + /// gained, lost or moved across the point on the way out. + /// + /// Restricted to literals whose expansion is bounded, because the + /// unbounded branch deliberately returns the exponent form instead, and + /// its exponent is an `i64` that need not fit the `i32` the parser + /// accepts. That branch is unreachable from `text_to_sql_type`, which + /// refuses such a value first. + #[test] + fn rendering_round_trips_through_the_parser(text in numeric_text()) { + let Some(literal) = parse_numeric_literal(&text) else { + return Ok(()); + }; + prop_assume!(literal.expansion_is_bounded()); + + let rendered = literal.to_decimal_string(); + let reparsed = parse_numeric_literal(&rendered) + .expect("a rendering of a literal must itself be a numeric literal"); + prop_assert_eq!(reparsed.to_decimal_string(), rendered); + } + + /// `to_integer` agrees with the standard library on text that is + /// already an integer, including on the widths it must refuse. + /// + /// An independent oracle rather than a restatement: `i128::from_str` + /// shares no code with the digit walk here. + #[test] + fn to_integer_agrees_with_the_standard_library(text in integer_text()) { + let Some(literal) = parse_numeric_literal(&text) else { + return Ok(()); + }; + prop_assert_eq!(literal.to_integer(), text.parse::().ok()); + } + + /// Truncating in two steps lands where truncating in one step does. + /// + /// The scales are ordered so the second truncation is the tighter one. + /// Textual equality is the right assertion because the two paths reach + /// the same digit slice at the same scale, not merely the same value. + #[test] + fn truncation_composes( + text in numeric_text(), + wide in 0_usize..40, + narrow in 0_usize..40, + ) { + let Some(literal) = parse_numeric_literal(&text) else { + return Ok(()); + }; + let (wide, narrow) = (wide.max(narrow), wide.min(narrow)); + + let stepwise = literal.truncated_to_scale(wide).truncated_to_scale(narrow); + let direct = literal.truncated_to_scale(narrow); + prop_assert_eq!(stepwise.to_decimal_string(), direct.to_decimal_string()); + } + + /// Truncating to a scale the literal already sits at or below changes + /// nothing at all. + #[test] + fn truncating_to_a_wider_scale_is_a_no_op(text in numeric_text()) { + let Some(literal) = parse_numeric_literal(&text) else { + return Ok(()); + }; + let Ok(scale) = usize::try_from(literal.scale) else { + return Ok(()); + }; + prop_assert_eq!( + literal.truncated_to_scale(scale).to_decimal_string(), + literal.to_decimal_string() + ); + } + + /// A `SQL_NUMERIC_STRUCT` built from a literal reconstructs it exactly. + /// + /// The struct is the same value in another base: sign, an unsigned + /// little-endian `u128` magnitude and a scale. Reading it back and + /// rendering it must return the literal's own rendering, which is what + /// pins the base conversion, the scale shift and the digit counting all + /// at once. + /// + /// The target is left unspecified, so the conversion derives the scale + /// from the value rather than being handed one. That is the case with + /// nothing to truncate, so it must also report no truncation. + /// + /// Compared as values rather than as strings. A conversion that derives + /// its own scale is entitled to pick a different one for the same + /// number, and it does: zero reports a required scale of 0, so `0.0` + /// comes back as `0`. + /// + /// Restricted to literals that render in plain decimal, which is what + /// [`same_value`] can normalise. The excluded case is real rather than + /// hypothetical: `0e-2147483647` converts perfectly well, because zero + /// requires a scale of 0 however large its exponent, while its own + /// rendering takes the exponent branch and would be compared against + /// the struct's `0`. + #[test] + fn an_unspecified_numeric_struct_reconstructs_the_literal(text in numeric_text()) { + let Some(literal) = parse_numeric_literal(&text) else { + return Ok(()); + }; + prop_assume!(literal.expansion_is_bounded()); + let Ok((numeric, fraction_lost)) = + literal.to_numeric_struct(NumericTarget::default()) + else { + return Ok(()); + }; + + prop_assert!( + !fraction_lost, + "a target that declared no scale cannot have truncated {text:?}" + ); + + let reconstructed = DecimalLiteral { + // odbc-sys: "1 if positive, 0 if negative". + negative: numeric.sign == 0, + digits: u128::from_le_bytes(numeric.val).to_string(), + scale: i32::from(numeric.scale), + }; + prop_assert_eq!( + same_value(&reconstructed.to_decimal_string()), + same_value(&literal.to_decimal_string()) + ); + } + + /// `text_to_sql_type` never panics, for any text, any declared type and + /// any column size or scale the application bound. + /// + /// All four arguments come straight from `SQLBindParameter`, so none of + /// them is trusted: the type code need not be one this driver knows, + /// and the size and scale need not describe the text. + #[test] + fn text_to_sql_type_is_total( + text in numeric_text(), + sql_type in sql_type(), + col_size in prop_oneof![0_usize..40, Just(usize::MAX), any::()], + decimal_digits in any::(), + ) { + let _ = text_to_sql_type(&text, sql_type, col_size, decimal_digits); + } + + /// The same, over text that is not numeric at all, which is the + /// character-to-character and character-to-datetime half of the table. + #[test] + fn text_to_sql_type_is_total_for_arbitrary_text( + text in ".{0,64}", + sql_type in sql_type(), + col_size in 0_usize..40, + decimal_digits in any::(), + ) { + let _ = text_to_sql_type(&text, sql_type, col_size, decimal_digits); + } + } +} diff --git a/src/test_support.rs b/src/test_support.rs index 66ac924..1b1d6d5 100644 --- a/src/test_support.rs +++ b/src/test_support.rs @@ -123,6 +123,60 @@ pub unsafe fn detach_connection( } } +/// [`crate::ffi::setup`]'s attribute-list parser, reachable from the fuzz crate. +/// +/// That parser is the only raw-pointer walk in core a fuzz target does not +/// already cover, and the only one where AddressSanitizer can report a genuine +/// overrun rather than a caught panic: it steps a `*const u16` the Driver +/// Manager supplied, looking for a terminator a caller may have got wrong. +/// `fuzz/` is a separate crate, so it can only reach a `pub` item, and the +/// parser is `pub(crate)`. +/// +/// It lives here rather than beside the parser because a `pub unsafe fn` in +/// `src/ffi/` means "an ODBC entry point" to the guard in +/// [`crate::types`]'s `diagnostics_table`, which then requires a transcribed +/// spec diagnostics table for it. This is a test hook, not an entry point, and +/// weakening that guard to say so would cost more than moving one function. +/// +/// A summary rather than the parser's own result type, for two reasons. It +/// keeps that type, and the `HashMap` inside it, out of the public API, +/// feature-gated or not. And it is a value the caller cannot discard: summing +/// the segment lengths forces every string the walk produced to be built and +/// read, so the reads ASAN is watching for cannot be optimised out of a target +/// that ignores the result. +/// +/// # Returns +/// +/// The number of pairs read, the total length in bytes of every key and value, +/// and whether a syntax error was reported. +/// +/// # Safety +/// +/// `ptr` must be null, or point to a valid double-null-terminated `u16` +/// sequence. A caller that passes an unterminated buffer is reading past its +/// own allocation, which is that caller's defect and not one the parser can be +/// fuzzed for. +/// +/// The sequence need **not** be `u16`-aligned. Every code unit is read with +/// `read_unaligned`, because the Driver Manager pointer the parser ordinarily +/// receives carries no alignment guarantee, so a fuzz target is free to offset +/// its buffer by a byte. +#[must_use] +pub unsafe fn parse_attributes_summary_w(ptr: *const u16) -> (usize, usize, bool) { + // SAFETY: forwarded verbatim; this function's contract is the callee's. + let parsed = unsafe { crate::ffi::setup::parse_attributes_w(ptr) }; + let bytes = parsed + .attributes + .iter() + .map(|(key, value)| key.len() + value.len()) + .sum(); + ( + parsed.attributes.len(), + bytes, + parsed.syntax_error.is_some(), + ) +} + #[cfg(test)] mod tests { use super::*; diff --git a/src/types/connect_params.rs b/src/types/connect_params.rs index 20b8be0..cd04765 100644 --- a/src/types/connect_params.rs +++ b/src/types/connect_params.rs @@ -774,7 +774,88 @@ mod proptest_connect_params { use super::*; use proptest::prelude::*; + /// Values assembled from the characters that end a value, quote one, or + /// separate one pair from the next. + /// + /// A `.*` strategy reaches `{`, `}` and `;` only by accident, and reaches a + /// `}` immediately followed by another `}` almost never, which is the exact + /// shape the brace-doubling in [`ConnectParams::to_connection_string`] + /// exists to handle. Drawing from these tokens directly puts a doubling + /// regression in the first few dozen cases rather than none of them. + fn hostile_value() -> impl Strategy { + proptest::collection::vec( + prop_oneof![ + Just("{".to_owned()), + Just("}".to_owned()), + Just("}}".to_owned()), + Just(";".to_owned()), + Just("=".to_owned()), + Just(" ".to_owned()), + Just("};".to_owned()), + Just("{fake=1;".to_owned()), + "[a-z]{0,4}", + ], + 0..8, + ) + .prop_map(|parts| parts.concat()) + } + + /// Lowercase so that two generated keywords can never collide under the + /// lowercasing [`ConnectParams::insert`] applies, which would silently drop + /// a pair and make the count assertion below ambiguous rather than wrong. + fn keyword() -> impl Strategy { + "[a-z][a-z0-9_]{0,7}" + } + proptest! { + /// The whole string round-trips, not just one pair of it. + /// + /// `any_value_round_trips_through_to_connection_string` below renders a + /// single keyword, so nothing follows the value it is checking and a + /// value that ends its own quoting early has nothing to run into. The + /// defect that matters needs a neighbour: a `}` in one value closing + /// the brace run early, so the rest of that value is read as further + /// keywords and the pair that genuinely came next is swallowed. + /// + /// Hence two assertions. The value check is the one that fires under + /// every mutation tried against this, because a keyword can only be + /// injected by mangling the value that produced it, so the two go + /// wrong together and the value check reports it more precisely. The + /// count check is kept regardless: it states the property in the form + /// the security argument is made in, an injection into the + /// application's *next* connect that `SQLBrowseConnectW` would + /// otherwise hand out, and it is the only one that would catch a + /// keyword appearing while every inserted pair survives intact. + #[test] + fn every_pair_survives_the_round_trip_with_no_keyword_injected( + pairs in proptest::collection::hash_map(keyword(), hostile_value(), 1..6), + ) { + let mut params = ConnectParams::parse("").expect("the empty string parses"); + for (key, value) in &pairs { + params.insert(key.clone(), value.clone()); + } + + let rendered = params.to_connection_string(); + let reparsed = ConnectParams::parse(&rendered) + .expect("a string this crate generated must parse"); + + prop_assert_eq!( + reparsed.keys().count(), + pairs.len(), + "a keyword appeared that was never inserted, in {:?}", + rendered + ); + for (key, value) in &pairs { + prop_assert_eq!( + reparsed.get(key), + Some(value.as_str()), + "{} lost its value in {:?}", + key, + rendered + ); + } + } + /// ConnectParams::parse never panics on arbitrary input. #[test] fn parse_never_panics(s in ".*") { diff --git a/src/types/constants.rs b/src/types/constants.rs index ff72cce..64184ce 100644 --- a/src/types/constants.rs +++ b/src/types/constants.rs @@ -1908,6 +1908,10 @@ mod tests { /// list this long and harmless to the subtraction itself, but it means the /// list was transcribed with an error. The next error may be a *missing* /// entry, which silently leaks an ODBC keyword into `SQL_KEYWORDS`. + #[cfg_attr( + miri, + ignore = "quadratic scan over the keyword list; no unsafe in this module for Miri to check" + )] #[test] fn odbc_reserved_keywords_are_unique() { let mut seen = std::collections::HashSet::new(); diff --git a/src/types/conversions.rs b/src/types/conversions.rs index 2de24d4..7e774b2 100644 --- a/src/types/conversions.rs +++ b/src/types/conversions.rs @@ -1223,4 +1223,174 @@ mod tests { assert_eq!(nullable_from_raw(2), Some(Nullable::SqlNullableUnknown)); assert_eq!(nullable_from_raw(99), None); } + + /// Every conversion in this module, swept across its whole input domain. + /// + /// The per-function tests above pin the values a reader would think to + /// check: the valid ones, a couple of invalid ones, the deprecated + /// spellings. This module checks the other 65,000, which is what catches a + /// match arm that names the wrong variant for a value nobody wrote a test + /// for. Two properties hold for all of them: + /// + /// - **Total.** No input panics. These run at the FFI boundary on an + /// integer an application chose, so a panic here is a panic crossing + /// `extern "system"`. + /// - **Round-tripping.** Whatever a conversion accepts converts back to the + /// integer it came from. AGENTS.md makes this the condition for a + /// conversion to exist at all: `odbc_sys::Operation` and `Lock` fail it, + /// which is why they are named constants rather than `*_from_raw` + /// functions. + /// + /// A 16-bit domain is 65,536 values, so these sweep it exhaustively rather + /// than sampling it. That is a few milliseconds per function and leaves no + /// gap for a property test's luck to fall into. The four `i32` conversions + /// are sampled with `proptest` instead, because 4.3 billion is not a few + /// milliseconds. + /// + /// # Every test here is `#[cfg_attr(miri, ignore)]` + /// + /// A few milliseconds native is four and a half minutes interpreted: this + /// module is 850,000 match evaluations, and the Miri job's `--skip proptest` + /// filter does not catch it, because these are not named proptests. There is + /// nothing here for Miri to find either. Not one of these conversions + /// contains `unsafe`, and the whole point of the module is breadth of input, + /// which is what the ordinary `cargo test` job already gives it. + /// + /// Skipping by attribute rather than by widening that filter, so a test + /// states its own cost. See the same attribute on + /// `types::diagnostics_table`'s two source-scanning guards. + mod from_raw_sweep { + use super::*; + use proptest::prelude::*; + + /// Sweep a 16-bit conversion across every value its ABI argument can + /// hold. `$back` is the inverse: the expression that writes the typed + /// value back out as the integer the Driver Manager passed in. + macro_rules! sweep16 { + ($name:ident, $f:ident, $int:ty, |$v:ident| $back:expr) => { + #[cfg_attr(miri, ignore = "65,536 interpreted iterations; no unsafe to check")] + #[test] + fn $name() { + for raw in <$int>::MIN..=<$int>::MAX { + if let Some($v) = $f(raw) { + assert_eq!( + $back, raw, + concat!( + stringify!($f), + " accepted a value that does not convert back to itself" + ) + ); + } + } + } + }; + } + + sweep16!(handle_type, handle_type_from_raw, i16, |v| v as i16); + sweep16!(desc, desc_from_raw, u16, |v| v as u16); + sweep16!(info_type, info_type_from_raw, u16, |v| v as u16); + sweep16!(interval, interval_from_raw, i16, |v| v as i16); + sweep16!(param_type, param_type_from_raw, i16, |v| v as i16); + sweep16!(free_stmt_option, free_stmt_option_from_raw, u16, |v| v + as u16); + sweep16!( + driver_connect_option, + driver_connect_option_from_raw, + u16, + |v| v as u16 + ); + sweep16!(completion_type, completion_type_from_raw, i16, |v| v as i16); + sweep16!(bulk_operation, bulk_operation_from_raw, i16, |v| v as i16); + sweep16!(fetch_orientation, fetch_orientation_from_raw, i16, |v| v + as i16); + sweep16!(scope, scope_from_raw, u16, |v| v as u16); + sweep16!(nullable, nullable_from_raw, u16, |v| v as u16); + + // `IdentifierType` carries no `#[repr]` and no inverse, because nothing + // in the crate writes one back over the ABI: `SQLSpecialColumns` reads + // the argument and branches on it. The inverse is spelled out here + // rather than added to the type, so the sweep covers it without + // widening the public API for a test. + sweep16!( + identifier_type, + identifier_type_from_raw, + u16, + |v| match v { + IdentifierType::BestRowId => SQL_BEST_ROWID, + IdentifierType::RowVer => SQL_ROWVER, + } + ); + + proptest! { + #[cfg_attr(miri, ignore = "sampled input; no unsafe to check")] + #[test] + fn environment_attribute(raw in any::()) { + if let Some(v) = environment_attribute_from_raw(raw) { + prop_assert_eq!(v as i32, raw); + } + } + + #[cfg_attr(miri, ignore = "sampled input; no unsafe to check")] + #[test] + fn attr_odbc_version(raw in any::()) { + if let Some(v) = attr_odbc_version_from_raw(raw) { + prop_assert_eq!(v as i32, raw); + } + } + + #[cfg_attr(miri, ignore = "sampled input; no unsafe to check")] + #[test] + fn declared_odbc_version(raw in any::()) { + if let Some(v) = declared_odbc_version_from_raw(raw) { + prop_assert_eq!(v.raw(), raw); + } + } + + #[cfg_attr(miri, ignore = "sampled input; no unsafe to check")] + #[test] + fn statement_attribute(raw in any::()) { + if let Some(v) = statement_attribute_from_raw(raw) { + prop_assert_eq!(v as i32, raw); + } + } + } + + /// `c_data_type_from_raw` is the one conversion that must not round-trip. + /// + /// ODBC 2.x spelled the signed integer C types without the sign: + /// `SQL_C_LONG` (4), `SQL_C_SHORT` (5) and `SQL_C_TINYINT` (-6). ODBC 3.x + /// renamed them `SQL_C_SLONG` (-16), `SQL_C_SSHORT` (-15) and + /// `SQL_C_STINYINT` (-26), and the conversion normalises the old spelling + /// to the new variant, so those three inputs deliberately come back as a + /// different integer. Every other accepted value round-trips, and this + /// sweep pins that the exception is exactly three values wide and does not + /// silently grow. + #[cfg_attr(miri, ignore = "65,536 interpreted iterations; no unsafe to check")] + #[test] + fn c_data_type_round_trips_except_the_three_deprecated_spellings() { + let deprecated = [ + (4_i16, CDataType::SLong), + (5, CDataType::SShort), + (-6, CDataType::STinyInt), + ]; + + for raw in i16::MIN..=i16::MAX { + let Some(converted) = c_data_type_from_raw(raw) else { + continue; + }; + if let Some((_, expected)) = deprecated.iter().find(|(alias, _)| *alias == raw) { + assert_eq!( + converted, *expected, + "the ODBC 2.x spelling {raw} must normalise to its 3.x variant" + ); + } else { + assert_eq!( + converted as i16, raw, + "c_data_type_from_raw accepted {raw}, which is not one of the three \ + deprecated spellings, yet it does not convert back to itself" + ); + } + } + } + } } diff --git a/src/types/diagnostics_table.rs b/src/types/diagnostics_table.rs index c54d08d..230dd61 100644 --- a/src/types/diagnostics_table.rs +++ b/src/types/diagnostics_table.rs @@ -2120,6 +2120,10 @@ FunctionDiagnostics { }, ]; +#[cfg_attr( + miri, + ignore = "1.86 MB string scan; no unsafe in this module for Miri to check" +)] #[test] fn the_transcription_is_well_formed() { let mut problems: Vec = Vec::new(); diff --git a/src/types/info_type_shape.rs b/src/types/info_type_shape.rs index 7fe579b..3feecb0 100644 --- a/src/types/info_type_shape.rs +++ b/src/types/info_type_shape.rs @@ -217,6 +217,10 @@ mod tests { /// were built from the same variant set: if `odbc-sys` ever adds a /// variant, the match itself already refuses to compile until this /// module is updated, this test documents *why* that invariant matters. + #[cfg_attr( + miri, + ignore = "65,536-value sweep; no unsafe in this module for Miri to check" + )] #[test] fn every_raw_info_type_has_a_declared_shape() { let mut count = 0;