zeroshade commented on PR #1131:
URL: https://github.com/apache/arrow-go/pull/1131#issuecomment-5244534603
Separate from the review above, and **not** something I'd block this PR on —
but worth capturing while it's fresh.
This change widens a panic path that's reachable from untrusted input.
`arrow/ipc/metadata.go:806` builds union types directly from IPC file metadata:
```go
return arrow.UnionOf(mode, children, typeIDs), nil
```
`UnionOf` → `SparseUnionOf`/`DenseUnionOf` → `validate`, and on any
validation failure the constructor calls `panic(err)`. There's no non-panicking
union constructor. So an IPC file carrying `typeIDs: [1, 1]` will now panic
during schema deserialization rather than loading. Anyone parsing untrusted
Arrow data gets a crash where they previously got a schema that was wrong but
non-fatal.
Two things sharpen it. The enclosing function is `concreteTypeFromFB(...)
(arrow.DataType, error)` — it returns errors — and the code immediately above
the call already rejects one class of malformed type ID gracefully:
```go
if int32(code) != id {
return nil, errors.New("union type id out of bounds")
}
```
So the function error-checks one case and then hands the rest to a
constructor that panics.
To be clear about attribution: **this hazard pre-exists this PR.** A
negative type ID already survives that `int32(code) != id` check and then
panics on `c < 0` inside `validate`. This change doesn't introduce the pattern,
it just adds an easier trigger, since `[1, 1]` is two perfectly valid in-range
`int8` values.
The fix belongs in a separate change, roughly one of:
1. Add an error-returning union constructor (`NewSparseUnionType` /
`NewDenseUnionType` returning `(T, error)`) and have `metadata.go` use it,
keeping the panicking `*Of` helpers for statically-known types.
2. Or have `concreteTypeFromFB` validate type codes itself before calling
`UnionOf`, consistent with the bounds check it already performs.
Option 1 is more thorough, since the panic is reachable from any
deserialization path that builds types from file metadata, not just this one
call site.
I'll file this as an issue — flagging it here so the context isn't lost.
---
_Note: this comment was drafted with AI assistance by a maintainer and may
contain mistakes. If anything here looks wrong, say so and I'll take another
look._
--
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]
