Skip to content

Commit 01a2b26

Browse files
maltesanderclaude
andcommitted
test: close the fuzz and property-test gaps a security review found
A review listed five gaps. Four of the five are pure safe Rust, so they land as `proptest` suites next to the code rather than as fuzz targets: `fuzz/` exists for the paths where AddressSanitizer can report a read or write outside an allocation, and where the worst outcome is a panic or a wrong answer, a property test finds both on stable, on every `cargo test`, for a fraction of the CPU time. `param_convert` and `numeric_convert` in particular contain no `unsafe` at all: the pointer and length arrive from `SQLBindParameter`, but `ffi/params.rs` has resolved them to a `&str` and two scalars before this code sees them. The one genuine `unsafe` gap gets the fuzz target. - `parse_attributes`, a new fuzz target over `ffi::setup::parse_attributes_w`, the raw `*const u16` walk behind `ConfigDSNW`. Three shapes: aligned, offset by a byte so every read is unaligned, and a segment past the per-segment scan limit. Every shape terminates its own buffer, because the parser's contract says the buffer is terminated and an ASAN report from an unterminated one would be a report about the fuzz target. - `test_support::parse_attributes_summary_w`, which is how that target reaches a `pub(crate)` parser from a separate crate. Behind the default-off `test-support` feature. It lives in `test_support` rather than beside the parser because a `pub unsafe fn` in `src/ffi/` means "ODBC entry point" to the diagnostics-table guard, and this is a test hook. - `param_convert`: rendering never expands past `MAX_DECIMAL_EXPANSION_DIGITS`, rendering round-trips through the parser, `to_integer` agrees with `i128::from_str`, truncation composes, and a `SQL_NUMERIC_STRUCT` reconstructs the literal it was built from. Exponents are generated on both sides of the expansion bound rather than left to a token soup that reaches `1e-1048576` by luck. - `numeric_convert`: the whole *C to SQL: Numeric* table is total over NaN and both infinities, integers reach their target exactly when they are in range, and a character target accepts exactly what fits. - `connect_params`: a whole connection string round-trips, not one pair. The existing proptest renders a single keyword, so nothing follows the value it checks and a `}` that ends its own quoting early has nothing to run into. - `escape`: a grammar of escape tokens against four dialects, plus an oracle the existing tests cannot be, since an input with no `{` returns on the early-out without the scanner running. - `types::conversions`: all eighteen `*_from_raw` swept across their entire 16-bit domain. `c_data_type_from_raw` is the documented exception, its three ODBC 2.x spellings normalising to their 3.x variants by design. Every oracle was checked by mutation: the code was broken in the way the property describes and the test watched to fail, then reverted. The `parse_attributes` target was checked the same way, and ASAN reported a heap-buffer-overflow. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent dddfeb0 commit 01a2b26

14 files changed

Lines changed: 1253 additions & 29 deletions

File tree

‎.github/workflows/build.yaml‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -261,6 +261,8 @@ jobs:
261261
run: cargo +nightly fuzz run utf16 --target x86_64-unknown-linux-gnu -- -max_total_time=30
262262
- name: Fuzz column_value
263263
run: cargo +nightly fuzz run column_value --target x86_64-unknown-linux-gnu -- -max_total_time=30
264+
- name: Fuzz parse_attributes
265+
run: cargo +nightly fuzz run parse_attributes --target x86_64-unknown-linux-gnu -- -max_total_time=30
264266

265267
# Verifies the crate can actually be packaged, without publishing anything.
266268
#

‎AGENTS.md‎

Lines changed: 22 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1324,20 +1324,35 @@ let ptr = unsafe { arena.as_mut_ptr().cast::<u8>().add(1) }.cast::<u16>();
13241324
### Fuzzing
13251325

