fix(array): guard BinaryViewBuilder size checks#888
Conversation
| func (b *BinaryViewBuilder) ReserveData(length int) { | ||
| if int32(length) > viewValueSizeLimit { | ||
| func checkBinaryViewValueSize(length int) { | ||
| if length > int(viewValueSizeLimit) { |
There was a problem hiding this comment.
if the concern is wrapping past the int32 max value, this should be explicitly int64(viewValueSizeLimit) instead of just int so that it is correct on 32-bit systems also.
7d9d3f9 to
6df0038
Compare
zeroshade
left a comment
There was a problem hiding this comment.
Thanks — extracting checkBinaryViewValueSize(length int64) addresses my earlier note: the comparison is now done in int64 (length > int64(viewValueSizeLimit)), so the previous int32(length) truncation on 64-bit is gone and Append/ReserveData share one guard. TestBinaryViewBuilderRejectsOversizedValues (size-guard + ReserveData, the latter correctly skipped on 32-bit) passes locally. LGTM.
Heads-up: #895 reuses the binaryViewNoAllocAllocator helper this PR introduces, so this one should merge first.
What changed
BinaryViewBuildernow checks view value lengths asintbefore any narrowing conversion toint32. BothReserveDataandAppenduse the same size guard.The regression test covers oversized
ReserveDatalengths and oversized byte slices without allocating multi-gigabyte buffers.Why
The previous check converted lengths to
int32before comparison. Values larger thanmath.MaxInt32could wrap negative and bypass the 2GB guard.Validation
go test ./arrow/array -run 'TestBinaryViewBuilderRejectsOversizedValues|TestBinaryViewStringRoundTrip'go test ./arrow/arraygo test ./arrow/...go test -tags assert ./arrow/array