zeroshade commented on code in PR #13381:
URL: https://github.com/apache/arrow/pull/13381#discussion_r897308624
##########
go/arrow/array/string.go:
##########
@@ -113,6 +113,12 @@ func (a *String) setData(data *Data) {
if offsets := data.buffers[1]; offsets != nil {
a.offsets = arrow.Int32Traits.CastFromBytes(offsets.Bytes())
}
+
+ expNumOffsets := a.array.data.offset + a.array.data.length + 1
+ if a.array.data.length > 0 &&
+ (len(a.offsets) < expNumOffsets ||
int(a.offsets[expNumOffsets-2]) > len(a.values)) {
+ panic("arrow/array: string offsets out of bounds of data
buffer")
+ }
Review Comment:
Ah I flipped the condition in my head, the `>=` is what indicates it's valid
and as such `<` is telling that it's invalid. That's my bad.
Looking at the test you're referencing however, it's considered an *error*
if it overflows like that. The fact that arrays can be constructed easily by
putting the buffers together can result in such an overflow occurring (because
right now we don't validate it) but notice that it's expected that the call to
`Concatenate` would return an error in such a case because that is not a valid
array. If we're going to panic if someone tries constructing such an invalid
array then we can probably remove that test since the situation could never
occur (which also means this should probably have a corresponding change in
`binary.go` for `Binary` arrays.
If we allow the last offset to go past the end of the data slice, it doesn't
solve the original panic you were trying to prevent in the first place as
calling `ValueBytes` would still result in an out of bounds panic in that
situation.
--
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]
