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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
