test: close the fuzz and property-test gaps a security review found - #5
Merged
Conversation
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) <[email protected]>
…tarted The `from_raw_sweep` module cost the Miri job four and a half minutes, and it should never have reached it. The job filters with `--skip proptest`, which catches every other suite added alongside it, but these tests are not named proptests: they are thirteen exhaustive walks of a 16-bit domain, 850,000 interpreted match evaluations, over conversions that contain no `unsafe` for Miri to look at. Measured at 282s locally for the module, 7.8s once ignored. While measuring, `--report-time` showed five older tests in the same position, each in a module with no `unsafe` in it at all: - the four `escape` nesting tests, 44s together and the largest single module in the run; - `types::info_type_shape`'s raw sweep, 7.8s; - `backend`'s two-backend `SQLGetInfo` comparison, 5.2s; - `types::diagnostics_table`'s well-formedness scan, 2.6s; - `types::constants`' keyword uniqueness scan, 2.2s. Each of these five sits in a file that *already* carries this exact attribute and this exact reasoning on a neighbouring test, so this finishes a pass rather than starting one. `escape.rs` is the clearest case: its `pathological_nesting_*` test was skipped with the note that "escape.rs contains no `unsafe` at all, so Miri has no undefined behaviour to find here", and its four siblings then kept running. Nothing loses coverage. Every test skipped here runs in full in the `unit-tests` job, where the whole set costs 1.6 seconds. Full `cargo miri test --lib -- --skip proptest`, locally: ~750s before this commit, 470s after the sweeps are ignored, 407s after the other five. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
A security review listed five gaps in the fuzz coverage. Only one of them is genuinely unsafe code, so only one becomes a fuzz target and the rest become property tests next to the code they cover.
The new parse_attributes fuzz target drives ffi::setup::parse_attributes_w, the raw *const u16 walk behind ConfigDSNW, over aligned buffers, buffers offset by a byte so every read is unaligned, and segments past the per-segment scan limit. It reaches that pub(crate) parser through test_support::parse_attributes_summary_w, a new export behind the default-off test-support feature.
The other four gaps are pure safe Rust. param_convert and numeric_convert contain no unsafe at all, because ffi/params.rs resolves the caller's pointer and length to a &str and two scalars before either module sees them, so AddressSanitizer has nothing to report there that a property test on stable does not already catch. Those two modules, along with the escape translator, ConnectParams and all eighteen *_from_raw conversions, get proptest suites asserting real oracles rather than only that nothing panics.
Every oracle was verified by breaking the code in the way the property describes, watching the test fail, and reverting. The fuzz target was verified the same way, and ASAN reported a heap-buffer-overflow.
Checklist
pre-commit run --all-filespasses — this is the single source of truth for what must pass.CHANGELOG.mdhas an entry under## [Unreleased], if this is user-facing. Any change to a public type, trait method or exported FFI contract counts, since driver crates consume them.If it applies
MIRIFLAGS="-Zmiri-disable-isolation" cargo +nightly miri test -p stackable-odbc-core --lib -- --skip proptestRUSTFLAGS="--cfg loom" cargo test --lib loom_testsNotes for the reviewer