fix(array): validate binary_view and string_view payloads - #994
Conversation
zeroshade
left a comment
There was a problem hiding this comment.
Thanks for this — the per-view invariants are a faithful, complete port of Arrow C++ ValidateBinaryView (negative size, inline padding-zero, buffer index/offset bounds, prefix match, and UTF-8 for StringView), and the cheap Validate / full ValidateFull split respects the O(1) vs O(n) contract. Tests pass and go vet is clean.
Requesting changes on two items:
- Blocking:
ValidateFullpanics with a nil-pointer dereference on any empty (length-0)BinaryView/StringView, including the standard builder output.validateViewValuesunconditionally dereferencesbuffers[1], which isnilfor empty arrays (Validatereturns early and never touches it). This regressed in the "tighten view validation helpers" commit. Repro + one-line guard inline. - CI lint:
gofmtfails on the inline-padding slice expression. Suggestion inline.
Non-blocking: a couple of tests worth adding (inline on the test file) — an empty-array case would have caught #1, and the out-of-line path is currently under-tested.
Details inline.
|
|
||
| func validateViewValues(arr ViewLike, dataBuffers []*memory.Buffer, validateValue func(int, []byte) error) error { | ||
| data := arr.Data().(*Data) | ||
| rawViews := data.buffers[1].Bytes() |
There was a problem hiding this comment.
Blocking: ValidateFull panics on empty (length-0) view arrays.
Validate() (via validateViewLayout) returns early when length == 0 and never dereferences buffers[1], but for an empty view array buffers[1] is nil, so this hoisted data.buffers[1].Bytes() panics.
Reproduced on the standard builder output (and on hand-built data):
array.ValidateFull(array.NewBinaryViewBuilder(mem).NewBinaryViewArray()) // panic: invalid memory address or nil pointer dereference
array.ValidateFull(array.NewStringViewBuilder(mem).NewStringViewArray()) // panicThis regressed in the "tighten view validation helpers" commit — patch 1 only touched buffers[1] inside the loop, so empty arrays were safe. It also undercuts the helper's intent: validation should report malformed data, not crash on a valid empty array.
| rawViews := data.buffers[1].Bytes() | |
| if data.length == 0 { | |
| return nil | |
| } | |
| rawViews := data.buffers[1].Bytes() |
| if view.IsInline() { | ||
| rawOffset := (data.offset + i) * arrow.ViewHeaderSizeBytes | ||
| raw := rawViews[rawOffset : rawOffset+arrow.ViewHeaderSizeBytes] | ||
| for _, b := range raw[4+view.Len():arrow.ViewHeaderSizeBytes] { |
There was a problem hiding this comment.
gofmt fails here (CI lint) — it wants spaces around : when the slice bounds are expressions:
| for _, b := range raw[4+view.Len():arrow.ViewHeaderSizeBytes] { | |
| for _, b := range raw[4+view.Len() : arrow.ViewHeaderSizeBytes] { |
| }) | ||
| } | ||
|
|
||
| func TestBinaryViewValidate(t *testing.T) { |
There was a problem hiding this comment.
Nice regression set. A few high-value cases are missing — the first would have caught the empty-array panic in validateViewValues:
- Empty (length-0) array via
array.ValidateFull(currently panics). - Drive the public
array.Validate/array.ValidateFullentrypoints, not just the methods directly, so framework dispatch + layout checks are covered. - An array with
offset > 0to lock in the(data.offset+i)*ViewHeaderSizeBytesindexing. - Out-of-line happy path plus untested failure modes: prefix mismatch, negative / out-of-range buffer offset, and
offset+size > buffer length.
zeroshade
left a comment
There was a problem hiding this comment.
Thanks for this — the per-view invariants are a faithful, complete port of Arrow C++ ValidateBinaryView (negative size, inline padding-zero, buffer index/offset bounds, prefix match, and UTF-8 for StringView), and the cheap Validate / full ValidateFull split respects the O(1) vs O(n) contract. Tests pass and go vet is clean.
Requesting changes on two items:
- Blocking:
ValidateFullpanics with a nil-pointer dereference on any empty (length-0)BinaryView/StringView, including the standard builder output.validateViewValuesunconditionally dereferencesbuffers[1], which isnilfor empty arrays (Validatereturns early and never touches it). This regressed in the "tighten view validation helpers" commit. Repro + one-line guard inline. - CI lint:
gofmtfails on the inline-padding slice expression. Suggestion inline.
Non-blocking: a couple of tests worth adding (inline on the test file) — an empty-array case would have caught #1, and the out-of-line path is currently under-tested.
Details inline.
Duplicate of the review directly above (accidental double-submit); see that one for the actual feedback.
zeroshade
left a comment
There was a problem hiding this comment.
Issues fixed, just needs to have the merge conflicts resolved
2e52107 to
dab1fe1
Compare
Summary
Why
BinaryView and StringView accessors currently trust header metadata directly. Malformed view headers can pass validation today even when they reference missing data or carry invalid payload metadata.
Validation