You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "fallintoplace (via GitHub)" <gi...@apache.org> on 2026/08/06 17:20:23 UTC
[PR] fix(parquet/variant): validate compound value bounds [arrow-go]
fallintoplace opened a new pull request, #1126:
URL: https://github.com/apache/arrow-go/pull/1126
### What does this change do?\n\nValidate compound variant headers and offset tables before returning a value.\n\n### Why?\n\nTruncated arrays and objects were accepted by NewWithMetadata and could panic when their lazy accessors were used.\n\n### Tests\n\n- go test ./parquet/variant
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(parquet/variant): validate compound value bounds [arrow-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #1126:
URL: https://github.com/apache/arrow-go/pull/1126#discussion_r3866486581
##########
parquet/variant/variant.go:
##########
@@ -556,23 +556,406 @@ 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 validationRange struct {
+ start uint64
+ end uint64
+ field int
+}
+
+type validationFrame struct {
+ value []byte
+ size uint64
+ dataSize uint32
+ dataStart uint32
+ offsetStart uint32
+ numChildren uint32
+ nextChild uint32
+ pendingIndex uint32
+ pendingStart uint32
+ pendingExpectedSize uint32
+ rangeStart uint32
+ offsetSize uint8
+ kind uint8
+ initialized bool
+}
+
+const (
+ validationStackInlineCapacity = 32
+ // Values that exceed the inline stack commonly need only a modest amount
+ // of additional depth, so avoid allocating the maximum stack for them.
+ validationStackIntermediateCapacity = 128
+ // The root value is at depth zero, so the stack needs one more frame than
+ // the maximum allowed nesting depth. Keeping this storage fixed prevents
+ // untrusted values from growing the validation stack on the heap.
+ validationStackCapacity = maxValidationDepth + 1
+ validationRangeInlineCapacity = 64
+)
+
+// validateValue walks compound values with an explicit stack so valid values
+// do not consume the Go call stack. Nesting is bounded to keep validation
+// memory usage independent of attacker-controlled input depth.
+func validateValue(meta Metadata, value []byte) (int, error) {
+ var stackStorage [validationStackInlineCapacity]validationFrame
+ stack := stackStorage[:1]
+ stack[0].value = value
+
+ var rangeStorage [validationRangeInlineCapacity]validationRange
+ ranges := rangeStorage[:0]
+ return validateValueLoop(meta, value, stack, ranges, 0)
+}
+
+func validateValueLoop(meta Metadata, value []byte, stack []validationFrame, ranges []validationRange, rangeTop int) (int, error) {
+ var (
+ resultSize int
+ resultErr error
+ hasResult bool
+ )
+
+ for len(stack) > 0 {
+ frame := &stack[len(stack)-1]
+ if hasResult {
+ hasResult = false
+
+ if resultErr != nil {
+ switch BasicType(frame.kind) {
+ case BasicArray:
+ return 0, fmt.Errorf("invalid variant value: array element %d: %w", frame.pendingIndex, resultErr)
+ case BasicObject:
+ return 0, fmt.Errorf("invalid variant value: object field %d: %w", frame.pendingIndex, resultErr)
+ default:
+ return 0, resultErr
+ }
+ }
+
+ switch BasicType(frame.kind) {
+ case BasicArray:
+ if uint64(resultSize) != uint64(frame.pendingExpectedSize) {
+ return 0, fmt.Errorf("invalid variant value: array element %d has trailing bytes", frame.pendingIndex)
+ }
+ case BasicObject:
+ end := uint64(frame.pendingStart) + uint64(resultSize)
+ if end > uint64(frame.dataSize) {
+ return 0, fmt.Errorf("invalid variant value: object field %d extends beyond data", frame.pendingIndex)
+ }
+ if rangeTop < len(ranges) {
+ ranges[rangeTop] = validationRange{
+ start: uint64(frame.pendingStart),
+ end: end,
+ field: int(frame.pendingIndex),
+ }
+ } else {
+ ranges = append(ranges, validationRange{
+ start: uint64(frame.pendingStart),
+ end: end,
+ field: int(frame.pendingIndex),
+ })
+ }
+ rangeTop++
+ }
+ 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 BasicType(frame.kind) == BasicObject {
+ frame.rangeStart = uint32(rangeTop)
+ }
+ }
+
+ if frame.kind == uint8(BasicArray) || frame.kind == uint8(BasicObject) {
+ if frame.nextChild < frame.numChildren {
+ if len(stack) == validationStackCapacity {
+ return 0, fmt.Errorf("invalid variant value: maximum nesting depth exceeded")
+ }
+ if len(stack) == cap(stack) {
+ if cap(stack) == validationStackInlineCapacity {
+ return validateValueIntermediate(meta, value, stack, ranges, rangeTop)
+ }
+ return validateValueDeep(meta, value, stack, ranges, rangeTop)
+ }
+
+ child, index, start, expectedSize, err := nextValidationChild(frame)
+ if err != nil {
+ stack = stack[:len(stack)-1]
+ if len(stack) == 0 {
+ return 0, err
+ }
+ resultErr = err
+ hasResult = true
+ continue
+ }
+
+ frame.nextChild++
+ frame.pendingIndex = uint32(index)
+ frame.pendingStart = uint32(start)
+ frame.pendingExpectedSize = uint32(expectedSize)
+ stack = append(stack, validationFrame{value: child})
+ continue
+ }
+
+ if err := finishValidationFrame(frame, ranges[int(frame.rangeStart):rangeTop]); err != nil {
+ if BasicType(frame.kind) == BasicObject {
+ rangeTop = int(frame.rangeStart)
+ }
+ stack = stack[:len(stack)-1]
+ if len(stack) == 0 {
+ return 0, err
+ }
+ resultErr = err
+ hasResult = true
+ continue
+ }
+ if BasicType(frame.kind) == BasicObject {
+ rangeTop = int(frame.rangeStart)
+ }
+ }
+
+ resultSize = int(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 validateValueIntermediate(meta Metadata, value []byte, initialStack []validationFrame, ranges []validationRange, rangeTop int) (int, error) {
+ var stackStorage [validationStackIntermediateCapacity]validationFrame
+ stack := stackStorage[:len(initialStack)]
+ copy(stack, initialStack)
+ return validateValueLoop(meta, value, stack, ranges, rangeTop)
+}
+
+func validateValueDeep(meta Metadata, value []byte, initialStack []validationFrame, ranges []validationRange, rangeTop int) (int, error) {
+ var stackStorage [validationStackCapacity]validationFrame
+ stack := stackStorage[:len(initialStack)]
+ copy(stack, initialStack)
+ return validateValueLoop(meta, value, stack, ranges, rangeTop)
+}
+
+func finishValidationFrame(frame *validationFrame, ranges []validationRange) error {
+ if BasicType(frame.kind) != BasicObject {
+ return nil
+ }
+
+ slices.SortFunc(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 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 != uint64(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 = uint8(basicTypeFromHeader(frame.value[0]))
+ switch BasicType(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 = uint64(want)
+ case BasicObject:
+ return prepareObjectValidationFrame(meta, frame)
+ case BasicArray:
+ return prepareArrayValidationFrame(frame)
+ case BasicPrimitive:
+ size, err := validatePrimitiveValue(frame.value)
+ frame.size = uint64(size)
+ return err
+ default:
+ return fmt.Errorf("invalid variant value: unknown basic type %d", BasicType(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))
+ }
+
+ var previousOffset uint32
+ for i := uint64(0); i <= uint64(numElements); i++ {
+ 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 < previousOffset {
+ return fmt.Errorf("invalid variant value: array offsets are not monotonic")
+ }
+ if dataStart+uint64(offset) > uint64(len(value)) || dataStart+uint64(offset) > math.MaxUint32 {
Review Comment:
**Blocking:** This treats `dataStart + offset` as though the complete encoded value were limited to `uint32`, but Variant's 4-byte field offsets are relative to the start of the fields region. A valid compound can therefore contain a `math.MaxUint32`-byte fields region plus its header and offset table. A sparse-mapped array with one max-sized binary child passes on the merge base but fails here with `array offset 4294967295 is out of range`. The equivalent object condition at line 907 has the same issue. Please retain total positions as `uint64` and compare them against `len(value)` without imposing this undocumented total-value limit.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(parquet/variant): validate compound value bounds [arrow-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on PR #1126:
URL: https://github.com/apache/arrow-go/pull/1126#issuecomment-5319804951
Since this is a draft, i'll hold off on further review until it's marked ready
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(parquet/variant): validate compound value bounds [arrow-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(parquet/variant): validate compound value bounds [arrow-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #1126:
URL: https://github.com/apache/arrow-go/pull/1126#discussion_r3865629466
##########
parquet/variant/variant.go:
##########
@@ -556,23 +555,376 @@ 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 validationRange struct {
+ start uint64
+ end uint64
+ field int
+}
+
+type validationFrame struct {
+ value []byte
+ kind BasicType
+ size int
+ dataSize uint64
+ dataStart uint64
+ offsetStart uint64
+ offsetSize uint8
+ numChildren uint32
+ nextChild uint32
+ pendingIndex int
+ pendingStart uint64
+ pendingExpectedSize uint64
+ rangeStart int
+ initialized bool
+ compound bool
+}
+
+const (
+ validationStackInlineCapacity = 32
+ validationRangeInlineCapacity = 64
+)
+
+// 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) {
+ var stackStorage [validationStackInlineCapacity]validationFrame
+ stack := stackStorage[:1]
+ stack[0].value = value
+
+ var rangeStorage [validationRangeInlineCapacity]validationRange
+ ranges := rangeStorage[:0]
+ rangeTop := 0
+
+ var (
+ resultSize int
+ resultErr error
+ hasResult bool
+ )
+
+ for len(stack) > 0 {
+ frame := &stack[len(stack)-1]
+ if hasResult {
+ hasResult = false
+
+ if resultErr != nil {
+ switch frame.kind {
+ case BasicArray:
+ return 0, fmt.Errorf("invalid variant value: array element %d: %w", frame.pendingIndex, resultErr)
+ case BasicObject:
+ return 0, fmt.Errorf("invalid variant value: object field %d: %w", frame.pendingIndex, resultErr)
+ default:
+ return 0, resultErr
+ }
+ }
+
+ switch frame.kind {
+ case BasicArray:
+ if uint64(resultSize) != frame.pendingExpectedSize {
+ return 0, fmt.Errorf("invalid variant value: array element %d has trailing bytes", frame.pendingIndex)
+ }
+ case BasicObject:
+ end := frame.pendingStart + uint64(resultSize)
+ if end > frame.dataSize {
+ return 0, fmt.Errorf("invalid variant value: object field %d extends beyond data", frame.pendingIndex)
+ }
+ if rangeTop < len(ranges) {
+ ranges[rangeTop] = validationRange{
+ start: frame.pendingStart,
+ end: end,
+ field: frame.pendingIndex,
+ }
+ } else {
+ ranges = append(ranges, validationRange{
+ start: frame.pendingStart,
+ end: end,
+ field: frame.pendingIndex,
+ })
+ }
+ rangeTop++
+ }
+ 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.kind == BasicObject {
+ frame.rangeStart = rangeTop
+ }
+ }
+
+ if frame.compound {
+ if frame.nextChild < frame.numChildren {
+ child, index, start, expectedSize, err := nextValidationChild(frame)
+ if err != nil {
+ stack = stack[:len(stack)-1]
+ if len(stack) == 0 {
+ return 0, err
+ }
+ resultErr = err
+ hasResult = true
+ continue
+ }
+
+ frame.nextChild++
+ frame.pendingIndex = index
+ frame.pendingStart = start
+ frame.pendingExpectedSize = expectedSize
+ stack = append(stack, validationFrame{value: child})
Review Comment:
**Blocking:** The common 40-child case is now allocation-free, but attacker-controlled nesting still grows this 120-byte frame stack with repeated slice reallocations. A valid 1,000-level nested array occupies 5,873 encoded bytes yet `NewWithMetadata` allocates about 393 KB; at 10,000 levels validation allocates about 6.2 MB. This leaves the prior deep-input memory-amplification/OOM concern unresolved on the row-read path. Please reduce or bound depth-dependent state and add a benchmark beyond `validationStackInlineCapacity`.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(parquet/variant): validate compound value bounds [arrow-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade merged PR #1126:
URL: https://github.com/apache/arrow-go/pull/1126
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org