diff --git a/.github/workflows/build.yaml b/.github/workflows/build.yaml index 72a776d..238694a 100644 --- a/.github/workflows/build.yaml +++ b/.github/workflows/build.yaml @@ -396,6 +396,64 @@ jobs: echo "=== logs ===" docker compose logs --tail=300 + fuzz: + name: Fuzz (ASAN smoke) + runs-on: ubuntu-24.04 + timeout-minutes: 20 + needs: [unit-tests] + steps: + # The fuzz binaries link this driver, which links libodbc through + # odbc-sys, so the unixODBC dev libraries must be present to link them. + - name: Install host dependencies + uses: awalsh128/cache-apt-pkgs-action@553a35bb8ebd9fcabcb1c9451aa4c98e1b4ca8a9 # v1.6.3 + with: + packages: unixodbc-dev + version: ubuntu-24.04 + + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false + + # cargo-fuzz builds with libFuzzer + AddressSanitizer, which require a + # nightly toolchain. The fuzz crate is its own Cargo workspace, so the + # pinned stable root build never touches it. + - name: Install nightly toolchain + uses: dtolnay/rust-toolchain@2c7215f132e9ebf062739d9130488b56d53c060c # nightly + with: + toolchain: nightly + + # fuzz/ declares its own [workspace], so its build artifacts land in + # fuzz/target, not the root target/. Without this the cache stores an + # empty directory and every run rebuilds nightly + ASAN from scratch. + - name: Setup Rust Cache + uses: Swatinem/rust-cache@c19371144df3bb44fab255c43d04cbc2ab54d1c4 # v2.9.1 + with: + key: fuzz + workspaces: "fuzz -> target" + + - name: Install cargo-fuzz + uses: taiki-e/install-action@1beb33eee6d086258184383af9a538940be190ed # v2.85.6 + with: + tool: cargo-fuzz + + # A short smoke run per target: long enough to catch a regression that + # reintroduces a shallow crash, short enough for per-PR CI. Finding a new + # defect is the job of a longer run, not of this gate. + # + # --target is pinned to the gnu triple explicitly: newer cargo-fuzz + # defaults to x86_64-unknown-linux-musl, whose statically linked libc is + # incompatible with AddressSanitizer ("sanitizer is incompatible with + # statically linked libc"). gnu uses a dynamic libc and ships with the + # nightly toolchain. + - name: Fuzz json_value + run: cargo +nightly fuzz run json_value --target x86_64-unknown-linux-gnu -- -max_total_time=30 + - name: Fuzz type_name + run: cargo +nightly fuzz run type_name --target x86_64-unknown-linux-gnu -- -max_total_time=30 + - name: Fuzz escape + run: cargo +nightly fuzz run escape --target x86_64-unknown-linux-gnu -- -max_total_time=30 + - name: Fuzz connect_params + run: cargo +nightly fuzz run connect_params --target x86_64-unknown-linux-gnu -- -max_total_time=30 + # Single required check for branch protection rules. finished: name: Finished Build and Test @@ -406,6 +464,7 @@ jobs: - unit-tests-windows - release-artifacts - integration-tests + - fuzz runs-on: ubuntu-24.04 timeout-minutes: 5 steps: diff --git a/CHANGELOG.md b/CHANGELOG.md index 8bfb717..6747299 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] +### Fixed + +A `TIME WITH TIME ZONE` value whose offset or hour field lies far outside a real +clock is now kept as text instead of being converted with wrapped arithmetic. +Both fields arrive as free-form text, so a number that no zone or clock could +hold still parses as an `i32`, and the conversion to minutes overflowed. Release +builds carry no overflow checks, so the driver reported a different time rather +than declining the value. + +### Added + +Fuzz targets for this driver's own parsers, in `fuzz/`: the JSON-to-value read +path, the Trino type-signature parsers, ODBC escape translation under the Trino +dialect, and the connection-string value parsing. They run as an +AddressSanitizer smoke test in CI. See `fuzz/README.md`. + ## [0.1.0] — 2026-08-04 First release, so this section describes what the driver offers rather than diff --git a/Cargo.toml b/Cargo.toml index 54b148a..7df7eb9 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -14,6 +14,15 @@ categories = ["database", "external-ffi-bindings", "api-bindings"] [lib] crate-type = ["cdylib", "rlib"] +[features] +# Exposes the `fuzz_api` module, which re-exports the private entry points the +# targets in `fuzz/` drive. +# +# Default-off because it is test scaffolding: the modules it widens are private +# on purpose, and a shipped driver has no reason to carry a second, unsupported +# way into its parsers. `fuzz/Cargo.toml` enables it; nothing else should. +fuzzing = [] + [dependencies] base64 = "0.23" chrono = { version = "0.4", default-features = false } diff --git a/fuzz/.gitignore b/fuzz/.gitignore new file mode 100644 index 0000000..fbd7cd1 --- /dev/null +++ b/fuzz/.gitignore @@ -0,0 +1,11 @@ +target +corpus +artifacts +coverage +# `-jobs=N` writes one of these per worker into whatever directory the run +# started from. +fuzz-*.log +# Untracked, matching stackable-odbc-core's fuzz workspace. These targets are +# never published and CI resolves them fresh, so pinning here would only add a +# second lockfile for Renovate to carry. +Cargo.lock diff --git a/fuzz/Cargo.toml b/fuzz/Cargo.toml new file mode 100644 index 0000000..2fd206d --- /dev/null +++ b/fuzz/Cargo.toml @@ -0,0 +1,50 @@ +[package] +name = "stackable-odbc-trino-fuzz" +version = "0.0.1" +publish = false +edition = "2021" + +[package.metadata] +cargo-fuzz = true + +# Own workspace: keeps `cargo fuzz` from resolving against the parent workspace +# (which pins a stable toolchain; libFuzzer needs nightly). +[workspace] + +[[bin]] +name = "json_value" +path = "fuzz_targets/json_value.rs" +test = false +doc = false +bench = false + +[[bin]] +name = "type_name" +path = "fuzz_targets/type_name.rs" +test = false +doc = false +bench = false + +[[bin]] +name = "escape" +path = "fuzz_targets/escape.rs" +test = false +doc = false +bench = false + +[[bin]] +name = "connect_params" +path = "fuzz_targets/connect_params.rs" +test = false +doc = false +bench = false + +[dependencies] +arbitrary = { version = "1", features = ["derive"] } +libfuzzer-sys = "0.4" +serde_json = "1" +stackable-odbc-trino = { path = "..", features = ["fuzzing"] } +# Needed to name `TrinoTy` when building the declared type for a fuzzed value. +# Kept in step with the root `Cargo.toml` entry: a different revision here +# would resolve to a second copy of the crate and the types would not match. +trino-rust-client = { git = "https://github.com/stackabletech/trino-rust-client.git", branch = "stackable-main" } diff --git a/fuzz/README.md b/fuzz/README.md new file mode 100644 index 0000000..edca93e --- /dev/null +++ b/fuzz/README.md @@ -0,0 +1,91 @@ +# Fuzz targets + +These targets cover this driver's own parsers: the code that turns text chosen +by a Trino coordinator, or by the application, into values the driver acts on. + +None of it contains `unsafe`, so AddressSanitizer is not what earns these their +keep, and that is a real difference from the targets in +`stackable-odbc-core`, which exist for the pointer marshalling. What earns +these is coverage guidance plus a structured generator. Every target below +builds its input from a grammar rather than from raw bytes, because the inputs +that reach these parsers are shaped: a random byte string is not a balanced +`{fn CONVERT(x, SQL_INTEGER)}`, is not `13:14:15+02:00`, and is not +`Host=h;Port=8443;Roles=x:y`. A `proptest` over `".*"` explores the same space +and essentially never lands in it. + +The property under test is that no input panics. Panics are caught at the FFI +boundary by core's `catch_unwind`, so the blast radius is a failed ODBC call +rather than a crashed application. That is the floor, not a reason to accept +one: a value a coordinator legitimately sent must fail safe, and where a +release build has no overflow checks the same defect returns a wrong answer +instead of an error. + +- `json_value` covers `json_to_column_value` and the dozen temporal, interval + and decimal scanners under it. This is the half of the read path core does + not see: core fuzzes `write_column_value`, which turns the resulting + `ColumnValue` into the caller's buffer, and nothing covered the step that + produces it. +- `type_name` covers `type_name_precision`, `type_name_scale` and + `trino_type_name_to_sql_type`, which read Trino type signatures as text out + of `DESCRIBE INPUT` rows and `information_schema` queries. +- `escape` covers core's escape translator driven by this crate's dialect. Core + fuzzes the parser against its own dialects; only this repo has the Trino + dialect, so only this repo reaches `escape_dialect::split_args`. +- `connect_params` covers the per-key value parsing layered on core's + `ConnectParams::parse`: durations, booleans, proxy URLs, time zones, selected + roles and four `key:value`-inside-a-value sublanguages. + +The targets reach these through `stackable_odbc_trino::fuzz_api`, behind the +default-off `fuzzing` feature. The modules are private on purpose and the +feature does not reach the shipped `cdylib`; `fuzz_api`'s doc comment lists what +is exposed and why. + +## Running + +[cargo-fuzz](https://github.com/rust-fuzz/cargo-fuzz) needs nightly, because +libFuzzer does. + +```bash +cargo install cargo-fuzz +cargo +nightly fuzz run json_value +``` + +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: + +```bash +cargo +nightly fuzz run json_value --target x86_64-unknown-linux-gnu +``` + +`cargo fuzz run` runs until it finds a crash or you stop it. To bound a run, +pass a libFuzzer flag after `--`: + +```bash +cargo +nightly fuzz run json_value -- -max_total_time=60 # stop after 60s +cargo +nightly fuzz run json_value -- -runs=1000000 # or a run count +``` + +A crash writes its input to `artifacts//`. To see it as the generator's +own types rather than as bytes: + +```bash +cargo +nightly fuzz fmt json_value artifacts/json_value/crash- +``` + +## Reading a clean run + +A target that finds nothing is only as good as the inputs its generator can +express. `type_name` ran fifteen million executions without reaching a slice +that a hand-read of the code said was reachable, and the reason was the +generator: it rendered `base(args)suffix`, so the opening parenthesis always +preceded the closing one and the shape under suspicion could not be built. When +a run comes back clean, check that the generator can actually produce the input +you had in mind before concluding the code is safe. + +## Workspace + +This is its own Cargo workspace, so `cargo build` in the repository root does +not touch it and no `pre-commit` hook compiles it. A change to +`type_conversion`, `escape_dialect` or the connection-string parsing can break +it while every root check still passes, so build it by hand after such a change. diff --git a/fuzz/fuzz_targets/connect_params.rs b/fuzz/fuzz_targets/connect_params.rs new file mode 100644 index 0000000..0df1f80 --- /dev/null +++ b/fuzz/fuzz_targets/connect_params.rs @@ -0,0 +1,116 @@ +#![no_main] + +use arbitrary::Arbitrary; +use libfuzzer_sys::fuzz_target; +use stackable_odbc_trino::fuzz_api::trino_connect_params; + +// Fuzzes connection-string handling: core splits the string into keys, then +// this driver reads the values it recognises. +// +// core already proptests `ConnectParams::parse` for panic-freedom, so the half +// under test here is the second one: the per-key value parsers, which are this +// crate's. They parse durations, booleans, proxy URLs, time zones, selected +// roles and four separate `key:value`-inside-a-value sublanguages. +// +// The input is user-supplied through a DSN rather than remote, so a panic here +// is less severe than one on the read path. It is also the cheapest of the four +// targets to run, because the surface is a pure string-to-Result function. +// +// The error is rendered to a String on the way out, so this covers the +// `Display` formatting too: several error arms interpolate the offending value. + +/// One `key=value` pair. +#[derive(Arbitrary, Debug)] +enum Pair { + /// A key this driver knows, with a fuzzed value. This is the arm that + /// reaches the value parsers; a random key would only ever be ignored. + Known { key: u8, value: String }, + /// A key the driver does not know, to cover the unrecognised-key handling. + Unknown { key: String, value: String }, + /// A value wrapped in braces, which is how the connection-string grammar + /// carries a value containing `;` or `=`. + Braced { key: u8, value: String }, +} + +/// The driver's connection-string keys, from `backend::types::connect_params`. +/// Host and Port are omitted here and always prepended, so an input does not +/// have to rediscover them to get past the required-parameter checks. +const KEYS: &[&str] = &[ + "protocol", + "tlsverify", + "sslverification", + "certificate", + "clientcertificate", + "querytimeout", + "logintimeout", + "catalog", + "schema", + "source", + "clienttags", + "accesstoken", + "token", + "sessionproperties", + "extracredentials", + "resourceestimates", + "path", + "clientinfo", + "tracetoken", + "proxy", + "proxyuser", + "proxypassword", + "extraheaders", + "clientcapabilities", + "timezone", + "roles", + "sessionuser", + "locale", + "disablecompression", + "maxattempts", + "encoding", + "externalauthentication", + "externalauthenticationtimeout", +]; + +#[derive(Arbitrary, Debug)] +struct Input { + /// Prepended so most inputs get past `MissingParam` and reach the value + /// parsing. Fuzzed rather than fixed, because the port parse is itself one + /// of the parsers under test. + host: String, + port: String, + user: String, + pairs: Vec, + /// Appended verbatim, so the fuzzer can still explore the raw grammar: + /// stray semicolons, unbalanced braces, embedded nulls. + tail: String, +} + +fn render(input: &Input) -> String { + let Input { + host, + port, + user, + pairs, + tail, + } = input; + let mut s = format!("Host={host};Port={port};UID={user};"); + for pair in pairs { + match pair { + Pair::Known { key, value } => { + let key = KEYS[*key as usize % KEYS.len()]; + s.push_str(&format!("{key}={value};")); + } + Pair::Unknown { key, value } => s.push_str(&format!("{key}={value};")), + Pair::Braced { key, value } => { + let key = KEYS[*key as usize % KEYS.len()]; + s.push_str(&format!("{key}={{{value}}};")); + } + } + } + s.push_str(tail); + s +} + +fuzz_target!(|input: Input| { + let _ = trino_connect_params(&render(&input)); +}); diff --git a/fuzz/fuzz_targets/escape.rs b/fuzz/fuzz_targets/escape.rs new file mode 100644 index 0000000..eaf0049 --- /dev/null +++ b/fuzz/fuzz_targets/escape.rs @@ -0,0 +1,114 @@ +#![no_main] + +use arbitrary::Arbitrary; +use libfuzzer_sys::fuzz_target; +use stackable_odbc_trino::fuzz_api::translate_escapes; + +// Fuzzes ODBC escape translation with this driver's dialect. +// +// The parser itself belongs to stackable-odbc-core, which fuzzes it against its +// own dialects. What only this repo can reach is the composition: core's +// scanner calling into `escape_dialect`'s callbacks, and in particular +// `split_args`, a hand-written state machine that tracks quotes, doubled +// quotes, line comments, block comments and parenthesis depth while mapping +// character indices onto byte offsets. Index arithmetic plus a +// `saturating_sub` and a `min` clamp is exactly the code a fuzzer is for. +// +// Input is application-supplied SQL, so the trust boundary is weaker than the +// read path's, but a panic still turns a legitimate `SQLPrepareW` into a +// failure the application cannot route around. + +/// One piece of SQL. Escape-shaped arms dominate on purpose: random bytes +/// essentially never produce a balanced `{fn CONVERT(x, SQL_INTEGER)}`, so a +/// `&str` target would spend its whole budget never entering the dialect. +#[derive(Arbitrary, Debug)] +enum Frag { + /// `{fn NAME(args)}`, the arm that reaches `rewrite_scalar_fn` and + /// `split_args`. + Fn { name: u8, args: String }, + /// The same, with a name the fuzzer chose, to reach the fall-through path. + FnRaw { name: String, args: String }, + /// The datetime literal escapes, which route to the render callbacks. + Date(String), + Time(String), + Timestamp(String), + /// `{escape ''}`, `{oj ...}` and `{call ...}`. + Escape(String), + OuterJoin(String), + Call(String), + /// Unbalanced braces, to attack the scanner's bracket tracking. + OpenBrace, + CloseBrace, + /// Quoting and comments, which `split_args` must skip over rather than + /// treat as structure. + SingleQuoted(String), + DoubleQuoted(String), + LineComment(String), + BlockComment(String), + /// Bare separators, so a fragment list can form argument lists and nesting + /// without the generator having to spell them inside a string. + Comma, + OpenParen, + CloseParen, + Raw(String), +} + +/// Scalar functions this dialect rewrites or remaps. Naming them explicitly is +/// what gets the fuzzer past the `remap_scalar_fn` lookup and into the arms +/// that actually reparse their arguments. +const FN_NAMES: &[&str] = &[ + "CONVERT", + "LOCATE", + "POSITION", + "TRUNCATE", + "RAND", + "TIMESTAMPADD", + "TIMESTAMPDIFF", + "EXTRACT", + "CHAR_LENGTH", + "SUBSTRING", + "CURDATE", + "CURTIME", + "NOW", + "DAYOFWEEK", + "IFNULL", + "LOG10", + "ATAN2", + "REPEAT", + "SPACE", + "UCASE", +]; + +fn render(frags: &[Frag]) -> String { + let mut sql = String::new(); + for frag in frags { + match frag { + Frag::Fn { name, args } => { + let name = FN_NAMES[*name as usize % FN_NAMES.len()]; + sql.push_str(&format!("{{fn {name}({args})}}")); + } + Frag::FnRaw { name, args } => sql.push_str(&format!("{{fn {name}({args})}}")), + Frag::Date(s) => sql.push_str(&format!("{{d '{s}'}}")), + Frag::Time(s) => sql.push_str(&format!("{{t '{s}'}}")), + Frag::Timestamp(s) => sql.push_str(&format!("{{ts '{s}'}}")), + Frag::Escape(s) => sql.push_str(&format!("{{escape '{s}'}}")), + Frag::OuterJoin(s) => sql.push_str(&format!("{{oj {s}}}")), + Frag::Call(s) => sql.push_str(&format!("{{call {s}}}")), + Frag::OpenBrace => sql.push('{'), + Frag::CloseBrace => sql.push('}'), + Frag::SingleQuoted(s) => sql.push_str(&format!("'{s}'")), + Frag::DoubleQuoted(s) => sql.push_str(&format!("\"{s}\"")), + Frag::LineComment(s) => sql.push_str(&format!("--{s}\n")), + Frag::BlockComment(s) => sql.push_str(&format!("/*{s}*/")), + Frag::Comma => sql.push(','), + Frag::OpenParen => sql.push('('), + Frag::CloseParen => sql.push(')'), + Frag::Raw(s) => sql.push_str(s), + } + } + sql +} + +fuzz_target!(|frags: Vec| { + let _ = translate_escapes(&render(&frags)); +}); diff --git a/fuzz/fuzz_targets/json_value.rs b/fuzz/fuzz_targets/json_value.rs new file mode 100644 index 0000000..00c126e --- /dev/null +++ b/fuzz/fuzz_targets/json_value.rs @@ -0,0 +1,182 @@ +#![no_main] + +use arbitrary::Arbitrary; +use libfuzzer_sys::fuzz_target; +use serde_json::Value; +use stackable_odbc_trino::fuzz_api::json_value; +use trino_rust_client::{TrinoFloat, TrinoInt, TrinoTy}; + +// Fuzzes the read path's first half: a coordinator's JSON value under its +// declared Trino type, becoming a ColumnValue. +// +// stackable-odbc-core already fuzzes the second half (`write_column_value`, +// ColumnValue -> the caller's buffer). Nothing covered the step before it, +// which is where this crate's temporal, interval and decimal parsers live: +// roughly a dozen hand-written scanners over text a Trino coordinator chose. +// Every one of them runs on the server's side of the trust boundary. +// +// The property is that no input panics. A panic here is caught at the FFI +// boundary by core's `catch_unwind`, so the blast radius is a failed ODBC call +// rather than a crashed application, but a query that cannot fail-safe on a +// value the server legitimately sent is still a defect. + +/// The scalar Trino types, mirroring `TrinoTy`'s non-recursive variants. +#[derive(Arbitrary, Debug)] +enum FuzzScalar { + // Listed first: every temporal parser reads `val.as_str()`, so these are + // the arms where fuzzed text actually reaches a scanner. + Date, + Time, + TimeWithTimeZone, + Timestamp, + TimestampWithTimeZone, + IntervalYearToMonth, + IntervalDayToSecond, + Uuid, + VarBinary, + // `u16` rather than `usize`: Trino caps CHAR at 65536, and a fuzzed + // `usize::MAX` would only ever prove that a 16-exabyte pad allocation + // fails, which is not a defect this target is looking for. + Char(u16), + Decimal(u8, u8), + Boolean, + Int(FuzzInt), + Float(FuzzFloat), + Varchar, + IpAddress, + Json, + Unknown, +} + +#[derive(Arbitrary, Debug)] +enum FuzzInt { + I8, + I16, + I32, + I64, +} + +#[derive(Arbitrary, Debug)] +enum FuzzFloat { + F32, + F64, +} + +/// A declared type: a scalar, or one level of container around scalars. +/// +/// Nesting is bounded at one level deliberately. `TrinoTy` is recursive, and a +/// derived `Arbitrary` on a recursive type spends most of its input budget +/// building depth instead of reaching the scanners. The container arms exist +/// to cover the recursion into element conversion, which one level already +/// does. +#[derive(Arbitrary, Debug)] +enum FuzzTy { + Scalar(FuzzScalar), + Nullable(FuzzScalar), + Array(FuzzScalar), + Map(FuzzScalar, FuzzScalar), + Row(Vec<(String, FuzzScalar)>), + Tuple(Vec), +} + +/// A JSON value. Not `serde_json::Value` directly, which is recursive and is +/// not `Arbitrary`; the leaves are flat for the same reason `FuzzTy` is. +#[derive(Arbitrary, Debug)] +enum FuzzValue { + // First, because every scanner in this crate reads strings. + Str(String), + Null, + Bool(bool), + I64(i64), + F64(f64), + Arr(Vec), + Obj(Vec<(String, FuzzLeaf)>), +} + +#[derive(Arbitrary, Debug)] +enum FuzzLeaf { + Str(String), + Null, + Bool(bool), + I64(i64), +} + +fn scalar_ty(s: &FuzzScalar) -> TrinoTy { + match s { + FuzzScalar::Date => TrinoTy::Date, + FuzzScalar::Time => TrinoTy::Time, + FuzzScalar::TimeWithTimeZone => TrinoTy::TimeWithTimeZone, + FuzzScalar::Timestamp => TrinoTy::Timestamp, + FuzzScalar::TimestampWithTimeZone => TrinoTy::TimestampWithTimeZone, + FuzzScalar::IntervalYearToMonth => TrinoTy::IntervalYearToMonth, + FuzzScalar::IntervalDayToSecond => TrinoTy::IntervalDayToSecond, + FuzzScalar::Uuid => TrinoTy::Uuid, + FuzzScalar::VarBinary => TrinoTy::VarBinary, + FuzzScalar::Char(n) => TrinoTy::Char(*n as usize), + FuzzScalar::Decimal(p, s) => TrinoTy::Decimal(*p as usize, *s as usize), + FuzzScalar::Boolean => TrinoTy::Boolean, + FuzzScalar::Int(i) => TrinoTy::TrinoInt(match i { + FuzzInt::I8 => TrinoInt::I8, + FuzzInt::I16 => TrinoInt::I16, + FuzzInt::I32 => TrinoInt::I32, + FuzzInt::I64 => TrinoInt::I64, + }), + FuzzScalar::Float(f) => TrinoTy::TrinoFloat(match f { + FuzzFloat::F32 => TrinoFloat::F32, + FuzzFloat::F64 => TrinoFloat::F64, + }), + FuzzScalar::Varchar => TrinoTy::Varchar, + FuzzScalar::IpAddress => TrinoTy::IpAddress, + FuzzScalar::Json => TrinoTy::Json, + FuzzScalar::Unknown => TrinoTy::Unknown, + } +} + +fn trino_ty(t: &FuzzTy) -> TrinoTy { + match t { + FuzzTy::Scalar(s) => scalar_ty(s), + FuzzTy::Nullable(s) => TrinoTy::Option(Box::new(scalar_ty(s))), + FuzzTy::Array(s) => TrinoTy::Array(Box::new(scalar_ty(s))), + FuzzTy::Map(k, v) => TrinoTy::Map(Box::new(scalar_ty(k)), Box::new(scalar_ty(v))), + FuzzTy::Row(fields) => TrinoTy::Row( + fields + .iter() + .map(|(name, s)| (name.clone(), scalar_ty(s))) + .collect(), + ), + FuzzTy::Tuple(items) => TrinoTy::Tuple(items.iter().map(scalar_ty).collect()), + } +} + +fn leaf_value(l: &FuzzLeaf) -> Value { + match l { + FuzzLeaf::Str(s) => Value::String(s.clone()), + FuzzLeaf::Null => Value::Null, + FuzzLeaf::Bool(b) => Value::Bool(*b), + FuzzLeaf::I64(n) => Value::from(*n), + } +} + +fn json_value_of(v: &FuzzValue) -> Value { + match v { + FuzzValue::Str(s) => Value::String(s.clone()), + FuzzValue::Null => Value::Null, + FuzzValue::Bool(b) => Value::Bool(*b), + FuzzValue::I64(n) => Value::from(*n), + // `Value::from` on a non-finite f64 yields Null, which is a legitimate + // input rather than a case to filter: the coordinator can send one. + FuzzValue::F64(f) => Value::from(*f), + FuzzValue::Arr(items) => Value::Array(items.iter().map(leaf_value).collect()), + FuzzValue::Obj(fields) => Value::Object( + fields + .iter() + .map(|(k, l)| (k.clone(), leaf_value(l))) + .collect(), + ), + } +} + +fuzz_target!(|input: (FuzzTy, FuzzValue)| { + let (ty, val) = input; + let _ = json_value(json_value_of(&val), &trino_ty(&ty)); +}); diff --git a/fuzz/fuzz_targets/type_name.rs b/fuzz/fuzz_targets/type_name.rs new file mode 100644 index 0000000..8ce58cc --- /dev/null +++ b/fuzz/fuzz_targets/type_name.rs @@ -0,0 +1,92 @@ +#![no_main] + +use arbitrary::Arbitrary; +use libfuzzer_sys::fuzz_target; +use stackable_odbc_trino::fuzz_api::{ + trino_type_name_to_sql_type, type_name_precision, type_name_scale, +}; + +// Fuzzes the Trino type-signature parsers. +// +// These read type names as *text*, not as a parsed `TrinoTy`: they run on the +// `data_type` column of an `information_schema` query and on the rows +// `DESCRIBE INPUT` returns, both of which are strings the coordinator chose. +// The parsing is index arithmetic over `find('(')` and `rfind(')')`, which is +// the shape that goes wrong when the parentheses are not where a well-formed +// signature would put them. +// +// The generator mixes free-form strings with signature-shaped ones. Pure random +// bytes rarely contain a parenthesis pair at all, so without the shaped arms +// almost every input would exit at the first `find`. + +/// A type name: free text, or something with a signature's shape. +#[derive(Arbitrary, Debug)] +enum FuzzName { + /// Free-form, to reach the arms that take no parameter at all. + Raw(String), + /// `()`, assembled so the parentheses are present but + /// their contents and ordering are the fuzzer's to choose. + Shaped { + base: String, + args: String, + suffix: String, + }, + /// A real base name with fuzzed parameters, which is what a coordinator + /// sends and therefore where a regression would actually be observed. + Known { + base: u8, + args: String, + suffix: String, + }, + /// The parentheses in the wrong order. The parsers locate the argument list + /// with `find('(')` and `rfind(')')` independently, so nothing guarantees + /// the opening one comes first; the other arms all render a well-formed + /// pair and can never express this. + Reversed { base: String, args: String }, +} + +/// The base names this driver resolves to a specific SQL type. Anything else +/// falls through to `trino_type_name_to_sql_type`'s default arm. +const KNOWN_BASES: &[&str] = &[ + "varchar", + "char", + "decimal", + "timestamp", + "time", + "date", + "interval year to month", + "interval day to second", + "array", + "map", + "row", + "varbinary", + "json", + "uuid", + "ipaddress", + "boolean", + "integer", + "bigint", + "smallint", + "tinyint", + "real", + "double", +]; + +fn render(name: &FuzzName) -> String { + match name { + FuzzName::Raw(s) => s.clone(), + FuzzName::Shaped { base, args, suffix } => format!("{base}({args}){suffix}"), + FuzzName::Known { base, args, suffix } => { + let base = KNOWN_BASES[*base as usize % KNOWN_BASES.len()]; + format!("{base}({args}){suffix}") + } + FuzzName::Reversed { base, args } => format!("{base}){args}("), + } +} + +fuzz_target!(|name: FuzzName| { + let name = render(&name); + let _ = type_name_precision(&name); + let _ = type_name_scale(&name); + let _ = trino_type_name_to_sql_type(&name); +}); diff --git a/src/backend.rs b/src/backend.rs index 558b106..db2c2b0 100644 --- a/src/backend.rs +++ b/src/backend.rs @@ -138,7 +138,7 @@ pub(crate) mod info; mod info; mod metadata; mod params; -mod types; +pub(crate) mod types; /// The interval units Trino's `date_add` / `date_diff` accept, reported for /// both `SQL_TIMEDATE_ADD_INTERVALS` and `SQL_TIMEDATE_DIFF_INTERVALS`. diff --git a/src/fuzz_api.rs b/src/fuzz_api.rs new file mode 100644 index 0000000..3b31357 --- /dev/null +++ b/src/fuzz_api.rs @@ -0,0 +1,70 @@ +//! The entry points the fuzz targets in `fuzz/` drive. +//! +//! The three surfaces worth fuzzing here are private modules, and two of them +//! expose their interesting functions as `pub(crate)`. Rather than widen each +//! module and each item, this one module re-exports exactly what the targets +//! call, so the fuzzed surface is a list someone can read in one screen. +//! +//! Behind the default-off `fuzzing` feature, so none of this reaches the +//! shipped `cdylib`. Nothing here is part of the driver's supported API: it +//! exists to let an out-of-tree crate reach in, and it may change with the +//! internals it wraps. +//! +//! What each target covers, and why it is here rather than in +//! `stackable-odbc-core`: +//! +//! - [`json_to_column_value`]: core fuzzes `write_column_value`, which turns a +//! `ColumnValue` into a caller's buffer. Nothing fuzzes the step before it, +//! where a coordinator's JSON becomes that `ColumnValue`, and that step is +//! this crate's own dozen-odd temporal and interval parsers. +//! - [`type_name_precision`] and its neighbours: parse Trino type signatures +//! (`decimal(10,2)`, `timestamp(6) with time zone`) that arrive as text in +//! `DESCRIBE INPUT` rows and `information_schema` queries. +//! - [`translate_escapes`]: core owns the ODBC escape parser and fuzzes it +//! against its own dialects; this crate owns the Trino dialect. Only the +//! composition exercises `escape_dialect::split_args`, so only this repo can +//! fuzz it. +//! - [`trino_connect_params`]: core's `ConnectParams::parse` splits the +//! connection string; the per-key value parsing on top of it is this +//! crate's. + +use serde_json::Value; +use stackable_odbc_core::types::ColumnValue; +use trino_rust_client::TrinoTy; + +pub use crate::type_conversion::{ + json_to_column_value, trino_type_name_to_sql_type, type_name_precision, type_name_scale, +}; + +/// Convert one coordinator JSON value under its declared Trino type. +/// +/// A thin alias for [`json_to_column_value`], kept so a target can name the +/// whole read-path conversion without importing the type-name helpers. +pub fn json_value(val: Value, ty: &TrinoTy) -> ColumnValue { + json_to_column_value(val, ty) +} + +/// Run core's ODBC escape translator with this driver's dialect. +/// +/// Returns `None` where the translator reports an error, which is a normal +/// outcome for malformed input and not the property under test. The property +/// is that neither the parser nor the dialect callbacks panic. +pub fn translate_escapes(sql: &str) -> Option { + stackable_odbc_core::escape::translate_escapes(sql, &crate::escape_dialect::dialect()).ok() +} + +/// Parse a connection string the way `SQLDriverConnectW` does: core splits it +/// into keys, then this driver reads the values it knows. +/// +/// The error is rendered to a `String` rather than returned as-is, so the +/// target exercises the `Display` formatting too. Error formatting is its own +/// panic surface: several arms interpolate the offending value. +pub fn trino_connect_params(connection_string: &str) -> Result<(), String> { + use crate::backend::types::connect_params::TrinoConnectParams; + + let params = stackable_odbc_core::types::ConnectParams::parse(connection_string) + .map_err(|e| e.to_string())?; + TrinoConnectParams::try_from(¶ms) + .map(|_| ()) + .map_err(|e| e.to_string()) +} diff --git a/src/lib.rs b/src/lib.rs index ed412a1..60fd274 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -18,6 +18,11 @@ mod backend; mod escape_dialect; mod type_conversion; +/// The entry points the fuzz targets in `fuzz/` drive. Behind the default-off +/// `fuzzing` feature: see `Cargo.toml`. +#[cfg(feature = "fuzzing")] +pub mod fuzz_api; + pub use backend::TrinoBackend; stackable_odbc_core::forward_ffi!(crate::backend::TrinoBackend); diff --git a/src/type_conversion.rs b/src/type_conversion.rs index 4839274..758491f 100644 --- a/src/type_conversion.rs +++ b/src/type_conversion.rs @@ -529,17 +529,33 @@ fn strip_precision_param(name: &str) -> String { } /// Extract the first numeric parameter from a type string like `"varchar(100)"` or `"decimal(10,2)"`. +/// +/// `name.get(..)` rather than `name[..]`, for the same reason +/// [`parse_fraction_nanos`] uses it: the two ends are located independently, by +/// `find` and `rfind`, so nothing about a caller-supplied string guarantees the +/// opening parenthesis precedes the closing one, and indexing a reversed range +/// panics where `get` returns `None`. Both callers currently gate on +/// [`TrinoTypeName::parse`], which no reversed-parenthesis name survives, so +/// this is what keeps that gate from being load-bearing rather than a fix for +/// a reachable defect. fn parse_precision_param(name: &str) -> Option { let start = name.find('(')?; let end = name.rfind(')')?; - name[start + 1..end].split(',').next()?.trim().parse().ok() + name.get(start + 1..end)? + .split(',') + .next()? + .trim() + .parse() + .ok() } /// Extract the second numeric parameter from a type string like `"decimal(10,2)"`. +/// +/// `get` rather than indexing, for the reason [`parse_precision_param`] gives. fn parse_scale_param(name: &str) -> Option { let start = name.find('(')?; let end = name.rfind(')')?; - let mut parts = name[start + 1..end].split(','); + let mut parts = name.get(start + 1..end)?.split(','); parts.next()?; parts.next()?.trim().parse().ok() } @@ -814,7 +830,12 @@ fn parse_trino_time_with_tz(s: &str) -> Option { let mut op = offset_body.splitn(2, ':'); let oh: i32 = op.next()?.parse().ok()?; let om: i32 = op.next().unwrap_or("0").parse().ok()?; - shift_time(time_part, sign * (oh * 60 + om)) + // Checked, because both fields are free-form text: `i32::MAX` parses + // happily and only the multiply reveals it is not an offset. Returning + // `None` keeps the value as text, which is what every other unparseable + // temporal string here already does. + let offset_minutes = oh.checked_mul(60)?.checked_add(om)?.checked_mul(sign)?; + shift_time(time_part, offset_minutes) } /// Shift `HH:MM:SS[.f]` by `offset_minutes`, wrapping within the day. @@ -837,7 +858,12 @@ fn shift_time(time_part: &str, offset_minutes: i32) -> Option { let second: i32 = sf.next()?.parse().ok()?; let fraction = sf.next().map(parse_fraction_nanos).unwrap_or(0); - let total = hour * 60 + minute - offset_minutes; + // Checked for the same reason as the offset above: `hour` and `minute` are + // whatever the text held, not values already bounded by a clock. + let total = hour + .checked_mul(60)? + .checked_add(minute)? + .checked_sub(offset_minutes)?; let wrapped = total.rem_euclid(24 * 60); Some(ColumnValue::Time { @@ -1702,6 +1728,33 @@ mod tests { assert_eq!(type_name_scale("decimal(10,2)"), Some(2)); } + #[test] + fn param_parsers_decline_reversed_parentheses() { + // Called directly, because the public entry points cannot deliver this + // shape: `type_name_precision` and `type_name_scale` both gate on + // `TrinoTypeName::parse`, and a name whose `)` precedes its `(` leaves + // that `)` in the base name the gate matches on, so the gate rejects it + // first. That makes these two the only place the ordering can be + // asserted, and it is worth asserting: `find`/`rfind` locate the ends + // independently, and the gate is not their contract. + assert_eq!(parse_precision_param(")("), None); + assert_eq!(parse_scale_param(")("), None); + assert_eq!(parse_precision_param("decimal)10,2("), None); + assert_eq!(parse_scale_param("decimal)10,2("), None); + } + + #[test] + fn param_parsers_read_well_formed_arguments() { + // The `get` guard must not change what a real signature yields. + assert_eq!(parse_precision_param("varchar(50)"), Some(50)); + assert_eq!(parse_precision_param("decimal(10,2)"), Some(10)); + assert_eq!(parse_scale_param("decimal(10,2)"), Some(2)); + // An empty argument list, and a scale that is not there at all: both + // already returned `None` by way of the parse failing. + assert_eq!(parse_precision_param("varchar()"), None); + assert_eq!(parse_scale_param("varchar(50)"), None); + } + // --- TrinoTypeName::parse: precision-argument-in-the-middle --- // // `TrinoTypeName::parse` must not truncate at the first `(`: that would @@ -2311,6 +2364,55 @@ mod tests { ); } + #[test] + fn time_with_timezone_offset_too_large_to_be_an_offset_stays_text() { + // Found by the `json_value` fuzz target. The offset fields are + // free-form text, so a value far outside any real zone parses as an + // `i32` and only overflows when converted to minutes. Release builds + // carry no overflow checks, so an unchecked multiply here reports a + // *different* time instead of declining the value. + assert_eq!(parse_trino_time_with_tz("00:00:00+949378864"), None); + assert_eq!( + json_to_column_value( + Value::String("+999\0\0\0\0+00949378864".into()), + &TrinoTy::Option(Box::new(TrinoTy::TimeWithTimeZone)), + ), + ColumnValue::String("+999\0\0\0\0+00949378864".into()) + ); + } + + #[test] + fn time_with_timezone_hour_field_too_large_stays_text() { + // The same class one frame down, in `shift_time`: the time-of-day hour + // is text too, so `hour * 60` overflows before the offset is ever + // applied. + assert_eq!(parse_trino_time_with_tz("2147483647:00:00+01:00"), None); + } + + #[test] + fn time_with_timezone_real_offsets_still_parse() { + // The checked arithmetic must not narrow what a valid offset can be: + // the widest zones in use are +14:00 and -12:00. + assert_eq!( + parse_trino_time_with_tz("13:14:15+14:00"), + Some(ColumnValue::Time { + hour: 23, + minute: 14, + second: 15, + fraction: 0, + }) + ); + assert_eq!( + parse_trino_time_with_tz("13:14:15-12:00"), + Some(ColumnValue::Time { + hour: 1, + minute: 14, + second: 15, + fraction: 0, + }) + ); + } + #[test] fn timestamp_string_parses_to_column_timestamp() { assert_eq!(