Repository navigation
GH-54908: [C++] Reject non-empty union arrays with missing type ids or offsets buffers - #54913
Open
JonSnow1807 wants to merge 1 commit into
Open
JonSnow1807 wants to merge 1 commit into
JonSnow1807 wants to merge 1 commit into
Conversation
… ids or offsets buffers Validate() accepted a non-empty union array whose type ids buffer is null, or a dense union whose offsets buffer is null, and ValidateFull() then dereferenced the null buffer and crashed. Report such arrays as invalid instead, like the existing "Non-empty array but offsets are null" check for list and binary types. Empty arrays may still omit these buffers.
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rationale for this change
Validate()accepts a non-empty union array whose type ids buffer is null, or a dense union array whose offsets buffer is null.ValidateFull()then readsdata.GetValues<int8_t>(1)/data.GetValues<int32_t>(2)unconditionally and segfaults (reproduced with the Python snippet from the issue: all three cases exit with SIGSEGV).What changes are included in this PR?
ValidateArrayImpl::Visit(const UnionType&)now returnsStatus::Invalidwhen the array is non-empty and the type ids buffer is null, or when it is a non-empty dense union and the offsets buffer is null. This mirrors the existing "Non-empty array but offsets are null" check for list and binary types. Empty arrays may still omit these buffers.Are these changes tested?
Yes.
TestSparseUnionArray.ValidateMissingTypeIdsandTestDenseUnionArray.ValidateMissingBuffersinarray_union_test.cccheck thatValidate()andValidateFull()both returnInvalidwith the new messages, that an array with all buffers present still validates, and that empty arrays without the buffers still validate. Without the fix the new tests fail ("Expected ... to fail with Invalid, but got OK"); with itarrow-array-testpasses (1075 passed, 1 skipped, Debug build with ASan/UBSan).Are there any user-facing changes?
Validate()andValidateFull()now return an error instead of crashing for these arrays.AI usage: the fix and the tests were written with an AI coding assistant (Claude Code) under my direction; I reviewed every line and ran the tests locally.