13261326
`fuzz/` holds [cargo-fuzz](https://github.com/rust-fuzz/cargo-fuzz) targets for
1327-
the memory-marshalling hot paths, `write_column_value` and `utf16`. Each
1328-
allocates its output buffer at exactly the caller-declared length, so
1329-
AddressSanitizer catches any overrun that clippy cannot see. It is its own
1330-
Cargo workspace, because libFuzzer needs nightly, so the root build ignores it.
1331-
A short smoke run of both targets runs on every PR (the `fuzz` job in
1332-
`build.yaml`).
1327+
the raw-pointer paths: `write_column_value`, `utf16` and
1328+
`ffi::setup::parse_attributes_w`. The first two allocate their output buffer at
1329+
exactly the caller-declared length, so AddressSanitizer catches any overrun that
1330+
clippy cannot see; the third walks a Driver-Manager pointer looking for a
1331+
terminator. It is its own Cargo workspace, because libFuzzer needs nightly, so
1332+
the root build ignores it. A short smoke run of every target runs on every PR
1333+
(the `fuzz` job in `build.yaml`).
13331334

13341335
```bash
13351336
cargo install cargo-fuzz
13361337
cargo +nightly fuzz run utf16
13371338
cargo +nightly fuzz run column_value
1339+
cargo +nightly fuzz run parse_attributes
13381340
```
13391341

1340-
See [`fuzz/README.md`](fuzz/README.md) for what is and is not worth fuzzing.
1342+
**A fuzz target is for `unsafe` code.** Where the code is safe Rust the worst
1343+
outcome is a panic or a wrong answer, and a `proptest` suite next to the code
1344+
finds both on stable, in far less CPU time, on every `cargo test` rather than in
1345+
a 30-second smoke run. `escape`, `types::connect_params`, `param_convert`,
1346+
`numeric_convert` and `types::conversions` are covered that way, with an oracle
1347+
rather than only a never-panics assertion wherever one exists.
1348+
1349+
Reaching a `pub(crate)` item from `fuzz/`, which is a separate crate, goes
1350+
through a wrapper gated on the default-off `test-support` feature.
1351+
`parse_attributes_summary_w` is the example to copy.
1352+
1353+
See [`fuzz/README.md`](fuzz/README.md) for what is and is not worth fuzzing, and
1354+
for why a fuzz target must terminate the buffer it hands to a parser whose
1355+
contract says it is terminated.
13411356

13421357
### Benchmarks
13431358

‎CHANGELOG.md‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,22 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
77

88
## [Unreleased]
99

10+
### Added
11+
12+
- `test_support::parse_attributes_summary_w`, behind the default-off
13+
`test-support` feature. It reports what the `ConfigDSNW` attribute-list parser
14+
read from a raw `*const u16`, so the new `parse_attributes` fuzz target can
15+
reach a `pub(crate)` parser from a separate crate. A driver has no reason to
16+
call it, and a build without the feature does not export it.
17+
- A `parse_attributes` fuzz target, covering that parser over aligned,
18+
deliberately misaligned and overlong-segment buffers. It joins the per-PR
19+
ASAN smoke run.
20+
- Property tests for the paths that carry no `unsafe` and so are not worth a
21+
fuzz target: the escape translator against a grammar of escape tokens and four
22+
dialects, `ConnectParams` round-tripping a whole connection string rather than
23+
one pair, the two parameter-conversion tables, and every `*_from_raw` swept
24+
across its entire input domain.
25+
1026
## [0.1.0] — 2026-08-04
1127

1228
First release, so this section describes what the crate offers rather than what

‎Cargo.toml‎

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,9 @@ exclude = [
3939
"rustfmt.toml",
4040
".pre-commit-config.yaml",
4141
".markdownlint.yaml",
42+
# Saved `proptest` failure seeds. Test-only data, and the tests that read it
43+
# are not compiled from the published tarball.
44+
"proptest-regressions/",
4245
]
4346

4447
# 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"]
5255
features = ["test-support"]
5356

5457
[features]
55-
# Test support for driver crates: the `conformance` module, which drives
56-
# `SQLGetInfoW` through the real C ABI to check an info type's return shape.
58+
# Test support for driver crates and for `fuzz/`. Two things:
59+
#
60+
# - the `conformance` module, which drives `SQLGetInfoW` through the real C ABI
61+
# to check an info type's return shape;
62+
# - `test_support::parse_attributes_summary_w`, which lets the `parse_attributes`
63+
# fuzz target reach a `pub(crate)` raw-pointer parser from a separate crate.
5764
#
5865
# Default-off because it is test code. Compiled unconditionally it lands in
5966
# every driver's production binary, and it reaches an `unreachable!()` through a
6067
# public `unsafe fn` taking a caller-supplied `u16` — a panic path a shipped
6168
# driver has no reason to carry. Driver test suites enable it under
62-
# `[dev-dependencies]`.
69+
# `[dev-dependencies]`; `fuzz/Cargo.toml` enables it on its path dependency.
6370
test-support = []
6471

6572
[dependencies]

‎fuzz/Cargo.toml‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,16 @@ test = false
2525
doc = false
2626
bench = false
2727

28+
[[bin]]
29+
name = "parse_attributes"
30+
path = "fuzz_targets/parse_attributes.rs"
31+
test = false
32+
doc = false
33+
bench = false
34+
2835
[dependencies]
2936
arbitrary = { version = "1", features = ["derive"] }
3037
libfuzzer-sys = "0.4"
31-
stackable-odbc-core = { path = ".." }
38+
# `test-support` is what makes `ffi::setup::parse_attributes_summary_w` visible.
39+
# The attribute parser itself is `pub(crate)`, and this is a separate crate.
40+
stackable-odbc-core = { path = "..", features = ["test-support"] }

‎fuzz/README.md‎

Lines changed: 53 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1,25 +1,58 @@
11
# Fuzz targets
22

3-
These targets cover the `unsafe` pointer-marshalling paths in
4-
`stackable-odbc-core`, which is where AddressSanitizer catches what clippy
5-
cannot see. Each one allocates its output buffer at exactly the size a correct
6-
application would, so any write past the end is a reported error rather than a
7-
silent overrun into neighbouring memory.
3+
These targets cover the `unsafe` raw-pointer paths in `stackable-odbc-core`,
4+
which is where AddressSanitizer catches what clippy cannot see.
85

9-
That size is the `BufferLength` argument for a variable-length C target, and the
10-
C type's own size for a fixed-length one, which ignores `BufferLength`
11-
altogether.
6+
The two marshalling targets allocate their output buffer at exactly the size a
7+
correct application would, so any write past the end is a reported error rather
8+
than a silent overrun into neighbouring memory. That size is the `BufferLength`
9+
argument for a variable-length C target, and the C type's own size for a
10+
fixed-length one, which ignores `BufferLength` altogether.
11+
12+
The third goes the other way: it hands a parser an *input* buffer sized exactly
13+
to its contents, so a read that runs past the terminator is reported rather than
14+
finding more of the same allocation.
1215

1316
- `utf16` covers `utf16_to_string` and `write_utf16`.
1417
- `column_value` covers `write_column_value` across every marshallable value
1518
variant and C target type, which is the full coercion matrix.
19+
- `parse_attributes` covers `ffi::setup::parse_attributes_w`, the attribute-list
20+
walk behind `ConfigDSNW`, over aligned, deliberately misaligned and
21+
overlong-segment buffers.
22+
23+
## What belongs here, and what does not
24+
25+
The line is `unsafe`, not importance. A target earns its nightly toolchain and
26+
its ASAN build when the failure it is hunting is a read or write outside an
27+
allocation. Where the code is safe Rust, the worst outcome is a panic or a wrong
28+
answer, and a property test finds both on stable, in a fraction of the CPU time,
29+
on every PR rather than in a 30-second smoke run.
30+
31+
So the pure-safe parsers are covered by [`proptest`](https://docs.rs/proptest)
32+
suites next to the code instead, asserting never-panics *and* an oracle wherever
33+
one exists:
1634

17-
The pure-safe parsers, `translate_escapes`, `ConnectParams::parse` and the
18-
drivers' own type-name parsers, contain no `unsafe`, so AddressSanitizer adds
19-
nothing over property tests. They are covered by
20-
[`proptest`](https://docs.rs/proptest) suites next to the code, which run on
21-
stable under an ordinary `cargo test` and assert both never-panics and
22-
round-trip invariants.
35+
| Module | What the properties assert |
36+
| --- | --- |
37+
| `escape` | A grammar of escape-ish tokens against four dialects; an unknown escape and its surroundings survive byte for byte |
38+
| `types::connect_params` | Every pair survives `parse` ∘ `to_connection_string`, and no keyword is injected by a value containing `}` |
39+
| `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 |
40+
| `numeric_convert` | The whole *C to SQL: Numeric* table is total; integers reach their target exactly when they are in range |
41+
| `types::conversions` | Every `*_from_raw` swept across its entire 16-bit domain, round-tripping |
42+
43+
`parse_attributes` is on this side of the line because it steps a raw `*const
44+
u16` looking for a terminator, and because the Driver Manager gives it no
45+
alignment guarantee.
46+
47+
### Terminate the buffer
48+
49+
The parser's safety contract is that the pointer is null or double-null
50+
terminated. A fuzz target that hands it an unterminated buffer will get an ASAN
51+
report, and the report will be about the target: the read past the end is the
52+
caller breaking a contract, not the parser exceeding one. `parse_attributes`
53+
appends the terminator itself and fuzzes what comes before it, and reaches the
54+
per-segment scan limit with a run that is long but still inside a real
55+
allocation.
2356

2457
## Running
2558

@@ -30,8 +63,14 @@ libFuzzer does.
3063
cargo install cargo-fuzz
3164
cargo +nightly fuzz run utf16
3265
cargo +nightly fuzz run column_value
66+
cargo +nightly fuzz run parse_attributes
3367
```
3468

69+
`parse_attributes` reaches `parse_attributes_w`, which is `pub(crate)`, through
70+
`test_support::parse_attributes_summary_w`. That wrapper is gated behind the
71+
default-off `test-support` feature, which this crate enables on its dependency,
72+
so a shipped driver never exports it.
73+
3574
If cargo-fuzz fails with "sanitizer is incompatible with statically linked
3675
libc", it picked a musl target. Pin the gnu triple explicitly, which is what CI
3776
does:
Lines changed: 120 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,120 @@
1+
//! `ConfigDSNW`'s attribute-list parser, driven over raw pointers.
2+
//!
3+
//! `test_support::parse_attributes_summary_w` wraps
4+
//! `ffi::setup::parse_attributes_w`, which walks a `*const u16` the Driver
5+
//! Manager supplied, hunting for the double null that ends the list. It is the
6+
//! one parser in core whose failure mode is a read past the end of an
7+
//! allocation rather than a panic, which is what makes AddressSanitizer worth
8+
//! the nightly toolchain here.
9+
//!
10+
//! # Every buffer is terminated
11+
//!
12+
//! The parser's safety contract is that the pointer is null, or points to a
13+
//! valid double-null-terminated `u16` sequence. So each shape below appends that
14+
//! terminator itself and fuzzes what comes *before* it. Handing the parser an
15+
//! unterminated buffer would certainly produce an ASAN report, and it would be a
16+
//! report about this file: the read past the end would be the caller breaking a
17+
//! contract it agreed to, not the parser exceeding one. What is worth fuzzing is
18+
//! the walk over a buffer that is terminated but says nothing else sensible.
19+
//!
20+
//! The parser bounds each segment at `i16::MAX` code units precisely so a caller
21+
//! that gets this wrong is contained rather than unbounded, and the
22+
//! `OverlongSegment` shape reaches that bound from inside a real allocation.
23+
24+
#![no_main]
25+
26+
use arbitrary::Arbitrary;
27+
use libfuzzer_sys::fuzz_target;
28+
use stackable_odbc_core::test_support::parse_attributes_summary_w;
29+
30+
/// One code unit past the parser's own per-segment scan limit, which is
31+
/// `i16::MAX`. Declared here rather than imported because it is crate-private,
32+
/// and a copy that drifts makes this shape stop reaching the bound rather than
33+
/// start failing, so the assertion below checks the bound was actually hit.
34+
const PAST_SEGMENT_SCAN_LIMIT: usize = i16::MAX as usize + 1;
35+
36+
#[derive(Arbitrary, Debug)]
37+
enum Shape {
38+
/// A `u16`-aligned buffer, which is the ordinary case.
39+
Aligned,
40+
/// A buffer whose `u16` sequence starts at an odd byte address.
41+
///
42+
/// The Driver Manager promises no alignment, and the parser reads every code
43+
/// unit with `read_unaligned` for that reason. An aligned read of this
44+
/// pointer is undefined behaviour, and in a debug build it aborts without
45+
/// unwinding, which no panic hook can contain. A regression to
46+
/// `slice::from_raw_parts` would be caught here and nowhere else.
47+
Unaligned,
48+
/// A segment longer than the parser will scan, so the scan limit fires with
49+
/// every read still inside the allocation.
50+
OverlongSegment,
51+
}
52+
53+
#[derive(Arbitrary, Debug)]
54+
struct Input {
55+
shape: Shape,
56+
units: Vec<u16>,
57+
}
58+
59+
fuzz_target!(|input: Input| {
60+
match input.shape {
61+
Shape::Aligned => {
62+
let mut buf = input.units;
63+
buf.extend_from_slice(&[0, 0]);
64+
// SAFETY: `buf` is non-empty and ends in two zero `u16`s, so it is a
65+
// double-null-terminated sequence, and it outlives the call.
66+
let _ = unsafe { parse_attributes_summary_w(buf.as_ptr()) };
67+
}
68+
69+
Shape::Unaligned => {
70+
// One byte of padding in front, so the `u16` sequence begins at an
71+
// odd address. Everything after it stays a whole number of code
72+
// units, which keeps the terminator two aligned-to-the-sequence
73+
// zeros rather than a split pair.
74+
let mut bytes = vec![0u8];
75+
for unit in &input.units {
76+
bytes.extend_from_slice(&unit.to_le_bytes());
77+
}
78+
bytes.extend_from_slice(&[0, 0, 0, 0]);
79+
80+
// SAFETY: offset 1 is inside `bytes`, which holds `1 + 2n + 4`
81+
// bytes, so the sequence from there is `n + 2` whole `u16`s ending
82+
// in two zeros. The pointer is read only with `read_unaligned`, so
83+
// the odd address is sound, and `bytes` outlives the call.
84+
let ptr = unsafe { bytes.as_ptr().add(1) }.cast::<u16>();
85+
let _ = unsafe { parse_attributes_summary_w(ptr) };
86+
}
87+
88+
Shape::OverlongSegment => {
89+
// The fuzzed units come first, so their bytes still drive real
90+
// segments, and the overlong run is appended behind a separator.
91+
let mut buf = input.units;
92+
buf.push(0);
93+
94+
// Decided on the prefix alone, before the run and the terminator are
95+
// appended: those end the list by construction, so a check made
96+
// after them would be true every time and assert nothing.
97+
let ends_early = ends_the_list(&buf);
98+
99+
buf.resize(buf.len() + PAST_SEGMENT_SCAN_LIMIT, u16::from(b'A'));
100+
buf.extend_from_slice(&[0, 0]);
101+
102+
// SAFETY: as the aligned case; `buf` ends in two zero `u16`s.
103+
let (_, _, syntax_error) = unsafe { parse_attributes_summary_w(buf.as_ptr()) };
104+
105+
// Where the run is reached at all, the scan limit must have fired.
106+
assert!(
107+
syntax_error || ends_early,
108+
"a segment of {PAST_SEGMENT_SCAN_LIMIT} code units must trip the scan limit"
109+
);
110+
}
111+
}
112+
});
113+
114+
/// Whether the parser stops inside `units` rather than walking off its end.
115+
///
116+
/// It stops at an empty segment, which is a leading null or two consecutive
117+
/// ones.
118+
fn ends_the_list(units: &[u16]) -> bool {
119+
units.first() == Some(&0) || units.windows(2).any(|pair| pair == [0, 0])
120+
}
Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
# Seeds for failure cases proptest has generated in the past. It is
2+
# automatically read and these particular cases re-run before any
3+
# novel cases are generated.
4+
#
5+
# It is recommended to check this file in to source control so that
6+
# everyone who runs the test benefits from these saved cases.
7+
cc b397f155c0024183da7ee227f989dce4f106122c05ddcd6d0a0fd6ea193de960 # shrinks to text = "0e-1048577"
8+
cc 14cac6f9937a37466197f0f46da27ef8f7bf8f4e4dcd139f7ff5b9985a5b79eb # shrinks to text = "0e-2147483647"
9+
cc a63740d711d952f1a19c605c2d34fd42607826298239bcf9051d443bead7282b # shrinks to text = "0e-1048577"
10+
cc c1710352100792598c4ba42876c7f85a26ded650c3c2b3acd0d4a4dcd3163a54 # shrinks to text = ".214748364721474836472147483647", wide = 2, narrow = 3

0 commit comments

Comments
 (0)