tests: sharpen the cargo-fuzz oracles, and fix what they found - #37970
Closed
def- wants to merge 22 commits into
Closed
tests: sharpen the cargo-fuzz oracles, and fix what they found#37970def- wants to merge 22 commits into
def- wants to merge 22 commits into
Conversation
`ProtoStateFieldDiffs::validate` summed the `data_lens` entries with unchecked arithmetic. Overflow checks are off in the release and optimized profiles, so a diff declaring `data_lens = [u64::MAX, 1]` with an empty `data_bytes` wrapped the sum to 0, matched the real byte length, and passed validation. `ProtoStateFieldDiffsIter::next` then sliced `0..u64::MAX` out of an empty slice. State diffs are read from consensus on every state update, so this input is untrusted and reachable from a corrupted or crafted blob. Under overflow checks (the `ci` profile, `cargo test`, cargo-fuzz builds) the panic is the addition itself. Sum with `checked_add` instead and reject on overflow, so a malformed diff becomes the same decode error as every other inconsistency `validate` catches, rather than a panic in the iterator. Adds `proto_state_diff_data_lens_overflow_is_error`, which panics inside `validate` without this change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`LazyPartStats` holds its `ProtoStructStats` undecoded, and its `RustType<Bytes>` impl stores the bytes without validating them, so a value read off blob carries whatever encoding was there. `Debug` called `decode()`, which `expect`s that encoding to be well-formed. That is reachable on the persist state-load path, and specifically on its rejection path: `Spine::validate` formats the whole spine into the message it rejects with, and `Trace::unflatten` turns that into the decode error a `Rollup::from_proto` caller sees. A rollup whose trace fails validation *and* carries malformed part stats therefore panicked while building the error meant to reject it, so the shard failed to load by panic rather than by error, on every attempt. Split the fallible half out as `try_decode` and have `Debug` render an `<undecodable: ...>` placeholder instead of panicking. `decode()` keeps its panicking contract for callers holding stats they encoded themselves, now documented as such. NOTE: the read-path callers of `decode()` (`fetch.rs`, filter pushdown in `operators/shard_source.rs`) run on the same untrusted bytes and still panic on a malformed encoding. Left alone here because each needs a decision about what a missing-stats fallback should do. Adds `lazy_part_stats_debug_does_not_panic_on_garbage`, which panics in `Debug` without this change. Found by the `rollup_proto_roundtrip` cargo-fuzz target in release-qualification 1330 (v26.35.0-rc.1). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`Spine::introduce_batch` sized each maintenance step as `(8 << batch_index) * effort`, where `batch_index` comes from the inserted batch's `len`. Both steps were unchecked, and the result was `as`-cast to `isize`. A `len` near `2^60` makes the shift land on the `isize` sign bit, so the budget handed to `apply_fuel` was negative. `FuelingMerge::work` then read that back with `*fuel as usize`, which wraps a negative budget to a huge one, spent `remaining_work` against it, and subtracted that from the still-negative `fuel` -- an underflow. This is reachable from a corrupted or crafted blob: `Trace::unflatten` replays decoded legacy batches through the real spine, and its `MAX_TOTAL_LEN` guard only caps the total at `usize::MAX >> 3`, which still admits such a `len`. Under overflow checks (the `ci` profile, `cargo test`, cargo-fuzz builds) the panic is the subtraction. In the release and optimized profiles it wraps silently and leaves the merge's fuel accounting meaningless instead. Compute the fuel with checked arithmetic and saturate at `isize::MAX`. Fuel is a budget, so an over-large one only completes merges sooner, whereas a wrapped one can go negative and starve them. In `work`, read a negative budget as zero available rather than as a huge one, which also retires the two `as` casts the `TODO` there asked about. Adds `spine_fuel_isize_overflow`, which panics in `work` without this change. Found by the `rollup_proto_roundtrip` cargo-fuzz target in release-qualification 1332 (v26.35.0-rc.2). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`Reader` reuses one buffer across object-container blocks, and `fill_buf` only ever grew it. A block shorter than its predecessor therefore left that predecessor's tail readable past its own payload, because everything downstream reads the buffer by its length: `read_next` slices `self.buf[self.buf_idx..]`, and `Codec::decompress` hands the whole buffer to the decompressor. A block whose declared object count outran its own bytes would then decode those stale bytes into values rather than running out of input. Truncate to the payload after reading it. `Vec::truncate` keeps the allocation, so the buffer is still reused. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A data block's object count is read straight off the wire and `safe_len` capped it only at `MAX_ALLOCATION_BYTES` — a sensible ceiling for a byte length, an enormous one for a count, and unrelated to the block it describes. A zero-width schema (an empty record, or a record of only `null` fields) encodes every object to no bytes, so a handful of wire bytes could claim hundreds of millions of objects and the reader would decode every one, allocating a `Vec` and per-field `String` each time. Measured: a ~30-byte file spends 47s on 100M objects, minutes at the old ceiling. Bound the count against the block that is supposed to hold those objects, deciding which bound applies the way `decode.rs` already does for an array block: the byte floor when the object schema has a proven positive one, otherwise the node-weighted `MAX_VALUE_NODES` cap, since no payload length can constrain a count of zero-width objects. Unlike an array block the two are alternatives, not both applied. An array materializes all of its elements at once so its cap is about retained memory, while a block's objects are yielded and dropped one at a time, so this cap is about work amplified out of a few bytes. Once the byte floor holds the work is linear in the file, and capping nodes as well would reject a legitimate large block of small records. The count is bounded after decompression, since the declared byte size is the compressed size. Found by the reader_decode cargo-fuzz target, which timed out within seconds of drawing block counts from the range `safe_len` accepts. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`parse_option_map` committed on the `MAP` keyword alone and then demanded
`[`, so a bare `map` in an option-value position was a hard parse error
rather than falling through to the item name it is.
`map` is a legal bare identifier everywhere else — `SELECT "map"` prints
as `SELECT map` and reparses fine — so `AstDisplay` prints an option whose
value is the item name `map` unquoted, and that output failed to reparse:
CREATE SINK s FROM t INTO KAFKA CONNECTION c (TOPIC CONFIG = "map")
prints `TOPIC CONFIG = map` and then fails with "Expected left square
bracket, found right parenthesis".
Look ahead for the bracket instead. The alternative, quoting `map` in
`can_be_printed_bare`, would change the printed form of every `map`
identifier in every position to work around one non-backtracking call
site; the sibling branches of `parse_option_value` all backtrack already.
Found by the sql_roundtrip cargo-fuzz target.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`AND` and `OR` discard an operand's error once another operand fixes the
result: `Or::eval` returns `true` as soon as it sees a `true` operand,
dropping any error it collected, and `And::eval` does the same for
`false`. So an erroring subexpression under one of them is not
observable.
`MapFilterProject::optimize`'s memoization pass descended into those
operands like any other child. A subexpression shared between two of them
therefore became a mapped column, and `SafeMfpPlan::evaluate_inner`
evaluates every mapped column up front, with `?`. That surfaced an error
the expression suppresses, so a row the plan should emit failed the whole
query instead. `MfpPlan::create_from` runs `optimize` unconditionally, so
this is on the path every dataflow operator's linear pipeline takes.
SELECT * FROM t WHERE ((a * a) <= (a * a)) IS NULL OR b IS NULL
with `a` large enough that `a * a` overflows int8 and `b` NULL should
return the row on the strength of the second disjunct. Sharing is what
makes it reachable: a singly-referenced part is inlined back by
`inline_expressions`, so only a part with two or more uses survives as a
column.
Withhold fallible `AND`/`OR` operands from eager memoization, alongside
the `If` branches and `COALESCE` tail already withheld. Infallible ones
stay eligible, which is what keeps the shared comparisons of a null-safe
join predicate in one column each -- gating all operands regressed
`join-implementation`'s plan for `(#0 = #1) OR (#0 IS NULL AND #1 IS
NULL)`. Also withhold `error_if_null`'s message operand, which is only
evaluated when the first operand is NULL and had the same exposure.
This costs plan quality wherever a fallible expression is shared across the
operands of an AND/OR. Every affected plan stays correct and recomputes the
expression instead of sharing a Map column:
* `canonicalize_mfp.spec` and two cases in `literal_constraints.slt`, each
annotated in place.
* TPC-H q22, in `tpch_select.slt`, `tpch_create_index.slt` and
`tpch_create_materialized_view.slt`, and CH-benCHmark q22 in
`chbench.slt`. `substring(c_phone, 1, 2)` leaves its Map column for all
seven operands of the `IN` list, in each of the two filters over
`customer`.
* Nine builtin-view plans in `catalog_server_explain.slt`.
`jsonb_array_length` doubles in `mz_clusters` and `mz_cluster_replicas`,
`parse_catalog_create_sql` doubles in `mz_sources`, and a
`case when ... coalesce` chain is recomputed per operand in
`mz_arrangement_sizes_per_worker`, the four `mz_records_per_dataflow`
variants, and `mz_aws_privatelink_connection_statuses`.
* `aoc_1219.slt` and `github-5536.slt`, where a shared `substr` and a shared
negation are recomputed.
In all of them the shared part occurs in *every* operand, which is the
refinement that would recover them: if no operand can reach the value that
ends the AND/OR without first evaluating the shared part, then the part's
error cannot be swallowed and hoisting it stays sound. Not implemented here,
and worth its own change given how much plan quality is on the table.
Adds `optimize_preserves_and_or_error_suppression`, which fails with
`Err(NumericFieldOverflow)` without this change. Found by the
`mfp_optimize` cargo-fuzz target, which failed release-qualification
1330, 1332, 1334 and 1335 (v26.35.0-rc.1 through rc.4) on this.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`Display for EvalError` panicked on `OutOfDomain(None, None, _)`, a state no constructor produces: all four `EvalError::OutOfDomain` call sites set at least one bound. It is reachable by decoding a corrupted or forged `ProtoEvalError`, because `from_proto` accepts any pair of present `ProtoDomainLimit`s, and `DomainLimit::None` is a present field with an unbounded value. That path is live in clusterd. `DataflowErrorSer::Display` decodes error bytes read straight out of a persist shard and delegates to `EvalError::fmt`, and the index peek path renders the error-trace key. So one such error in a shard panics the dataflow, and re-panics on every retry. Render the degenerate domain instead. Rejecting it in `from_proto` would not help: `DataflowErrorSer::deserialize` expects the conversion to succeed, so the same bytes would panic a few frames earlier on the same path. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`CharLength`, `VarCharMaxLength`, `NumericMaxScale` and `TimestampPrecision` each have a `TryFrom<i64>` that rejects out-of-domain values, but their `from_proto` copied the wire value straight through. `from_proto` is a trust boundary for durable catalog state and for the compute/storage protocol, so a corrupt or forged value became a type modifier no SQL statement could have produced. Consumers rely on the invariant being real: a `TimestampPrecision` above `MAX_PRECISION` panics `round_to_precision`. Route all four through `TryFrom<i64>` and surface the rejection as an `InvalidFieldError`. Narrow the `Arbitrary` impls to the same domains, which the validation above makes load-bearing. `CharLength` and `VarCharMaxLength` mapped an arbitrary `u32` through `% 300`, so they generated 0, and there is no `char(0)` or `varchar(0)`. `NumericMaxScale` and `TimestampPrecision` used `derive(Arbitrary)`, which knows nothing of `NUMERIC_DATUM_MAX_PRECISION` or `MAX_PRECISION`. Hand-written strategies keep the generated values inside the domain the type promises, so a round-trip property cannot fail on a value the constructor would have refused to build. `source-datas.txt` holds such a value, generated before the narrowing: 10 of its 40 SourceData stability entries carry a `numeric` max scale or a `timestamp` precision outside the domain, so `from_proto` now refuses to decode them. Those 10 are regenerated from the narrowed strategies. The other 30 stay byte-identical, and the encoding itself does not change, so `MINIMUM_CONSOLIDATED_VERSION` does not move.
`VersionedRelationDesc::validate` treats a names/column-types length mismatch and an out-of-bounds key column index as corruption, but `RelationDesc::from_proto` accepted both. Nothing downstream catches them: such a desc decodes and re-encodes cleanly and only panics at first use, `iter()` indexing `typ.columns()[typ_idx]` out of bounds or `into_iter()` tripping `zip_eq`. Both are reachable from untrusted proto bytes. Also record why only dense descs may be handed to `Codec::encode_schema`. `ProtoRelationDesc` has no field for the `ColumnIndex` keys, only the values in `ColumnIndex` order, so a sparse desc, which is what `VersionedRelationDesc::at_version` returns once a column has been dropped and what `RelationDesc::apply_demand` returns, comes back from `from_proto` renumbered to `0..n`. `ColumnIndex::to_stable_name` is the arrow field name that `RowColumnarEncoder` and `RowColumnarDecoder` agree on, so encoding a sparse desc as the schema of data written under the original indexes builds a decoder that looks up the wrong fields.
`VersionedRelationDesc::validate` derived each column's versions with `dropped.unwrap_or(added)`, which discards the add's version whenever a column was also dropped. Every version past the root is stamped exactly once, by the add or the drop that created it, so a column added after the root and later dropped accounts for two of them and both have to be counted. Collapsing them lost any desc where a non-root column was later dropped. Count both, filtering the root, which is the one version many columns can share.
`decPackedToNumber` writes one BCD digit per input nibble into the coefficient of the `decNumber` it is handed, without checking that the coefficient can hold them. `Numeric` is a `Decimal<13>`, whose coefficient is a fixed `[u16; 13]` holding 39 digits, so a `ProtoNumeric` whose `bcd` exceeds 20 bytes overruns a struct on our stack. At 21 bytes that silently corrupts the value, yielding a `Datum::Numeric` holding `Infinity`, which the encoder treats as impossible for this datum type. A few KB of `bcd` walks off the frame and segfaults. `ProtoRow` decoding is reachable from untrusted bytes via `Codec for Row` and `Codec for SourceData`, so a corrupted or tampered persist blob turns what should be a decode error into memory corruption in environmentd or clusterd. The empty-bcd segfault was already rejected here, so bound the upper end next to it. `to_packed_bcd` never emits more than 20 bytes, so nothing we write is rejected. Adds `proto_row_oversized_numeric_bcd_is_error`, which covers the silent corruption and the segfault, and asserts a max-precision numeric still round-trips so the bound cannot drift below what we encode. Found by the row_proto_roundtrip cargo-fuzz target. That target cannot detect this on its own: the corrupted value re-encodes and re-decodes to itself, so its round-trip oracle stays silent and only the multi-KB segfault shows up.
`MAX_REGEX_SIZE_BEFORE_COMPILATION` caps `pattern.len()` at 1 MiB to keep a
user-supplied regex from OOMing envd. A byte budget cannot do that job. The
memory goes to `regex-syntax` translating the pattern's AST into its HIR, a
stage that runs before anything `size_limit` measures, and while cost there is
linear in the AST's node count, the cost per node spans two orders of magnitude:
group, alternation, repetition, assertion 0.33 - 0.53 KB
literal, `.`, `[a-c]` 0.42 - 0.60 KB
`\d`, `\s` 0.65 - 1.1 KB
`\w`, `\W`, case-sensitive `\p{L}` 6 - 18 KB
case-insensitive `\p{L}`, `\p{Alphabetic}` 39 - 41 KB
So a pattern one byte under the limit could allocate ~7.8 GB. Confirmed on a
single-node environment with unprivileged SQL, no table and no cluster work,
from ~90 bytes of query text:
SELECT 'x' ~* repeat(chr(92) || 'p{L}', 209715);
envd VmRSS went 320 MB -> 7 620 MB over ~18 s (VmHWM 8 303 MB) while clusterd
stayed flat at ~420 MB, and only then did the statement error with "Compiled
regex exceeds size limit". `statement_timeout` does not help, since the
allocation precedes the error. Any user who can issue a `SELECT` could do this
on demand, and two concurrent queries exceed any realistic envd memory limit.
Project the cost from the AST instead, which is cheap to obtain (~30 ms and
~10 MB at 174 KB, flat in the pattern's shape) and charge each node a
conservative upper bound: 48 KB for a Unicode-property or Perl class item, the
kinds that expand to hundreds of ranges, and 768 bytes for everything else.
Reject above 1 GiB. Classes nested in a bracketed class are charged too, else
`[\p{L}]` repeated evades the budget at the same cost.
This is additive. The byte limit stays, since it is what bounds the AST parse
this check needs, and its number stays valid in the user-facing docs. Patterns
that were cheap remain accepted: a 1 MiB pattern of plain literals, the shape a
machine-generated alternation takes, still compiles. A counted repetition needs
no budget of its own, since `\p{L}{200000}` stays two AST nodes and the NFA
compiler's incremental `size_limit` check already rejects it in 40 ms at 17 MB.
A pattern we cannot parse falls through to `RegexBuilder` so users keep its
error message. That is not an escape hatch: both parse through the same
`regex-syntax` 0.8.11 with the same default configuration, verified against 35
syntax corner cases to never split in the direction that would skip the check.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ection Leap-second handling in `timezone()` was covered only in the TIMESTAMPTZ -> TIMESTAMP direction, which goes through `checked_add_with_leapsecond`. The mirror direction uses `checked_sub_with_leapsecond` and needs the same fold. `timezone()` reads a numeric offset as POSIX, where a positive offset is west of Greenwich, so `+00:00:01` has a `local_minus_utc` of -1 and moves the leap second forward past the minute boundary, while `-00:00:01` moves it back onto `:58`. Neither lands on `:59`, so both take the folding branch. A zero offset does keep the result on `:59`, exercising the `else` branch that restores the leap-second nanos.
`parse_timestamptz` applied the time-zone offset with chrono's `NaiveDateTime - FixedOffset`, which panics on overflow. `HIGH_DATE` is exactly `chrono::NaiveDate::MAX`, so a westward offset on a late time that day left chrono's range and aborted the process. The subtraction runs before `CheckedTimestamp::from_timestamplike`, so the range check could not catch it. A 24-byte literal such as `262142-12-31 23:00:00-01` was enough, reachable from a bind parameter, the HTTP SQL API, a string cast, or a source cast. The same subtraction also produced a value that cannot be represented. chrono stores a parsed `:60` as a leap second, legal only at a second-of-minute of 59, and copied that sub-second verbatim while shifting the second. An offset that is not a whole number of minutes, as in `1970-01-01 00:00:60+00:00:30`, therefore yielded a leap second at `:30`, which panics in row encoding on every re-render. Folding it into the next regular second matches what PostgreSQL does with `:60`. Both are fixed by the leap-aware checked offset helpers that `timezone_timestamp` already used for this defect class. They move from `mz_expr` to `mz_repr::adt::timestamp`, next to the range they enforce, so the two crates share one copy rather than keeping the leap-second reasoning in two places. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`round_to_precision` rounded with chrono's `+` and `-`, which panic on overflow. Rounding *up* off the last fraction of `HIGH_DATE`, itself exactly `chrono::NaiveDate::MAX`, leaves chrono's range and aborted the process. The rounding runs after the `CheckedTimestamp` range check, so the check could not catch it either. Every precision reaches this, each with its own quantum, and the lower the precision the fewer fractional digits it takes: at precision 0 a single `.5` rounds up a whole second, so `SELECT '262142-12-31 23:59:59.5'::timestamptz(0)` was enough. The precision is an ordinary type modifier on the column, so this is not a corner only the microsecond default can reach. Report `TimestampError::OutOfRange` instead, which the callers already propagate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`format_nanos_to_micros` rounded a nanosecond fraction to microseconds and
wrote the result as a fractional field. It could not carry out of that
field, so a fraction of `.9999995` or more became `1000000` microseconds,
which the trailing-zero stripper then reduced to `1`. The value rendered
about a second early with a nonsense fraction:
'2020-01-01 00:00:00.9999999' -> 2020-01-01 00:00:00.1
'12:34:56.9999999' -> 12:34:56.1
The same happened to chrono's leap-second representation, a sub-second of
one second or more, which `%S` has already rendered as `60` by then.
Nothing rounds on the way in, so these values reach the renderer as
stored. `mz_pgrepr::Value::decode_text` pushes the parsed nanoseconds
straight into the `Row` behind COPY in TEXT and CSV format, COPY FROM S3,
and text-format bind parameters, and `Row` keeps all of them. The TIME
cast does not round either, nor does DATE + TIME. Only the TIMESTAMP cast
rounds, which is why it hid this. The result was silent corruption: the
stored value displayed as something else, and the displayed text did not
re-parse to it, so text round trips through COPY TO and sinks lost it.
Split the helper in two. `split_nanos_to_micros` returns the whole-second
carry separately from a fraction that is guaranteed below one second, and
`format_micros` asserts that bound before writing. The carry is applied to
the timestamp itself, so it reaches `%H:%M:%S` and rolls the minute, day,
month or year as needed. Clearing the fraction before adding the second is
what makes this correct for a leap second too: the second after `23:59:60`
is `00:00:00` of the next minute.
Where the carry cannot be represented it is dropped and the fraction
saturates at `.999999`, rather than the value rolling over to something
unrepresentable. That is the last microsecond of `HIGH_DATE`, which is
exactly `chrono::NaiveDate::MAX`, and the last second of the day for
`NaiveTime`, which wraps to midnight rather than reaching PostgreSQL's
`24:00:00`.
Covered by `test_format_subsecond_carry`, which drives all three renderers
across the second, minute, day, month and year boundaries plus both
saturation edges, by `test_split_nanos_to_micros` on the helper, and by
`simple` blocks in `dates-times.slt` for the SQL-visible TIME and
DATE + TIME paths. Those have to be `simple`: they assert on the text
encoding, and the extended protocol that plain `query` uses returns TIME
and TIMESTAMP in binary.
The `datums.pt` golden moves with the fix. The DATUMS load generator's
leap-second TIMESTAMP is `23:59:59` plus 1234 milliseconds, whose whole
nanosecond count divided straight down to 1234000 microseconds and stripped to
a `.1234` fraction. Reducing it below one second first leaves the leap in the
seconds field where `%S` already put it, so the value renders as
`2019-07-24 23:59:60.234`.
`parse_time("")` returned `00:00:00`. PostgreSQL raises `invalid input
syntax for type time: ""`. The same held for `" "` and `":"`.
Such a string tokenizes to nothing the time grammar has to consume:
`determine_format_w_datetimefield` returns no format, `fill_pdt_time`
falls into its catch-all arm without consuming anything, the
leftover-token check passes because the deque is empty, and
`compute_time` then defaults hour, minute and second to zero. The value
looked deliberate rather than defaulted, and it reached `''::time`, the
text-format bind parameter path, and COPY in TEXT and CSV format.
Reject it in `parse_time` instead. Hour, minute and second are the only
fields the time grammar fills, and the only ones `compute_time` reads, so
requiring one of the three to be set is the whole condition.
The storage source cast `CastStringToTime` keeps the old reading via a new
frozen `parse_time_legacy`, as `CastStringToDate` and `CastStringToOid`
already do. Its stability contract is not stylistic: replication re-casts
the old tuple on delete, so a row ingested pre-upgrade as `00:00:00` would
post-upgrade be retracted as a cast error, leaving a phantom value in the
ok shard and a negative multiplicity in the err shard. Nothing is lost by
freezing it, because a PostgreSQL `time` column never renders as one of
these strings, so no export could have held one.
`parity_time` drops its fieldless input, since storage and SQL now
intentionally diverge there, and
`test_cast_string_to_time_frozen_fieldless` pins the frozen side.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`format_range` emits a bound quoted whenever the element renderer reports
`Nestable::MayNeedEscaping` and the rendering needs escaping, but
`parse_range_inner` extracted bounds with a bare
`take_while(|c| c != ',')` and handed the substring to the element parser
with the quotes still on it.
That mostly went unnoticed because the datetime parser tolerates a stray
`"`. It stops being tolerable the moment the quote abuts a token the
parser attributes meaning to: the `BC"` of a pre-Common-Era bound is read
as a time zone, so
SELECT tsrange('0001-02-24 BC'::timestamp, '2020-01-01'::timestamp)::text::tsrange
failed with "invalid input syntax for type range: 'BC' is not a valid
timezone". A `tsrange` or `tstzrange` with a pre-Common-Era bound could
not be read back from its own rendering.
Use `lex_quoted_element`, the same helper the list and array parsers use,
so the quotes come off and `\` escapes are processed. Unquoted, absent and
`empty` bounds take the path they always did.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`format_date` appends a ` BC` suffix for a pre-Common-Era date and then
returned `Nestable::Yes` regardless. `Nestable::Yes` is documented as
being for renderings that can *never* need escaping, and the whitespace in
` BC` is exactly what every element escaper quotes for, so the promise was
false.
It is not bookkeeping. `stringify_datum` returns this value for the `Date`
arm and the `Array`, `List`, `Map`, `Record` and `Range` arms use it to
decide whether to call `escape_elem`, as does the pgwire text encoder for
`Value::Date` nested in a container. So the quoting step was skipped for
every BC date inside a container, on both `::text` and the wire text
format:
SELECT ARRAY[('0001-02-24'::date - interval '1 YEAR')::date];
-- was {0001-02-24 BC}, PostgreSQL gives {"0001-02-24 BC"}
`format_timestamp`, `format_timestamptz` and `format_interval` all return
`MayNeedEscaping` for the same reason. `format_date` was the outlier, and
the same value already quoted correctly when rendered as a timestamp.
Same class as SQL-466, whose fix extended `needs_escaping` and left this
side untouched.
Return `MayNeedEscaping` only for the BC case, so a Common Era date keeps
the cheaper answer and its rendering is unchanged.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The numeric wire format's `0xD000`/`0xF000` sign words spell `±Infinity`, and `Numeric::from_sql` decodes them because it also decodes query *results*, where an infinite numeric is legitimate: aggregation overflow produces one. As a parameter it has to be rejected. The text path already rejects `'Infinity'::numeric` in `strconv::parse_numeric`, so accepting the binary spelling let a client smuggle in a `Datum::Numeric` value that no SQL literal can name. Covered by a pgtest pinning both signs, and pinning that `NaN`, which the text path does accept, and a finite value still decode.
Everything here is a fuzz target, its corpus, dictionary or seeding script, or
the runner. The edits to non-fuzz sources are all test-only or test-enabling:
a `#[derive(Debug, Eq, PartialEq)]` on `SourceExportStatementDetails` so its
round-trip oracle can compare and report, unit tests added inside
`mz_avro::schema`'s `mod tests`, a proptest added to `mz_repr::adt::interval`,
and a `pub use` in `mz_pgwire` exposing the auth sub-parsers that
`Codec::decode` never calls, so the pre-auth SASL/password grammars are
reachable from the codec fuzzer at all, and a note on `mz_sql`'s exhaustive
`OptimizerFeatures` destructuring pointing at the fuzz crate's `fuzz_features`
helper, and a `strconv::element_needs_escaping`
predicate exposing the union of the list, map and record escaper rules so that a
formatter's `Nestable` verdict can be checked against the real ones.
The recurring theme is oracles that could pass while the behaviour they name is
broken, generators that stall before the decode they exist to drive, and
coverage the module docs claimed but never reached.
Formatting and tooling:
* `bin/fmt` skipped the fuzz crates entirely. `cargo metadata` only reports the
workspace it is pointed at, and the `src/*/fuzz` crates attach to
`test/cargo-fuzz`, which the root workspace does not include. Run it over both
manifests, keyed by edition, since `gen` is an identifier in 2021 but a
reserved keyword in 2024 and rustfmt cannot parse a file at the wrong edition.
* Pin `style_edition = "2024"` in rustfmt.toml. Formatting style is repo-wide
and independent of the edition a crate parses as, but overriding `edition`
per package would otherwise drag the 2021 import-sorting style along with it.
Runner and profiles:
* Release qualification runs `--profile all`, not `--profile fruitful`. A target
nobody runs is a target whose oracle can quietly stop asserting anything, and
the 24h budget is the only place the low-yield targets get exercised at all.
The profile docs said the opposite.
* Pair the two durable-state decoders (rollup = full snapshot, state diff =
incremental) in `fruitful`. Both are read from blob/consensus on every state
load, so a decode panic there makes the shard unloadable.
* Add `strconv_parse_interval` to `fruitful`. Every other hand-written strconv
text parser is in the "untrusted text/bytes parsers" bucket, so what that
file's own doc calls "the most intricate parser in strconv" was the one
omission, and did not run under the 24h release-qualification profile at all.
* Record that ASan does not cover the cc-built C dependencies. The
justification for defaulting `--sanitizer=none` claimed our targets cannot
find memory-corruption bugs. Some can: `ProtoNumeric` decode hands untrusted
bytes to libdecnumber's `decPackedToNumber`, which writes past the coefficient
it is given. Keep the default, because ASan does not help there either.
cargo-fuzz instruments through RUSTFLAGS while the C is compiled by the `cc`
crate without them, so the FFI writes stay invisible unless CFLAGS carry
`-fsanitize=address` too. Say so, so the next reader does not read the old
wording as "no target can hit memory corruption".
mz-avro `schema_resolve`:
* Require the resolutions documented as infallible instead of guarding them.
Identity and writer->reader resolution succeed by construction, so
`if let Ok(..)` meant a promotion that regressed at a non-union position came
back as a top-level `Err` and switched the decode oracle off for exactly the
inputs that found it. Deleting `ResolveIntDouble` from reader.rs now fails
within seconds; before it left the target green.
* Draw the encoded union branch from the input. Decoding consults only the
resolved permutation entry for the branch on the wire, so the fixed
first-promotable choice left every other entry's deferred `Err` unobserved,
the exact failure mode this target exists for.
* Reach the other two union arms: the reader rendering now sometimes collapses a
union to the encoded branch (`ResolveUnionConcrete`) and sometimes wraps a
concrete node in one (`ResolveConcreteUnion`), whose no-match error path has a
fixed out-of-bounds history. Unions also carry named variants now, for the
name-keyed side of variant matching.
* Drop trailing reader enum symbols so the `default` becomes load-bearing, and
require each decode to consume every byte the writer wrote.
mz-avro `reader_decode`:
* Straddle `safe_len` deliberately. All three "corrupt framing" magnitudes sat
above `MAX_ALLOCATION_BYTES`, so every one of them stopped at that guard: the
short-read case was dead and duplicated the negative-size case, and the
accepted-but-oversized count band, where the bug this target is credited with
finding still lived, was never generated.
* Stop generating schemas the parser rejects outright (`fixed` size 0,
`["null","null"]`, nested unions), which discarded ~6% of inputs at the header
with no block decoding at all.
mz-avro `avro_schema_parse`:
* Reach `MAX_SCHEMA_DEPTH`. It was unreachable through `Schema::from_str`:
serde_json's own 128-container limit fires one level first, so the target's
headline claim of driving the guard past its limit was never true. The deep
case now goes through `Schema::parse`, and the text case is bounded to nesting
serde_json accepts. Adds the unit test the depth guard never had, against a
hand-built `Value`, plus one pinning why the text entry point cannot reach it.
* Generate two named types sharing a fullname, the only shape that reaches
`alloc_name`'s duplicate rejection.
* Correct the doc: two of the "must reject" shapes are accepted today
(`parse_record` has no duplicate-field-name check, and `parse_bytes` swallows
a decimal `scale > precision`), and alias collisions mean nothing until
`resolve_schemas`, which this target never calls.
* Seeds are prefixed with the byte that selects `reader_decode`'s raw-bytes
branch. `reader_decode` reads its input as an `Unstructured` recipe, so the
seeded container file was being consumed as generator choices and never
reached the magic-header check that prepare-corpus.sh exists to skip past.
Catalog serde round-trip:
* Route the oracle through the durable encoding. The on-disk form is not JSON
text: `StateUpdateKindJson` packs the serde value into a `Jsonb`, i.e. an
`mz_repr::Row`, and reads it back through `to_serde_json` + `from_value`. That
leg is the one that can lose information, since every number becomes a
`Datum::Numeric` and object keys are deduplicated and reordered by the `Row`
map encoding, and `to_vec`/`from_slice` never touches it. The text form is
still asserted alongside it.
* Add `StateUpdateKind`, the durable envelope every catalog write goes through
and the only member of this family whose round trip is not trivially total: it
is `#[serde(tag = "kind")]`, so it deserializes through serde's
content-buffering path. Every other reachable field bottoms out in integers,
bools, strings, `Vec`, `Option`, or fieldless enums, for which the derived
`Serialize`/`Deserialize` are inverses by construction.
* Drop the `json == json2` assertion. Serialization is a pure function of the
fields and every reachable type derives structural `PartialEq`, so it was
implied by the value equality above it. It earns its keep in the proto
siblings this was modelled on, where `from_rust` + `encode_to_vec` can differ
for equal values, but not here.
* Both arms now dispatch over one type list in the same order, so a corpus
entry's type byte survives a one-bit change to the arm byte. Seed the corpus
and add a dictionary: the raw arm feeds the input straight to
`from_slice::<T>`, which returns nothing until the input is a complete object
with the right field names and variant tags, so the nested types were
unreachable.
`build_regex`:
* The target capped patterns at 4096 chars on the premise that "the `regex`
crate is size-limited, so an oversized pattern returns an error rather than
OOMing". That premise is wrong. `size_limit` bounds only the compiled NFA. The
memory is spent earlier, translating the pattern's AST into `regex-syntax`'s
HIR, which `size_limit` never sees, and cost there scales with the pattern's
source length. Measured with regex 1.12.4 and the exact `RegexBuilder`
configuration `build_regex` uses, `\p{L}` repeated under `i`, peak RSS is
~7.4 KB per pattern byte against ~0.35 KB for a plain literal: 4 095 bytes ->
47 MB, 524 285 -> 3 903 MB, 1 048 575 -> 7 792 MB, all returning
`CompiledTooBig`. The cap sat 87x under the runner's `-rss_limit_mb=4096` and
was dead besides, since the runner passes no `-max_len` and libFuzzer's
default is 4096 bytes.
* Add a rare `gen_long_pattern` arm that synthesizes a long pattern by repeating
one small unit, so the length comes from a handful of input bytes rather than
from `-max_len`, and straddles the 1 MiB guard. Drop the pattern cap, keep the
text cap. The arm now reaches 4043 MB of peak RSS, and libFuzzer reports an
11-byte input that spends 59 s building a 513 845-byte pattern.
* Correct three doc claims: the module premise above, `gen_near_limit`'s claim
that its output can hit `PatternTooLarge` (those patterns are <= ~45 bytes, so
only `CompiledTooBig` is reachable), and its example, which put the innermost
quantifier inside the group. `REPLACEMENT`'s `$99` was documented as an
out-of-range capture index but parses as the capture *name* `99y`, since an
unbraced `$name` takes the longest run of `[0-9A-Za-z_]`, which took the
name-lookup path and silently swallowed the trailing literal. Brace it as
`${99}`.
`EvalError` proto round-trip:
* The round-trip assertion alone certified a decoded `EvalError` that aborted
the process the moment anything looked at it: `OutOfDomain(None, None, _)`
survives the wire intact, and `Display` panicked on exactly that state. Assert
that the decoded value also renders, which is what `DataflowErrorSer::Display`
does with untrusted bytes on the index peek path. Verified falsifiable both
ways: a 15-byte mode B input crafted to decode as
`OutOfDomain(None, None, "x")` crashes the target against the pre-fix
`Display` and passes after it, and 1.3M executions are clean on the fixed
tree.
* Handle a `new_tree` rejection by returning rather than panicking, matching the
sibling round-trip targets, so a generator limit cannot be reported as a
product crash. Correct the module doc: `EvalError` carries no
`NumericMaxScale`, and it is mode B, not mode A, that reaches the rejection
branches of `char::from_proto` and `usize::from_proto`, since mode A always
starts from an already-valid Rust value.
Transform equivalence oracles:
* Add `hits_non_strict_error_fold`, which skips plans matching the open bug
CLU-137 rather than rediscovering it on every run. `Or::eval` returns `true`
the moment it sees a true operand and drops any error it collected,
`And::eval` does the same for `false`, and `error_if_null` evaluates its
message operand only when the first operand is NULL, yet `reduce`'s generic
variadic fold replaces the whole call with an operand's literal error and
`undistribute_and_or` can recombine an erroring operand across the
short-circuit boundary. Deliberately conservative: it asks whether an operand
*could* error, because `reduce` folds a column-free fallible operand to a
literal error first and absorbs it after. Measured at 3.6% of
`gen_rel(depth = 4)` plans skipped.
`interchange::avro::Decoder` over a fuzzer-generated reader schema:
* Gate every "don't build a valid body" branch on remaining input.
`Unstructured::int_in_range` does not fail once the data is exhausted, it
returns the low end of the range forever, and schema generation consumes
input greedily, so the ungated `== 0` branches were taken by every exhausted
input. Measured over 400k executions with the arms instrumented: empty bodies
fell from 36.5% to 4.3% of schema-directed runs, the clean-body oracle armed
on 5.8% before and 13.6% after, and the same budget found 1035 new coverage
units against 608, at a 7% throughput cost.
* Make the corruption arms corrupt. `truncate(keep)` drew `keep` from
`0..=b.len()`, so a `keep == b.len()` draw was a no-op, and the byte flip used
`^= arbitrary::<u8>()`, so a zero mask was one too.
* Stop generating schemas mz-avro rejects, which discarded the whole input:
`fixed` size 0 (`parse_fixed` requires a positive size) and
`["null",["null",T]]` (`UnionSchema::new` rejects a union directly inside a
union). Schema rejection went from 3.4% of schema-directed runs to zero.
* Bound a `decimal`-over-`fixed`'s precision by the chosen byte run. Precision
and size were drawn independently, and `parse_fixed` silently demotes an
over-precision decimal to a plain `fixed` with a `warn!` rather than an error,
so 10.2% of generated `DecimalFixed` types never reached the decimal path
while the harness still believed they had.
* Assert a clean body is consumed in full. A length-accounting bug that
under-consumes while still packing a well-formed `Row` passed every other
oracle.
* Correct two coverage claims. The `assert_success` oracle cannot reach the
deferred union-promotion class it cited (MaterializeInc#37087): with
`WriterSchemaProvider::None` the resolver returns the reader schema verbatim,
so no `SchemaPiece::Resolve*` piece is ever built, and that class is covered
by `avro/fuzz::schema_resolve` instead. And the target never runs
`validate_schema_1`/`validate_schema_2`/`get_union_columns`, which are
reachable only from `schema_to_relationdesc`, so what it exercises is the
runtime `AvroFlatDecoder::union_branch`, not the column-name invention.
`mz_repr::adt::interval` proto round-trip:
* Replace the `interval_proto_roundtrip` fuzz target with a proptest next to the
type. `Interval` <-> `ProtoInterval` is a field-for-field copy of three
integers, so there is no untrusted-input surface for a fuzzer to explore: the
property only bites once the two structs drift, which a proptest over
`any::<Interval>()` catches just as well and runs in CI on every commit.
`strconv` time and timestamp round-trips:
* `strconv_parse_time` skipped every parsed value whose nanosecond field was not
an exact multiple of 1000, then asserted exact equality across
`format_time`/`parse_time`. Post-filter that pair is a bijection, so neither
the assertion nor the re-parse-failure return could ever fire and the
effective property was only "`parse_time` must not panic". The skipped region
was also the only part of the space where the renderer does anything
interesting, so `format_time` never saw a sub-microsecond fraction or a leap
second at all. Render unconditionally and assert bounded drift instead: the
renderer writes microseconds rounding half away from zero, so a faithful
rendering moves the value by at most half a microsecond. Measure the drift as
nanoseconds from midnight rather than field by field, because a fraction that
rounds up legitimately carries into the seconds field and on past a minute or
hour boundary. The one looser case is a carry out of the last second of the
day, where there is no second to carry into and the fraction saturates at
`.999999`. Falsifiable: dropping the renderer back to truncation is reported
in under a minute (`00:08:00.000000800` renders as `00:08:00`).
* Both timestamp targets fixed the rounding precision at `None`, i.e.
microseconds, which makes `round_to_precision`'s rounding branch dead: the
quantum is a single microsecond, so every value is already on a boundary and
only the seventh-digit nudge runs. The cast they mirror carries an arbitrary
`Option<TimestampPrecision>` from the column's type modifier, and a precision
below 6 is where rounding up off the end of `HIGH_DATE` is easy to reach, a
single `.5` at precision 0 rather than seven exact digits. Take the precision
from the input.
* Add a second oracle for the consumer that does not round.
`mz_pgrepr::Value::decode_text`, behind COPY and text-format bind parameters,
stores the parsed nanoseconds as they are, so for it the renderer is itself
the rounding step, and rendering plus re-parsing must land exactly on the
microsecond-rounded value. The carve-out is where rounding up leaves chrono's
range: the renderer has no error channel and saturates the fraction instead,
so there is nothing for the two to agree on.
* Drop both escape hatches. A parsed `:60` now round-trips, so the leap-second
carve-out's comment no longer holds. The re-parse-failure return's stated
reason, a year of more than four digits, was simply wrong:
`262142-12-31 23:59:59` parses and round-trips byte-identically, and over
395,498 successful parses the guard fired zero times. A renderer that emits
text the parser rejects is a bug to report, not tolerate, so both are now
`expect`.
* Add `strconv_parse_timestamp.dict` and `strconv_parse_timestamptz.dict`.
Neither end of the accepted range is reachable by mutation alone: `HIGH_DATE`
is the single date `262142-12-31`, so hitting it means mutating a 6-digit
ASCII integer onto one exact value while a late time-of-day and a westward
offset are already in place, and the range check offers no coverage gradient
to climb, just one saturated edge for "accepted" and one for "rejected".
`src/repr/fuzz` shipped no dictionary at all, and its corpus directory is
gitignored, so there were no seeds either. With the boundary dates, the
round-up fractions and the sub-minute offsets spelled out as tokens, an
empty-corpus run reproduces the timestamptz rounding overflow in under three
minutes, on `(precision 2, "262142-12-31 ?23:59:59.9999995")`.
* Verified by A/B: against the pre-fix renderer the unrounded oracle fails on
`2020-01-01 00:00:00.9999999` with `00:00:01` vs `00:00:00.100`, and on a leap
second because the rendering no longer parses. Against the fixes, three
minutes each of `strconv_parse_timestamp` (7.1M execs),
`strconv_parse_timestamptz` (7.2M) and `strconv_parse_time` (3.6M) are clean.
`strconv_parse_interval`:
* Assert the round trip the module doc names. The oracle returned silently when
the rendering failed to re-parse, so only the "if it re-parses, values agree"
half was checked and any rendering the parser rejects was a silent success. The
justification given for that carve-out does not exist: the hours field is the
only unbounded one and is derived as `(micros / 1_000_000).abs() / 3600`, so
multiplying it back out during the re-parse cannot exceed `|micros|` and cannot
overflow. `i64::MAX` micros renders as `2562047788:00:54.775807` and re-parses
exactly, as does `i64::MIN`. A direct sweep of the `(months, days, micros)`
space, 300,776 triples over both extremes, an edge grid and a pseudo-random
fan-out, produced zero re-parse failures and zero mismatches, so re-parse is
total and the branch was dead.
* What the tolerance cost, measured by mutating `Display`'s months spelling to
one `parse_interval` rejects: the tolerant oracle ran 3,570,705 executions and
reported success, while the strict one reports the 4-byte input `1mon`
immediately. Against the real renderer 5.2M executions are clean.
* Render through `strconv::format_interval` rather than `Display` directly, since
that is the entry point `::text` and the pgwire encoders call. The two agree
today, `format_interval` being a `write!` of `Display`, so this only keeps the
target pointed at the right function if that stops being true. This matters for
a type whose rendering is load-bearing and has already changed once: the unit
spelling moved to `mon`/`mons` in v26.35 (SQL-463), filed as a wrong-result bug
because a client's parser stopped at the old spelling.
`strconv_parse_date`:
* Assert the `Nestable` verdict, not just the round trip. `format_date` returns a
`Nestable` alongside the rendering and the target dropped it, leaving half the
contract unchecked, and that half was wrong: the ` BC` suffix carries
whitespace, which every element escaper quotes for, yet `Nestable::Yes` was
returned unconditionally. A wrong `Yes` is invisible to a round-trip oracle,
because the wrongly-unquoted rendering still re-parses, so it needs an
assertion of its own. Checked against the real escapers through
`element_needs_escaping` rather than restating their rules in the target, where
a copy would go stale: SQL-466 already added a rule to that predicate.
* Make the re-parse a hard failure. The tolerance was borrowed from the timestamp
targets and neither reason applies to DATE. `format_date` writes no time
component, so the leap-second gap cannot appear in its output, and a year of
more than four digits parses, since `fill_pdt_date` reads a 6-digit leading
number followed by a dash as a full year. Sweeping the whole `Date` range,
exhaustively at both ends and across the BC/AD transition, leaves exactly four
reachable forms, a 4, 5 or 6 digit Common Era year and a 4 digit BC year, and
100,962 sampled dates produced zero re-parse failures. The branch was dead.
* Verified by A/B: with `format_date`'s verdict put back to an unconditional
`Nestable::Yes` the target reports `8==8=8= BC` within seconds, and against the
fix 7.9M executions are clean.
`sql_server_table_desc_proto_roundtrip`:
* Assert the constraint-string arm's oracle. The module doc and an inline comment
both stated it, "garbage strings must be rejected, valid ones must parse", and
neither half was checked: `Err` was discarded by an early `return`, and on `Ok`
nothing looked at which spelling produced it. The arm's whole input space is
`CONSTRAINT_TYPES.len() * 4` fixed cases, so without that assertion it
contributed nothing beyond "does not panic", and it was blind to all three ways
the parser can regress. A normalization or catch-all arm would start accepting
`primary key`; a valid spelling ceasing to parse would make every input take the
early return, leaving an inert arm indistinguishable from a passing one; and
swapping the two variants is preserved by a round trip. The variant is not
cosmetic, it drives `is_primary` on the `TableConstraint::Unique` that
purification writes into the generated subsource DDL, so misparsing a non-unique
upstream constraint as `PrimaryKey` declares a key that does not hold. Verified
by mutating the product parser three ways: trim/uppercase normalization,
`"PRIMARY KEY"` no longer parsing, and the two variants swapped. Each is now
reported within seconds, and the unmutated tree is clean.
* Give every arm its bytes from offset 1. Only the proptest arm consumes the
32-byte seed, yet all four skipped past it, so the other three read their
selector bytes from offset 33 and saw nothing but defaults until libFuzzer grew
inputs that far. That also meant the raw-bytes arm, the one meant to consume a
real encoded descriptor, decapitated any genuine blob added to the corpus, which
`--corpus-sync` accumulates across runs. The same 3-minute budget now reaches
3594 edges and 18382 features against 3507 and 17215 before.
* Unlock two `parse_data_type` branches that were unreachable by construction, not
merely rare. `precision` and `scale` were drawn `% 39`, capping both at 38, so
the `precision > 39` rejection and the `NumericMaxScale::try_from` failure could
never fire, while the header claimed the type list exercises every branch of the
mapping. `% 45` reaches both, and `SqlServerColumnDesc::new` folds either
rejection into an `Unsupported` decode type that still round-trips.
* Pin `max_length` 4 and 8. `real`/`float`/`double precision` select `F32`/`F64`
by byte width, not by name or precision, so the two decode types the header
documents were reachable only by luck through the two random length branches.
The inline comments claimed precision selected them, which was never the rule.
* Drop the per-column leaf round-trip loop. The table's `into_proto`/`from_proto`
delegate per element and equality is structural, so the table-level assertion
already implies it. The loop just doubled the arm's conversion work.
* Give the constraint arm's descriptor the columns its constraint names, instead
of pairing a constraint over `c0..cn` with an empty column list. Harmless against
today's oracle, but a constraint/column consistency check in `from_proto` would
have been reported as a crash on the arm's own inputs.
Transform oracle helpers:
* Stop swallowing `TransformError`. `optimize` discarded the whole enum with
`.ok()`, documented as skipping the case where "the `Typecheck` pass rejects the
plan". That case cannot happen. `Typecheck::actually_perform_transform` returns
`Ok(())` on every path and routes a type error through
`type_error!(true, ..)` -> `soft_panic_or_log!`, as does `Fixpoint`
non-convergence, and soft assertions default to `cfg!(debug_assertions)`, which
cargo-fuzz enables. Confirmed by handing `optimize` a `Filter` with an `int4`
predicate in the built fuzz binary: it panics at `typecheck.rs:1672`, it does
not return `None`. So the comment sent anyone extending the generators looking
for a silent skip that does not exist, while a rejected plan actually lands as a
`crash-*` artifact pointing into `Optimizer::optimize`, which CI reports as an
optimizer bug.
* What could still reach `.ok()` was the opposite of benign: every remaining
`TransformError` over these plans is an optimizer invariant violation, a
`Let`/`Get` on an unbound local id, a `Let` whose type changed under it, an ANF
rebinding that lost an identifier, all of which reach users as `internal error`.
`NormalizeLets` runs on every input and `RelationCSE` in `fixpoint_logical_02`,
so the harness ran through this code constantly while being structurally unable
to report anything in it. Measured false-pass width by making
`NormalizeLets::actually_perform_transform` return
`Internal("injected invariant violation")` on every input: the old oracle
completed **367,428 executions and reported success**. It now panics on the
first one. `fold_to_multiset`'s `.ok()?` and `collapse`'s two
`is_err() -> StuckFixpoint` arms folded the same class into a skip and get the
same treatment; `StuckFixpoint` still covers genuine non-convergence.
* Record why the `EvalError` skip in `fold_to_multiset` is required rather than a
blind spot. `predicate_pushdown` knowingly pushes a predicate that can error
into a join input and manufactures an error the unoptimized plan never raises
(database-issues#6258), so one side folding to an error while the other yields
rows is accepted behaviour, measured at roughly 1 in 1,500 executions. Without
the reference the next reader is likely to "fix" it into an assertion.
* Document `gen_rel`'s arity contract. The doc spelled out the non-negativity
contract for the caller-supplied `leaf` but not that it must return at least one
column. A zero-arity leaf makes the `Project` arm's `int_in_range(1..=arity)`
an empty range and underflows `arity - 1`, and worse than either panic,
`join_scalars` drops an arity-0 single-row input *after* `join` computed the
equivalences' global column offsets over the full input list, so the
equivalences and the returned schema would silently point at the wrong columns.
Both in-tree leaves return 1-3 columns, so this is a contract note, not a fix.
Transform generators and the three transform targets:
* Build some nodes as raw variants instead of through `MirRelationExpr`'s smart
constructors, which normalize away exactly the shapes the structural fusions
collapse: `map` extends an existing `Map`'s scalar list, `negate` cancels an
enclosing `Negate`, `union` flattens its operands, `project` composes into an
existing `Project`, `filter` merges/sorts/dedups predicates. `MapFusion`,
`NegateFusion` and `UnionFusion` therefore could not fire on *any* input, so
their two assertions compared a plan against a clone of itself. Measured over
25,714 plans before: 0, 0 and 0 firings against FilterFusion's 16,526 (which
survives only through its `canonicalize_predicates` call) and ProjectFusion's
6,046 (through identity-projection removal). After: 939, 2,934 and 10,504,
with the other three unchanged or slightly up.
* Rebuild `wrap_preserving` from raw variants. Two of its four arms were
unconditional no-ops, `filter` dropping a literal-true predicate and
`negate().negate()` cancelling to the identity, which silently reduced the
structural-rewrite oracle to `fold(rel) == fold(rel)`. It now changes the plan
on every input (40,001 of 40,001) against 81.6% before.
* Apply each fusion in the traversal order its real driver uses. `fusion::union`
and `ProjectionExtraction` call `visit_mut_post`; only the
`Filter`/`Project`/`Map`/`Negate` fusions are pre-order. The comment claimed
all six matched their drivers.
* Report the input plan, not the folded one, in `FoldConstants`' shape assertion.
Every other call site passes the original; this one had moved it.
* Plan with production's `OptimizerFeatures`, not `Default::default()`, which is
all-`false` and is what no deployment runs. 14 of the 19 boolean optimizer flags
default to `true` in production, including the three `EquivalencePropagation`
reads. `enable_eq_classes_withholding_errors` is the pointed one: it exists to
stop equivalence propagation suppressing errors, and `gen_scalar` deliberately
seeds error literals, so the one feature built for this generator's input class
was the one turned off. `fuzz_features` names only the flags whose
production default is `true` and takes the rest from `Default`, so it cannot
break this crate's slow sanitizer build. The tripwire for a newly added flag
stays where it is cheap: `mz_sql`'s
`optimizer_features_no_enable_for_item_parsing` already destructures
`OptimizerFeatures` exhaustively, and now says to enable the flag in
`fuzz_features` too when it ships on, per the repo convention that a new flag is
ON in the test configuration.
* Cap the oracles' constant folding at `FOLD_ROW_LIMIT` rows. `FoldConstants`'
join arm materializes the full cross product before applying equivalences and
`limit: None` disables its only size check, so the harness could ask for a
`Vec<(Row, Diff)>` bounded only by the product of every leaf. Declining to fold
is already a benign skip on both oracles, and the cap sits well above the
largest product these generators were measured to reach.
* Optimize before deciding the baseline is comparable, in
`optimizer_symbolic_equiv`. A plan whose result is an evaluation error folds to
an `Err` constant, which `collapse` reports as a skip, and returning there meant
never calling the optimizer on it: `gen_scalar` spends one of three literal-leaf
choices on `EvalError::DivisionByZero`, so a deliberately generated class of
input never reached the code under test at all, not even for panic coverage.
* Share one `collapse` between the two equivalence targets.
`full_optimizer_equiv` folded the optimized side with a single `FoldConstants`
pass, which cannot fold through a `Let`, while its sibling already iterated
`FoldConstants` + `NormalizeLets` to a fixpoint "to collapse any `Let`s the
optimizer's CSE introduced". `RelationCSE` gets perfect candidates from the
`Union` arm cloning `inner` into both branches, and the skip landed precisely on
the executions where the post-collapse stages had run on real structure.
* Correct two doc claims. Neither target skips on an optimizer error any more, so
the `Typecheck`-rejection rationale is gone from both. And of the five
transforms `full_optimizer_equiv` claimed as its unique coverage, only
`NonNullRequirements` and `RedundantJoin` run before step 2's trailing constant
folding; `Demand`, `SemijoinIdempotence` and `ReduceElision` sit in
`fixpoint_logical_01`/`_02` and see a bare `Constant`. Nothing in the suite
covers those three against exact key facts, which the doc now says.
* Record that `Collapse::StuckFixpoint` conflates "folding could not evaluate it"
with "it folded to an `Err`", that only the second is a determined result, and
why the tempting `Ok -> Err` assertion is not available today
(`predicate_pushdown` manufactures such errors, database-issues#6258).
Carving out CLU-137, once, in one place:
* Give the shape a single definition,
`MirScalarExpr::could_hit_nonstrict_error_fold`, in `mz_expr` next to the fold it
describes. Four oracles now compare evaluation across `reduce` and each needs to
skip it; two had already drifted, since the transform helper counted
`ErrorIfNull` as non-strict and `mir_scalar_reduce`'s copy did not.
* Apply it to `mfp_optimize`, which had no carve-out at all. `optimize` reduces
every map expression and predicate, and that target generates `.and()`/`.or()`
over a fallible vocabulary, so all three of its assertion arms were exposed: a
folded error makes a row the raw MFP passed error out, the literal is typed
non-nullable so a nullability-dependent rewrite above it can change the projected
value, and a predicate folded that way drops the row. Its existing tolerance
covers none of those, because it only forgives an error on a row the raw MFP
*filtered*. Measured cost of the skip: 2.2% of MFPs (30,576 of 1,400,000).
* `mir_scalar_reduce` keeps its row-precise variant rather than adopting the coarse
predicate, because the shape is common in its generator and skipping every
expression carrying it would cost most of the comparisons. The relationship
between the two is now written down at both ends.
`mir_scalar_reduce`:
* Skip a row that absorbs a non-strict AND/OR operand error outright, instead of
merely allowing it to reduce to an error. The old carve-out was gated on the
reduced side also being an error, justified as "an absorbed error cannot
resurface as a value". The fuzzer falsified that after 2.2M executions:
`Or([Literal(Err(DivisionByZero)), Column(3)])` evaluates to `Ok(True)`, because
`Or::eval` returns true on its first true operand and drops the error it
collected, while reduce replaces the whole call with the literal error. From
that point the reduced expression is folding a value the original never
computed, and the rewrites downstream do not all propagate it: the reported case
turned `CastBoolToString(IsNull(If(..)))` from `Ok("true")` into a literal
`Ok("false")`.
* That is CLU-137, which PR MaterializeInc#37299 did not fix, and the wrong value is downstream
of it rather than a second defect, verified by reducing the `Or` in isolation.
So the whole row goes, matching how the transform targets skip the same shape
via `hits_non_strict_error_fold`. The skip stays per-row rather than per-
expression, so every non-absorbing row of an affected expression is still
compared.
* Checked that this does not blunt the oracle: swapping `reduce_if`'s literal-
condition branches is still reported within seconds, and 4.8M executions are
clean against the unmutated tree.
Workspace-wide sweep of the remaining targets:
* The same three corrections applied across the rest of the suite: generators
that reach the decode instead of stalling at an early reject, oracles that
assert the property they name rather than a weaker one it implies, and module
docs pruned of coverage that was never reached. Touches the targets in
`repr` (row/arrow/codec round-trips, `strconv` parsers, `numeric_arith`,
`range_ops`, the type-modifier proto round-trips), `expr` (`jsonb_get`,
`jsonb_path`, `cast_string`, `like_pattern_compile` and `like_pattern_escape`,
`agg_decompose`, `mir_scalar_reduce`, `timezone_convert`, `mfp_optimize`),
`pgwire` (`codec_decode`, plus corpus dictionary and seeding), `pgcopy`,
`pgrepr`, `pgtz`, `storage` (the four upsert targets), `storage-types`,
`sql-parser`, `mysql-util`, `postgres-util`, `sql-server-util`,
`persist-client` and `interchange`.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Author
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
1 participant
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.
Sharpens the cargo-fuzz suite's oracles, and fixes the product bugs the sharpened
oracles then found.
The branch is ordered so the product fixes come first, grouped by area, and the
fuzz-harness work lands last as a single commit. Every product commit touches zero
files under a
fuzz/directory, so each can be reviewed, reverted or backported onits own. The final commit is deliberately the one that would be red without the
fixes below it: its whole purpose is oracles that assert what they claim to.
Product fixes
persist
data_lenssum overflowsLazyPartStatswith malformed bytesavro
sql-parser
MAPexpr
AND/OROutOfDomaininstead of panickingrepr, proto and catalog decode
ProtoRelationDescshapes when decodingProtoNumeric.bcdlength before the BCD FFIrepr, regex
repr, datetime and strconv
parse_timestamptzround_to_precision's rounding arithmeticpgrepr
Two of these want a second look before merge
repr: check the offset subtraction in parse_timestamptzis security-relevant.A 24-byte text value in a pgwire bind parameter, the HTTP SQL API, or a source
cast aborted the process: remote input, no
catch_unwindabove it, and theenhanced panic hook ends in
process::abort(). Same shape as SS-157. Worthdeciding whether that needs an advisory rather than just a changelog entry.
repr: report a BC date as needing escapingchanges user-visible output.{0001-02-24 BC}becomes{"0001-02-24 BC"}, matching PostgreSQL'sarray_out.It is a correctness fix, but anyone parsing
date[]text sees the change, so itbelongs in release notes.
Ordering constraint worth knowing if these get split up:
strip the quotes from a quoted range boundhas to land beforereport a BC date as needing escaping. Thedate fix makes
daterangestart emitting quoted BC bounds, and without the rangeparser fix those cannot be read back. The range parser was already broken for
tsrangewith a pre-Common-Era bound before either change.Known deviation left unfixed
PostgreSQL processes
\escapes in unquoted range bounds; we still do not. Notreachable from any renderer output, since
format_rangequotes whenever escapingis needed, so round trips stay total. It manifests only as erroring where PG
succeeds on hand-written SQL. Happy to file it separately.
Fuzz-harness work
One commit,
tests: sharpen the cargo-fuzz oracles across the workspace, whosemessage carries the per-target detail and the measurements. The recurring themes:
structural fusions whose assertions compared a plan against a clone of itself on
100% of inputs, a
Nestableverdict the DATE target discarded, an intervalround-trip whose failure branch was dead, a constraint-string parser whose
documented accept/reject boundary was never asserted;
unreachable by capped parameters, selector bytes parked past a 32-byte seed
window,
Unstructuredexhaustion silently pinning a branch;Typecheckrejection that panics rather than erroring, a
>4-digit yearthe parser acceptsfine, an absorbed error that can resurface as a wrong value;
over, including three transforms that cannot see the exact key facts this suite
was said to give them.
Every behavioural change is A/B verified: the mutation that should trip each new
assertion is reported, and the unmutated tree runs clean. The commit message states
the execution counts.
Verification
mz-repr,mz-expr,mz-transform,mz-sql,mz-pgrepr,mz-avroandmz-persist-clientunit suites pass (590 tests).dates-times,range,arrays,list,map,record,timestamp,timestamptz,timezoneandfuncssqllogictest files pass. All four touched fuzz crates build under
cargo +nightly fuzz build, and each modified target was run to convergence.🤖 Generated with Claude Code
https://claude.ai/code/session_01E9wiXMn5L34GwkmwpaFGif