zeroshade commented on code in PR #1126:
URL: https://github.com/apache/arrow-go/pull/1126#discussion_r3855422663
##########
parquet/variant/variant.go:
##########
@@ -556,20 +557,331 @@ func validateScalarValue(value []byte) error {
want = 17
case PrimitiveBinary, PrimitiveString:
if len(value) < 5 {
- return fmt.Errorf("invalid variant value: %s length
prefix requires 5 bytes, got %d", primitiveType, len(value))
+ return 0, fmt.Errorf("invalid variant value: %s length
prefix requires 5 bytes, got %d", primitiveType, len(value))
}
dataLen := uint64(binary.LittleEndian.Uint32(value[1:5]))
if dataLen > uint64(len(value)-5) {
- return fmt.Errorf("invalid variant value: %s data
requires %d bytes, got %d", primitiveType, dataLen, len(value)-5)
+ return 0, fmt.Errorf("invalid variant value: %s data
requires %d bytes, got %d", primitiveType, dataLen, len(value)-5)
}
- return nil
+ return 5 + int(dataLen), nil
default:
- return fmt.Errorf("invalid variant value: unknown primitive
type %d", primitiveType)
+ return 0, fmt.Errorf("invalid variant value: unknown primitive
type %d", primitiveType)
}
if len(value) < want {
- return fmt.Errorf("invalid variant value: %s requires %d bytes,
got %d", primitiveType, want, len(value))
+ return 0, fmt.Errorf("invalid variant value: %s requires %d
bytes, got %d", primitiveType, want, len(value))
+ }
+ return want, nil
+}
+
+type validationChild struct {
+ value []byte
+ index int
+ start uint64
+ expectedSize uint64
+}
+
+type validationRange struct {
+ start uint64
+ end uint64
+ field int
+}
+
+type validationFrame struct {
+ value []byte
+ kind BasicType
+ size int
+ dataSize uint64
+ children []validationChild
+ ranges []validationRange
+ nextChild int
+ pending validationChild
+ initialized bool
+ compound bool
+}
+
+// validateValue walks compound values with an explicit stack so valid values
+// are not limited by the Go call stack or an implementation-defined nesting
+// depth.
+func validateValue(meta Metadata, value []byte) (int, error) {
+ stack := []validationFrame{{value: value}}
+ var (
+ resultSize int
+ resultErr error
+ hasResult bool
+ )
+
+ for len(stack) > 0 {
+ frame := &stack[len(stack)-1]
+ if hasResult {
+ child := frame.pending
+ hasResult = false
+
+ if resultErr != nil {
+ switch frame.kind {
+ case BasicArray:
+ return 0, fmt.Errorf("invalid variant
value: array element %d: %w", child.index, resultErr)
+ case BasicObject:
+ return 0, fmt.Errorf("invalid variant
value: object field %d: %w", child.index, resultErr)
+ default:
+ return 0, resultErr
+ }
+ }
+
+ switch frame.kind {
+ case BasicArray:
+ if uint64(resultSize) != child.expectedSize {
+ return 0, fmt.Errorf("invalid variant
value: array element %d has trailing bytes", child.index)
+ }
+ case BasicObject:
+ end := child.start + uint64(resultSize)
+ if end > frame.dataSize {
+ return 0, fmt.Errorf("invalid variant
value: object field %d extends beyond data", child.index)
+ }
+ frame.ranges = append(frame.ranges,
validationRange{
+ start: child.start,
+ end: end,
+ field: child.index,
+ })
+ }
+ continue
+ }
+
+ if !frame.initialized {
+ frame.initialized = true
+ if err := prepareValidationFrame(meta, frame); err !=
nil {
+ stack = stack[:len(stack)-1]
+ if len(stack) == 0 {
+ return 0, err
+ }
+ resultErr = err
+ hasResult = true
+ continue
+ }
+ }
+
+ if frame.compound {
+ if frame.nextChild < len(frame.children) {
+ child := frame.children[frame.nextChild]
+ frame.nextChild++
+ frame.pending = child
+ stack = append(stack, validationFrame{value:
child.value})
+ continue
+ }
+
+ if err := finishValidationFrame(frame); err != nil {
+ stack = stack[:len(stack)-1]
+ if len(stack) == 0 {
+ return 0, err
+ }
+ resultErr = err
+ hasResult = true
+ continue
+ }
+ }
+
+ resultSize = frame.size
+ stack = stack[:len(stack)-1]
+ if len(stack) == 0 {
+ return resultSize, nil
+ }
+ hasResult = true
+ }
+
+ return 0, errors.New("invalid variant value: validation stack
exhausted")
+}
+
+func finishValidationFrame(frame *validationFrame) error {
+ if frame.kind != BasicObject {
+ return nil
+ }
+
+ slices.SortFunc(frame.ranges, func(a, b validationRange) int {
+ switch {
+ case a.start < b.start:
+ return -1
+ case a.start > b.start:
+ return 1
+ default:
+ return 0
+ }
+ })
+
+ var (
+ next uint64
+ previousField int
+ )
+ for _, child := range frame.ranges {
+ switch {
+ case child.start < next:
+ return fmt.Errorf("invalid variant value: object fields
%d and %d overlap", previousField, child.field)
+ case child.start > next:
+ return fmt.Errorf("invalid variant value: object data
has a gap before field %d", child.field)
+ }
+ next = child.end
+ previousField = child.field
+ }
+ if next != frame.dataSize {
+ return fmt.Errorf("invalid variant value: object data has
trailing bytes")
+ }
+ return nil
+}
+
+func prepareValidationFrame(meta Metadata, frame *validationFrame) error {
+ if len(frame.value) == 0 {
+ return errors.New("invalid variant value: empty")
}
+
+ frame.kind = basicTypeFromHeader(frame.value[0])
+ switch frame.kind {
+ case BasicShortString:
+ want := 1 + int(frame.value[0]>>basicTypeBits)
+ if len(frame.value) < want {
+ return fmt.Errorf("invalid variant value: short string
requires %d bytes, got %d", want, len(frame.value))
+ }
+ frame.size = want
+ case BasicObject:
+ frame.compound = true
+ return prepareObjectValidationFrame(meta, frame)
+ case BasicArray:
+ frame.compound = true
+ return prepareArrayValidationFrame(frame)
+ case BasicPrimitive:
+ var err error
+ frame.size, err = validatePrimitiveValue(frame.value)
+ return err
+ default:
+ return fmt.Errorf("invalid variant value: unknown basic type
%d", frame.kind)
+ }
+ return nil
+}
+
+func prepareArrayValidationFrame(frame *validationFrame) error {
+ value := frame.value
+ typeInfo := value[0] >> basicTypeBits
+ offsetSize := uint8(typeInfo&0b11) + 1
+ isLarge := ((typeInfo >> 2) & 0x1) != 0
+
+ var (
+ numElements uint32
+ offsetStart uint64
+ )
+ if isLarge {
+ if len(value) < 5 {
+ return fmt.Errorf("invalid variant value: array size
requires 5 bytes, got %d", len(value))
+ }
+ numElements = readLEU32(value[1:5])
+ offsetStart = 5
+ } else {
+ if len(value) < 2 {
+ return fmt.Errorf("invalid variant value: array size
requires 2 bytes, got %d", len(value))
+ }
+ numElements = uint32(value[1])
+ offsetStart = 2
+ }
+
+ dataStart := offsetStart + (uint64(numElements)+1)*uint64(offsetSize)
+ if dataStart > uint64(len(value)) || dataStart > math.MaxUint32 {
+ return fmt.Errorf("invalid variant value: array offset table
ends at %d, got %d bytes", dataStart, len(value))
+ }
+
+ offsets := make([]uint32, int(numElements)+1)
+ for i := range offsets {
+ pos := offsetStart + uint64(i)*uint64(offsetSize)
+ offset := readLEU32(value[int(pos) : int(pos)+int(offsetSize)])
+ if i == 0 && offset != 0 {
+ return fmt.Errorf("invalid variant value: array first
offset must be zero, got %d", offset)
+ }
+ if i > 0 && offset < offsets[i-1] {
+ return fmt.Errorf("invalid variant value: array offsets
are not monotonic")
+ }
+ if dataStart+uint64(offset) > uint64(len(value)) ||
dataStart+uint64(offset) > math.MaxUint32 {
+ return fmt.Errorf("invalid variant value: array offset
%d is out of range", offset)
+ }
+ offsets[i] = offset
+ }
+
+ frame.children = make([]validationChild, 0, len(offsets)-1)
Review Comment:
**Blocking:** Validation eagerly allocates a `validationChild` for every
child; objects subsequently allocate another `validationRange` per field. This
happens on every `NewWithMetadata`, including the Parquet row-read path. A
direct 40-field benchmark changed from approximately `4.9 ns/op, 0 B/op, 0
allocs/op` on main to `1.58 µs/op, 5576 B/op, 10 allocs/op`. Wide or deeply
nested untrusted values therefore create substantial memory amplification and
potential OOM pressure. Please iterate child offsets incrementally or otherwise
bound/reuse validator state, and add representative allocation benchmarks.
##########
parquet/variant/variant.go:
##########
@@ -512,25 +513,25 @@ func NewWithMetadata(meta Metadata, value []byte) (Value,
error) {
if len(value) == 0 {
return Value{}, errors.New("invalid variant value: empty")
}
- if err := validateScalarValue(value); err != nil {
+ if err := validateValueBytes(meta, value); err != nil {
return Value{}, err
}
return Value{value: value, meta: meta}, nil
}
-func validateScalarValue(value []byte) error {
- if basicTypeFromHeader(value[0]) == BasicShortString {
- want := 1 + int(value[0]>>basicTypeBits)
- if len(value) < want {
- return fmt.Errorf("invalid variant value: short string
requires %d bytes, got %d", want, len(value))
- }
- return nil
+func validateValueBytes(meta Metadata, value []byte) error {
+ size, err := validateValue(meta, value)
+ if err != nil {
+ return err
}
- if basicTypeFromHeader(value[0]) != BasicPrimitive {
- return nil
+ if size != len(value) {
Review Comment:
**Blocking:** The exact-length check rejects byte slices returned by the
public indexed accessors. `ArrayValue.Value`, `ObjectValue.ValueByKey`, and
`ObjectValue.FieldAt` return an untrimmed slice extending through the parent’s
remaining data, unlike the `Values` iterators. Reproduction: parse
`{"a":1,"b":2}`, retrieve `"a"` with `ValueByKey`, then call
`NewWithMetadata(v.Metadata(), field.Value.Bytes())`; this PR returns `invalid
variant value: trailing bytes`. Please trim values returned by all indexed
accessors to their encoded size and add round-trip tests for arrays and objects.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
