Skip to content

Vector unmarshalling accepts surplus trailing bytes #1086

Description

@nikagra

unmarshalVector's generic path infers element width by division rather than from the
declared element type:

elemSize := len(data) / info.Dimensions

A payload longer than the column's elements therefore decodes without complaint, and how it
fails depends on how far past the end it runs. For vector<int, 3> — 12 bytes of real data:

payload result
13 B, 14 B decodes, correct values, surplus silently dropped
15 B and up failed to unmarshal int ...: the length of the data should be 0 or 4

The second row is the same defect wearing a confusing message: the division yields 5, so
each element is handed 5 bytes and the element unmarshaller rejects it.

Two more cases, both wider than the fixed-width one:

  • Variable-length elements are unbounded. Element sizes come from vint prefixes, so
    leftover bytes are simply never examined. A vector<text, 3> accepts any amount of
    trailing data.
  • Nested vectors are not covered by the obvious fix. vectorFixedElemSize returns 0 for
    a VectorType, while isVectorVariableLengthType reports a vector<vector<float,3>,2>
    as fixed — so a check keyed on either one leaves the division fallback in place, and 25
    bytes still decode where 24 are real.

It also makes the same column behave differently by destination, because the float fast
paths do check exactly:

var a []float32
iter.Scan(&a)   // 13 bytes -> "unmarshal vector<float>: expected 12 bytes, got 13"

var b interface{}
iter.Scan(&b)   // 13 bytes -> decodes, trailing byte dropped

No server sends surplus bytes, so this is hardening rather than a live failure, and no data
is corrupted in the tolerated cases — the decoded values are correct.

Fix shape: give vectorFixedElemSize a recursive VectorType arm so a nested fixed vector
reports its true width; require len(data) == Dimensions * width wherever it returns > 0;
and check for leftover bytes after the variable-length loop.

Two knock-ons for whoever takes it. #1094's payload with a trailing byte subtest pins the
current tolerance — it exists to show that PR's new minimum bound does not over-reject, not
to claim surplus is valid, and it flips to expecting an error. And elemMin in
unmarshalVector's dimension bound reads the same helper, so it tightens as a side effect.
(The vector work was split out of #1042 into #1094.)

Related: #1106 — a variable-length element whose vint length narrows to a negative is the
other half of "leftover bytes are never examined", and decodes to empty values with a nil
error.

Refs: #1094

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions