Skip to content

repr: reject corrupt proto shapes and out-of-domain type modifiers when decoding - #37975

Open
def- wants to merge 4 commits into
mainfrom
def/fuzz-05-repr-proto
Open

repr: reject corrupt proto shapes and out-of-domain type modifiers when decoding#37975
def- wants to merge 4 commits into
mainfrom
def/fuzz-05-repr-proto

Conversation

@def-

@def- def- commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Four fixes to mz-repr's proto decoding. from_proto is a trust boundary for
durable catalog state and for the compute/storage protocol, so every one of
these is reachable from a corrupt or forged message, and each produced a value
that no SQL statement could have built.

  • Type-modifier domains were not enforced. 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. Consumers rely on the invariant being
    real, since a TimestampPrecision above MAX_PRECISION panics
    round_to_precision. All four now route through TryFrom<i64> and surface
    the rejection as an InvalidFieldError.

    The Arbitrary impls are narrowed to the same domains, which the validation
    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.

  • Corrupt ProtoRelationDesc shapes were accepted.
    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, and 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.

    This commit also records 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
    comes back from from_proto renumbered to 0..n. Since
    ColumnIndex::to_stable_name is the arrow field name that
    RowColumnarEncoder and RowColumnarDecoder agree on, encoding a sparse desc
    as the schema of data written under the original indexes builds a decoder that
    looks up the wrong fields.

  • validate miscounted versions for a dropped non-root column. It derived
    each column's versions with dropped.unwrap_or(added), discarding the add's
    version whenever a column was also dropped. Every version past the root is
    stamped exactly once, so a column added after the root and later dropped
    accounts for two and both must be counted. Collapsing them rejected any desc
    where a non-root column was later dropped.

  • ProtoNumeric.bcd was unbounded before the BCD FFI.
    decPackedToNumber writes one BCD digit per input nibble into the coefficient
    of the decNumber it is handed, without checking the coefficient can hold
    them.

Tests

Each fix comes with a red test that fails without it. The
relation_desc_proto_roundtrip, scalar_type_proto_roundtrip and
column_type_proto_roundtrip cargo-fuzz targets cover this area and are
sharpened in #37979, which should land after this.

Note for reviewers: src/storage-types/src/snapshots/source-datas.txt, the
SourceData serialization-stability corpus, was itself generated by the old wide
strategies. 10 of its 40 entries carry a numeric max scale or timestamp
precision outside the domain, so from_proto now refuses them. Those 10 are
regenerated from the narrowed strategies, and the other 30 are byte-identical.
The encoding is untouched, so MINIMUM_CONSOLIDATED_VERSION does not move.

@def- def- changed the title def/fuzz 05 repr proto repr: reject corrupt proto shapes and out-of-domain type modifiers when decoding Jul 31, 2026
@def-
def- marked this pull request as ready for review July 31, 2026 09:58
@def-
def- requested review from a team as code owners July 31, 2026 09:58
@def-
def- changed the base branch from def/fuzz-04-expr to main July 31, 2026 10:05
@def-
def- requested a review from a team as a code owner July 31, 2026 10:05
def- added 3 commits July 31, 2026 10:06
`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.
@def-
def- force-pushed the def/fuzz-05-repr-proto branch from bbf4254 to eba595f Compare July 31, 2026 10:18
@def-
def- requested a review from antiguru July 31, 2026 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant